From fc1c282845f6b8436d1ea4c68eb3511034c29bea Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 5 May 2021 14:07:52 -0400 Subject: [PATCH 1/7] rpc/blockchain: Use existing blockman in gettxoutsetinfo Was missed in last bundle --- src/rpc/blockchain.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index f2b99579b76..fb1c7c73a25 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1196,7 +1196,7 @@ static RPCHelpMan gettxoutsetinfo() CCoinsStats prev_stats{hash_type}; if (pindex->nHeight > 0) { - GetUTXOStats(coins_view, WITH_LOCK(::cs_main, return std::ref(g_chainman.m_blockman)), prev_stats, node.rpc_interruption_point, pindex->pprev); + GetUTXOStats(coins_view, *blockman, prev_stats, node.rpc_interruption_point, pindex->pprev); } UniValue block_info(UniValue::VOBJ); From 9ecade14252ad1972f668d2d2e4ef44fdfcb944a Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 23 Apr 2021 15:18:04 -0400 Subject: [PATCH 2/7] rest: Add GetChainman function and use it This is not the cleanest change but: 1. It fixes the erroneous use of RPC's Ensure*() in rest.cpp, which cause crashes in REST contexts. RPC code wraps all calls in a try/except, REST code does not. Ensure*(), being part of RPC, expects that its throw's will get caught by a try/except. But if you use Ensure*() in REST code, since it doesn't have a try/except wrap, a crash will happen. 2. It is consistent with other functions like GetMemPool. Someone can probably make this a bit prettier. --- src/rest.cpp | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/src/rest.cpp b/src/rest.cpp index d41f374c499..747c7aea19e 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -107,6 +107,27 @@ static CTxMemPool* GetMemPool(const std::any& context, HTTPRequest* req) return node_context->mempool.get(); } +/** + * Get the node context chainstatemanager. + * + * @param[in] req The HTTP request, whose status code will be set if node + * context chainstatemanager is not found. + * @returns Pointer to the chainstatemanager or nullptr if none found. + */ +static ChainstateManager* GetChainman(const std::any& context, HTTPRequest* req) +{ + auto node_context = util::AnyPtr(context); + if (!node_context || !node_context->chainman) { + RESTERR(req, HTTP_INTERNAL_SERVER_ERROR, + strprintf("%s:%d (%s)\n" + "Internal bug detected: Chainman disabled or instance not found!\n" + "You may report this issue here: %s\n", + __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT)); + return nullptr; + } + return node_context->chainman; +} + static RetFormat ParseDataFormat(std::string& param, const std::string& strReq) { const std::string::size_type pos = strReq.rfind('.'); @@ -181,7 +202,9 @@ static bool rest_headers(const std::any& context, std::vector headers; headers.reserve(count); { - ChainstateManager& chainman = EnsureAnyChainman(context); + ChainstateManager* maybe_chainman = GetChainman(context, req); + if (!maybe_chainman) return false; + ChainstateManager& chainman = *maybe_chainman; LOCK(cs_main); CChain& active_chain = chainman.ActiveChain(); tip = active_chain.Tip(); @@ -252,7 +275,9 @@ static bool rest_block(const std::any& context, CBlockIndex* pblockindex = nullptr; CBlockIndex* tip = nullptr; { - ChainstateManager& chainman = EnsureAnyChainman(context); + ChainstateManager* maybe_chainman = GetChainman(context, req); + if (!maybe_chainman) return false; + ChainstateManager& chainman = *maybe_chainman; LOCK(cs_main); tip = chainman.ActiveChain().Tip(); pblockindex = chainman.m_blockman.LookupBlockIndex(hash); @@ -541,7 +566,9 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std:: std::string bitmapStringRepresentation; std::vector hits; bitmap.resize((vOutPoints.size() + 7) / 8); - ChainstateManager& chainman = EnsureAnyChainman(context); + ChainstateManager* maybe_chainman = GetChainman(context, req); + if (!maybe_chainman) return false; + ChainstateManager& chainman = *maybe_chainman; { auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) { for (const COutPoint& vOutPoint : vOutPoints) { @@ -644,7 +671,9 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req, CBlockIndex* pblockindex = nullptr; { - ChainstateManager& chainman = EnsureAnyChainman(context); + ChainstateManager* maybe_chainman = GetChainman(context, req); + if (!maybe_chainman) return false; + ChainstateManager& chainman = *maybe_chainman; LOCK(cs_main); const CChain& active_chain = chainman.ActiveChain(); if (blockheight > active_chain.Height()) { From e6b4aa6eb53dc555ecab2922af35e7a2572faf4f Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 23 Apr 2021 15:24:47 -0400 Subject: [PATCH 3/7] miner: Pass in chainman to RegenerateCommitments Pass in chainman instead of prev_block so that we can enforce the block.hashPrevBlock refers to prev_block invariant in the function itself. We should probably rethink BlockAssembler's API and somehow include commitment regeneration functionality in there. Something like a variant of CreateNewBlock that takes in a std::vector and return a CBlock instead of CBlockTemplate. That could avoid reaching for LookupBlockIndex at all. --- src/miner.cpp | 12 ++++++++++-- src/miner.h | 2 +- src/rpc/mining.cpp | 3 +-- src/test/util/setup_common.cpp | 3 +-- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 3bc7fdd458f..eccddbb04f1 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -39,13 +39,21 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam return nNewTime - nOldTime; } -void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block) +void RegenerateCommitments(CBlock& block, ChainstateManager& chainman) { CMutableTransaction tx{*block.vtx.at(0)}; tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block)); block.vtx.at(0) = MakeTransactionRef(tx); - WITH_LOCK(::cs_main, assert(g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock) == prev_block)); + CBlockIndex* prev_block; + { + // TODO: Temporary scope to check correctness of refactored code. + // Should be removed manually after merge of + // https://github.com/bitcoin/bitcoin/pull/20158 + LOCK(::cs_main); + assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman)); + prev_block = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock); + } GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus()); block.hashMerkleRoot = BlockMerkleRoot(block); diff --git a/src/miner.h b/src/miner.h index becf362b79d..10a80f4392e 100644 --- a/src/miner.h +++ b/src/miner.h @@ -203,6 +203,6 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev); /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */ -void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block); +void RegenerateCommitments(CBlock& block, ChainstateManager& chainman); #endif // BITCOIN_MINER_H diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 8190a2f006e..6826e6fd07d 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -378,8 +378,7 @@ static RPCHelpMan generateblock() // Add transactions block.vtx.insert(block.vtx.end(), txs.begin(), txs.end()); - CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)); - RegenerateCommitments(block, prev_block); + RegenerateCommitments(block, chainman); { LOCK(cs_main); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index f92e4c4b990..f53c505e7c7 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -246,8 +246,7 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector Date: Wed, 9 Sep 2020 15:37:31 -0400 Subject: [PATCH 4/7] bench: Use existing NodeContext in DuplicateInputs --- src/bench/duplicate_inputs.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bench/duplicate_inputs.cpp b/src/bench/duplicate_inputs.cpp index 25d1a2b56c4..4f6e1122b8b 100644 --- a/src/bench/duplicate_inputs.cpp +++ b/src/bench/duplicate_inputs.cpp @@ -25,7 +25,8 @@ static void DuplicateInputs(benchmark::Bench& bench) CMutableTransaction naughtyTx{}; LOCK(cs_main); - CBlockIndex* pindexPrev = ::ChainActive().Tip(); + assert(std::addressof(::ChainActive()) == std::addressof(testing_setup->m_node.chainman->ActiveChain())); + CBlockIndex* pindexPrev = testing_setup->m_node.chainman->ActiveChain().Tip(); assert(pindexPrev != nullptr); block.nBits = GetNextWorkRequired(pindexPrev, &block, chainparams.GetConsensus()); block.nNonce = 0; From f4a47a1febfa35ab077f2a841fe31a8cd9618250 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 18 Sep 2020 13:32:17 -0400 Subject: [PATCH 5/7] bench: Use existing chainman in AssembleBlock --- src/bench/block_assemble.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index aa72981cb4f..b4b33d115f3 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -37,7 +37,7 @@ static void AssembleBlock(benchmark::Bench& bench) LOCK(::cs_main); // Required for ::AcceptToMemoryPool. for (const auto& txr : txs) { - const MempoolAcceptResult res = ::AcceptToMemoryPool(::ChainstateActive(), *test_setup->m_node.mempool, txr, false /* bypass_limits */); + const MempoolAcceptResult res = ::AcceptToMemoryPool(test_setup->m_node.chainman->ActiveChainstate(), *test_setup->m_node.mempool, txr, false /* bypass_limits */); assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID); } } From db33cde80fff749c6adff9e91fca5f27f4bb6278 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 14 Oct 2020 16:48:17 -0400 Subject: [PATCH 6/7] index: Add chainstate member to BaseIndex --- src/index/base.cpp | 35 +++++++++++++++------------- src/index/base.h | 6 +++-- src/index/coinstatsindex.cpp | 2 +- src/index/txindex.cpp | 2 +- src/init.cpp | 6 ++--- src/test/blockfilter_index_tests.cpp | 2 +- src/test/coinstatsindex_tests.cpp | 2 +- src/test/txindex_tests.cpp | 3 ++- 8 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 4079fc45694..88a66cc0d5a 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -63,30 +63,31 @@ bool BaseIndex::Init() if (locator.IsNull()) { m_best_block_index = nullptr; } else { - m_best_block_index = g_chainman.m_blockman.FindForkInGlobalIndex(::ChainActive(), locator); + m_best_block_index = m_chainstate->m_blockman.FindForkInGlobalIndex(m_chainstate->m_chain, locator); } - m_synced = m_best_block_index.load() == ::ChainActive().Tip(); + CChain& active_chain = m_chainstate->m_chain; + m_synced = m_best_block_index.load() == active_chain.Tip(); if (!m_synced) { bool prune_violation = false; if (!m_best_block_index) { // index is not built yet // make sure we have all block data back to the genesis - const CBlockIndex* block = ::ChainActive().Tip(); + const CBlockIndex* block = active_chain.Tip(); while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) { block = block->pprev; } - prune_violation = block != ::ChainActive().Genesis(); + prune_violation = block != active_chain.Genesis(); } // in case the index has a best block set and is not fully synced // check if we have the required blocks to continue building the index else { const CBlockIndex* block_to_test = m_best_block_index.load(); - if (!ChainActive().Contains(block_to_test)) { + if (!active_chain.Contains(block_to_test)) { // if the bestblock is not part of the mainchain, find the fork // and make sure we have all data down to the fork - block_to_test = ::ChainActive().FindFork(block_to_test); + block_to_test = active_chain.FindFork(block_to_test); } - const CBlockIndex* block = ::ChainActive().Tip(); + const CBlockIndex* block = active_chain.Tip(); prune_violation = true; // check backwards from the tip if we have all block data until we reach the indexes bestblock while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) { @@ -104,20 +105,20 @@ bool BaseIndex::Init() return true; } -static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain& chain) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); if (!pindex_prev) { - return ::ChainActive().Genesis(); + return chain.Genesis(); } - const CBlockIndex* pindex = ::ChainActive().Next(pindex_prev); + const CBlockIndex* pindex = chain.Next(pindex_prev); if (pindex) { return pindex; } - return ::ChainActive().Next(::ChainActive().FindFork(pindex_prev)); + return chain.Next(chain.FindFork(pindex_prev)); } void BaseIndex::ThreadSync() @@ -140,7 +141,7 @@ void BaseIndex::ThreadSync() { LOCK(cs_main); - const CBlockIndex* pindex_next = NextSyncBlock(pindex); + const CBlockIndex* pindex_next = NextSyncBlock(pindex, m_chainstate->m_chain); if (!pindex_next) { m_best_block_index = pindex; m_synced = true; @@ -203,7 +204,7 @@ bool BaseIndex::Commit() bool BaseIndex::CommitInternal(CDBBatch& batch) { LOCK(cs_main); - GetDB().WriteBestBlock(batch, ::ChainActive().GetLocator(m_best_block_index)); + GetDB().WriteBestBlock(batch, m_chainstate->m_chain.GetLocator(m_best_block_index)); return true; } @@ -279,7 +280,7 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator) const CBlockIndex* locator_tip_index; { LOCK(cs_main); - locator_tip_index = g_chainman.m_blockman.LookupBlockIndex(locator_tip_hash); + locator_tip_index = m_chainstate->m_blockman.LookupBlockIndex(locator_tip_hash); } if (!locator_tip_index) { @@ -320,7 +321,7 @@ bool BaseIndex::BlockUntilSyncedToCurrentChain() const // Skip the queue-draining stuff if we know we're caught up with // ::ChainActive().Tip(). LOCK(cs_main); - const CBlockIndex* chain_tip = ::ChainActive().Tip(); + const CBlockIndex* chain_tip = m_chainstate->m_chain.Tip(); const CBlockIndex* best_block_index = m_best_block_index.load(); if (best_block_index->GetAncestor(chain_tip->nHeight) == chain_tip) { return true; @@ -337,8 +338,10 @@ void BaseIndex::Interrupt() m_interrupt(); } -bool BaseIndex::Start() +bool BaseIndex::Start(CChainState& active_chainstate) { + assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); + m_chainstate = &active_chainstate; // Need to register this ValidationInterface before running Init(), so that // callbacks are not missed if Init sets m_synced to true. RegisterValidationInterface(this); diff --git a/src/index/base.h b/src/index/base.h index 59eefab29ef..df4bdff1eaa 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -12,6 +12,7 @@ #include class CBlockIndex; +class CChainState; struct IndexSummary { std::string name; @@ -75,8 +76,9 @@ private: /// to a chain reorganization), the index must halt until Commit succeeds or else it could end up /// getting corrupted. bool Commit(); - protected: + CChainState* m_chainstate{nullptr}; + void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex) override; void ChainStateFlushed(const CBlockLocator& locator) override; @@ -117,7 +119,7 @@ public: /// Start initializes the sync state and registers the instance as a /// ValidationInterface so that it stays in sync with blockchain updates. - [[nodiscard]] bool Start(); + [[nodiscard]] bool Start(CChainState& active_chainstate); /// Stops the instance from staying in sync with blockchain updates. void Stop(); diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 7c8b2b186e0..6f841f16620 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -266,7 +266,7 @@ bool CoinStatsIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* n { LOCK(cs_main); - CBlockIndex* iter_tip{g_chainman.m_blockman.LookupBlockIndex(current_tip->GetBlockHash())}; + CBlockIndex* iter_tip{m_chainstate->m_blockman.LookupBlockIndex(current_tip->GetBlockHash())}; const auto& consensus_params{Params().GetConsensus()}; do { diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 3feefe8619a..09984b1abfd 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -204,7 +204,7 @@ bool TxIndex::Init() // Attempt to migrate txindex from the old database to the new one. Even if // chain_tip is null, the node could be reindexing and we still want to // delete txindex records in the old database. - if (!m_db->MigrateData(*pblocktree, ::ChainActive().GetLocator())) { + if (!m_db->MigrateData(*pblocktree, m_chainstate->m_chain.GetLocator())) { return false; } diff --git a/src/init.cpp b/src/init.cpp index 89e152e56fb..cd77e8da3bc 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1550,21 +1550,21 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // ********************************************************* Step 8: start indexers if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { g_txindex = std::make_unique(nTxIndexCache, false, fReindex); - if (!g_txindex->Start()) { + if (!g_txindex->Start(::ChainstateActive())) { return false; } } for (const auto& filter_type : g_enabled_filter_types) { InitBlockFilterIndex(filter_type, filter_index_cache, false, fReindex); - if (!GetBlockFilterIndex(filter_type)->Start()) { + if (!GetBlockFilterIndex(filter_type)->Start(::ChainstateActive())) { return false; } } if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) { g_coin_stats_index = std::make_unique(/* cache size */ 0, false, fReindex); - if (!g_coin_stats_index->Start()) { + if (!g_coin_stats_index->Start(::ChainstateActive())) { return false; } } diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index 1cb1c002f4f..2f532ef5983 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -131,7 +131,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup) // BlockUntilSyncedToCurrentChain should return false before index is started. BOOST_CHECK(!filter_index.BlockUntilSyncedToCurrentChain()); - BOOST_REQUIRE(filter_index.Start()); + BOOST_REQUIRE(filter_index.Start(::ChainstateActive())); // Allow filter index to catch up with the block index. constexpr int64_t timeout_ms = 10 * 1000; diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index bf7a80ae5cd..106fcd2a33e 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -32,7 +32,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) // is started. BOOST_CHECK(!coin_stats_index.BlockUntilSyncedToCurrentChain()); - BOOST_REQUIRE(coin_stats_index.Start()); + BOOST_REQUIRE(coin_stats_index.Start(::ChainstateActive())); // Allow the CoinStatsIndex to catch up with the block index that is syncing // in a background thread. diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp index 082655d811f..d47c54fd6e7 100644 --- a/src/test/txindex_tests.cpp +++ b/src/test/txindex_tests.cpp @@ -7,6 +7,7 @@ #include