Add ChainstateManager, remove BlockManager global

jamesob commented at 7:24 pm on December 12, 2019: member

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., 4813167d98 is most easily reviewed with

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

fanquake added the label Validation on Dec 12, 2019

fanquake added this to the "In progress" column in a project

DrahtBot commented at 7:52 pm on December 12, 2019: 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:

#18267 (BIP-325: Signet [consensus] by kallewoof)
#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.

jamesob force-pushed on Dec 12, 2019

jamesob force-pushed on Dec 13, 2019

in src/validation.cpp:4942 in 6d1491603f outdated

4907@@ -4908,6 +4908,13 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
4908     assert(nNodes == forward.size());
4909 }
4910 
4911+std::string CChainState::ToString()
4912+{
4913+    CBlockIndex* tip = m_chain.Tip();

MarcoFalke commented at 3:51 pm on December 13, 2019:

in commit 6d1491603f6dfb9f755e60d77685635c35d5a48b:

Is m_chain.SetTip assumed to be under cs_main? If yes, this should say ... = WITH_LOCK(cs_main, return m_chain.Tip());

in src/validation.cpp:4945 in 6d1491603f outdated

4907@@ -4908,6 +4908,13 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
4908     assert(nNodes == forward.size());
4909 }
4910 
4911+std::string CChainState::ToString()
4912+{
4913+    CBlockIndex* tip = m_chain.Tip();
4914+    return strprintf("Chainstate [%s] @ height %d",
4915+        tip ? tip->nHeight : -1, tip ? tip->GetBlockHash().ToString() : "null");

MarcoFalke commented at 3:52 pm on December 13, 2019:

in commit 6d14916:

I think the arguments are switched?

MarcoFalke commented at 3:57 pm on December 13, 2019:

in commit 6b62d2f15397226b273cb87f56b8978404fd415f:

Now the arguments are correct. Might just squash the commits?

in src/validation.h:619 in 6b62d2f153 outdated

610@@ -611,6 +611,13 @@ class CChainState {
611     //! @see CChain, CBlockIndex.
612     CChain m_chain;
613 
614+    /**
615+     * The blockhash which is the base of the snapshot this chainstate was created from.
616+     *
617+     * IsNull() if this chainstate was not created from a snapshot.
618+     */
619+    uint256 m_from_snapshot_blockhash{};

MarcoFalke commented at 3:55 pm on December 13, 2019:

in commit 6b62d2f15397226b273cb87f56b8978404fd415f:

Should be const to ensure at compile time that we know whether a chainstate was created from a snapshot?

in src/validation.h:586 in 6b62d2f153 outdated

582@@ -583,7 +583,7 @@ class CChainState {
583 
584 public:
585     CChainState(BlockManager& blockman) : m_blockman(blockman) {}
586-    CChainState();
587+    CChainState(uint256 from_snapshot_blockhash = uint256());

MarcoFalke commented at 3:56 pm on December 13, 2019:

in commit 6b62d2f15397226b273cb87f56b8978404fd415f:

constructors (especially single arg ones) should be explicit. Otherwise any hash can be passed into a function that accepts a chainstate. Hmmm

in src/validation.cpp:1248 in 6b62d2f153 outdated

1244@@ -1245,7 +1245,9 @@ void CoinsViews::InitCache()
1245 
1246 // NOTE: for now m_blockman is set to a global, but this will be changed
1247 // in a future commit.
1248-CChainState::CChainState() : m_blockman(g_blockman) {}
1249+CChainState::CChainState(uint256 from_snapshot_blockhash) :

MarcoFalke commented at 3:58 pm on December 13, 2019:

style-nit in 6b62d2f15397226b273cb87f56b8978404fd415f:

The colon goes into a new line according to our clang-format

in src/validation.h:885 in 6a629cfd02 outdated

828+    //!                                 is based on a snapshot.
829+    CChainState& InitializeChainstate(
830+        bool activate = true, const uint256& snapshot_blockhash = uint256());
831+
832+    //! Get all chainstates currently being used.
833+    std::vector<CChainState*> GetAll();

MarcoFalke commented at 4:07 pm on December 13, 2019:

in commit 6a629cfd02:

Wouldn’t this need to happen under some chainstate manager lock? Otherwise the chainstate might point to uninitialized memory in case of a race where " Its contents will be freed when background validation of the snapshot has completed."

promag commented at 2:14 am on December 14, 2019:

Agree. Same question for other methods.

jamesob commented at 3:23 pm on December 17, 2019:

Thanks for raising this point. Initially, I had all pointers in the ChainstateManager protected by a private m_cs lock. I later found that there were irreconcilable lock inversions that happened during initialization - because a function like ChainActive() would necessarily have to acquire ChainstateManager.m_cs, there were many tricky inversions that happened between that lock and others like cs_main since ChainActive() is used so pervasively throughout the code.

As a result, I removed the chainman-specific lock and replaced it with cs_main in a later commit (https://github.com/bitcoin/bitcoin/pull/15606/commits/f1fa917e2081dd8d1847cbb23a0eae5a2cded2e5). Because ChainActive() isn’t formally annotated as being protected by cs_main, and use of ChainstateManager always happens while under the protection of cs_main, I figured I could forgo adding those annotations now. But now that the point has been raised, I think it’s probably worth addressing in this PR.

So I’ll add the cs_main annotations to the relevant ChainManager parts. Though of course this will entail annotating ChainActive() and there will come with many other (mostly trivial) changes.

jamesob commented at 3:30 pm on December 17, 2019:

Also, it’s worth noting that the comment you quote is out of date (my fault!). We don’t actually free the contents of the background validation chainstate immediately when validation completes - we do it during shutdown when g_chainman.Reset() is called.

The reason I don’t do it immediately after validation is complete is so that we can be sure that no other parts of the code are still making reference to that pointer. This also relates to your question below about IsSnapshotValidated() - the extra piece of state (m_snapshot_validated) is used to track whether or not we need to do this cleanup.

in src/validation.h:902 in 6a629cfd02 outdated

840+    bool IsSnapshotActive() const;
841+
842+    uint256 SnapshotBlockhash() const { return m_snapshot_blockhash; }
843+
844+    //! Is there a snapshot in use and has it been fully validated?
845+    bool IsSnapshotValidated() const { return m_snapshot_validated; }

MarcoFalke commented at 4:11 pm on December 13, 2019:

in commit 6a629cfd029071:

How and why is this different from return !m_ibd_chainstate?

ariard commented at 10:58 pm on December 16, 2019:

Agree, could be removed in GetAll, if idea is to garbage collect m_ibd_chainstate when foreground utxo get validated

jamesob commented at 3:31 pm on December 17, 2019:

See my comment above for background: #17737 (review). Perhaps we can do this without necessarily needing to maintain extra state (m_snapshot_validated) for it. I’ll look into it.

in src/validation.cpp:5181 in 6a629cfd02 outdated

5140+
5141+    to_modify.reset(new CChainState(snapshot_blockhash));
5142+
5143+    if (activate) {
5144+        LogPrintf("Switching active chainstate to %s\n", snapshot_blockhash.ToString());
5145+        m_active_chainstate = to_modify.get();

MarcoFalke commented at 4:16 pm on December 13, 2019:

in commit 6a629cfd02907162ce:

I wonder if this method needs to happen under some kind of lock. Otherwise there might be a race in validation or net processing where the chainstate is switched down under during a call.

jamesob commented at 3:32 pm on December 17, 2019:

In practice all usages of this happen under cs_main, but as I said above I’ll do explicit annotations.

in src/validation.h:842 in 6a629cfd02 outdated

837+    int ActiveHeight() const { return ActiveChain().Height(); }
838+    CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
839+
840+    bool IsSnapshotActive() const;
841+
842+    uint256 SnapshotBlockhash() const { return m_snapshot_blockhash; }

MarcoFalke commented at 4:21 pm on December 13, 2019:

in commit 6a629cfd0290716:

How and why is this different from return m_active_chainstate->m_snapshot_blockhash;?

jamesob commented at 3:32 pm on December 17, 2019:

I think you’re right here, I’ll remove this.

in src/init.cpp:1514 in 4813167d98 outdated

1466@@ -1463,8 +1467,7 @@ bool AppInitMain(NodeContext& node)
1467             bool is_coinsview_empty;
1468             try {
1469                 LOCK(cs_main);
1470-                // This statement makes ::ChainstateActive() usable.

MarcoFalke commented at 4:29 pm on December 13, 2019:

in commit 4813167d989c:

Is this comment no longer relevant?

jamesob commented at 5:21 pm on January 2, 2020:

I think it was incorrect since we haven’t initialized the coins views at that point.

MarcoFalke approved

MarcoFalke commented at 4:31 pm on December 13, 2019: member

ACK 0226408999, did not read the tests 🔲

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 0226408999, did not read the tests 🔲
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
 7pUixgQv9ELRmdepH+VtXfc0mvkvHjZZAGrvdNEAGLqlEK+nbDy0YtUnZtECto3aJ
 8K/fFg7heGCu3/pRgx25GGDnaRNrOPc+q56kWXUjSS7lweiyx1JYw4V8sVlgas3Ih
 94Cxbgy3GxmPL88e1mRBpqBdyKxGi3cPxpPjew63/sllo9ZDPBMsA5g0NcpUwIC2H
10TMgdLJbffnHCgCKkIH5/lXah8W7fF6Tt/wyzK4RaijwcxjVrdFheJTv0kLiBJDAG
11AmHMVhKDQTBeZFn5CydqeupqV90u7rdFd/S6MliuawQGwtlobeqKqs7jxeR23+Yx
12AfAAk6nKLhQFQ5x998j/rGamIcYO7RCRE0be0dZC1KGdhTwIY1LOsSKHlJAn64Ik
13XKNJXDbg8o1vnBgAEmzslunwooaOoF2oSyEB4crt++JNYLl38imrOdZ+0kdlQ/BC
14GCRJWD/y2WSNRxCrkyJZ6uxTkUf7KmEbh8qwOavXby2DR2rOSOGE9nM9ewXgqbsK
15BDO3176X
16=Ygdp
17-----END PGP SIGNATURE-----

Timestamp of file with hash c4bdc7880969f1e6719cf25c6f715a20e103935d1147395578a097e3179189ff -

in src/validation.h:780 in 6a629cfd02 outdated

775+ * A few quick definitions:
776+ *
777+ * *IBD chainstate*: a chainstate populated via initial block download.
778+ *
779+ * *Snapshot chainstate*: a chainstate populated by loading in an
780+ *    assumed-valid UTXO snapshot.

ariard commented at 10:45 pm on December 16, 2019:

6b62d2f

Even if I understand what you mean by an assumed-valid UTXO, I think it may confuse readers try to learn what’s the difference is between assume-utxo and assume-valid. Wouldn’t be shock by a simple assume-utxo snapshot.

jamesob commented at 5:22 pm on January 2, 2020:

Fixed, thanks.

in src/validation.h:807 in 6a629cfd02 outdated

786+ *
787+ * *Background validation chainstate*: an IBD chainstate for which the
788+ *    IBD process is happening in the background while use of the
789+ *    active chainstate allows the rest of the system to function.
790+ *
791+ * *Validated chainstate*: a chainstate whose validity is not assumed.

ariard commented at 10:48 pm on December 16, 2019:

6b62d2f

It’s ambiguous here, do you mean a chainstate for which validity is assumed ? Overall, what’s the definition of validity w.r.t to chainstates types ? I.e a IBD chainstate is always considered valid (because all consensus checks are done by us) or only when it’s out-of-IBD? What’s about a assume-valid IBD chainstate ?

ryanofsky commented at 8:23 pm on January 28, 2020:

In commit “validation: introduce unused ChainstateManager” (10837ab1d33e126c4f36221cb5077038ca65f00d)

re: #17737 (review)

It’s ambiguous here, do you mean a chainstate for which validity is assumed

Agree “whose validity is not assumed” could be misinterpreted as “that isn’t necessarily valid” instead of “that has been validated locally”

Maybe define “Validated chainstate” as something like “the most work chainstate which has been validated locally. This will be the snapshot chainstate if a snapshot was loaded and all blocks up to the snapshot starting point have been downloaded and validated, otherwise it will be the IBD chainstate.”

jamesob commented at 3:40 pm on January 29, 2020:

Fixed, using Russ’ definition.

in src/validation.cpp:5144 in 6a629cfd02 outdated

5139+        snapshot_blockhash.IsNull() ? m_ibd_chainstate : m_snapshot_chainstate);
5140+
5141+    to_modify.reset(new CChainState(snapshot_blockhash));
5142+
5143+    if (activate) {
5144+        LogPrintf("Switching active chainstate to %s\n", snapshot_blockhash.ToString());

ariard commented at 10:50 pm on December 16, 2019:

6b62d2f

Logger is weird, given than default activate is true, no blockhash is going to be display, maybe snapshot_blockhash.IsNull ? "an IBD one" : snapshot_blockhash.ToString()

jamesob commented at 5:23 pm on January 2, 2020:

Fixed, thanks.

in src/validation.h:830 in 6a629cfd02 outdated

825+    //!
826+    //! @param[in] activate   If true, make this new chainstate the active one.
827+    //! @param[in] snapshot_blockhash   If given, signify that this chainstate
828+    //!                                 is based on a snapshot.
829+    CChainState& InitializeChainstate(
830+        bool activate = true, const uint256& snapshot_blockhash = uint256());

ariard commented at 10:52 pm on December 16, 2019:

6b62d2f

I think consequences of activating a non valid chainstate by mistake can be pretty bad, would be better to have default to false in case of buggy call of this method.

jamesob commented at 3:34 pm on December 17, 2019:

I think this method is called so infrequently and has such major effects that it’d be hard to accidentally do anything one way or the other, and the results would be immediately obvious and buggy. But to your point, maybe I should remove the default parameters so that everything has to be explicit.

jamesob commented at 5:24 pm on January 2, 2020:

Removed the default parameters, thanks.

in src/validation.h:850 in 6a629cfd02 outdated

845+    bool IsSnapshotValidated() const { return m_snapshot_validated; }
846+
847+    //! @returns true if this chainstate is being used to validate an active
848+    //!          snapshot in the background.
849+    bool IsBackgroundValidationChainstate(CChainState* chainstate) const
850+    {

ariard commented at 10:59 pm on December 16, 2019:

6a629cf

nit: you can move body function to validation.cpp, like IsSnapshotActive

in src/validation.cpp:5228 in 6a629cfd02 outdated

5177+
5178+void ChainstateManager::Reset()
5179+{
5180+    m_ibd_chainstate.reset();
5181+    m_snapshot_chainstate.reset();
5182+    m_active_chainstate = nullptr;

ariard commented at 11:00 pm on December 16, 2019:

6a629cf

Maybe reset m_snapshot_blockhash to null and m_snapshot_validated to false?

in src/init.cpp:1569 in 175e5704ad outdated

1565@@ -1566,7 +1566,7 @@ bool AppInitMain(NodeContext& node)
1566                 // It both disconnects blocks based on ::ChainActive(), and drops block data in
1567                 // BlockIndex() based on lack of available witness data.
1568                 uiInterface.InitMessage(_("Rewinding blocks...").translated);
1569-                if (!RewindBlockIndex(chainparams)) {
1570+                if (!::ChainstateActive().RewindBlockIndex(chainparams)) {

ariard commented at 11:08 pm on December 16, 2019:

175e570

I’m not sure if RewindBlockIndex is still relevant.

Following Suhas comment #8149 (comment), it was added to let upgrading nodes after segwit activation redownload consensus data without redownloading the whole blockchain.

According to sipa in #15402, the rewinding logic “is probably becoming increasingly unnecessary, as very few pre-0.13.1 nodes remain that would care to upgrade”.

Further, it doesn’t make sense for the assumed-valid UTXO chainstate to rewind until first insufficiently-validated block, all utxo hash are going to be post segwit. It only makes sense for the ibd chainstate.

jamesob commented at 5:24 pm on January 2, 2020:

Agree this is worth investigating, but probably not within this PR. I’d rather just preserve existing behavior.

ariard commented at 10:54 pm on January 3, 2020:

FYI, #17862. I had a second look, I think it’s safe to maintain logic as it is for this PR even it should be never triggered because both chainstates will belong to segwit nodes.

in src/init.cpp:1658 in 4813167d98 outdated

1657 
1658-                    if (!CVerifyDB().VerifyDB(chainparams, &::ChainstateActive().CoinsDB(), gArgs.GetArg("-checklevel", DEFAULT_CHECKLEVEL),
1659-                                  gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS))) {
1660-                        strLoadError = _("Corrupted block database detected").translated;
1661-                        break;
1662+                for (CChainState* chainstate : g_chainman.GetAll()) {

ariard commented at 11:12 pm on December 16, 2019:

4813167

Just iterate for the VerifyDB, pruning and best block seen by RPC maybe should be common to both chainstates ?

jamesob commented at 10:06 pm on January 2, 2020:

Good point - I’m changing this to only report a new tip to RPC for the active chain. The tip/time check is very cheap and doesn’t hurt to be done for both. There isn’t anything that actually happens for pruning here other than a possible log statement, so I think leaving that as-is should be fine. Thanks!

ariard commented at 10:46 pm on February 28, 2020:

f349e96

Still I would prefer only iterate for VerifyDB because multiple log/ui messages may be confusing and adding failed_verification, it’s more control flow. Okay for now, but eager to clean a bit this part aroud parameterize of VerifyDB

ariard commented at 11:13 pm on December 16, 2019: member

Reviewed 6d14916 to 4813167.

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

jamesob force-pushed on Dec 17, 2019

jamesob force-pushed on Dec 18, 2019

jamesob force-pushed on Jan 2, 2020

DrahtBot added the label Needs rebase on Jan 13, 2020

jamesob force-pushed on Jan 13, 2020

DrahtBot removed the label Needs rebase on Jan 13, 2020

laanwj added this to the "Blockers" column in a project

jonatack commented at 7:18 pm on January 16, 2020: member

Bringing in this IRC comment, if helpful:

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

in src/validation.cpp:5181 in 16d20d91b4 outdated

5179+
5180+    to_modify.reset(new CChainState(snapshot_blockhash));
5181+
5182+    if (activate) {
5183+        LogPrintf("Switching active chainstate to %s\n", to_modify->ToString());
5184+        m_active_chainstate = to_modify.get();

MarcoFalke commented at 7:48 pm on January 24, 2020:

in commit 16d20d91b43b1c30333fe14272d69cfb543229b3:

You mention that “// If a snapshot chainstate exists, it will always be our active.”

However, then you provide code here to activate a ibd chainstate even though a snapshot chainstate might exist. This is confusing and the code should probably not provide such a backdoor.

MarcoFalke commented at 7:54 pm on January 24, 2020:

Maybe a check like if (m_active_chainstate && to_modify != m_snapshot_chainstate) LogPrintf("Error!"); else ... or similar could prevent that?

jamesob commented at 8:15 pm on January 27, 2020:

Yep, this is a really good point. Fixed it to only allow a single activation sequence.

ryanofsky commented at 4:40 pm on January 28, 2020:

Yep, this is a really good point. Fixed it to only allow a single activation sequence.

If you’re going to do this I think it would be clearer as:

0LogPrintf("Switching active chainstate to %s\n", to_modify->ToString());
1assert (is_snapshot || !m_active_chainstate);
2m_active_chainstate = to_modify.get();

In current version, it’s confusing to mentally reconcile the “always become active” comment with the bool condition and the “unexpected activation” error string below, instead of having a one shot assertion

in src/validation.cpp:5192 in 16d20d91b4 outdated

5187+    return *to_modify.get();
5188+}
5189+
5190+CChain& ChainstateManager::ActiveChain() const
5191+{
5192+    return m_active_chainstate->m_chain;

MarcoFalke commented at 8:05 pm on January 24, 2020:

in commit 16d20d91b43b1c30333fe14272d69cfb543229b3:

Should assert m_active_chainstate first?

in src/validation.h:830 in 16d20d91b4 outdated

825+    //! to cautiously avoid a case where some other part of the system is still
826+    //! using this pointer (e.g. net_processing).
827+    std::unique_ptr<CChainState> m_ibd_chainstate;
828+
829+    //! A chainstate initialized on the basis of a UTXO snapshot. If this is
830+    //! non-null, it is always our active chainstate unless proven invalid.

MarcoFalke commented at 8:10 pm on January 24, 2020:

in commit 16d20d9:

“proven invalid” sounds odd. In normal operation (with the assumevalid hash baked in) an assumevalid chainstate can never be invalid. And if it is, there is no way to recover from this, right? So proving that one or the other chainstate is invalid is impossible with conflicting information. The only way to handle this is to crash the node immediately. I’d say to remove the “unless proven invalid” phrase here.

jamesob commented at 7:47 pm on January 27, 2020:

Yep, good point.

MarcoFalke commented at 8:20 pm on January 24, 2020: member

ACK 16d20d91b43b1c30333fe14272d69cfb543229b3 only, will continue review later on 🚀

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 16d20d91b43b1c30333fe14272d69cfb543229b3 only, will continue review later on 🚀
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjn6wv/cGwiv/yWquRmx2FEjuO62k8716JXtWFhx7zE1OcbgSYtOx2i4QAPQJhJ
 8FYhRtUuVArBrkz9lrn9VSCo3Aueb7w8b/j3/8dGyH/zqUU7IhWgrjC7qlfbdP4lp
 9DzDGE5ITqlvZlEkYAloI3zMoVRaUHDlGTK+bEHK3WdRnsPL9oR/C+S1UNcPNqq1m
10I4G5UrneAHnQnIiWJfjfzpZ0BlXCXCfs3S4BBwKNTQAMlqlgzz1sCQr5R2d8kcW3
11mNexZ76vn3IlrmFAyXWNwzQvGvfrGMF4RcIYQiS5OLfcjlzLfZOxSQJjerHzMe7C
12EdnUsD+OX6BA6ijgU7eO7k5mdRxSkUVLanAzINUB5dupiw1NscUyZlqsH2J549DT
13Q2g6va4jFXAULCFg2Rwx18k3GOQ0spLsRyv6wn/PxpwzEaZnEnSsoCe8pdiwCaOk
14SyIJzqx3vzfA3IhEgHVnLHqLN8iYk9F5XXevDDA7fPN1A/KAWDcQXjVEpu3jA03w
15nuOXSfZ+
16=G8F6
17-----END PGP SIGNATURE-----

Timestamp of file with hash 7e94e353f869ccf97b1947f28a0c0193a5e2cf8d984598592f1296d23f14168d -

jamesob force-pushed on Jan 27, 2020

in src/init.cpp:1559 in e89fbd3cf0 outdated

1588+
1589+                    // The on-disk coinsdb is now in a good state, create the cache
1590+                    chainstate->InitCoinsCache();
1591+                    assert(chainstate->CanFlushToDisk());
1592+
1593+                    is_coinsview_empty = fReset || fReindexChainState ||

MarcoFalke commented at 3:48 pm on January 28, 2020:

in commit e89fbd3cf0820ee263a94c49751a97a784acb049:

I have a hard time following the logic-change.

You replace all calls to ChainActive with a for loop over all chainstates
You only initialize the “ibd” chainstate, which will retain all logic as it was before
However, is_coinsview_empty will be set to the result of whatever chainstate was processed last, and then used (in the next loop) on whatever chainstate is processed first. This seems fragile or at least requires a comment to explain why this can be done.

jamesob commented at 4:30 pm on January 28, 2020:

Maybe it’s a little hard to see in the diff, but the current value of is_coinsview_empty is only ever used within a single loop. In the next loop, the value is reset based on the chainstate being worked on.

It’s safe to reset the value on line 1619 because it’s not possible for the coinstip’s best block to be set to null between this is_coinsview_empty = ... assignment and the one in the next loop.

To be extra sure there’s no change, I could create a chainstate_to_coinsview_empty map and only set the values once, but I’ll only do this if you think it’s worthwhile.

MarcoFalke commented at 5:57 pm on January 28, 2020:

Oh, I missed that the value is reset. Maybe it would help to limit the scope of the variable to make that more clear and avoid this issue accidentally coming up in the future?

ryanofsky commented at 8:39 pm on January 28, 2020:

Oh, I missed that the value is reset. Maybe it would help to limit the scope of the variable to make that more clear and avoid this issue accidentally coming up in the future?

I agree it would be clearer to limit the scope of this variable. It’s crazy to give a variable a 200 line scope if it should have a 2 line scope.

jamesob commented at 3:40 pm on January 29, 2020:

Fixed by removing the variable entirely and replacing it with a function.

in src/init.cpp:1593 in e89fbd3cf0 outdated

1630+                    uiInterface.InitMessage(_("Rewinding blocks...").translated);
1631+                    if (!chainstate->RewindBlockIndex(chainparams)) {
1632+                        strLoadError = _(
1633+                            "Unable to rewind the database to a pre-fork state. "
1634+                            "You will need to redownload the blockchain").translated;
1635+                        break;

MarcoFalke commented at 3:49 pm on January 28, 2020:

in commit e89fbd3cf0820ee263a94c49751a97a784acb049:

Previously we’d break out, now we continue. Why?

jamesob commented at 3:41 pm on January 29, 2020:

Good catch! Fixed.

MarcoFalke commented at 3:52 pm on January 28, 2020: member

ACK e7bc059491d51919f676b35b6843d1c395d452a6 only, have not reviewed later commits closely 🕧

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e7bc059491d51919f676b35b6843d1c395d452a6 only, have not reviewed later commits closely 🕧
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjKXwwApeBAJd4ix/yrfHo9bHi30caJGej5Xk3cCvsbTorNmxVgMjJdbcALeADr
 8AbWHnVntgHiI7oLtzbKrFlKTGi826+stmRIy2VPyVs31L/z7k2pLKJsRFtYO+kzv
 9ATQ4POaLd58wzFI4bxGtSLBu7I+91f6iLzdtdINYDVJj6g89eBd74ICseFoQjM6x
10lAugPzMXzrL8kh71IFBumNpQKq3rNZ9Wb/6oqVCBiZUGUnW4qC3a5aVDhwfnBnKX
11YQia7Cej4Ex22+FAxh+4LUl8nO6I+UMH53TASaMG/ZoHUrXpHouWyo9vmUAwBMyp
12gqN9AX/V5Hv7YCk3hehviw6p9ZI+Gvs3hrXYlU2FLUFTGXJfQz5yM6jSipnK5tZC
13vqx46MazAjQ7x2de/HJIfwf+uEFpZkohauhobu5pMGss0tuVSiWVcofIoS8I5zei
14mlEZ6atiGucuEMZmiC8+z3g4TbGo5kztOB00YOVVbY1+jDmuOOs+T220p4FtrDTl
15B6J3CRmr
16=Ik+8
17-----END PGP SIGNATURE-----

Timestamp of file with hash fe215ba014f43a48217fc4a11afdd4fb671d3ccd7e48f8bed366d49affae1228 -

in src/init.cpp:1680 in e89fbd3cf0 outdated

1680+                            failed_verification = true;
1681+                            break;
1682+                        }
1683+
1684+                        if (!CVerifyDB().VerifyDB(
1685+                                chainparams, &chainstate->CoinsDB(),

MarcoFalke commented at 6:15 pm on January 28, 2020:

in commit e89fbd3:

VerifyDB will verify the db based on the active chain. How is this supposed to work when you pass in a db that is not from the active chainstate?

jamesob commented at 8:49 pm on January 28, 2020:

Oh good point - looks like I split the commits up a bit out of order. I could pull this (https://github.com/bitcoin/bitcoin/pull/15606/commits/23a074b6630dd75fd4a4a5b6495fe046012f2cc9) in or only do verification for the active chain… any preference?

MarcoFalke commented at 0:06 am on January 29, 2020:

If you think that the commit that switches AppInitMain to use for-loops is blowing up this pull request too much, you can also leave it for later, I guess. I haven’t checked, but it should be correct to just call g_chainman.InitializeChainstate(); to initialize the “ibd” chainstate and then leave all calls as they were previously, i.e. ChainstateActive()....

in src/init.cpp:705 in 1c173812ba outdated

705-    if (!ActivateBestChain(state, chainparams)) {
706-        LogPrintf("Failed to connect best block (%s)\n", FormatStateMessage(state));
707-        StartShutdown();
708-        return;
709+
710+    // XXX this is kind of a hack to avoid a lock inversion.

MarcoFalke commented at 6:58 pm on January 28, 2020:

in commit 1c173812bac34e263ae49e0bc80e45b437e95e8b:

Why is this a hack and what should be used instead of the hack? If there is nothing that can be used instead, this is not a hack and the comment should go.

ryanofsky commented at 9:08 pm on January 28, 2020:

in commit 1c17381:

Why is this a hack and what should be used instead of the hack? If there is nothing that can be used instead, this is not a hack and the comment should go.

Tend to agree with Marco that if it is a hack you should say what the specific problem is. Maybe a problem could be that one of the CChainState* pointers in the vector could get deleted before the loop below rolls around to it?

If something like this is the problem, you could work around it by replacing the GetAll method returning a vector with a ForEach method taking a callback like connman’s ForEachNode or jimpo’s ForEachBlockFilterIndex. You might also need to add chainstate m_dont_delete_this_chainstate or chainstatemanager m_dont_delete_any_chainstates variables if the async deletion is actually a real thing.

UPDATE: Sounds like the lifetime thing is not an issue. It would be good to have a ChainstateManager class comment about lifetime of various chainstate objects is, and specifically what happens if a second UTXO snapshot is loaded

jamesob commented at 3:42 pm on January 29, 2020:

Fixed by cleaning up inline comments and adding documentation to the ChainstateManager pointer definitions.

in src/net_processing.cpp:1407 in 1c173812ba outdated

1402@@ -1403,7 +1403,15 @@ void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, c
1403     } // release cs_main before calling ActivateBestChain
1404     if (need_activate_chain) {
1405         BlockValidationState state;
1406-        if (!ActivateBestChain(state, Params(), a_recent_block)) {
1407+
1408+        // XXX this is kind of a hack here to get around the fact that we can't

MarcoFalke commented at 7:00 pm on January 28, 2020:

Same here and everywhere else you use the XXX

MarcoFalke commented at 7:01 pm on January 28, 2020: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 1c173812bac34e263ae49e0bc80e45b437e95e8b, but it looks like the tests fail 🍢
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjFNgwApRAQI5N2D4113l6z1rLjgUjlixBjaHFnZC0rG5naAL/Gu55ofeAmzg5Z
 8TYF1WvqL9spMC+CDCweOyRGKsWb81Ryv8Ieni+Dn8RE+pu0VLakDu9Lw95+bd43j
 91P6RLJYa2NYFlCuJssbs5ziEPJqs3/DlelgWPslfbdgwLXWFT54CvijXuFEF43b/
10T9/NBr1OO1ByA0+AEM0eT3iHH55Guna544fuY8UjEDabnjqs8EiatmXArGlqi6Wr
11D6U7J33USdtpof1bb+2zR8KLSb4MCFFxezr2yrSREHf5tqqamfN3qJ2dYHazNEMd
12S4ka7rOrQqY/c2XIBs4oYD8pxI3thvxuo4nAnsG0sxjxkW0mm4O5OBqTFP/aGilX
13NNkR1TTl1DcuqbTFK3FNoiqmG/yuh2OWw3jm0EG6jhEcf3sdf1d2e5RUQ0xKPDRm
14h/PVqKPlmRDkEHiT9XoLgWDw6C9fH86HdGh0HLBZmTJ2jOt+sVeIvSXtth7sVznG
15vtfM49Wg
16=0mLY
17-----END PGP SIGNATURE-----

Timestamp of file with hash 6a38ed3fc7e109b015b25d8cd883d76b0d7c7f56f8c96dd80e10381889e51bbd -

in src/validation.h:787 in 10837ab1d3 outdated

781@@ -775,6 +782,109 @@ bool InvalidateBlock(BlockValidationState& state, const CChainParams& chainparam
782 /** Remove invalidity status from a block and its descendants. */
783 void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
784 
785+/**
786+ * Provides an interface for creating and interacting with multiple
787+ * chainstates, as well as a means to initialize chainstates from a

ryanofsky commented at 7:50 pm on January 28, 2020:

In commit “validation: introduce unused ChainstateManager” (10837ab1d33e126c4f36221cb5077038ca65f00d)

Would be nice to drop ambiguity of “multiple” chainstates here and say more specifically “one or two chainstates: an IBD chainstate generated by downloading blocks, and an optional snapshot chainstate loaded from a UTXO snapshot”

jamesob commented at 3:42 pm on January 29, 2020:

Fixed, thanks!

in src/validation.h:809 in 10837ab1d3 outdated

804+ *    wallet) as a reflection of the current chain and UTXO set.
805+ *    This may either be an IBD chainstate or a snapshot chainstate.
806+ *
807+ * *Background validation chainstate*: an IBD chainstate for which the
808+ *    IBD process is happening in the background while use of the
809+ *    active chainstate allows the rest of the system to function.

ryanofsky commented at 7:57 pm on January 28, 2020:

In commit “validation: introduce unused ChainstateManager” (10837ab1d33e126c4f36221cb5077038ca65f00d)

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.

jamesob commented at 10:41 pm on January 28, 2020:

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.

ryanofsky commented at 11:28 pm on January 28, 2020:

re: #17737 (review)

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”

jamesob commented at 3:44 pm on January 29, 2020:

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.

in src/test/validation_chainstatemanager_tests.cpp:27 in 997bf7f386 outdated

22+{
23+    BOOST_CHECK_EQUAL(v1.size(), v2.size());
24+    for (size_t i = 0; i < v1.size(); ++i) {
25+        BOOST_CHECK_EQUAL(v1[i], v2[i]);
26+    }
27+}

ryanofsky commented at 8:47 pm on January 28, 2020:

In commit “test: add basic tests for ChainstateManager” (997bf7f38626978cce84a33ee1c6e353a3440e25)

I think boosts BOOST_CHECK_EQUAL_COLLECTIONS macro might give better error reporting than this function. We’re using the macro already in some other tests

jamesob commented at 3:44 pm on January 29, 2020:

Fixed, thanks.

in src/test/validation_chainstatemanager_tests.cpp:71 in 997bf7f386 outdated

76+    c2.LoadGenesisBlock(chainparams);
77+    BlockValidationState _;
78+    BOOST_CHECK(c2.ActivateBestChain(_, chainparams, nullptr));
79+
80+    BOOST_CHECK(manager.IsSnapshotActive());
81+    BOOST_CHECK(!manager.IsSnapshotValidated());

ryanofsky commented at 8:52 pm on January 28, 2020:

In commit “test: add basic tests for ChainstateManager” (997bf7f38626978cce84a33ee1c6e353a3440e25)

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

in src/init.cpp:238 in e89fbd3cf0 outdated

236@@ -237,8 +237,10 @@ void Shutdown(NodeContext& node)
237     // may not have been initialized yet.
238     {
239         LOCK(cs_main);
240-        if (g_chainstate && g_chainstate->CanFlushToDisk()) {
241-            g_chainstate->ForceFlushStateToDisk();
242+        for (CChainState* chainstate : g_chainman.GetAll()) {

ryanofsky commented at 9:34 pm on January 28, 2020:

In commit “use ChainstateManager to initialize chainstate” (e89fbd3cf0820ee263a94c49751a97a784acb049)

There’s a stray reference to g_chainstate a few lines up in a comment which should be updated

Also in AppInitMain line 1713 below, PruneAndFlush is only called on active chain, not both chains. I’m probably missing something, but it seem like a loop might be right there as well. Or maybe a comment could say why pruning just one chain makes sense there.

jamesob commented at 3:44 pm on January 29, 2020:

Good catches! Fixed, thanks.

in src/init.cpp:1586 in e89fbd3cf0 outdated

1623-                    strLoadError = _("Unable to rewind the database to a pre-fork state. You will need to redownload the blockchain").translated;
1624-                    break;
1625+            for (CChainState* chainstate : g_chainman.GetAll()) {
1626+                if (!fReset) {
1627+                    // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate.
1628+                    // It both disconnects blocks based on ::ChainActive(), and drops block data in

ryanofsky commented at 9:39 pm on January 28, 2020:

In commit “use ChainstateManager to initialize chainstate” (e89fbd3cf0820ee263a94c49751a97a784acb049)

Should this line refer to chainstate now instead of ::ChainActive?

jamesob commented at 3:45 pm on January 29, 2020:

Fixed.

in src/validation.h:897 in 1c173812ba outdated

890@@ -891,16 +891,16 @@ class ChainstateManager
891     void Reset();
892 };
893 
894-extern ChainstateManager g_chainman;
895+extern ChainstateManager g_chainman GUARDED_BY(::cs_main);
896 
897 /** @returns the most-work valid chainstate. */
898-CChainState& ChainstateActive();
899+CChainState& ChainstateActive() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

ryanofsky commented at 9:51 pm on January 28, 2020:

In commit “protect g_chainman with cs_main” (1c173812bac34e263ae49e0bc80e45b437e95e8b)

Adding these lock annotations seems to makes these convenience functions a lot less convenient and blow up the size of this commit with lots of new cs_main locks. Would it be bad to lock cs_main inside ChainstateActive(), ChainstateActive(), and BlockIndex() instead of requiring it to be locked beforehand?

jamesob commented at 3:46 pm on January 29, 2020:

Fixed by taking your suggestion. I didn’t initially do this because I thought it created some kind of lock annotation problem, but after trying this change and rebasing the parent PR onto it, clang threw no warnings. Subsequently the diff has contracted a bit - thanks!

ryanofsky approved

ryanofsky commented at 10:02 pm on January 28, 2020: member

Code review ACK 1c173812bac34e263ae49e0bc80e45b437e95e8b. 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.

jamesob commented at 10:11 pm on January 28, 2020: member

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

jamesob force-pushed on Jan 29, 2020

jamesob commented at 3:39 pm on January 29, 2020: member

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

jamesob force-pushed on Jan 29, 2020

in src/validation.cpp:5137 in ead4c5fada outdated

5135+        g_chainman.BlockIndex().clear();
5136     }
5137 };
5138 static CMainCleanup instance_of_cmaincleanup;
5139+
5140+uint256 ChainstateManager::SnapshotBlockhash() const {

laanwj commented at 3:52 pm on January 29, 2020:

I’d prefer this to return an Optional<uint256> instead of special-casing 00000000000000000000000000000000.

jamesob commented at 7:40 pm on January 29, 2020:

Good point, fixed.

in src/init.cpp:708 in ead4c5fada outdated

708+    // the g_chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
709+    // the relevant pointers before the ABC call.
710+    std::vector<CChainState*> chainstates;
711+    WITH_LOCK(::cs_main, chainstates = g_chainman.GetAll());
712+
713+    for (CChainState* chainstate : chainstates) {

ryanofsky commented at 3:57 pm on January 29, 2020:

In commit “protect g_chainman with cs_main” (d88f75e495a46ae474d8ad67faaec13ef1950f5e)

Should keep current style if you prefer it, but it’d be possible to shorten this initializing the chainstates variable on one line instead of two lines, or just dropping it:

0for (CChainState* chainstate : WITH_LOCK(::cs_main, return g_chainman.GetAll())) {
1   ...
2}

jamesob commented at 7:40 pm on January 29, 2020:

Ah yeah, that’s a nice improvement. Fixed.

in src/validation.cpp:5180 in ead4c5fada outdated

5178+        m_active_chainstate = to_modify.get();
5179+    } else {
5180+        throw std::logic_error("unexpected chainstate activation");
5181+    }
5182+
5183+    return *to_modify.get();

laanwj commented at 4:00 pm on January 29, 2020:

to_modify->get()?

MarcoFalke commented at 6:33 pm on January 29, 2020:

Heh, member access has a higher precedence than indirection (dereference), so this is correct. Though, the same could be written shorter as just *to_modify

jamesob commented at 7:40 pm on January 29, 2020:

Went with Marco short-form.

jamesob commented at 4:04 pm on January 29, 2020: member

au.chainman.9 -> au.chainman.11

Pushed fixes for some small bugs, @MarcoFalke’s VerifyDB comment, and one big embarrassing typo.

jamesob force-pushed on Jan 29, 2020

in src/validation.cpp:3824 in d88f75e495 outdated

3820@@ -3818,7 +3821,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
3821     NotifyHeaderTip();
3822 
3823     BlockValidationState state; // Only used to report errors, not invalidity - ignore it
3824-    if (!::ChainstateActive().ActivateBestChain(state, chainparams, pblock))
3825+    if (!ActivateBestChain(state, chainparams, pblock))

ryanofsky commented at 4:13 pm on January 29, 2020:

In commit “protect g_chainman with cs_main” (d88f75e495a46ae474d8ad67faaec13ef1950f5e)

I don’t understand the two swaps between ::ActivateBestChain(...) and ::ChainstateActive().ActivateBestChain(...) in this commit. They don’t seem to do anything, and if I revert them there don’t seem to be obvious problems.

Not specific to this PR but generally it seems confusing to have an instance method and a global function both named ActivateBestChain. Would suggest renaming one or the other (this is the reason I like lowercase method names) or maybe just dropping the global function since it is only one line.

jamesob commented at 7:44 pm on January 29, 2020:

Yeah, this was churn from when I was reverting the inline cs_main locking I was initially doing for the last commit. Reverted, good catch.

I’m for your ActivateBestChain suggestions, but would prefer to do them in a follow-up PR to limit the scope of this one. If anyone thinks that’d be best done here, I’m happy to.

in src/validation.h:841 in d88f75e495 outdated

835 
836     //! A chainstate initialized on the basis of a UTXO snapshot. If this is
837     //! non-null, it is always our active chainstate.
838+    //!
839+    //! Once this pointer is set to a corresponding chainstate, it will not
840+    //! be reset until init.cpp:Shutdown(). This means it is safe to acquire

ryanofsky commented at 4:27 pm on January 29, 2020:

In commit “protect g_chainman with cs_main” (d88f75e495a46ae474d8ad67faaec13ef1950f5e)

This doesn’t explain what happens if you load two snapshots (one after the other). It seems like ChainstateManager::InitializeChainstate explicitly allows this so it would be good to add some clarification here or there.

jamesob commented at 7:41 pm on January 29, 2020:

I don’t think InitializeChainstate allows this because of this check: https://github.com/jamesob/bitcoin/blob/8fe8b2e499ceb61c6fd3b73bf1c5dbc5861ccf62/src/validation.cpp#L5164-L5170

I wrote it to not allow the initialization of more than one “kind” of chainstate.

in src/init.cpp:1678 in 741db94e52 outdated

1681+                            break;
1682+                        }
1683+
1684+                        // Only verify the DB of the active chainstate. This is fixed in later
1685+                        // work when we allow VerifyDB to be parameterized by chainstate.
1686+                        if (&::ChainstateActive() == chainstate &&

ryanofsky commented at 5:06 pm on January 29, 2020:

In commit “use ChainstateManager to initialize chainstate” (741db94e52b51d4df8173d56bfb1258ccf9ddbb3)

Just to understand, is checking ChainstateActive() == chainstate here an optimization to avoid verifying the same db multiple times? Could update comment, but no need to since hopefully this will be pretty shortlived

jamesob commented at 7:43 pm on January 29, 2020:

Yeah, it’s just because VerifyDB at the moment makes explicit reference to ChainActive: https://github.com/jamesob/bitcoin/blob/8fe8b2e499ceb61c6fd3b73bf1c5dbc5861ccf62/src/validation.cpp#L4224-L4234

ryanofsky approved

ryanofsky commented at 5:59 pm on January 29, 2020: member

Code review ACK d88f75e495a46ae474d8ad67faaec13ef1950f5e. Biggest change since last review is last commit which is rewritten to move locking inside validation.cpp accessors requiring locking by callers. Also there were some tweaks & minor fixes to the init code, some comment/renaming/test cleanups, and a qt test logic_error workaround

in src/test/util/setup_common.cpp:123 in d88f75e495 outdated

119+    } catch (const std::logic_error& e) {
120+        std::string msg = e.what();
121+        if (msg.find("overwriting a chainstate") == std::string::npos) {
122+            throw;
123+        }
124+    }

MarcoFalke commented at 6:24 pm on January 29, 2020:

Please remove this hack in the unit tests. It will lead to potentially not resetting global state and preserving a chainstate of previous tests. The test failure is due to a missing reset in the gui tests, which can be fixed with a one-line patch:

 0diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
 1index 14a75b23f3..f9eb4cde30 100644
 2--- a/src/qt/test/apptests.cpp
 3+++ b/src/qt/test/apptests.cpp
 4@@ -82,6 +82,7 @@ void AppTests::appTests()
 5     // Reset global state to avoid interfering with later tests.
 6     AbortShutdown();
 7     UnloadBlockIndex();
 8+    g_chainman.Reset();
 9 }
10 
11 //! Entry point for BitcoinGUI tests.

jamesob commented at 7:46 pm on January 29, 2020:

Ah, thanks for the patch. Fixed.

MarcoFalke commented at 6:34 pm on January 29, 2020: member

re-ACK d88f75e495a46ae474d8ad67faaec13ef1950f5e will probably do a fresh review some time later 🍶

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK d88f75e495a46ae474d8ad67faaec13ef1950f5e  will probably do a fresh review some time later 🍶
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjJuQv9Fd31SrPN6kA/2g2p3Z99NXkVHa4CFGg9Eo+sCalKZ7YvQ3KwrKsC1AXK
 8uf9phsF6KBNX3cx7E9MEJ1OUCh5s3zJeBjZ3D6uK8xb9qvqA5ow87xb858h87isY
 9NRHXWjNYJHYORCitPhoK3hFws5lPxSf8K6K9FbbNISg4PY2zWEeaWGKmmxCPtRF2
10ho6E5YSK/MyHVxXLUdiWQ3lpv0OEC9CustJ0KtSWOL4Vix5OVQJYlyxJTfnrE4Bn
11I7axZLEQ6H0RhJWKha0/+3ntclZS4MC1dO6Qk12pQzxXl7K9tz5ppePXIR9/8dRt
12SG38Ht92ff1XMq0ENIaMdiwn/XiH1b78ZZGztuXQuEAkUGj1w/eM+nBTVvSm8Ndj
13Fu17KtFvE8lbDdKXRhdlvTHLpWEzzeaOkJQHslCMG+QpipzYHOvm4Iu173CHjXDi
14PARCm2B9vQ3PDcvSD+kJxkbo53pSStv1uoM5zHyGRAXpjawmpB9PJpWuPdRTrGlx
15V/E5uVAQ
16=0Exq
17-----END PGP SIGNATURE-----

Timestamp of file with hash 8e3d2a68db66c60ea8e7ee3a4ff26522646e5da2bf7ceb11af7b3d8750f0973d -

jamesob force-pushed on Jan 29, 2020

jamesob commented at 7:47 pm on January 29, 2020: member

au.chainman.11 -> au.chainman.12(changes)

Thanks for the continued reviews!

MarcoFalke commented at 9:14 pm on January 29, 2020: member

Tests fail when compiled with --enable-debug

ryanofsky approved

ryanofsky commented at 8:59 pm on February 7, 2020: member

Code review ACK 8fe8b2e499ceb61c6fd3b73bf1c5dbc5861ccf62. Changes since last review: making SnapshotBlockhash return optional, removing qt test workaround, minor style / include tweaks

Are there still problems with tests?

ryanofsky commented at 9:40 pm on February 7, 2020: member

Code review ACK 8fe8b2e. Changes since last review: making SnapshotBlockhash return optional, removing qt test workaround, minor style / include tweaks

Are there still problems with tests?

Running this locally, I see a simple error in a wallet test that is locking cs_main in ChainActive() after cs_wallet is locked instead of before. Following fix works:

 0--- a/src/wallet/test/wallet_tests.cpp
 1+++ b/src/wallet/test/wallet_tests.cpp
 2@@ -456,7 +456,7 @@ public:
 3         CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
 4         wallet = MakeUnique<CWallet>(m_chain.get(), WalletLocation(), WalletDatabase::CreateMock());
 5         {
 6-            LOCK(wallet->cs_wallet);
 7+            LOCK2(::cs_main, wallet->cs_wallet);
 8             wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
 9         }
10         bool firstRun;

 02020-02-07T21:36:34Z POTENTIAL DEADLOCK DETECTED
 12020-02-07T21:36:34Z Previous lock order was:
 22020-02-07T21:36:34Z  (1)
 3 cs_main interfaces/chain.cpp:239 (in thread )
 42020-02-07T21:36:34Z  (2)
 5 cs_wallet wallet/wallet.cpp:2973 (in thread )
 62020-02-07T21:36:34Z Current lock order is:
 72020-02-07T21:36:34Z  (2)
 8 wallet->cs_wallet wallet/test/wallet_tests.cpp:459 (in thread )
 92020-02-07T21:36:34Z  (1)
10 ::cs_main validation.cpp:92 (in thread )
11Assertion failed: detected inconsistent lock order at sync.cpp:119, details in debug log.

jamesob force-pushed on Feb 14, 2020

jamesob commented at 4:50 pm on February 14, 2020: member

au.chainman.12 -> au.chainman.13 (changes)

Thanks for the fix, @ryanofsky. Tests are now passing.

ryanofsky commented at 9:35 pm on February 14, 2020: member

Code review ACK abfc15273f817d30c3f53f910a8b340a72a6377a. Changes since last review:

0+Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

with my test fix. Waiting for my github royalties to start rolling in.

promag commented at 2:06 am on February 26, 2020: member

Code review ACK abfc15273f817d30c3f53f910a8b340a72a6377a

in src/validation.h:786 in 68998eca8e outdated

782@@ -775,6 +783,111 @@ bool InvalidateBlock(BlockValidationState& state, const CChainParams& chainparam
783 /** Remove invalidity status from a block and its descendants. */
784 void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
785 
786+/**
787+ * Provides an interface for creating and interacting with one or two
788+ * chainstates: an IBD chainstate generated by downloading blocks, and
789+ * an optional snapshot chainstate loaded from a UTXO snapshot. Managed
790+ * chainstates can be maintained at different heights simultaneously.

ariard commented at 9:40 pm on February 28, 2020:

68998ec

Maybe precise what happens at chainstates intersection. Are they merged altogether, drop away, … ?

Also, is this targeted to be part of libconsensus ? If yes, maybe add same design requirements than CChainState, “Eventually, the API here is targeted at being exposed externally as a consumable libconsensus library, …”

in src/validation.h:924 in abfc15273f outdated

920@@ -894,7 +921,7 @@ class ChainstateManager
921     void Reset();
922 };
923 
924-extern ChainstateManager g_chainman;
925+extern ChainstateManager g_chainman GUARDED_BY(::cs_main);

ariard commented at 11:05 pm on February 28, 2020:

abfc152

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 ?

ryanofsky commented at 10:08 pm on March 25, 2020:

re: #17737 (review)

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

ariard commented at 11:06 pm on February 28, 2020: member

Mostly minor points, just need another parse on new lock implications to Code Review ACK.

in src/validation.cpp:5210 in 68998eca8e outdated

5214+    assert(m_ibd_chainstate);
5215+    return *m_ibd_chainstate.get();
5216+}
5217+
5218+bool ChainstateManager::IsBackgroundIBD(CChainState* chainstate) const
5219+{

ariard commented at 11:07 pm on February 28, 2020:

68998ec

Assert m_snapshot_chainstate == m_active_chainstate ?

DrahtBot added the label Needs rebase on Mar 1, 2020

validation: add CChainState.m_from_snapshot_blockhash

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.

8e2ecfe249

validation: introduce unused ChainstateManager

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.

Incorporates feedback from Marco Falke. Additional documentation written
by Russ Yanofsky.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

89cdf4d569

refactor: move RewindBlockIndex to CChainState

This is in preparation for multiple chainstate initialization in init.

5b690f0aae

use ChainstateManager to initialize chainstate

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

4ae29f5f0c

test: add basic tests for ChainstateManager

Feedback incorporated from Russell Yanofsky.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

2b081c4568

protect g_chainman with cs_main

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>

c9017ce3bc

in src/validation.cpp:1256 in cdf6bee5a4 outdated

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

fjahr commented at 4:00 pm on March 17, 2020:

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.

in src/validation.cpp:5178 in 68998eca8e outdated

5182+        throw std::logic_error("should not be overwriting a chainstate");
5183+    }
5184+
5185+    to_modify.reset(new CChainState(snapshot_blockhash));
5186+
5187+    // Snapshot chainstates and initial IBD chaintates always become active.

fjahr commented at 4:29 pm on March 17, 2020:

typo: chaintates

in src/init.cpp:711 in abfc15273f outdated

697@@ -698,11 +698,17 @@ static void ThreadImport(std::vector<fs::path> vImportFiles)
698     }
699 
700     // scan for better chains in the block chain database, that are not yet connected in the active best chain

fjahr commented at 6:07 pm on March 17, 2020:

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

jamesob force-pushed on Mar 17, 2020

fjahr commented at 6:24 pm on March 17, 2020: member

Code Review ACK abfc15273f817d30c3f53f910a8b340a72a6377a

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 commented at 7:55 pm on March 17, 2020: member

Code Review Re-ACK abfc15273f817d30c3f53f910a8b340a72a6377a

DrahtBot removed the label Needs rebase on Mar 17, 2020

MarcoFalke commented at 3:08 pm on March 18, 2020: member

re-ACK c9017ce3bc27665594c9d80f395780d40755bb22 📙

Only changes since my last review:

Fix lock order in tests
Making the snapshot blockhash optional

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK c9017ce3bc27665594c9d80f395780d40755bb22 📙
 4
 5Only changes since my last review:
 6
 7* Fix lock order in tests
 8* Making the snapshot blockhash optional
 9
10-----BEGIN PGP SIGNATURE-----
11
12iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
13pUjHLQv/RAhLnsTpzdzOpmyvTALXqbxQut9dguj1Tt46DsWnRzIcrwwcsYHbDreU
14B3DmuqYHMdcb1o/OWV5IvWiHMnUbY/EbFfD6Zw9/4HNhHxmruCgUrG9+zP+oD1ck
15nacI226jyJQWVKEz6XtbuF7uhW1an9J2XyPbnh6EaUywTB+CabLI/mnOFiHerWGd
16Q4VwcxSFYeBjmoDP1ekDaTafAphHhDSDO7MaissHBPq+1HliMURndLGgDKpRnT0y
17Fn7dfmN/fN3bS5JWufUCaatPdYjES5UOWRaDasub0ipmlvBXzTk/Z48lYzqz4QEp
18Owa9fl6EoiRrhsQQSMOdEm6Mi2RCkwIXTK85+Ix7VP7ys+uQov/5fJbxel9zhInK
19/4nAtO6uum9XscLqWqHi6AFEVGOEVOfYPN+CJbRW1y5PinpR0t1kRVhEjfXgzWag
20bZfFWB2qs9bo+pWMCBDB/xeucJbJC2aDfnDqE5Gqpf1Knrg7t2tVbgU7IWJBaQLb
219KZNfpTL
22=hs4D
23-----END PGP SIGNATURE-----

Timestamp of file with hash f61dcf850f62e4c3ff5ca63de6c032928f672c45311925c6f66a2497f5bc0c48 -

MarcoFalke commented at 3:10 pm on March 18, 2020: member

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

fjahr commented at 3:14 pm on March 18, 2020: member

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

in src/validation.cpp:91 in c9017ce3bc

86     return *g_chainman.m_active_chainstate;
87 }
88 
89 CChain& ChainActive()
90 {
91+    LOCK(::cs_main);

ariard commented at 5:39 pm on March 19, 2020:

c9017ce

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

ryanofsky commented at 10:21 pm on March 25, 2020:

re: #17737 (review)

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)

ariard commented at 5:41 pm on March 19, 2020: member

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.

ryanofsky approved

ryanofsky commented at 10:30 pm on March 25, 2020: member

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

jamesob commented at 4:32 pm on March 31, 2020: member

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 commented at 4:14 am on April 1, 2020: member

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.

MarcoFalke merged this on Apr 10, 2020

MarcoFalke closed this on Apr 10, 2020

sidhujag referenced this in commit d35bad573a on Apr 13, 2020

laanwj removed this from the "Blockers" column in a project

MarcoFalke referenced this in commit 793e0ff22c on May 23, 2020

fanquake moved this from the "In progress" to the "Done" column in a project

jasonbcox referenced this in commit 1b8a0c38d4 on Nov 30, 2020

deadalnix referenced this in commit 4946c4f34a on Nov 30, 2020

jasonbcox referenced this in commit aa2a535f9b on Nov 30, 2020

jasonbcox referenced this in commit fc121f2533 on Nov 30, 2020

jasonbcox referenced this in commit 5b944843a3 on Nov 30, 2020

PhotoshiNakamoto referenced this in commit cda98e1821 on Dec 11, 2021

DrahtBot locked this on Feb 15, 2022

Add ChainstateManager, remove BlockManager global #17737

Conflicts