Add ChainstateManager, remove BlockManager global #17737

jamesob · 2019-12-12T19:24:32Z

Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

This changeset introduces ChainstateManager, which is responsible for creating and managing access to multiple chainstates. Until we allow chainstate creation from UTXO snapshots (next assumeutxo PR?) it's basically unnecessary, but it is a prerequisite for background IBD support.

Changes are also made to the initialization process to make use of g_chainman and thus clear the way for multiple chainstates being loaded on startup.

One immediate benefit of this change is that we no longer have the g_blockman global, but instead have the ChainstateManager inject a reference of its shared BlockManager into any chainstate it creates.

Another immediate benefit is that uses of ChainActive() and ChainstateActive() are now covered by lock annotations. Because use of g_chainman is annotated to require cs_main, these two functions subsequently follow.

Because of whitespace changes, this diff looks bigger than it is. E.g., 4813167 is most easily reviewed with

git show --color-moved=dimmed_zebra -w 4813167d98

DrahtBot · 2019-12-12T19:52:13Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

BIP-325: Signet [consensus] #18267 (BIP-325: Signet [consensus] by kallewoof)
qt: Use SynchronizationState enum for signals to GUI #18152 (qt: Use SynchronizationState enum for signals to GUI by hebasto)

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.

maflcko

ACK 0226408, did not read the tests 🔲

Show signature and timestamp

Signature:

Timestamp of file with hash c4bdc7880969f1e6719cf25c6f715a20e103935d1147395578a097e3179189ff -

src/validation.cpp

src/validation.h

src/validation.cpp

src/validation.h

src/validation.cpp

src/validation.h

src/init.cpp

ariard

Reviewed 6d14916 to 4813167.

Need to dig into parent PR a bit more before to review forward.

src/validation.h

src/validation.cpp

src/validation.h

src/validation.cpp

src/init.cpp

jonatack · 2020-01-16T19:18:22Z

Bringing in this IRC comment, if helpful:

"recent utxo snapshots for testing are available here: #15606 (comment)"

src/validation.cpp

src/validation.h

maflcko · 2020-01-24T20:20:09Z

ACK 16d20d9 only, will continue review later on 🚀

Show signature and timestamp

Signature:

Timestamp of file with hash 7e94e353f869ccf97b1947f28a0c0193a5e2cf8d984598592f1296d23f14168d -

src/init.cpp

maflcko

ACK e7bc059 only, have not reviewed later commits closely 🕧

Show signature and timestamp

Signature:

Timestamp of file with hash fe215ba014f43a48217fc4a11afdd4fb671d3ccd7e48f8bed366d49affae1228 -

src/init.cpp

src/net_processing.cpp

maflcko · 2020-01-28T19:01:28Z

ACK 1c17381, but it looks like the tests fail 🍢

Show signature and timestamp

Signature:

Timestamp of file with hash 6a38ed3fc7e109b015b25d8cd883d76b0d7c7f56f8c96dd80e10381889e51bbd -

ryanofsky

Code review ACK 1c17381. I did have a bunch of questions and suggestions below, but the suggestions aren't important, and if the questions don't point to real problems, I think this looks good to merge as is.

src/validation.cpp

src/validation.h

ryanofsky · 2020-01-28T19:57:34Z

src/validation.h

+ * *Background validation chainstate*: an IBD chainstate for which the
+ *    IBD process is happening in the background while use of the
+ *    active chainstate allows the rest of the system to function.


In commit "validation: introduce unused ChainstateManager" (10837ab)

The "Background validation chainstate" term seems a little awkward to me, where I guess every background validation chainstate is an IBD chainstate, but an IBD chainstate isn't a background validation chainstate until a separate snapshot chainstate has been loaded.

It seems like if you replaced the IsBackgroundValidationChainstate() method with plainer ChainStateManger::IsIBD(chainstate) and ChainStateManager::HasSnapshot() methods you could drop the "background validation chainstate" term and method and make things easier to follow.

Or if having the term is more useful, maybe call it a "background IBD chainstate" instead of "background validation chainstate" to be more obvious it's always refers to the IBD chain.

What gets kind of confusing here is that chainstates created from snapshots still technically go through IBD, since by the time the snapshot is actually used to create a chainstate, it's far enough behind the tip that it technically undergoes IBD (albeit an abbreviated one). So I think referring to what I now call the background validation chainstate as the IBD chainstate might be confusing in that sense.

But I agree with you that the naming here isn't ideal. Definitely open to changing this if you have other good ideas.

re: #17737 (comment)

I guess I see that but it would seem more like a reason to avoid the term "IBD chainstate" entirely.

