Skip to content

Commit 702cfc8

Browse files
committed
Merge #21055: [Bundle 3/n] Prune remaining g_chainman usage in validation functions
e11b649 validation: CVerifyDB::VerifyDB: Use locking annotation (Carl Dong) 03f75c4 validation: Use existing chain member in CChainState::LoadGenesisBlock (Carl Dong) 5e4af77 validation: Use existing chain member in CChainState::AcceptBlock (Carl Dong) fee7334 validation: Pass in chain to FindBlockPos+SaveBlockToDisk (Carl Dong) a9d28bc validation: Use *this in CChainState::ActivateBestChainStep (Carl Dong) 4744efc validation: Pass in chainstate to CTxMemPool::check (Carl Dong) 1fb7b2c validation: Use *this in CChainState::InvalidateBlock (Carl Dong) 8cdb2f7 validation: Move LoadBlockIndexDB to CChainState (Carl Dong) 8b99efb validation: Move invalid block handling to CChainState (Carl Dong) 2bdf37f validation: Pass in chainstate to CVerifyDB::VerifyDB (Carl Dong) 31eac50 validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics} (Carl Dong) 63e4c73 validation: Pass in chainstate to ::PruneBlockFilesManual (Carl Dong) 4bada76 validation: Pass in chainstate to UpdateTip (Carl Dong) a3ba08b validation: Remove global ::{{Precious,Invalidate}Block,ResetBlockFailureFlags} (Carl Dong) 4927c9e validation: Remove global ::LoadGenesisBlock (Carl Dong) 9da106b validation: Check chain tip is non-null in CheckFinalTx (Carl Dong) Pull request description: Overall PR: #20158 (tree-wide: De-globalize ChainstateManager) Based on: - [x] #20750 | [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions 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` Note to self: - [x] Address: bitcoin/bitcoin#20750 (comment) ACKs for top commit: laanwj: Code review ACK e11b649 Tree-SHA512: 205a451a741e32f17d5966de289f2f5a3f0817738c0087b70ff4755ddd217b53d01050ed396669bda2b1d216a88d927b9778777f9ff95ab1fe20e59c5f341776
2 parents 83bdbbd + e11b649 commit 702cfc8

File tree

9 files changed

+101
-125
lines changed

9 files changed

+101
-125
lines changed

src/init.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,7 @@ static void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImp
720720
fReindex = false;
721721
LogPrintf("Reindexing finished\n");
722722
// To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
723-
LoadGenesisBlock(chainparams);
723+
::ChainstateActive().LoadGenesisBlock(chainparams);
724724
}
725725

726726
// -loadblock=
@@ -1636,7 +1636,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
16361636
// If we're not mid-reindex (based on disk + args), add a genesis block on disk
16371637
// (otherwise we use the one already on disk).
16381638
// This is called again in ThreadImport after the reindex completes.
1639-
if (!fReindex && !LoadGenesisBlock(chainparams)) {
1639+
if (!fReindex && !::ChainstateActive().LoadGenesisBlock(chainparams)) {
16401640
strLoadError = _("Error initializing block database");
16411641
break;
16421642
}
@@ -1747,7 +1747,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
17471747
// work when we allow VerifyDB to be parameterized by chainstate.
17481748
if (&::ChainstateActive() == chainstate &&
17491749
!CVerifyDB().VerifyDB(
1750-
chainparams, &chainstate->CoinsDB(),
1750+
chainparams, *chainstate, &chainstate->CoinsDB(),
17511751
args.GetArg("-checklevel", DEFAULT_CHECKLEVEL),
17521752
args.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS))) {
17531753
strLoadError = _("Corrupted block database detected");

src/net_processing.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2100,7 +2100,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
21002100
break;
21012101
}
21022102
}
2103-
m_mempool.check(&::ChainstateActive().CoinsTip());
2103+
m_mempool.check(m_chainman.ActiveChainstate());
21042104
}
21052105

