From 4693f8165ff01a7354f78b219794616b351f582a Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 20 Mar 2018 21:04:27 -0700 Subject: [PATCH 1/6] Fix csBestBlock/cvBlockChange waiting in rpc/mining (cherry picked from commit bitcoin/bitcoin@45dd13503918e75a45ce33eb5c934b998790fdc8) --- src/main.cpp | 9 ++++++++- src/main.h | 5 +++++ src/rpc/mining.cpp | 14 +++++++------- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index a3f1d506e..0a7c44417 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -68,6 +68,8 @@ CBlockIndex *pindexBestHeader = NULL; static std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block CWaitableCriticalSection csBestBlock; CConditionVariable cvBlockChange; +uint256 hashBestBlock; +int heightBestBlock; int nScriptCheckThreads = 0; std::atomic_bool fImporting(false); std::atomic_bool fReindex(false); @@ -3757,7 +3759,12 @@ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) { RenderPoolMetrics("sapling", saplingPool); RenderPoolMetrics("transparent", transparentPool); - cvBlockChange.notify_all(); + { + boost::unique_lock lock(csBestBlock); + hashBestBlock = pindexNew->GetBlockHash(); + heightBestBlock = pindexNew->nHeight; + cvBlockChange.notify_all(); + } } /** diff --git a/src/main.h b/src/main.h index a76fe63e6..f9df0527a 100644 --- a/src/main.h +++ b/src/main.h @@ -160,8 +160,13 @@ extern BlockMap mapBlockIndex; extern std::optional last_block_num_txs; extern std::optional last_block_size; extern const std::string strMessageMagic; + +// These prevent lock-ordering problems in getblocktemplate() RPC extern CWaitableCriticalSection csBestBlock; extern CConditionVariable cvBlockChange; +extern uint256 hashBestBlock; +extern int heightBestBlock; + extern std::atomic_bool fImporting; extern std::atomic_bool fReindex; extern int nScriptCheckThreads; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index c9f6fc73d..03d7fa291 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -607,15 +607,15 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) nTransactionsUpdatedLastLP = nTransactionsUpdatedLast; } + // Release the main lock while waiting + // Don't call chainActive->Tip() without holding cs_main + LEAVE_CRITICAL_SECTION(cs_main); { checktxtime = boost::get_system_time() + boost::posix_time::seconds(10); boost::unique_lock lock(csBestBlock); - while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning()) + while (hashBestBlock == hashWatchedChain && IsRPCRunning()) { - // Release the main lock while waiting - LEAVE_CRITICAL_SECTION(cs_main); - // Before waiting, generate the coinbase for the block following the next // block (since this is cpu-intensive), so that when next block arrives, // we can quickly respond with a template for following block. @@ -629,11 +629,10 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) next_cb_mtx = cached_next_cb_mtx; } bool timedout = !cvBlockChange.timed_wait(lock, checktxtime); - ENTER_CRITICAL_SECTION(cs_main); // Optimization: even if timed out, a new block may have arrived // while waiting for cs_main; if so, don't discard next_cb_mtx. - if (chainActive.Tip()->GetBlockHash() != hashWatchedChain) break; + if (hashBestBlock != hashWatchedChain) break; // Timeout: Check transactions for update if (timedout && mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) { @@ -643,11 +642,12 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) } checktxtime += boost::posix_time::seconds(10); } - if (chainActive.Tip()->nHeight != nHeight + 1) { + if (heightBestBlock != nHeight + 1) { // Unexpected height (reorg or >1 blocks arrived while waiting) invalidates coinbase tx. next_cb_mtx = nullopt; } } + ENTER_CRITICAL_SECTION(cs_main); if (!IsRPCRunning()) throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, "Shutting down"); From c079a518c066a0c8e24d844892f8a10c7638b9d1 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 3 Apr 2018 21:53:07 -0700 Subject: [PATCH 2/6] Modernize best block mutex/cv/hash variable naming (cherry picked from commit bitcoin/bitcoin@4a6c0e3dcfdca98270cb96b73db4c3d4446dba50) --- src/init.cpp | 2 +- src/main.cpp | 16 ++++++++-------- src/main.h | 8 ++++---- src/rpc/mining.cpp | 10 +++++----- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index f815416e5..e070bf944 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -305,7 +305,7 @@ bool static Bind(const CService &addr, unsigned int flags) { void OnRPCStopped() { - cvBlockChange.notify_all(); + g_best_block_cv.notify_all(); LogPrint("rpc", "RPC stopped.\n"); } diff --git a/src/main.cpp b/src/main.cpp index 0a7c44417..244624bb5 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -66,10 +66,10 @@ BlockMap mapBlockIndex; CChain chainActive; CBlockIndex *pindexBestHeader = NULL; static std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block -CWaitableCriticalSection csBestBlock; -CConditionVariable cvBlockChange; -uint256 hashBestBlock; -int heightBestBlock; +CWaitableCriticalSection g_best_block_mutex; +CConditionVariable g_best_block_cv; +uint256 g_best_block; +int g_best_block_height; int nScriptCheckThreads = 0; std::atomic_bool fImporting(false); std::atomic_bool fReindex(false); @@ -3760,10 +3760,10 @@ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) { RenderPoolMetrics("transparent", transparentPool); { - boost::unique_lock lock(csBestBlock); - hashBestBlock = pindexNew->GetBlockHash(); - heightBestBlock = pindexNew->nHeight; - cvBlockChange.notify_all(); + boost::unique_lock lock(g_best_block_mutex); + g_best_block = pindexNew->GetBlockHash(); + g_best_block_height = pindexNew->nHeight; + g_best_block_cv.notify_all(); } } diff --git a/src/main.h b/src/main.h index f9df0527a..b5c8ba90c 100644 --- a/src/main.h +++ b/src/main.h @@ -162,10 +162,10 @@ extern std::optional last_block_size; extern const std::string strMessageMagic; // These prevent lock-ordering problems in getblocktemplate() RPC -extern CWaitableCriticalSection csBestBlock; -extern CConditionVariable cvBlockChange; -extern uint256 hashBestBlock; -extern int heightBestBlock; +extern CWaitableCriticalSection g_best_block_mutex; +extern CConditionVariable g_best_block_cv; +extern uint256 g_best_block; +extern int g_best_block_height; extern std::atomic_bool fImporting; extern std::atomic_bool fReindex; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 03d7fa291..ef3621d7b 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -613,8 +613,8 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) { checktxtime = boost::get_system_time() + boost::posix_time::seconds(10); - boost::unique_lock lock(csBestBlock); - while (hashBestBlock == hashWatchedChain && IsRPCRunning()) + boost::unique_lock lock(g_best_block_mutex); + while (g_best_block == hashWatchedChain && IsRPCRunning()) { // Before waiting, generate the coinbase for the block following the next // block (since this is cpu-intensive), so that when next block arrives, @@ -628,11 +628,11 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) Params(), CAmount{0}, minerAddress, cached_next_cb_height); next_cb_mtx = cached_next_cb_mtx; } - bool timedout = !cvBlockChange.timed_wait(lock, checktxtime); + bool timedout = !g_best_block_cv.timed_wait(lock, checktxtime); // Optimization: even if timed out, a new block may have arrived // while waiting for cs_main; if so, don't discard next_cb_mtx. - if (hashBestBlock != hashWatchedChain) break; + if (g_best_block != hashWatchedChain) break; // Timeout: Check transactions for update if (timedout && mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) { @@ -642,7 +642,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) } checktxtime += boost::posix_time::seconds(10); } - if (heightBestBlock != nHeight + 1) { + if (g_best_block_height != nHeight + 1) { // Unexpected height (reorg or >1 blocks arrived while waiting) invalidates coinbase tx. next_cb_mtx = nullopt; } From e170c3abd6e4ea448d37b08819ed94b4ed822eba Mon Sep 17 00:00:00 2001 From: Larry Ruane Date: Tue, 1 Mar 2022 17:43:43 -0700 Subject: [PATCH 3/6] document global variables --- src/main.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main.h b/src/main.h index b5c8ba90c..da065f63a 100644 --- a/src/main.h +++ b/src/main.h @@ -161,11 +161,17 @@ extern std::optional last_block_num_txs; extern std::optional last_block_size; extern const std::string strMessageMagic; -// These prevent lock-ordering problems in getblocktemplate() RPC +//! These four variables are used to notify getblocktemplate RPC of new tips. +//! When UpdateTip() establishes a new tip (best block), it must awaken a +//! waiting getblocktemplate RPC (if there is one) immediately. But upon waking +//! up, getblocktemplate cannot call chainActive->Tip() because it does not +//! (and cannot) hold cs_main. So the g_best_block_height and g_best_block variables +//! (protected by g_best_block_mutex) provide the needed height and block +//! hash respectively to getblocktemplate without it requiring cs_main. extern CWaitableCriticalSection g_best_block_mutex; extern CConditionVariable g_best_block_cv; -extern uint256 g_best_block; extern int g_best_block_height; +extern uint256 g_best_block; extern std::atomic_bool fImporting; extern std::atomic_bool fReindex; From 2230ba912f7d178ae2e634b2d3ad5208340b522e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 2 Mar 2022 22:51:09 +0000 Subject: [PATCH 4/6] Ensure the view's best Orchard anchor matches the previous block `ConnectBlock` was already checking that the given view's "best block" was the previous block. However, it then assumed the view was correct on its claimed best anchors. For Orchard, we know that `hashFinalOrchardRoot` field of `CBlockIndex` will only ever be set when a block is (attempted to be) connected to the main chain, and so we can instead add assertions around its value and ensure the view is consistent with the previous block. --- src/main.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 244624bb5..8179b6b2a 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3136,7 +3136,19 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin assert(view.GetSaplingAnchorAt(view.GetBestAnchor(SAPLING), sapling_tree)); OrchardMerkleFrontier orchard_tree; - assert(view.GetOrchardAnchorAt(view.GetBestAnchor(ORCHARD), orchard_tree)); + if (pindex->pprev && chainparams.GetConsensus().NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_NU5)) { + // Verify that the view's current state corresponds to the previous block. + assert(pindex->pprev->hashFinalOrchardRoot == view.GetBestAnchor(ORCHARD)); + // We only call ConnectBlock() on top of the active chain's tip. + assert(!pindex->pprev->hashFinalOrchardRoot.IsNull()); + + assert(view.GetOrchardAnchorAt(pindex->pprev->hashFinalOrchardRoot, orchard_tree)); + } else { + if (pindex->pprev) { + assert(pindex->pprev->hashFinalOrchardRoot.IsNull()); + } + assert(view.GetOrchardAnchorAt(OrchardMerkleFrontier::empty_root(), orchard_tree)); + } // Grab the consensus branch ID for this block and its parent auto consensusBranchId = CurrentEpochBranchId(pindex->nHeight, chainparams.GetConsensus()); From ae3d2a3525687011a1627c534da8599d461bc0dc Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 3 Mar 2022 00:16:40 +0000 Subject: [PATCH 5/6] Add missing `view.PopAnchor(_, ORCHARD)` in `DisconnectBlock` By forgetting to pop the tree, we were leaving the view in an inconsistent state, where it believed the best Orchard anchor was for a tree that didn't correspond to the rest of the view. This would then propagate in subsequent block connections, and the chain history commitments to Orchard tree roots eventually result in inconsistent `blockcommitments` values in subsequent blocks. --- src/coins.cpp | 8 ++++++++ src/main.cpp | 11 +++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/coins.cpp b/src/coins.cpp index 2a62e549d..4ed2907af 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -683,6 +683,14 @@ void CCoinsViewCache::PopAnchor(const uint256 &newrt, ShieldedType type) { hashSaplingAnchor ); break; + case ORCHARD: + AbstractPopAnchor( + newrt, + ORCHARD, + cacheOrchardAnchors, + hashOrchardAnchor + ); + break; default: throw std::runtime_error("Unknown shielded type"); } diff --git a/src/main.cpp b/src/main.cpp index 8179b6b2a..5e646a734 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2909,6 +2909,17 @@ static DisconnectResult DisconnectBlock(const CBlock& block, CValidationState& s view.PopAnchor(SaplingMerkleTree::empty_root(), SAPLING); } + // Set the old best Orchard anchor back. We can get this from the + // `hashFinalOrchardRoot` of the last block. However, if the last + // block was not on or after the Orchard activation height, this + // will be set to `null`. For logical consistency, in this case we + // set the last anchor to the empty root. + if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_NU5)) { + view.PopAnchor(pindex->pprev->hashFinalOrchardRoot, ORCHARD); + } else { + view.PopAnchor(OrchardMerkleFrontier::empty_root(), ORCHARD); + } + // This is guaranteed to be filled by LoadBlockIndex. assert(pindex->nCachedBranchId); auto consensusBranchId = pindex->nCachedBranchId.value(); From 9a60cdeed95c80a19959f235b9bc02f7ba55f554 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 3 Mar 2022 01:45:30 +0000 Subject: [PATCH 6/6] Add RPC test for the Orchard commitment tree bug on first NU5 testnet --- qa/pull-tester/rpc-tests.py | 1 + qa/rpc-tests/orchard_reorg.py | 146 ++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100755 qa/rpc-tests/orchard_reorg.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index f22051bc7..7c8ed42f0 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -60,6 +60,7 @@ BASE_SCRIPTS= [ 'wallet_persistence.py', 'wallet_listnotes.py', # vv Tests less than 60s vv + 'orchard_reorg.py', 'fundrawtransaction.py', 'reorg_limit.py', 'mempool_limit.py', diff --git a/qa/rpc-tests/orchard_reorg.py b/qa/rpc-tests/orchard_reorg.py new file mode 100755 index 000000000..53ec9bf2a --- /dev/null +++ b/qa/rpc-tests/orchard_reorg.py @@ -0,0 +1,146 @@ +#!/usr/bin/env python3 +# Copyright (c) 2022 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + +# +# Test the effect of reorgs on the Orchard commitment tree. +# + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + BLOSSOM_BRANCH_ID, + HEARTWOOD_BRANCH_ID, + CANOPY_BRANCH_ID, + NU5_BRANCH_ID, + assert_equal, + connect_nodes_bi, + get_coinbase_address, + nuparams, + start_nodes, + stop_nodes, + sync_blocks, + wait_and_assert_operationid_status, + wait_bitcoinds, +) + +from finalsaplingroot import ORCHARD_TREE_EMPTY_ROOT + +from decimal import Decimal + +class OrchardReorgTest(BitcoinTestFramework): + def __init__(self): + super().__init__() + self.num_nodes = 4 + self.setup_clean_chain = True + + def setup_nodes(self): + return start_nodes(self.num_nodes, self.options.tmpdir, extra_args=[[ + nuparams(BLOSSOM_BRANCH_ID, 1), + nuparams(HEARTWOOD_BRANCH_ID, 5), + nuparams(CANOPY_BRANCH_ID, 5), + nuparams(NU5_BRANCH_ID, 10), + '-nurejectoldversions=false', + '-experimentalfeatures', + '-orchardwallet', + # '-debug', + ]] * self.num_nodes) + + def run_test(self): + # Activate NU5 so we can test Orchard. + self.nodes[0].generate(10) + self.sync_all() + + # Generate a UA with only an Orchard receiver. + account = self.nodes[0].z_getnewaccount()['account'] + addr = self.nodes[0].z_getaddressforaccount(account, ['orchard']) + assert_equal(addr['account'], account) + assert_equal(set(addr['pools']), set(['orchard'])) + ua = addr['unifiedaddress'] + + # Before mining any Orchard notes, finalorchardroot should be the empty Orchard root. + assert_equal( + ORCHARD_TREE_EMPTY_ROOT, + self.nodes[0].getblock(self.nodes[0].getbestblockhash())['finalorchardroot'], + ) + + # finalorchardroot should not change if we mine additional blocks without Orchard notes. + self.nodes[0].generate(100) + self.sync_all() + assert_equal( + ORCHARD_TREE_EMPTY_ROOT, + self.nodes[0].getblock(self.nodes[0].getbestblockhash())['finalorchardroot'], + ) + + # Create an Orchard note. + recipients = [{'address': ua, 'amount': Decimal('12.5')}] + opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], opid) + + # After mining a block, finalorchardroot should have changed. + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + orchardroot_oneleaf = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['finalorchardroot'] + print("Root of Orchard commitment tree with one leaf:", orchardroot_oneleaf) + assert(orchardroot_oneleaf != ORCHARD_TREE_EMPTY_ROOT) + + # finalorchardroot should not change if we mine additional blocks without Orchard notes. + self.nodes[0].generate(4) + self.sync_all() + assert_equal( + orchardroot_oneleaf, + self.nodes[0].getblock(self.nodes[0].getbestblockhash())['finalorchardroot'], + ) + + # Split the network so we can test the effect of a reorg. + print("Splitting the network") + self.split_network() + + # Create another Orchard note on node 0. + recipients = [{'address': ua, 'amount': Decimal('12.5')}] + opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], opid) + + # Mine two blocks on node 0. + print("Mining 2 blocks on node 0") + self.nodes[0].generate(2) + self.sync_all() + orchardroot_twoleaf = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['finalorchardroot'] + print("Root of Orchard commitment tree with two leaves:", orchardroot_twoleaf) + assert(orchardroot_twoleaf != ORCHARD_TREE_EMPTY_ROOT) + assert(orchardroot_twoleaf != orchardroot_oneleaf) + + # Generate 10 blocks on node 2. + print("Mining alternate chain on node 2") + self.nodes[2].generate(10) + self.sync_all() + assert_equal( + orchardroot_oneleaf, + self.nodes[2].getblock(self.nodes[2].getbestblockhash())['finalorchardroot'], + ) + + # Reconnect the nodes; node 0 will re-org to node 2's chain. + print("Re-joining the network so that node 0 reorgs") + # We can't use `self.join_network()` because the coinbase-spending second Orchard + # transaction doesn't propagate from node 1's mempool to node 2 on restart. Inline + # the block-syncing parts here. + assert self.is_network_split + stop_nodes(self.nodes) + wait_bitcoinds() + self.nodes = self.setup_nodes() + connect_nodes_bi(self.nodes, 1, 2) + sync_blocks(self.nodes[1:3]) + connect_nodes_bi(self.nodes, 0, 1) + connect_nodes_bi(self.nodes, 2, 3) + self.is_network_split = False + sync_blocks(self.nodes) + + # Verify that node 0's latest Orchard root matches what we expect. + orchardroot_postreorg = self.nodes[0].getblock(self.nodes[2].getbestblockhash())['finalorchardroot'] + print("Root of Orchard commitment tree after reorg:", orchardroot_postreorg) + assert_equal(orchardroot_postreorg, orchardroot_oneleaf) + + +if __name__ == '__main__': + OrchardReorgTest().main()