Since chain state manager only has a snapshot chainstate and a non snapshot chainstate, I like the idea of just referring to the two chainstates with the same names consistently, and not muddying the water with other chainstate names and definitions. "IBD chainstate" is what you call the non-snapshot chainstate everywhere except in this one method and comment, which is my why first suggestion is delete "background validation chainstate" comment and have separate methods replacing IsBackgroundValidationChainstate. Switching to "Background IBD chainstate" was just a runner up suggestion.

I guess also I don't find the "IBD chainstate" name to be too confusing because I take IBD chainstate to mean chainstate generated entirely from IBD, not a state with some UTXOs that happened to come from IBD. But for another naming suggestion, maybe an alternative would be to use "snapshot chainstate" and "blocksonly chainstate" instead of "snapshot chainstate" and "IBD chainstate"

Partially (?) fixed by changing IsBackgroundValidationChainstate() to IsBackgroundIBD(). I think blocksonly is somewhat confusing because there'd be a confusing overlap with the P2P definition of blocksonly, which I think is different or unrelated.

src/validation.h

src/init.cpp

ryanofsky · 2020-01-28T20:52:03Z

src/test/validation_chainstatemanager_tests.cpp

+    BOOST_CHECK(c2.ActivateBestChain(_, chainparams, nullptr));
+
+    BOOST_CHECK(manager.IsSnapshotActive());
+    BOOST_CHECK(!manager.IsSnapshotValidated());


In commit "test: add basic tests for ChainstateManager" (997bf7f)

No test for the case where IsSnapshotValidated returns true, I guess. Understandable if it would make the test setup too complicated

src/init.cpp

src/validation.h

jamesob · 2020-01-28T22:11:29Z

Thanks for the good reviews! I'll be following up shortly with corresponding changes.

jamesob · 2020-01-29T15:39:49Z

au.chainman.6 -> au.chainman.9 (changes)

I've incorporated most if not all of the feedback from @MarcoFalke and @ryanofsky which has nicely reduced the size of the diff. Changes include:

fixing variable scoping, control flow, omissions in the init changes
acquiring ::cs_main directly in the chainstate convenience functions (instead of having to annotate calls with LOCK)
clarifying chainstate lifetimes in ChainstateManager documentation
fixing the unittests for QT use

This allows us to easily initialize multiple chainstates on startup in future commits. It retires the g_chainstate global in lieu of g_chainman.

Feedback incorporated from Russell Yanofsky. Co-authored-by: MarcoFalke <falke.marco@gmail.com>

I'd previously attempted to create a specialized lock for ChainstateManager, but it turns out that because that lock would be required for functions like ChainActive() and ChainstateActive(), it created irreconcilable lock inversions since those functions are used so broadly throughout the codebase. Instead, I'm just using cs_main to protect the contents of g_chainman. Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

fjahr

Code Review ACK abfc152

Had a few nits that can well be ignored or addressed in next PR in the assume-utxo series.

General note: I was not sure about some of the ChainstateManager API, some of the methods seemed like they could be private (IsSnapshotValidated() seems to be only used internally) and others are not used at all (IsSnapshotValidated and SnapshotBlockhash for example) so they could have been included in the next PRs when they are needed. But I can wait and see how they are used in the next PR. :)

fjahr · 2020-03-17T16:00:08Z

src/validation.cpp

@@ -1254,6 +1256,10 @@ void CChainState::InitCoinsDB(
    bool should_wipe,
    std::string leveldb_name)
 {
+    if (!m_from_snapshot_blockhash.IsNull()) {
+        leveldb_name += "_" + m_from_snapshot_blockhash.ToString();


nit: Do you anticipate more than one snapshot to be present? From the rest of the code it seems to me that this will not be the case. Then I think naming the db just "snapshot" or so is more intuitive.

fjahr · 2020-03-17T16:29:07Z

src/validation.cpp

+
+    to_modify.reset(new CChainState(snapshot_blockhash));
+
+    // Snapshot chainstates and initial IBD chaintates always become active.


typo: chaintates

fjahr · 2020-03-17T18:07:33Z

src/init.cpp

@@ -698,11 +698,17 @@ static void ThreadImport(std::vector<fs::path> vImportFiles)
    }

    // scan for better chains in the block chain database, that are not yet connected in the active best chain


nit: That comment is still relevant to the code below but because of the empty line it seems detached.

fjahr · 2020-03-17T19:55:29Z

Code Review Re-ACK abfc152

maflcko · 2020-03-18T15:08:03Z

re-ACK c9017ce 📙

Only changes since my last review:

Fix lock order in tests
Making the snapshot blockhash optional

Show signature and timestamp

Signature:

Timestamp of file with hash f61dcf850f62e4c3ff5ca63de6c032928f672c45311925c6f66a2497f5bc0c48 -

maflcko · 2020-03-18T15:10:08Z

@fjahr It looks like your commit hash is the same in the re-ACK

fjahr · 2020-03-18T15:14:19Z

@fjahr It looks like your commit hash is the same in the re-ACK

Ugh, how did I do that? I definitely re-reviewed the new code.

Code Review Re-ACK c9017ce

ariard

Code Review ACK c9017ce

Side-node: there is a g_chainman access in LoadBlockIndex (src/validation.cpp L4577), not an issue right now because caller already holds cs_main but if function is moved in the future a AssertLockHeld would be better.

ariard · 2020-03-19T17:39:21Z

src/validation.cpp

    assert(g_chainman.m_active_chainstate);
    return *g_chainman.m_active_chainstate;
 }

 CChain& ChainActive()
 {
+    LOCK(::cs_main);


c9017ce

Is this LOCK really necessary given we call ::ChainstateActive() just after ? Doesn't hurt because we use recursive_mutex but still..

re: #17737 (comment)

Is this LOCK really necessary given we call ::ChainstateActive() just after ? Doesn't hurt because we use recursive_mutex but still..

Agree it's definitely not necessary (but harmless)

ryanofsky

Code review ACK c9017ce. No changes since last review other than a straight rebase

ryanofsky · 2020-03-25T22:08:15Z

src/validation.h

@@ -894,7 +921,7 @@ class ChainstateManager
    void Reset();
 };

