From b8aacd660eb9460b6a86d4402d886f0bb32af2b4 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 9 May 2018 13:14:21 -0400 Subject: [PATCH 1/5] [qa] Handle disconnect_node race Several tests call disconnect_nodes() on each node-pair in rapid succession, resulting in a race condition if a node disconnects a peer in-between the calculation of the nodeid's to disconnect and the invocation of the disconnectnode rpc call. Handle this. GitHub-Pull: #13201 Rebased-From: 09c6699 --- test/functional/test_framework/util.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index ead1f844f..8251da5f2 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -335,7 +335,14 @@ def set_node_times(nodes, t): def disconnect_nodes(from_connection, node_num): for peer_id in [peer['id'] for peer in from_connection.getpeerinfo() if "testnode%d" % node_num in peer['subver']]: - from_connection.disconnectnode(nodeid=peer_id) + try: + from_connection.disconnectnode(nodeid=peer_id) + except JSONRPCException as e: + # If this node is disconnected between calculating the peer id + # and issuing the disconnect, don't worry about it. + # This avoids a race condition if we're mass-disconnecting peers. + if e.error['code'] != -29: # RPC_CLIENT_NODE_NOT_CONNECTED + raise # wait to disconnect wait_until(lambda: [peer['id'] for peer in from_connection.getpeerinfo() if "testnode%d" % node_num in peer['subver']] == [], timeout=5) From 4087dd08e7df5beedc335f4518b64eeab6a382c1 Mon Sep 17 00:00:00 2001 From: "David A. Harding" Date: Mon, 7 May 2018 09:59:17 -0400 Subject: [PATCH 2/5] RPC Docs: gettxout*: clarify bestblock and unspent counts GitHub-Pull: #13184 Rebased-From: f30e9be --- src/rpc/blockchain.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 346672e45..14d0541cd 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -934,9 +934,9 @@ UniValue gettxoutsetinfo(const JSONRPCRequest& request) "\nResult:\n" "{\n" " \"height\":n, (numeric) The current block height (index)\n" - " \"bestblock\": \"hex\", (string) the best block hash hex\n" - " \"transactions\": n, (numeric) The number of transactions\n" - " \"txouts\": n, (numeric) The number of output transactions\n" + " \"bestblock\": \"hex\", (string) The hash of the block at the tip of the chain\n" + " \"transactions\": n, (numeric) The number of transactions with unspent outputs\n" + " \"txouts\": n, (numeric) The number of unspent transaction outputs\n" " \"bogosize\": n, (numeric) A meaningless metric for UTXO set size\n" " \"hash_serialized_2\": \"hash\", (string) The serialized hash\n" " \"disk_size\": n, (numeric) The estimated size of the chainstate on disk\n" @@ -979,7 +979,7 @@ UniValue gettxout(const JSONRPCRequest& request) " Note that an unspent output that is spent in the mempool won't appear.\n" "\nResult:\n" "{\n" - " \"bestblock\" : \"hash\", (string) the block hash\n" + " \"bestblock\": \"hash\", (string) The hash of the block at the tip of the chain\n" " \"confirmations\" : n, (numeric) The number of confirmations\n" " \"value\" : x.xxx, (numeric) The transaction value in " + CURRENCY_UNIT + "\n" " \"scriptPubKey\" : { (json object)\n" From 4c14e7b67cd411b501e4a77385389da0afc9dd16 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 19 Apr 2018 11:07:54 -0400 Subject: [PATCH 3/5] [wallet] Fix zapwallettxes/multiwallet interaction. -zapwallettxes should be disallowed when starting bitcoin in multiwallet mode. GitHub-Pull: #13030 Rebased-From: 3476e3c --- src/wallet/init.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index ace95204b..6e0b11cd9 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -82,19 +82,19 @@ bool WalletParameterInteraction() } } - int zapwallettxes = gArgs.GetArg("-zapwallettxes", 0); + bool zapwallettxes = gArgs.GetBoolArg("-zapwallettxes", false); // -zapwallettxes implies dropping the mempool on startup - if (zapwallettxes != 0 && gArgs.SoftSetBoolArg("-persistmempool", false)) { - LogPrintf("%s: parameter interaction: -zapwallettxes=%s -> setting -persistmempool=0\n", __func__, zapwallettxes); + if (zapwallettxes && gArgs.SoftSetBoolArg("-persistmempool", false)) { + LogPrintf("%s: parameter interaction: -zapwallettxes enabled -> setting -persistmempool=0\n", __func__); } // -zapwallettxes implies a rescan - if (zapwallettxes != 0) { + if (zapwallettxes) { if (is_multiwallet) { return InitError(strprintf("%s is only allowed with a single wallet file", "-zapwallettxes")); } if (gArgs.SoftSetBoolArg("-rescan", true)) { - LogPrintf("%s: parameter interaction: -zapwallettxes=%s -> setting -rescan=1\n", __func__, zapwallettxes); + LogPrintf("%s: parameter interaction: -zapwallettxes enabled -> setting -rescan=1\n", __func__); } } From 5ff571e90c9fa71e189863e9cb21d15d5991f7da Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 19 Apr 2018 11:13:47 -0400 Subject: [PATCH 4/5] [wallet] [tests] Test disallowed multiwallet params Add a test to check that bitcoind fails to start when specifying -zapwallettxes, -salvagewallet and -upgradewallet when running in multiwallet mode. GitHub-Pull: #13030 Rebased-From: 1f83839 --- test/functional/wallet_multiwallet.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index b07e45166..3a34c43b7 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -56,6 +56,19 @@ class MultiWalletTest(BitcoinTestFramework): open(not_a_dir, 'a').close() self.assert_start_raises_init_error(0, ['-walletdir=' + not_a_dir], 'Error: Specified -walletdir "' + not_a_dir + '" is not a directory') + self.log.info("Do not allow -zapwallettxes with multiwallet") + self.assert_start_raises_init_error(0, ['-zapwallettxes', '-wallet=w1', '-wallet=w2'], "Error: -zapwallettxes is only allowed with a single wallet file") + self.assert_start_raises_init_error(0, ['-zapwallettxes=1', '-wallet=w1', '-wallet=w2'], "Error: -zapwallettxes is only allowed with a single wallet file") + self.assert_start_raises_init_error(0, ['-zapwallettxes=2', '-wallet=w1', '-wallet=w2'], "Error: -zapwallettxes is only allowed with a single wallet file") + + self.log.info("Do not allow -salvagewallet with multiwallet") + self.assert_start_raises_init_error(0, ['-salvagewallet', '-wallet=w1', '-wallet=w2'], "Error: -salvagewallet is only allowed with a single wallet file") + self.assert_start_raises_init_error(0, ['-salvagewallet=1', '-wallet=w1', '-wallet=w2'], "Error: -salvagewallet is only allowed with a single wallet file") + + self.log.info("Do not allow -upgradewallet with multiwallet") + self.assert_start_raises_init_error(0, ['-upgradewallet', '-wallet=w1', '-wallet=w2'], "Error: -upgradewallet is only allowed with a single wallet file") + self.assert_start_raises_init_error(0, ['-upgradewallet=1', '-wallet=w1', '-wallet=w2'], "Error: -upgradewallet is only allowed with a single wallet file") + # if wallets/ doesn't exist, datadir should be the default wallet dir wallet_dir2 = data_dir('walletdir') os.rename(wallet_dir(), wallet_dir2) From acdf4338221e35d5ec7b299bb18ccd953e28c985 Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Sun, 15 Apr 2018 11:22:28 -0400 Subject: [PATCH 5/5] Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip Ensures that callbacks are invoked in the order in which the chain is updated Resolves #12978 GitHub-Pull: #12988 Rebased-From: d86edd3 --- src/validation.cpp | 19 +++++++++---------- src/validationinterface.cpp | 4 ++++ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1f0ceba70..f981d2659 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -351,7 +351,7 @@ bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool CBlockIndex* tip = chainActive.Tip(); assert(tip != nullptr); - + CBlockIndex index; index.pprev = tip; // CheckSequenceLocks() uses chainActive.Height()+1 to evaluate @@ -2607,18 +2607,17 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& assert(trace.pblock && trace.pindex); GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); } - } - // When we reach this point, we switched to a new tip (stored in pindexNewTip). - // Notifications/callbacks that can run without cs_main + // Notify external listeners about the new tip. + // Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected + GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); - // Notify external listeners about the new tip. - GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); - - // Always notify the UI if a new block tip was connected - if (pindexFork != pindexNewTip) { - uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); + // Always notify the UI if a new block tip was connected + if (pindexFork != pindexNewTip) { + uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); + } } + // When we reach this point, we switched to a new tip (stored in pindexNewTip). if (nStopAtHeight && pindexNewTip && pindexNewTip->nHeight >= nStopAtHeight) StartShutdown(); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 928df4fa6..746263f11 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -139,6 +139,10 @@ void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason } void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { + // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which + // the chain actually updates. One way to ensure this is for the caller to invoke this signal + // in the same critical section where the chain is updated + m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] { m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); });