foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#21767: [Bundle 6/n] Prune g_chainman usage in auxiliary modules

Browse source 
7a799c9c2b index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde80f index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1feb bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb917 bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6eb5 miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade1425 rest: Add GetChainman function and use it (Carl Dong)
fc1c282845 rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)

Pull request description:

  Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)

  The first 2 commits are fixups addressing review for the last bundle: #21391

  NEW note:
  1. I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!

  Note to reviewers:
  1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
  1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
  2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
  3. Remove `old_function`

ACKs for top commit:
  jarolrod:
    ACK  7a799c9
  ariard:
    Code Review ACK 7a799c9
  fjahr:
    re-ACK 7a799c9c2b
  MarcoFalke:
    review ACK 7a799c9c2b 🌠
  ryanofsky:
    Code review ACK 7a799c9c2b. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
  jamesob:
    conditional ACK 7a799c9c2b ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))

Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e
This commit is contained in:
MarcoFalke 2021-06-01 13:34:08 +02:00
commit f63fc53c2a
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
16 changed files with 82 additions and 40 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/bench/block_assemble.cpp
View file

@ -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);
}
}

3
src/bench/duplicate_inputs.cpp
View file

@ -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;

35
src/index/base.cpp
View file

@ -60,33 +60,34 @@ bool BaseIndex::Init()
}
LOCK(cs_main);
CChain& active_chain = m_chainstate->m_chain;
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(active_chain, locator);
}
m_synced = m_best_block_index.load() == ::ChainActive().Tip();
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);

6
src/index/base.h
View file

@ -12,6 +12,7 @@
#include <validationinterface.h>
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<const CBlock>& 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();

2
src/index/coinstatsindex.cpp
View file

@ -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 {

2
src/index/txindex.cpp
View file

@ -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;
}

6
src/init.cpp
View file

@ -1549,21 +1549,21 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// ********************************************************* Step 8: start indexers
if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
g_txindex = std::make_unique<TxIndex>(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<CoinStatsIndex>(/* cache size */ 0, false, fReindex);
if (!g_coin_stats_index->Start()) {
if (!g_coin_stats_index->Start(::ChainstateActive())) {
return false;
}
}

12
src/miner.cpp
View file

@ -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);

2
src/miner.h
View file

@ -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

37
src/rest.cpp
View file

@ -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<NodeContext>(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<const CBlockIndex *> 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<bool> 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()) {

2
src/rpc/blockchain.cpp
View file

@ -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);

3
src/rpc/mining.cpp
View file

@ -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);

2
src/test/blockfilter_index_tests.cpp
View file

@ -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;

2
src/test/coinstatsindex_tests.cpp
View file

@ -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.

3
src/test/txindex_tests.cpp
View file

@ -7,6 +7,7 @@
#include <script/standard.h>
#include <test/util/setup_common.h>
#include <util/time.h>
#include <validation.h>
#include <boost/test/unit_test.hpp>
@ -27,7 +28,7 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
// BlockUntilSyncedToCurrentChain should return false before txindex is started.
BOOST_CHECK(!txindex.BlockUntilSyncedToCurrentChain());
BOOST_REQUIRE(txindex.Start());
BOOST_REQUIRE(txindex.Start(::ChainstateActive()));
// Allow tx index to catch up with the block index.
constexpr int64_t timeout_ms = 10 * 1000;

3
src/test/util/setup_common.cpp
View file

@ -246,8 +246,7 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
for (const CMutableTransaction& tx : txns) {
block.vtx.push_back(MakeTransactionRef(tx));
}
CBlockIndex* prev_block = WITH_LOCK(::cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
RegenerateCommitments(block, prev_block);
RegenerateCommitments(block, *Assert(m_node.chainman));
while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;