21062106
/**
@@ -3061,7 +3061,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
30613061
const TxValidationState& state = result.m_state;
30623062

30633063
if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
3064-
m_mempool.check(&::ChainstateActive().CoinsTip());
3064+
m_mempool.check(m_chainman.ActiveChainstate());
30653065
// As this version of the transaction was acceptable, we can forget about any
30663066
// requests for it.
30673067
m_txrequest.ForgetTxHash(tx.GetHash());

src/rpc/blockchain.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,7 +1015,7 @@ static RPCHelpMan pruneblockchain()
10151015
height = chainHeight - MIN_BLOCKS_TO_KEEP;
10161016
}
10171017

1018-
PruneBlockFilesManual(height);
1018+
PruneBlockFilesManual(::ChainstateActive(), height);
10191019
const CBlockIndex* block = ::ChainActive().Tip();
10201020
CHECK_NONFATAL(block);
10211021
while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
@@ -1200,7 +1200,7 @@ static RPCHelpMan verifychain()
12001200

12011201
LOCK(cs_main);
12021202

1203-
return CVerifyDB().VerifyDB(Params(), &::ChainstateActive().CoinsTip(), check_level, check_depth);
1203+
return CVerifyDB().VerifyDB(Params(), ::ChainstateActive(), &::ChainstateActive().CoinsTip(), check_level, check_depth);
12041204
},
12051205
};
12061206
}
@@ -1232,7 +1232,7 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
12321232
if (consensusParams.vDeployments[id].nTimeout <= 1230768000) return;
12331233

12341234
UniValue bip9(UniValue::VOBJ);
1235-
const ThresholdState thresholdState = VersionBitsTipState(consensusParams, id);
1235+
const ThresholdState thresholdState = VersionBitsState(::ChainActive().Tip(), consensusParams, id, versionbitscache);
12361236
switch (thresholdState) {
12371237
case ThresholdState::DEFINED: bip9.pushKV("status", "defined"); break;
12381238
case ThresholdState::STARTED: bip9.pushKV("status", "started"); break;
@@ -1246,12 +1246,12 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
12461246
}
12471247
bip9.pushKV("start_time", consensusParams.vDeployments[id].nStartTime);
12481248
bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
1249-
int64_t since_height = VersionBitsTipStateSinceHeight(consensusParams, id);
1249+
int64_t since_height = VersionBitsStateSinceHeight(::ChainActive().Tip(), consensusParams, id, versionbitscache);
12501250
bip9.pushKV("since", since_height);
12511251
if (ThresholdState::STARTED == thresholdState)
12521252
{
12531253
UniValue statsUV(UniValue::VOBJ);
1254-
BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
1254+
BIP9Stats statsStruct = VersionBitsStatistics(::ChainActive().Tip(), consensusParams, id);
12551255
statsUV.pushKV("period", statsStruct.period);
12561256
statsUV.pushKV("threshold", statsStruct.threshold);
12571257
statsUV.pushKV("elapsed", statsStruct.elapsed);
@@ -1562,7 +1562,7 @@ static RPCHelpMan preciousblock()
15621562
}
15631563

15641564
BlockValidationState state;
1565-
PreciousBlock(state, Params(), pblockindex);
1565+
::ChainstateActive().PreciousBlock(state, Params(), pblockindex);
15661566

15671567
if (!state.IsValid()) {
15681568
throw JSONRPCError(RPC_DATABASE_ERROR, state.ToString());
@@ -1598,7 +1598,7 @@ static RPCHelpMan invalidateblock()
15981598
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
15991599
}
16001600
}
1601-
InvalidateBlock(state, Params(), pblockindex);
1601+
::ChainstateActive().InvalidateBlock(state, Params(), pblockindex);
16021602

16031603
if (state.IsValid()) {
16041604
::ChainstateActive().ActivateBestChain(state, Params());
@@ -1637,7 +1637,7 @@ static RPCHelpMan reconsiderblock()
16371637
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
16381638
}
16391639

1640-
ResetBlockFailureFlags(pblockindex);
1640+
::ChainstateActive().ResetBlockFailureFlags(pblockindex);
16411641
}
16421642

16431643
BlockValidationState state;

src/test/util/setup_common.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
179179
assert(!::ChainstateActive().CanFlushToDisk());
180180
::ChainstateActive().InitCoinsCache(1 << 23);
181181
assert(::ChainstateActive().CanFlushToDisk());
182-
if (!LoadGenesisBlock(chainparams)) {
182+
if (!::ChainstateActive().LoadGenesisBlock(chainparams)) {
183183
throw std::runtime_error("LoadGenesisBlock failed.");
184184
}
185185

src/txmempool.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -619,7 +619,7 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m
619619
UpdateCoins(tx, mempoolDuplicate, std::numeric_limits<int>::max());
620620
}
621621

622-
void CTxMemPool::check(const CCoinsViewCache *pcoins) const
622+
void CTxMemPool::check(CChainState& active_chainstate) const
623623
{
624624
if (m_check_ratio == 0) return;
625625

@@ -633,8 +633,11 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
633633
CAmount check_total_fee{0};
634634
uint64_t innerUsage = 0;
635635

636-
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));
637-
const int64_t spendheight = g_chainman.m_blockman.GetSpendHeight(mempoolDuplicate);
636+
CCoinsViewCache& active_coins_tip = active_chainstate.CoinsTip();
637+
assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(active_coins_tip)); // TODO: REVIEW-ONLY, REMOVE IN FUTURE COMMIT
638+
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
639+
const int64_t spendheight = active_chainstate.m_chain.Height() + 1;
640+
assert(g_chainman.m_blockman.GetSpendHeight(mempoolDuplicate) == spendheight); // TODO: REVIEW-ONLY, REMOVE IN FUTURE COMMIT
638641

639642
std::list<const CTxMemPoolEntry*> waitingOnDependants;
640643
for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) {
@@ -655,7 +658,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
655658
fDependsWait = true;
656659
setParentCheck.insert(*it2);
657660
} else {
658-
assert(pcoins->HaveCoin(txin.prevout));
661+
assert(active_coins_tip.HaveCoin(txin.prevout));
659662
}
660663
// Check whether its inputs are marked in mapNextTx.
661664
auto it3 = mapNextTx.find(txin.prevout);

src/txmempool.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -605,7 +605,7 @@ class CTxMemPool
605605
* all inputs are in the mapNextTx array). If sanity-checking is turned off,
606606
* check does nothing.
607607
*/
608-
void check(const CCoinsViewCache *pcoins) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
608+
void check(CChainState& active_chainstate) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
609609

610610
// addUnchecked must updated state for all ancestors of a given transaction,
611611
// to track size/count of descendant transactions. First version of

0 commit comments

Comments
 (0)