-extern ChainstateManager g_chainman;
+extern ChainstateManager g_chainman GUARDED_BY(::cs_main);


re: #17737 (comment)

If understand well the new lock model, g_chainman content is under cs_main but not CChainState. If yes, why some GetAll access in init.cpp don't use lock for access ?

I don't think this is true. Maybe it was resolved? You can add EXCLUSIVE_LOCKS_REQUIRED(cs_main) to the GetAll declaration, and it will only fail in test code using a local chain state manager not tied to the cs_main global

ryanofsky · 2020-03-25T22:21:33Z

src/validation.cpp

    assert(g_chainman.m_active_chainstate);
    return *g_chainman.m_active_chainstate;
 }

 CChain& ChainActive()
 {
+    LOCK(::cs_main);


re: #17737 (comment)

Is this LOCK really necessary given we call ::ChainstateActive() just after ? Doesn't hurt because we use recursive_mutex but still..

Agree it's definitely not necessary (but harmless)

jamesob · 2020-03-31T16:32:01Z

This tip has accumulated a good number of code ACKs, so I'm hesitant to address nits here. If anyone thinks that's the right move though I'm happy to.

Edit: and thanks for the reviews, everyone!

fanquake · 2020-04-01T04:14:10Z

If anyone thinks that's the right move though I'm happy to.

I think we can merge this after the 0.20 branch off.

c9017ce protect g_chainman with cs_main (James O'Beirne) 2b081c4 test: add basic tests for ChainstateManager (James O'Beirne) 4ae29f5 use ChainstateManager to initialize chainstate (James O'Beirne) 5b690f0 refactor: move RewindBlockIndex to CChainState (James O'Beirne) 89cdf4d validation: introduce unused ChainstateManager (James O'Beirne) 8e2ecfe validation: add CChainState.m_from_snapshot_blockhash (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: bitcoin#15606 Issue: bitcoin#15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal --- This changeset introduces `ChainstateManager`, which is responsible for creating and managing access to multiple chainstates. Until we allow chainstate creation from UTXO snapshots (next assumeutxo PR?) it's basically unnecessary, but it is a prerequisite for background IBD support. Changes are also made to the initialization process to make use of `g_chainman` and thus clear the way for multiple chainstates being loaded on startup. One immediate benefit of this change is that we no longer have the `g_blockman` global, but instead have the ChainstateManager inject a reference of its shared BlockManager into any chainstate it creates. Another immediate benefit is that uses of `ChainActive()` and `ChainstateActive()` are now covered by lock annotations. Because use of `g_chainman` is annotated to require cs_main, these two functions subsequently follow. Because of whitespace changes, this diff looks bigger than it is. E.g., 4813167 is most easily reviewed with ```sh git show --color-moved=dimmed_zebra -w 4813167 ``` ACKs for top commit: MarcoFalke: re-ACK c9017ce 📙 fjahr: Code Review Re-ACK c9017ce ariard: Code Review ACK c9017ce ryanofsky: Code review ACK c9017ce. No changes since last review other than a straight rebase Tree-SHA512: 3f250d0dc95d4bfd70852ef1e39e081a4a9b71a4453f276e6d474c2ae06ad6ae6a32b4173084fe499e1e9af72dd9007f4a8a375c63ce9ac472ffeaada41ab508

fab6b9d validation: Mark g_chainman DEPRECATED (MarcoFalke) fa1d97b validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke) fa24d49 validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke) fa84b1c validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke) fa05fdf net: Pass chainman into PeerLogicValidation (MarcoFalke) fa7b626 node: Add chainman alias for g_chainman (MarcoFalke) Pull request description: The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future. The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager. I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all. ACKs for top commit: ryanofsky: Code review ACK fab6b9d. Had to be rebased but still looks good Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f

Summary: ``` This parameter is unused, but in future commits will allow ChainstateManager to differentiate between chainstates created from a UTXO snapshot from those that weren't. ``` Partial backport (1/5) of core [[bitcoin/bitcoin#17737 | PR17737]]: bitcoin/bitcoin@8e2ecfe Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8537

Summary: ``` ChainstateManager is responsible for creating and managing multiple chainstates, and will provide a high-level interface for accessing the appropriate chainstate based upon a certain use. ``` Partial backport (2/5) of core [[bitcoin/bitcoin#17737 | PR17737]]: bitcoin/bitcoin@89cdf4d Depends on D8537. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8538

Summary: ``` This allows us to easily initialize multiple chainstates on startup in future commits. It retires the g_chainstate global in lieu of g_chainman. ``` Partial backport (3/5) of core [[bitcoin/bitcoin#17737 | PR17737]]: bitcoin/bitcoin@4ae29f5 Includes a fix to `qt/test/apptests.cpp` from the next commit to unbreak the bitcoin-qt tests: bitcoin/bitcoin@2b081c4#diff-41e328d9ba5879d2bd026a700743339b5b300b9bee50844795ef9e27f60bdfb8R85 Depends on D8538. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8539

Summary: Partial backport (4/5) of core [[bitcoin/bitcoin#17737 | PR17737]]: bitcoin/bitcoin@2b081c4 Depends on D8539. Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8540

Summary: ``` I'd previously attempted to create a specialized lock for ChainstateManager, but it turns out that because that lock would be required for functions like ChainActive() and ChainstateActive(), it created irreconcilable lock inversions since those functions are used so broadly throughout the codebase. Instead, I'm just using cs_main to protect the contents of g_chainman. ``` Completes backport (5/5) of core [[bitcoin/bitcoin#17737 | PR17737]]: bitcoin/bitcoin@c9017ce The lock change in `wallet_tests.cpp` is not needed due to out of order backports. Depends on D8540. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8541

Bitcoin Core PR: bitcoin/bitcoin#17737 Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: bitcoin/bitcoin#15606 Issue: bitcoin/bitcoin#15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal --- This changeset introduces `ChainstateManager`, which is responsible for creating and managing access to multiple chainstates. Until we allow chainstate creation from UTXO snapshots (next assumeutxo PR?) it's basically unnecessary, but it is a prerequisite for background IBD support. Changes are also made to the initialization process to make use of `g_chainman` and thus clear the way for multiple chainstates being loaded on startup. One immediate benefit of this change is that we no longer have the `g_blockman` global, but instead have the ChainstateManager inject a reference of its shared BlockManager into any chainstate it creates. Another immediate benefit is that uses of `ChainActive()` and `ChainstateActive()` are now covered by lock annotations. Because use of `g_chainman` is annotated to require cs_main, these two functions subsequently follow. Because of whitespace changes, this diff looks bigger than it is. E.g., bitcoin/bitcoin@4813167 is most easily reviewed with ```sh git show --color-moved=dimmed_zebra -w bitcoin/bitcoin@4813167 ```

fanquake added the Validation label Dec 12, 2019

jamesob force-pushed the 2019-12-au.chainman branch 2 times, most recently from ce74d24 to 2328317 Compare December 13, 2019 15:12

maflcko approved these changes Dec 13, 2019

View reviewed changes

ariard reviewed Dec 16, 2019

View reviewed changes

jamesob force-pushed the 2019-12-au.chainman branch 2 times, most recently from 7eac28c to 9a6fb54 Compare December 18, 2019 21:14

jamesob force-pushed the 2019-12-au.chainman branch from 9a6fb54 to 4cab02a Compare January 2, 2020 22:07

ariard mentioned this pull request Jan 3, 2020

Remove RewindBlockIndex logic #17862

Closed

DrahtBot added the Needs rebase label Jan 13, 2020

jamesob force-pushed the 2019-12-au.chainman branch from 4cab02a to ec64f5c Compare January 13, 2020 18:28

DrahtBot removed the Needs rebase label Jan 13, 2020

maflcko reviewed Jan 24, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

maflcko reviewed Jan 24, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

maflcko reviewed Jan 24, 2020

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

jamesob force-pushed the 2019-12-au.chainman branch from ec64f5c to 1c17381 Compare January 27, 2020 20:15