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.
DrahtBot added the label
Consensus
on Oct 15, 2020
DrahtBot added the label
GUI
on Oct 15, 2020
DrahtBot added the label
Mempool
on Oct 15, 2020
DrahtBot added the label
Mining
on Oct 15, 2020
DrahtBot added the label
P2P
on Oct 15, 2020
DrahtBot added the label
RPC/REST/ZMQ
on Oct 15, 2020
DrahtBot added the label
UTXO Db and Indexes
on Oct 15, 2020
DrahtBot added the label
Validation
on Oct 15, 2020
DrahtBot added the label
Wallet
on Oct 15, 2020
DrahtBot
commented at 9:03 pm on October 15, 2020:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#21148 (Split orphan handling from net_processing into txorphanage by ajtowns)
#21146 ([WIP] validation/coins: limit MemPoolAccept access to coins cache by glozow)
#21142 (fuzz: Add tx_pool fuzz target by MarcoFalke)
#21121 ([test] Small unit test improvements, including helper to make mempool transaction by amitiuttarwar)
#21090 (Default to NODE_WITNESS in nLocalServices by dhruv)
#21062 (refactor: return MempoolAcceptResult from ATMP by glozow)
#21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
#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.
laanwj
commented at 10:01 am on October 16, 2020:
member
Concept ACK.
jamesob
commented at 4:03 pm on October 16, 2020:
member
Concept ACK - really like the direction of this work.
ryanofsky
commented at 9:41 pm on October 22, 2020:
In commit “validation: Move LookupBlockIndex to BlockManager” (8615dbd1250c2dddec2f54bf80e2a1b7013bc871)
Commit doesn’t compile, not sure if this is intended. Could make this a wrapper function until after the scripted diff
dongcarl
commented at 7:20 pm on November 4, 2020:
Done!
in
src/validation.h:423
in
8dbedcb3bfoutdated
436@@ -440,6 +437,9 @@ class BlockManager
437438 CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
439440+ /** 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);
ryanofsky
commented at 9:47 pm on October 22, 2020:
In commit “validation: Move FindForkInGlobalIndex to BlockManager” (8dbedcb3bf123411ebcd9ccb7521b44cacc8e385)
re: “Let me know if this should be changed”, this makes sense to me. A possible alternative might be to make this a chain method, too
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:
member
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 9:58 pm on October 23, 2020:
In commit “miner: Pass in blockman to ::RegenerateCommitments” (a1acd859bd06ead8fb38fac2d10340a658d7a061)
If this lock is needed now can RegenerateCommitments be annotated to require cs_main? Also, why is this lock needed now if it wasn’t needed before?
in
src/node/coinstats.cpp:57
in
a9e77bb0a7outdated
53@@ -54,16 +54,18 @@ static void ApplyStats(CCoinsStats& stats, std::nullptr_t, const uint256& hash,
5455 //! Calculate statistics about the unspent transaction output set
56 template <typename T>
57-static bool GetUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point)
58+static bool GetUTXOStats(CChainState& chainstate, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point)
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 }
563564-/** 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:
member
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:
member
0Thread1"b-test" received signal SIGABRT, Aborted. 1__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 251../sysdeps/unix/sysv/linux/raise.c: No such file or directory. 3(gdb) bt
4[#0](/bitcoin-bitcoin/0/) __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 5[#1](/bitcoin-bitcoin/1/) 0x00007ffff337f8b1 in __GI_abort () at abort.c:79 6[#2](/bitcoin-bitcoin/2/) 0x0000555555ac21ab in __sanitizer::Abort() () 7[#3](/bitcoin-bitcoin/3/) 0x0000555555abf4d8 in __sanitizer::Die() () 8[#4](/bitcoin-bitcoin/4/) 0x0000555555aa175d in __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) () 9[#5](/bitcoin-bitcoin/5/) 0x0000555555aa1f48 in __asan_report_load4 ()10[#6](/bitcoin-bitcoin/6/) 0x00005555575f4d03 in base_uint<256u>::CompareTo (this=0x60d00016c958, b=...) at arith_uint256.cpp:11211[#7](/bitcoin-bitcoin/7/) 0x000055555615213a in operator< (a=..., b=...) at ./arith_uint256.h:22012[#8](/bitcoin-bitcoin/8/) 0x0000555556a710e4 in BlockManager::AddToBlockIndex (this=0x611000129720, block=...) at validation.cpp:318313[#9](/bitcoin-bitcoin/9/) 0x0000555556a9466d in CChainState::LoadGenesisBlock (this=0x612000101440, chainparams=...) at validation.cpp:463114[#10](/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:18315[#11](/bitcoin-bitcoin/11/) 0x0000555555af39df in RPCNestedTests::rpcNestedTests (this=0x7fffffffded0) at qt/test/rpcnestedtests.cpp:3616[#12](/bitcoin-bitcoin/12/) 0x0000555555b990be in RPCNestedTests::qt_static_metacall (_o=0x7fffffffded0, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffffd2c0)17 at qt/test/moc_rpcnestedtests.cpp:7018[#13](/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.519[#14](/bitcoin-bitcoin/14/) 0x00007ffff66dc79a in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.520[#15](/bitcoin-bitcoin/15/) 0x00007ffff66dd4f0 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.521[#16](/bitcoin-bitcoin/16/) 0x00007ffff66dda61 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.522[#17](/bitcoin-bitcoin/17/) 0x00007ffff66de02b in QTest::qExec(QObject*, int, char**) () from /usr/lib/x86_64-linux-gnu/libQt5Test.so.523[#18](/bitcoin-bitcoin/18/) 0x0000555555afead1 in main (argc=1, argv=0x7fffffffe298) at qt/test/test_main.cpp:85
ryanofsky
commented at 5:01 pm on October 26, 2020:
member
Following change partially reverting 50f680b402a84b605aa9b9aa58cb571ef32e190e seems to fix the error:
0--- a/src/qt/test/apptests.cpp
1+++ b/src/qt/test/apptests.cpp
2@@ -84,6 +84,10 @@ void AppTests::appTests()
3 // Reset global state to avoid interfering with later tests.
4 LogInstance().DisconnectTestLogger();
5 AbortShutdown();
6+ {
7+ LOCK(cs_main);
8+ ::pindexBestHeader = nullptr;
9+ }
10 }
1112 //! Entry point for BitcoinGUI tests.
Error is happening here:
0[#8](/bitcoin-bitcoin/8/) 0x0000555556a710e4 in BlockManager::AddToBlockIndex (this=0x611000129860, block=...) at validation.cpp:3183
13183 if (pindexBestHeader == nullptr || pindexBestHeader->nChainWork < pindexNew->nChainWork)
dongcarl force-pushed
on Oct 27, 2020
dongcarl force-pushed
on Oct 27, 2020
dongcarl force-pushed
on Oct 28, 2020
practicalswift
commented at 6:59 am on October 28, 2020:
contributor
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.
dongcarl force-pushed
on Nov 2, 2020
dongcarl force-pushed
on Nov 2, 2020
in
src/validation.h:422
in
5d587c26afoutdated
436@@ -440,6 +437,9 @@ class BlockManager
437438 CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
439440+ /** 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);
442443+ /**
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.
in
src/validation.cpp:3379
in
c6a46621fcoutdated
3470@@ -3472,14 +3471,15 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
3471 }
34723473 //! Returns last CBlockIndex* that is a checkpoint
3474-static CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
3475+CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
3476 {
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
556557 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);
troygiorshev
commented at 6:41 am on November 4, 2020:
contributor
Concept ACK, excited for this to be sliced into smaller PRs for deeper review!
dongcarl force-pushed
on Nov 5, 2020
dongcarl force-pushed
on Nov 5, 2020
dongcarl
commented at 7:01 pm on November 5, 2020:
member
Reviewers: Just split off the first few fix commits in a non-draft PR #20323!
in
src/validation.h:172
in
6a4f33ad98outdated
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?
Maybe drag comments here ? Can’t remember the style policy on this.
ariard
commented at 1:54 am on November 8, 2020:
member
Approach ACK, sound work. I had a look on everything, though more lightly on tests-touching commits, I’ll try to review the first split-off soon.
ryanofsky approved
ryanofsky
commented at 4:13 pm on November 10, 2020:
member
Code review ACK912512a27c1caa59eb928e10f1022d1b33c2d711. Just rebase and responses to various suggestions since last review
in
src/validation.h:187
in
f75c7ca1cboutdated
191@@ -192,7 +192,7 @@ void PruneBlockFilesManual(int nManualPruneHeight);
192 /** (try to) add transaction to memory pool
193 * plTxnReplaced will be appended to with all transactions replaced from mempool
194 * @param[out] fee_out optional argument to return tx fee to the caller **/
195-bool AcceptToMemoryPool(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
196+bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
ryanofsky
commented at 4:36 pm on November 10, 2020:
In commit “validation: Pass in chainstate to ::AcceptToMemoryPool” (f75c7ca1cb46eb5824c3c5db666143f9a74c0eda)
This commit also doesn’t compile, looks like there are calls in net_processing to update
MarcoFalke referenced this in commit
a3586d5920
on Dec 9, 2020
ryanofsky
commented at 2:37 pm on December 18, 2020:
member
#20323 | tests: Create or use existing properly initialized NodeContexts
#20323 is now merged, so it would be good to post a followup or rebase this
dongcarl force-pushed
on Dec 18, 2020
dongcarl
commented at 10:13 pm on December 18, 2020:
member
Pushed 912512a27c1caa59eb928e10f1022d1b33c2d711 -> abcde917ddec206cd938d6d418094d5db645f740 | Thanks ryanofsky for the reminder!
Rebased over current master
All commits should now compile
ryanofsky approved
ryanofsky
commented at 2:27 pm on December 22, 2020:
member
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:
member
@jnewbery Yup, got a bit caught up on other things but will open that first PR today!
Sjors
commented at 2:21 pm on December 30, 2020:
member
Concept ACK. Thanks for chopping this into smaller PR’s.
dongcarl referenced this in commit
12e90007f6
on Jan 20, 2021
dongcarl referenced this in commit
b8ac27c636
on Jan 20, 2021
dongcarl referenced this in commit
b396467053
on Jan 20, 2021
MarcoFalke referenced this in commit
1f45e85509
on Jan 21, 2021
sidhujag referenced this in commit
c0b3bb2d2a
on Jan 21, 2021
remyers referenced this in commit
3ccc0834cb
on Jan 26, 2021
dongcarl force-pushed
on Jan 28, 2021
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:
member
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
dongcarl force-pushed
on Feb 2, 2021
ryanofsky approved
ryanofsky
commented at 8:40 pm on February 2, 2021:
member
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
828bb776d2
on Feb 20, 2021
alizeyni approved
alizeyni
commented at 2:47 pm on February 27, 2021:
none
A
fanquake deleted a comment
on Feb 27, 2021
laanwj referenced this in commit
702cfc8c53
on Mar 4, 2021
laanwj referenced this in commit
767bb7d5c5
on Mar 11, 2021
MarcoFalke referenced this in commit
0dd7b23489
on Apr 17, 2021
MarcoFalke referenced this in commit
599000903e
on May 24, 2021
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:
member
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.
dongcarl closed this
on Jun 16, 2021
Fabcien referenced this in commit
ab4c27192a
on Jan 19, 2022
MarcoFalke referenced this in commit
98e9d8e8e2
on Mar 24, 2022
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: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
