From my reading of past conversations and from a few offline chats, it seems that modularizing our consensus engine is a worthwhile first step towards a more complete isolation of said engine from non-consensus code.
Modularizing our consensus engine means that:
We get clearer visibility into what currently lies in consensus codepaths and what depends on our consensus engine
We can coalesce duplicate consensus initialization codepaths, mitigating against bugs that arise out of test/non-test initialization inconsistencies
In order to modularize our consensus engine, we need to first de-globalize the global ChainstateManager -- namely g_chainman -- as it and its dependencies are what makes up the bulk of our consensus engine. A few direct references to g_chainman have already been removed in #19927, however, its indirect uses (mainly via ::Chain(state|)Active()) are numerous in our codebase and often used to cheat avoid obtaining a ChainstateManager reference.
Description
This changeset moves the global ChainstateManger to NodeContext and removes ::Chain{state,}Active() as a first step towards better modularization of our consensus engine.
The above ordering is constructed according to the overall call graph of our codebase such that we avoid touching the same function/module twice.
Due to the length of this overall change, each commit aims to be trivially-reviewable and only requires the reviewer to reason about the correctness of one function/module at a time.
There are a lot of review-only assertions which can be used to check for correctness. They are removed in 2236237070a45fe570cd0113f0025b0a46ac89be.
#19259 (fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) by practicalswift)
#16981 (Improve runtime performance of --reindex by LarryRuane)
#15946 (Allow maintaining the blockfilterindex when using prune by jonasschnelli)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
ryanofsky
commented at 9:57 PM on October 22, 2020:
In commit "validation: Move GetSpendHeight to BlockManager" (05a9e983b421f7728c6cf4c18525abba8a68c82a)
This isn't changing behavior, but I don't think it's correct to lock cs_main after mempool.cs. Other code locks in the opposite order so there could be a deadlock if cs_main lock is actually acquired here and this isn't a redundant recursive lock. Maybe it is possible to clean this up simply by annotating CTxMemPool::check to already require cs_main.
ryanofsky
commented at 9:34 PM on October 23, 2020:
In commit "validation: Move GetSpendHeight to BlockManager" (05a9e98)
This isn't changing behavior, but I don't think it's correct to lock cs_main after mempool.cs. Other code locks in the opposite order so there could be a deadlock if cs_main lock is actually acquired here and this isn't a redundant recursive lock. Maybe it is possible to clean this up simply by annotating CTxMemPool::check to already require cs_main.
Another way to address this could be to move later commit 0af807545a93891df691acb43e07fd465c996ecd up, and pass the height into check() instead of locking cs_main to figure out the height. Though it seems like cs_main probably needs to be held during the whole check() call anyway and maybe it should just be annotated
dongcarl
commented at 6:53 PM on November 4, 2020:
The reason why I thought it'd be okay to lock cs_main after mempool.cs was because that's basically what's happening in GetSpendHeight prior to this changeset. As in:
We lock mempool.cs at the start of CTxMemPool::check
We call GetSpendHeight, which locks cs_main
Are you saying that this was wrong all along?
ryanofsky
commented at 3:14 PM on November 10, 2020:
In commit "validation: Move GetSpendHeight to BlockManager" (f2fded4d0d938eec936076a65bfab6932b73fb50)
The reason why I thought it'd be okay to lock cs_main after mempool.cs was because that's basically what's happening in GetSpendHeight prior to this changeset. As in:
We lock mempool.cs at the start of CTxMemPool::check
We call GetSpendHeight, which locks cs_main
Are you saying that this was wrong all along?
Yes that can't be right because if you have different threads that lock two mutexes, all the threads have to lock the mutexes in the same order to prevent deadlocks:
ryanofsky
commented at 10:33 PM on October 22, 2020:
In commit "validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}" (b60c7e5a5a60fe60a3bc8ba540dc7c8d5c935869)
I'm a little unclear why it's safe to drop these locks, or why they were here to begin with, or if VersionBits or ChainActive or Tip functions should have been annotated to require cs_main. It seems like something should have be annotated if cs_main was required here
dongcarl
commented at 7:19 PM on November 4, 2020:
Digging in the git history a bit, it seems that the earliest of this trio -- namely VersionBitsTipState -- was introduced in d23f6c6a0d2. My hypothesis is that the LOCK(cs_main) was added in order to stop the chain from advancing and make sure that the caller got info on the actual tip. However, it seems like a AssertLockHeld(cs_main); annotation would have been much more suitable in that case (and existed in the codebase at d23f6c6a0d2).
I'm not entirely sure about the original intent here, so perhaps @sipa can enlighten me and make sure I'm not missing something!
In commit "validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}" (ff894f9f710118a13b51f2496e1261c795844840)
Digging in the git history a bit, it seems that the earliest of this trio -- namely VersionBitsTipState -- was introduced in d23f6c6.
Thanks for checking! I see EXCLUSIVE_LOCKS_REQUIRED(cs_main) is used where the new code is called so there should not actually be a concern here.
At least add a comment about the fact the cs_main lock is already taken in this code path L1317 (src/rpc/blockchain.cpp) ?
Line 1317 seems to be referring to LOCK(cs_main) getblockchaininfo. Writing a comment referencing a specific lock in a specific calling function seems a little fragile, and hopefully unnecessary with EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation.
dongcarl
commented at 10:24 PM on December 18, 2020:
@ariard@ryanofsky would it be preferable to add a EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation to VersionBitsTip{State,SinceHeight,Statistics} here just in case they get new callers in the future?
ryanofsky
commented at 1:38 PM on December 22, 2020:
In commit "validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}" (6ed743d86fb1b5466076eaacf1e8ebc830abde62)
@ariard@ryanofsky would it be preferable to add a EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation to VersionBitsTip{State,SinceHeight,Statistics} here just in case they get new callers in the future?
I'd keep what you have now unless there's a reason to think conclusion from digging into history is incorrect.
It would be confusing to add a lock required annotation if we don't think a lock is required. In theory you could write EXCLUSIVE_LOCKS_REQUIRED(cs_main) with a comment "There is no known reason to think a lock is required to call this function, but the original code had an explicit recursive lock, so locking is stilll required to be conservative in case there is an unknown reason locking is needed." This would probably be overkill though.
ryanofsky
commented at 10:41 PM on October 22, 2020:
contributor
This is all pretty straightforward. I suppose I'm about halfway done reviewing (will update list below with progress).
b419519dd2535ad1e4a62f7ea151cc22672057be test: Add new ChainTestingSetup and use it (1/79)
ef1b08c0738da2248fbddc0190320f5ed34b7f44 bench: [FIX] Use existing NodeContext in WalletBalance benchmarks (2/79)
a9539e725bb109fb8d98970223fa8eff6a582b4a wallet/test: [FIX] Use existing NodeContext in wallet_tests (3/79)
b927d6891eb21d364e92a6e4363836e3e9849a24 qt/test: [FIX] Add forgotten Context setting in RPCNestedTests (4/79)
8615dbd1250c2dddec2f54bf80e2a1b7013bc871 validation: Move LookupBlockIndex to BlockManager (5/79)
7ef17f7a52fcce19fa9380ca876310b14ae3c803 scripted-diff: Use BlockManager::LookupBlockIndex (6/79)
8dbedcb3bf123411ebcd9ccb7521b44cacc8e385 validation: Move FindForkInGlobalIndex to BlockManager (7/79)
05a9e983b421f7728c6cf4c18525abba8a68c82a validation: Move GetSpendHeight to BlockManager (8/79)
6d66fca3cfa891554ed1840df9231de91e60a5f1 validation: Move GetLastCheckpoint to BlockManager (9/79)
8f2aadb89fdfe36274743352048382bafb6c389d validation: Pass in blockman to ContextualCheckBlockHeader (10/79)
867330aedfd4dc12379bc0dcd27c50c26a5ebd9f validation: Make CChainState.m_blockman public (11/79)
dfdb9eaa9c1ff62696c07cf38f5b7b7efcce8b2a validation: Pass in chainstate to TestBlockValidity (12/79)
af1e05864588309a50e36c927d343b06a8bfe5ce validation: Pass in chainstate to ::NotifyHeaderTip (13/79)
7e15a2c88e49ec6719997e2cdd8481ddd4e7f6be validation: Remove global ::ActivateBestChain (14/79)
2f2e8a23959ad8f5dcd9d568af0019274a2757df validation: Move LoadExternalBlockFile to CChainState (15/79)
18fab989be1b994f27fd607a12180694c0836337 validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders (16/79)
0fc234246f068dfe969ef8ea9461186a857a8b57 validation: Use existing chainman in ChainstateManager::ProcessNewBlock (17/79)
0cca6ce81f1bd4852b4bf8963164d38c1fe60491 validation: Pass in chainstate to ::LimitMempoolSize (18/79)
27586e82658d6bf4c4600a7c5e220b145293e899 validation: Pass in chainstate to IsCurrentForFeeEstimation (19/79)
72b4896f5c8ceb3fdead4381260821e6d6f4b9bd validation: Pass in chainstate to CheckInputsFromMempoolAndCache (20/79)
6ca04c8deca436cb98bfe8206dd3d2a29737c672 validation: Pass in chain tip to ::CheckFinalTx (21/79)
6c57bfbfd1a755b3db156ff2c7b1db65652eb684 scripted-diff: Invoke ::CheckFinalTx with chain tip (22/79)
ea1cedfd4a4eff5bf58b6be2f098fa01f8c36231 validation: Pass in chainstate to ::CheckSequenceLocks (23/79)
bf33651c004f2bafc8cf60c882543f3e5e9a9224 validation: Add chainstate member to MemPoolAccept (24/79)
dcbfe76e89370962aae2cf100263ea9ea608b084 validation: Pass in chainstate to AcceptToMemoryPoolWithTime (25/79)
44970c069aa24c4b9daf214424c326719b03fd5e validation: Pass in chainstate to ::LoadMempool (26/79)
a1c43172255c958ae37abf1d2476d5f5c4e99bf1 validation: Pass in chainstate to ::AcceptToMemoryPool (27/79)
d6c12f35a5d1a62365874d46f290e759e77c34c8 scripted-diff: Invoke ::AcceptToMemoryPool with chainstate (28/79)
ryanofsky
commented at 9:50 PM on October 23, 2020:
In commit "miner: Add chainstate member to BlockAssembler" (9d74aa3fd30810197d24ec0768f3e28467abe114)
This appears to be safe because cs_main is held the entire time a BlockAssembler object is constructed is used cs_main is held, but because unlike m_mempool, m_active_chainstate will be able to change at runtime with utxo snapshot loading, it might be safer to just pass CChainState as a regular function argument to methods that need it. Another alternative would might be to add BlockAssembler documentation to say that it's meant to be used and thrown away, not kept alive and reused, at least if the active chain might change
ryanofsky
commented at 10:03 PM on October 23, 2020:
In commit "node/coinstats: Pass in CChainState to GetUTXOStats" (a9e77bb0a70a473aab0a52e0d411af8f692ab700)
From the commit description "ATTENTION" this may be a work in progress, but a suggestion here might be to just pass in CCoinsView and blockman as separate arguments, instead of passing the whole chain state class.
in
src/net_processing.cpp:673
in
053c84ce5aoutdated
560 | @@ -563,41 +561,6 @@ static bool MarkBlockAsInFlight(CTxMemPool& mempool, NodeId nodeid, const uint25
561 | return true;
562 | }
563 |
564 | -/** Check whether the last unknown block a peer advertised is not yet known. */
ryanofsky
commented at 10:09 PM on October 23, 2020:
In commit "net_processing: Move some static functions to PeerManager" (053c84ce5a99f0ef87fe23164dc11a0eac725680)
Might be nice to split this commit, and move the function bodies in a MOVEONLY commit. But this is maybe less important than it used to be now that git diff displays moved code better than it used to
ryanofsky approved
ryanofsky
commented at 10:34 PM on October 23, 2020:
contributor
Code review near-ACK2a05215114f2e4ebf764647af7e48ca1a8cb9ff9. Main change I'd like to see is for all the intermediate commits to compile and work. Otherwise this looks good and cleans up a lot of leftover mess from previous changes.
ryanofsky
commented at 4:20 PM on October 26, 2020:
contributor
Thread 1 "b-test" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
[#0](/github-metadata-backup-bitcoin-bitcoin/0/) __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
[#1](/github-metadata-backup-bitcoin-bitcoin/1/) 0x00007ffff337f8b1 in __GI_abort () at abort.c:79
[#2](/github-metadata-backup-bitcoin-bitcoin/2/) 0x0000555555ac21ab in __sanitizer::Abort() ()
[#3](/github-metadata-backup-bitcoin-bitcoin/3/) 0x0000555555abf4d8 in __sanitizer::Die() ()
[#4](/github-metadata-backup-bitcoin-bitcoin/4/) 0x0000555555aa175d in __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) ()
[#5](/github-metadata-backup-bitcoin-bitcoin/5/) 0x0000555555aa1f48 in __asan_report_load4 ()
[#6](/github-metadata-backup-bitcoin-bitcoin/6/) 0x00005555575f4d03 in base_uint<256u>::CompareTo (this=0x60d00016c958, b=...) at arith_uint256.cpp:112
[#7](/github-metadata-backup-bitcoin-bitcoin/7/) 0x000055555615213a in operator< (a=..., b=...) at ./arith_uint256.h:220
[#8](/github-metadata-backup-bitcoin-bitcoin/8/) 0x0000555556a710e4 in BlockManager::AddToBlockIndex (this=0x611000129720, block=...) at validation.cpp:3183
[#9](/github-metadata-backup-bitcoin-bitcoin/9/) 0x0000555556a9466d in CChainState::LoadGenesisBlock (this=0x612000101440, chainparams=...) at validation.cpp:4631
[#10](/github-metadata-backup-bitcoin-bitcoin/10/) 0x0000555556d1dcaa in TestingSetup::TestingSetup (this=0x7fffffffb2b0, chainName="main", extra_args=std::vector of length 0, capacity 0) at test/util/setup_common.cpp:183
[#11](/github-metadata-backup-bitcoin-bitcoin/11/) 0x0000555555af39df in RPCNestedTests::rpcNestedTests (this=0x7fffffffded0) at qt/test/rpcnestedtests.cpp:36
[#12](/github-metadata-backup-bitcoin-bitcoin/12/) 0x0000555555b990be in RPCNestedTests::qt_static_metacall (_o=0x7fffffffded0, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffffd2c0)
at qt/test/moc_rpcnestedtests.cpp:70
[#13](/github-metadata-backup-bitcoin-bitcoin/13/) 0x00007ffff6b9e003 in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
[#14](/github-metadata-backup-bitcoin-bitcoin/14/) 0x00007ffff66dc79a in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.5
[#15](/github-metadata-backup-bitcoin-bitcoin/15/) 0x00007ffff66dd4f0 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.5
[#16](/github-metadata-backup-bitcoin-bitcoin/16/) 0x00007ffff66dda61 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.5
[#17](/github-metadata-backup-bitcoin-bitcoin/17/) 0x00007ffff66de02b in QTest::qExec(QObject*, int, char**) () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.5
[#18](/github-metadata-backup-bitcoin-bitcoin/18/) 0x0000555555afead1 in main (argc=1, argv=0x7fffffffe298) at qt/test/test_main.cpp:85
</p>
</details>
ryanofsky
commented at 5:01 PM on October 26, 2020:
contributor
Following change partially reverting 50f680b402a84b605aa9b9aa58cb571ef32e190e seems to fix the error:
--- a/src/qt/test/apptests.cpp
+++ b/src/qt/test/apptests.cpp
@@ -84,6 +84,10 @@ void AppTests::appTests()
// Reset global state to avoid interfering with later tests.
LogInstance().DisconnectTestLogger();
AbortShutdown();
+ {
+ LOCK(cs_main);
+ ::pindexBestHeader = nullptr;
+ }
}
//! Entry point for BitcoinGUI tests.
Error is happening here:
[#8](/github-metadata-backup-bitcoin-bitcoin/8/) 0x0000555556a710e4 in BlockManager::AddToBlockIndex (this=0x611000129860, block=...) at validation.cpp:3183
3183 if (pindexBestHeader == nullptr || pindexBestHeader->nChainWork < pindexNew->nChainWork)
jnewbery
commented at 12:37 PM on October 28, 2020:
This is only used in validation_chainstatemanager_tests.cpp. Rather than adding complexity to the common setup for all tests, can you define this struct inside validation_chainstatemanager_tests.cpp and leave setup_common unchanged?
jnewbery
commented at 12:46 PM on October 28, 2020:
member
Super duper concept ACK. Managing ChainstateManager's lifetime and access through init and node.context is delightful.
The first 4 commits to test/bench code could easily by sliced off into a small PR.
436 | @@ -440,6 +437,9 @@ class BlockManager
437 |
438 | CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
439 |
440 | + /** Find the last common block between the parameter chain and a locator. */
What would be the utility of finding a fork against a chain which isn't the current active one ? May we have a future module interested in learning the common ancestor between a discovered locator and a chain known to not-be the best one ?
E.g, in src/net_processing, at NetMsgType::GETBLOCKS reception, this common ancestor is used at a starting point to send the rest of the chain to the forked-off GETBLOCKS requester. IMO, I don't see how it can be useful computation for a requester to chain-sync on a chain which is known as not being the best. Even if you don't have live-access to the rest of the p2p network, you still have a notion of a most-work chain.
in
src/validation.h:429
in
4d1456824boutdated
439 | @@ -440,6 +440,13 @@ class BlockManager
440 | /** Find the last common block between the parameter chain and a locator. */
441 | CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
442 |
443 | + /** 444 | + * Return the spend height, which is one more than the inputs.GetBestBlock().
I think interpretation of this comment can be confusing.
It does say "one more than the inputs.GetBestBlock()" where GetBestBlock is defined as "Get best block at the time this cursor was created" (L173, in src/coins.h). If GetSpendHeight means the spend height of a utxo, you may have between "one to infinite numbers of blocks" between utxo creation height and its spending height.
But still the code of this function return pindexPrev->nHeight + 1 where pindexPrev is the indice of inputs.GetBestBlock(). So this function return the earliest valid spending height of a given utxo set ?
ryanofsky
commented at 3:25 PM on November 10, 2020:
I think interpretation of this comment can be confusing.
Comment is not new, just moved.
It does say "one more than the inputs.GetBestBlock()" where GetBestBlock is defined as "Get best block at the time this cursor was created" (L173, in src/coins.h). If GetSpendHeight means the spend height of a utxo, you may have between "one to infinite numbers of blocks" between utxo creation height and its spending height.
But still the code of this function return pindexPrev->nHeight + 1 where pindexPrev is the indice of inputs.GetBestBlock(). So this function return the earliest valid spending height of a given utxo set ?
Yes this seems all correct. I think there could be a standalone PR if the hope is to clarify something here, but on the surface this seems like a simple two line function and one sentence description of what the function returns. Nothing seems too related to this PR.
Should you introduce a AssertLockHeld(cs_main) here to demonstrate the lock tacking is still enforced ? Some moved function (FindForkInGlobalIndex) already have such one.
More generally, I think you PR description could talk about what the actual lock model of this subsystem and how do you convey to reviewers that old code lock model is equivalent to the new one (or not if you've changed some locks for good reasons).
in
src/validation.h:536
in
959f626823outdated
556 |
557 | public:
558 | + //! Reference to a BlockManager instance which itself is shared across all 559 | + //! CChainState instances. Keeping a local reference allows us to test more 560 | + //! easily as opposed to referencing a global. 561 | + BlockManager& m_blockman GUARDED_BY(::cs_main);
174 | @@ -175,13 +175,6 @@ void ThreadScriptCheck(int worker_num);
175 | * @returns The tx if found, otherwise nullptr
176 | */
177 | CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock);
178 | -/** 179 | - * Find the best known block, and make it the tip of the block chain 180 | - * 181 | - * May not be called with cs_main held. May not be called in a
This comment still holds on CChainState::ActivateBestChain. But I wonder if it shouldn't be changed to a "Must not be called with cs_main held given we assert that lock isn't held L2878 (src/validation.cpp`).
dongcarl
commented at 8:38 PM on November 17, 2020:
Maybe add a friendly comment in commit about the fact that versionbitscache is currently a global and thus the stays the same. Though we can't assert it ?
in
src/net_processing.cpp:2269
in
d7db6ec342outdated
I don't follow what you mean by commit message "This is the only instance where validation reaches for something outside of it", you mean the mempool accessing spendheight ?
dongcarl
commented at 9:02 PM on November 17, 2020:
I mean that in the codepath I'm touching, CChainState::ActivateBestChainStep (which resides in validation.cpp) is calling (reaching for) CTXMemPool::Check (which resides in txmempool.cpp), which means that I need to resolve CTXMemPool::Check's reference of g_chainman first.
in
src/test/util/setup_common.cpp:220
in
d227dfedfdoutdated
Can we pass directly the result of LookupBlockIndex(block.hashPrevBlock) ? Block is already known here. Because AFAIU, the lock isn't held once RegenerateCommitment argument are loaded on the stack thus do you have guarantee you're calling the new blockman.LookupBlockIndex() with a lock ?
I think so otherwise LookupBlockIndex's lock annotation would yelled but I can't convince myself of it...
dongcarl
commented at 9:28 PM on November 17, 2020:
Then the lock is released as RegenerateCommitments's assembly function prologue is run, and the same lock is taken again when LookupBlockIndex is called inside RegenerateCommitments.
The original reason why I wanted to just pass in the blockman is because I didn't want to impose the constraint of "arg 2 must be the block index of arg 1" on RegenerateCommitments's arguments, which seems like something that might be easy to cause a bug if someone were to work with multiple blocks or multiple block indices. Perhaps adequate documentation can prevent this?
ryanofsky
commented at 2:27 PM on December 22, 2020:
contributor
Code review ACKabcde917ddec206cd938d6d418094d5db645f740. Changes since last review: rebase, adding temporary CheckFinalTx, AcceptToMemoryPool, CreateNewBlock wrappers I think to fix intermediate commits, adding new temporary asserts in Node/Chain interfaces, dropping ::pindexBestHeader == null shutdown assert
I see todo list includes resolving #20158 (review) about consistent cs_main/mempool.cs lock order for GetSpendHeight, and it would be nice to resolve, but it's about preexisting behavior so maybe isn't too important
jnewbery
commented at 5:45 PM on December 22, 2020:
member
@dongcarl are you planning on carving off some more commits into a separate PR? I'd love to help this make some progress.
dongcarl
commented at 5:47 PM on December 22, 2020:
contributor
@jnewbery Yup, got a bit caught up on other things but will open that first PR today!
laanwj referenced this in commit 44f4bcd302 on Feb 1, 2021
jnewbery
commented at 12:16 PM on February 1, 2021:
member
#20749 is merged. I think next up is #20750.
@dongcarl - do you mind linking these bundle PRs in the PR description so it's easy to see the project progress at a glance?
dongcarl
commented at 4:51 PM on February 1, 2021:
contributor
qt/test: Reset chainman in ~ChainstateManager instead
There are some mutable, global state variables that are currently reset
by UnloadBlockIndex such as pindexBestHeader which should be cleaned up
whenever the ChainstateManager is unloaded/reset/destructed/etc.
Not cleaning them up leads to bugs like a use-after-free that happens
like so:
1. At the end of a test, ChainstateManager is destructed, which also
destructs BlockManager, which calls BlockManager::Unload to free all
CBlockIndexes in its BlockMap
2. Since pindexBestHeader is not cleaned up, it now points to an invalid
location
3. Another test starts to init, and calls LoadGenesisBlock, which calls
AddToBlockIndex, which compares the genesis block with an invalid
location
4. Cute puppies perish by the hundreds
Previously, for normal codepaths (e.g. bitcoind), we relied on the fact
that our program will be unloaded by the operating system which
effectively resets these variables. The one exception is in QT tests,
where these variables had to be manually reset.
Since now ChainstateManager is no longer a global, we can just put this
logic in its destructor to make sure that callers are always correct.
Over time, we should probably move these mutable global state variables
into ChainstateManager or CChainState so it's easier to reason about
their lifecycles.
62893af086
validation: Farewell, global Chainstate!6bda9eb9e2
ryanofsky
commented at 8:40 PM on February 2, 2021:
contributor
Light code review re-ACK6bda9eb9e26012fd75db079555d2f75e742c8c18. Changes since last review: rebase after #20749 merge, adding new chainstatemanager m_state commits, adding COMMITS-AFTER-THIS-ARE-NON-BASE commits, making other rebase updates including adding consts, updating peermanager method declarations, updating IsInitialBlockDownload calls.
MarcoFalke referenced this in commit f1239b70d1 on Feb 4, 2021
sidhujag referenced this in commit 203242fcfc on Feb 4, 2021
MarcoFalke referenced this in commit 599000903e on May 24, 2021
jamesob cross-referenced this on May 27, 2021 from issue assumeutxo by jamesob
MarcoFalke referenced this in commit f63fc53c2a on Jun 1, 2021
fanquake referenced this in commit a55904a80c on Jun 12, 2021
dongcarl
commented at 4:19 PM on June 16, 2021:
contributor
Thanks everyone for the reviews, seems like the last bundle is merged. I would like to call attention to #21766, where a few leftover improvements were collated.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC
