Rework validation logic for assumeutxo #27746

534b6f99a3779465678f39ed2704da6049c6469d if it’s broken in master, can we fix it before refactoring? Alternatively there could be a This will be fixed in the next commit comment.

I’m also confused by the comment. TryAddBlockIndexCandidate is only called from ReceivedBlockTransactions after it sets BLOCK_VALID_TRANSACTIONS. So are you saying we’re currently too strict? Was there a regression somewhere?

Sorry that comment was confusing. I dropped it, and instead change the behavior around managing setBlockIndexCandidates in the last two commits.

I think master has a couple bugs. When we start up, in LoadBlockIndex() we weren’t adding blocks that have been downloaded and have more work than the background chainstate’s tip to setBlockIndexCandidates, which prevents those blocks from being validated and connected on startup. Also, because AcceptBlock() is a member of Chainstate, it’s at best confusing to reason about whether a block that would advance the tip of a chainstate would always be added to a chainstate’s setBlockIndexCandidates (it depends on yet-more logic around where a block came from and guessing about which chainstate might need it).

The new code tries to simplify things by establishing that the background chainstate is only trying to connect blocks towards the snapshot block, and that all blocks for which we HAVE_DATA and have more work than the background chainstate’s tip (and which are ancestors of the snapshot block) will be added to the background chainstate’s setBlockIndexCandidates. Also, the logic bug in LoadBlockIndex should be fixed in the last commit here.

re: #27746 (review)e

I think master has a couple bugs.

I’m curious if there is actually more than one bug. The one bug I know about is the bug introduced 0fd599a from #23174 where LoadBlockIndex is misinterpreting the BLOCK_ASSUMED_VALID flag, and setting first_assumed_valid_height to the height of the first assumed valid block, instead of the height of the first block after the last assumed valid block (i.e. the snapshot height). I think it’s pretty bad that we didn’t catch this bug in review, or push for a more realistic test that would have caught it, or followed up on the suggestion from Marco #23174 (review) to just add blocks to setBlockIndexCandidates linearly, which is what this PR does, and is much more straightforward.

Are there other known bugs? Are the changes to AcceptBlock meant to fix a bug or more to prevent a potential bug? Does the change to ChainstateManager::ReceivedBlockTransactions fix a bug? It seems like maybe it could prevent inappropriate blocks being added as candidates to the background chainstate, but it’s not clear.

I guess the fairest thing to say is that the changes to AcceptBlock() are to prevent a future bug – it’s possible that we could write code that somehow gets it right, but the design of selectively adding blocks to one chainstate’s block index, and not the other, is one that I found very confusing.

As an example, if you took #24008 (which proposed logic for determining which chainstate to invoke AcceptBlock() on) and combined that with changing the logic in AcceptBlock() so that the background chainstate wouldn’t bother processing blocks that are past the snapshot base, then that would be problematic if we were to allow pruning the snapshot-based-chain while the background sync is running (because according to the logic in #24008, if the block is already on the active chain, then we give it to the background chainstate to process). That’s 2 hypotheticals so certainly this is speculative, but I think that those changes would all make sense on their own, so the fact that they fail to compose with the code in master is a sign to me that we should take a different design.

If you’re asking if it’s ok to call ActivateBestChain() on both chainstates, the answer to that is essentially always “yes”. ABC() doesn’t do anything if no new candidates for most-work chain have been added to our block index; but if any block that has more work than our tip is now available we want to try to connect it.

When calling LoadExternalBlockFile(), we’re adding blocks that theoretically could be of interest to either chainstate; so I think it makes sense to separate the block storage and import from any particular chainstate.

I’m not sure how easy this would be to separate out into its own commit, because this invokes AcceptBlock() as well… Hopefully the commit is more manageable now that I’ve split a bunch of other stuff out? Let me know if you see a good way to shrink this commit more and I can take a stab at it.

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896) in LoadExternalBlockFile:

It seems inconsistent to call ActivateBestChain on all chainstates here for the genesis block, but only call ActivateBestChain on the active chainstate a few lines below (line 4631).

I think I can understand the reasons from the discussion above (“we’re adding blocks that theoretically could be of interest to either chainstate; so I think it makes sense to separate the block storage and import from any particular chainstate”) and from pruning context below. But in both cases I think the code would be clearer if there were explicit comments saying why ActiveChainstate() is used and why GetAll() is used in each place.

In commit “Move block-storage-related logic to ChainstateManager” (c8f4a8702851c3097ebd038a814d974dce4dcc78)

It seems inconsistent to call ActivateBestChain on all chainstates here for the genesis block, but only call ActivateBestChain on the active chainstate a few lines below (line 4631).

This inconsistency is resolved now in commit “Move block-storage-related logic to ChainstateManager” (c8f4a8702851c3097ebd038a814d974dce4dcc78) by calling ActivateBestChain on all chains in both cases.

Since the case below is a pruning case, and pruning already has problems when snapshot activated and a background chainstate is being loaded, skipping the ActivateBestChain for the background chainstate probably did not cause any issues. But if pruning is going to be supported in the future, probably better if the code starts off treating chainstates the same and doesn’t have unexplained inconsistencies.

In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 “Tighten requirements for adding elements to setBlockIndexCandidates”

Is this test case expected to fail with the changes in this commit or is it just expected to be incorrect? I tried re-enabling it and it passes.

this test should be re-written so that snapshots are always at blocks in the block index.

Does it? AFAICT, it doesn’t test anything within the block index at all, and the only part that does is LoadGenesisBlock.

See https://github.com/bitcoin/bitcoin/pull/27746/files#diff-dbada1fe3a3d0af884304dd28be8c9df74b592401dec2c6400f6b491aefe6c9bL141

0Chainstate& c2 = WITH_LOCK(cs_main, return manager.ActivateExistingSnapshot(&mempool, GetRandHash()));

Trying to activate a snapshot for a block that is not in the block index ought to be an illegal operation.

Ah I see. Seems like this could be easily resolved by using TestChain100Setup

 0diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
 1index 2d0e1705fd..3d90bdcc26 100644
 2--- a/src/test/validation_chainstatemanager_tests.cpp
 3+++ b/src/test/validation_chainstatemanager_tests.cpp
 4@@ -105,7 +105,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
 5 }
 6 
 7 //! Test rebalancing the caches associated with each chainstate.
 8-BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
 9+BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, TestChain100Setup)
10 {
11     ChainstateManager& manager = *m_node.chainman;
12     CTxMemPool& mempool = *m_node.mempool;
13@@ -118,7 +118,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
14 
15     // Create a legacy (IBD) chainstate.
16     //
17-    Chainstate& c1 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(&mempool));
18+    Chainstate& c1 = manager.ActiveChainstate();
19     chainstates.push_back(&c1);
20     c1.InitCoinsDB(
21         /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false);
22@@ -126,8 +126,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
23     {
24         LOCK(::cs_main);
25         c1.InitCoinsCache(1 << 23);
26-        BOOST_REQUIRE(c1.LoadGenesisBlock());
27-        c1.CoinsTip().SetBestBlock(InsecureRand256());
28         manager.MaybeRebalanceCaches();
29     }
30 
31@@ -136,7 +134,8 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
32 
33     // Create a snapshot-based chainstate.
34     //
35-    Chainstate& c2 = WITH_LOCK(cs_main, return manager.ActivateExistingSnapshot(&mempool, GetRandHash()));
36+    CBlockIndex* snapshot_base{WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()[manager.ActiveChain().Height() / 2])};
37+    Chainstate& c2 = WITH_LOCK(cs_main, return manager.ActivateExistingSnapshot(&mempool, *snapshot_base->phashBlock));
38     chainstates.push_back(&c2);
39     c2.InitCoinsDB(
40         /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false);
41@@ -144,8 +143,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
42     {
43         LOCK(::cs_main);
44         c2.InitCoinsCache(1 << 23);
45-        BOOST_REQUIRE(c2.LoadGenesisBlock());
46-        c2.CoinsTip().SetBestBlock(InsecureRand256());
47         manager.MaybeRebalanceCaches();
48     }
49 

In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 “Tighten requirements for adding elements to setBlockIndexCandidates”

Is this check expected to fail? I tried uncommenting it and the test passes.

In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 “Tighten requirements for adding elements to setBlockIndexCandidates”

This diff makes the test work, assuming that TryAddBlockIndexCandidate is working as it is supposed to be.

 0diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
 1index 8fab53c5c5..b9fe054a64 100644
 2--- a/src/test/validation_chainstatemanager_tests.cpp
 3+++ b/src/test/validation_chainstatemanager_tests.cpp
 4@@ -413,7 +411,6 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, SnapshotTestSetup)
 5 //!   chainstate only contains fully validated blocks and the other chainstate contains all blocks,
 6 //!   even those assumed-valid.
 7 //!
 8-#if 0
 9 BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
10 {
11     ChainstateManager& chainman = *Assert(m_node.chainman);
12@@ -427,6 +424,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
13     const int assumed_valid_start_idx = last_assumed_valid_idx - expected_assumed_valid;
14 
15     CBlockIndex* validated_tip{nullptr};
16+    CBlockIndex* assumed_base{nullptr};
17     CBlockIndex* assumed_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())};
18 
19     auto reload_all_block_indexes = [&]() {
20@@ -440,10 +438,10 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
21         WITH_LOCK(::cs_main, chainman.LoadBlockIndex());
22     };
23 
24-    // Ensure that without any assumed-valid BlockIndex entries, all entries are considered
25-    // tip candidates.
26+    // Ensure that without any assumed-valid BlockIndex entries, only the current tip is
27+    // considered as a candidate.
28     reload_all_block_indexes();
29-    BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), cs1.m_chain.Height() + 1);
30+    BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), 1);
31 
32     // Mark some region of the chain assumed-valid.
33     for (int i = 0; i <= cs1.m_chain.Height(); ++i) {
34@@ -462,27 +460,33 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
35             validated_tip = index;
36             BOOST_CHECK(!index->IsAssumedValid());
37         }
38+        // Note the block after the last assumed valid block as the snapshot base
39+        if (i == last_assumed_valid_idx) {
40+            assumed_base = index;
41+            BOOST_CHECK(!index->IsAssumedValid());
42+        }
43     }
44 
45     BOOST_CHECK_EQUAL(expected_assumed_valid, num_assumed_valid);
46 
47     Chainstate& cs2 = WITH_LOCK(::cs_main,
48-        return chainman.ActivateExistingSnapshot(&mempool, GetRandHash()));
49+        return chainman.ActivateExistingSnapshot(&mempool, *assumed_base->phashBlock));
50+
51+    // Set tip of the fully validated chain to be the validated tip
52+    cs1.m_chain.SetTip(*validated_tip);
53 
54     reload_all_block_indexes();
55 
56-    // The fully validated chain only has candidates up to the start of the assumed-valid
57-    // blocks.
58+    // The fully validated chain should have the current validated tip, the assumed valid
59+    // blocks, and the assumed valid base as candidates.
60     BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.count(validated_tip), 1);
61-    BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.count(assumed_tip), 0);
62-    BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), assumed_valid_start_idx);
63+    BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), expected_assumed_valid + 2);
64 
65     // The assumed-valid tolerant chain has all blocks as candidates.
66     BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(validated_tip), 1);
67     BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_tip), 1);
68     BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.size(), num_indexes);
69 }
70-#endif
71 
72 //! Ensure that snapshot chainstates initialize properly when found on disk.
73 BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)

https://github.com/bitcoin/bitcoin/pull/27746/commits/55e27ee995944d188d7453c6e78a6538f6739d64

If you retouch, I think you can simply say bool is_bg_chainstate = this != &ActiveChainstate();

I don’t think this line does anything. It’s called during init from LoadChainstate() -> DetectSnapshotChainstate(), at which point m_snapshot_chainstate->m_snapshot_entry isn’t set yet. That only happens right after, in CompleteChainstateInitialization() -> LoadBlockIndex().

The ctor of Chainstate also wouldn’t have set m_snapshot_entry for the same reason (the block index is loaded after the chainstate).

In commit “Explicitly track maximum block height stored in undo files” (6cc09f4759444218d1b493a1bcf44bb63f3160d2)

When I first read this comment, I misinterpreted “whose undo data is in m_last_blockfile” to mean “whose undo data belongs in m_last_blockfile” as opposed to “whose undo data has already been written to m_last_blockfile”. So I was confused about how it was working. Would suggest changing this sentence to something like “Track the height of the highest block in m_last_blockfile whose undo data has been written.”

I also don’t think if would hurt if there could be more explanation after this like: “Block data is written to block files in download order, but is written to undo files in validation order, which is usually in order by height. To avoid wasting disk space, undo files will be will trimmed whenever the corresponding block file is finalized and the height of the highest block written to the block file equals the height of the highest block written to the undo file. This is a heuristic and can sometimes preemptively trim undo files that will write more data later, and sometimes fail to trim undo files that can’t have more data written later.”

In commit “Move block-arrival information / preciousblock counters to ChainstateManager” (2ebdfa8529b222de0db6d7114e9e5a1622fecfd0)

Just IMO, but I think it would good to delete the m_chainman.nBlockSequenceId = 1 line in this commit instead of next commit “Add wrapper for adding entries to a chainstates block index candidates” (df6576d48e4daff2b9787fe7d2d254ffce2eb998). I also think it would also be good to rename UnloadBlockIndex to ClearBlockIndexCandidates here. Doing these things would leave the code in a less confusing state after this commit, even if it makes this diff a little bigger.

In commit “Tighten requirements for adding elements to setBlockIndexCandidates” (b0b48b3e986319fe8cd5a80052099070279ce5c0)

Note for reviewers: A real hash is being passed instead of GetRandHash() now, because of discussion #27746 (review), because ActivateExistingSnapshot should not be expected to work when called with an invalid blockhash even if it does work now.

In commit “Update CheckBlockIndex invariants for snapshotted chains” (bb47ea918c19cd7063f529c0087bd156af18b499):

I think this also needs to add if (pindex == pindexFirstAssumeValid) pindexFirstAssumeValid = nullptr; at line 4881 to reset pindexFirstAssumeValid when moving to a sibling.

In commit “Update CheckBlockIndex invariants for snapshotted chains” (bb47ea918c19cd7063f529c0087bd156af18b499)

Note: This change is partially reverting 8f5710f from #21526

In commit “Update CheckBlockIndex invariants for snapshotted chains” (bb47ea918c19cd7063f529c0087bd156af18b499)

Should also delete this “Assumed-valid index entries will not have data” comment, I think, since it doesn’t really explain anything and no longer seems relevant here

In commit “Update CheckBlockIndex invariants for snapshotted chains” (bb47ea918c19cd7063f529c0087bd156af18b499)

This is very pedantic but the phrase “snapshotted chain” here and in the commit description doesn’t really parse for me. It sounds like a chain a snapshot was created from, not a chain created from a snapshot. Could maybe replace “snapshotted chain” with “chain based on assumeutxo snapshot”

Good point, fixed in this place in the code in 95680a6e90bbf27183dcf09b5c450494d356ebe5. Will try to fix up the commit descriptions when I squash/rebase.

Edit: this is now fixed in the commit messages as well.

In commit “Fix initialization of setBlockIndexCandidates when working with multiple chainstates” (f02125f1c7ba1c77c6d4ddb028999090fe5298eb)

I think it would be good to drop this whole for-loop. The loop was originally added in 0fd599a from #23174 to compute first_assumed_valid_height. Adding the asserts there was really an afterthought, so keeping this loop just to make assertions about ChainStateManager doesn’t seem worth it. Dropping the loop and asserts would also allow dropping the reliesOnAssumedValid method, which would be nice to get rid of.

Ah, ok – I’ve removed this in 1aebb99a10f9977e1d1bf4b923e2a2e984c76160.

So, one thing I haven’t fixed yet is the reference to pindex->IsAssumedValid() just below this for loop:

 0
 1        for (CBlockIndex* pindex : vSortedByHeight) {
 2            if (ShutdownRequested()) return false;
 3            if (pindex->IsAssumedValid() ||
 4                    (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) &&
 5                     (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))) {
 6
 7                for (Chainstate* chainstate : GetAll()) {
 8                    chainstate->TryAddBlockIndexCandidate(pindex);
 9                }
10            }

Really, the assumed-valid status bit has nothing to do with whether a block index should be a most-work-chain candidate, except that right when a snapshot is activated, we need the base block to be added to setBlockIndexCandidates for the chain that is based on that snapshot. Probably I should fix this in this PR as well?

In b0b48b3e986319fe8cd5a80052099070279ce5c0 “Tighten requirements for adding elements to setBlockIndexCandidates”

nit: can inline

0            if (m_snapshot_entry->GetAncestor(pindex->nHeight) == pindex) {

In commit “Explicitly track snapshot block header when using multiple chainstates” (d2ccbd3541622147ab0d4b808a1b9388ddcd7948)

I don’t think adding this Chainstate::m_snapshot_entry variable is a good idea. I don’t like how it has different meanings for different chainstates (starting point of validation for the snapshot chainstate, and ending point for the background chainstate), and the lack of documentation, and the complicated initialization which happens in the Chainstate constructor for the snapshot chainstate, but also in ActivateSnapshot for both chainstates, and then again in LoadBlockIndex for both chainstates, and pointedly not in ActivateExistingSnapshot according to a new comment there saying it will be initialized later… It just seems like a lot of complexity for new variable which is redundant with the existing m_from_snapshot_blockhash variable and is only used one place, in Chainstate::TryAddBlockIndexCandidate.

I think it would be better if Chainstate::TryAddBlockIndexCandidate just used snapshot chainstate’s m_from_snapshot_blockhash value directly. Or if that is too inefficient, we could add a m_from_snapshot_blockindex member that mirrors m_from_snapshot_blockhash and is const and could be initialized straightforwardly in the ChainState constructor.

I agree with your criticisms that the way this is written is pretty confusing (it took me a while to get it right in the first place, and I found this hard to reason about).

I wanted to capture the idea that the hash m_from_snapshot_blockhash must always be in our block index, so that we never have to worry when we look it up in the block index that it might be missing, and we have to deal with a nullptr case.

However, I couldn’t see how to make it a const that is straightforwardly initialized in Chainstate, because there are two different cases to consider (activating a snapshot for the first time, versus detecting that we have a snapshot on restart).

Also, while this PR introduces one place where this CBlockIndex is used, I expect that we’ll use it again in the net_processing code which will download blocks toward the snapshot base.

Here are a few options I can think of to improve this:

• just look the hash up every time we need to use it (both in TryAddBlockIndexCandidate and wherever it is needed in net_processing, likely one additional place), avoiding the storage of redundant information
• lazily fill it in so that we only look it up once, and then use the looked-up value for future requests
• move the knowledge of the snapshot base to ChainstateManager, so that we don’t store redundant information in both chainstates (this could be done along with either of the first two ideas, I think?)

EDIT: Gah, I just noticed ChainstateManager::GetSnapshotBaseBlock(), didn’t realize we already had that. I think I know what to do now to fix this!

re: #27746 (review)

EDIT: Gah, I just noticed ChainstateManager::GetSnapshotBaseBlock(), didn’t realize we already had that. I think I know what to do now to fix this!

Thanks, I didn’t realize how clunky ChainstateManager initialization was when I made that suggestion. Particularly how it allowed construction with that snapshot block that wasn’t actually in the index. I think initialization can definitely be simplified later. But even without doing that I experimented a little and came up with a change that adds CS::SnapshotBase() and CSM::ActiveSnapshotBase() methods that I think should provide a good interface for external code to use, while allowing us to clean up the internal representation and init process later:

  0--- a/src/validation.h
  1+++ b/src/validation.h
  2@@ -528,12 +528,8 @@ protected:
  3     //! is set to true on the snapshot chainstate.
  4     bool m_disabled GUARDED_BY(::cs_main) {false};
  5 
  6-    void UpdateSnapshotBaseEntry(const CBlockIndex *entry) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
  7-        { m_snapshot_entry = entry; }
  8-
  9-    //! Track the snapshot entry
 10-    const CBlockIndex* m_snapshot_entry GUARDED_BY(::cs_main) {nullptr};
 11-
 12+    //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash)
 13+    const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main) {nullptr};
 14 
 15 public:
 16     //! Reference to a BlockManager instance which itself is shared across all
 17@@ -586,6 +582,13 @@ public:
 18      */
 19     const std::optional<uint256> m_from_snapshot_blockhash;
 20 
 21+    /**
 22+     * The base of the snapshot this chainstate was created from.
 23+     *
 24+     * nullptr if this chainstate was not created from a snapshot.
 25+     */
 26+    const CBlockIndex* SnapshotBase() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 27+
 28     //! Return true if this chainstate relies on blocks that are assumed-valid. In
 29     //! practice this means it was created based on a UTXO snapshot.
 30     bool reliesOnAssumedValid() { return m_from_snapshot_blockhash.has_value(); }
 31@@ -1093,6 +1096,11 @@ public:
 32         return m_snapshot_chainstate && m_ibd_chainstate && m_ibd_chainstate->m_disabled;
 33     }
 34 
 35+    const CBlockIndex* ActiveSnapshotBase() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 36+    {
 37+        return m_active_chainstate ? m_active_chainstate->SnapshotBase() : nullptr;
 38+    }
 39+
 40     /**
 41      * Import blocks from an external file
 42      *
 43--- a/src/validation.cpp
 44+++ b/src/validation.cpp
 45@@ -1579,9 +1579,13 @@ Chainstate::Chainstate(
 46       m_chainman(chainman),
 47       m_from_snapshot_blockhash(from_snapshot_blockhash)
 48 {
 49-    if (m_from_snapshot_blockhash) {
 50-        m_snapshot_entry = WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(*m_from_snapshot_blockhash));
 51-    }
 52+}
 53+
 54+const CBlockIndex* Chainstate::SnapshotBase()
 55+{
 56+    if (!m_from_snapshot_blockhash) return nullptr;
 57+    if (!m_cached_snapshot_base) m_cached_snapshot_base = Assert(m_chainman.m_blockman.LookupBlockIndex(*m_from_snapshot_blockhash));
 58+    return m_cached_snapshot_base;
 59 }
 60 
 61 void Chainstate::InitCoinsDB(
 62@@ -3423,8 +3427,9 @@ void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex)
 63             // For the background chainstate, we only consider connecting blocks
 64             // towards the snapshot base (which can't be nullptr or else we'll
 65             // never make progress).
 66-            assert(m_snapshot_entry != nullptr);
 67-            if (m_snapshot_entry->GetAncestor(pindex->nHeight) == pindex) {
 68+            const CBlockIndex* snapshot_base{m_chainman.ActiveSnapshotBase()};
 69+            assert(snapshot_base != nullptr);
 70+            if (snapshot_base->GetAncestor(pindex->nHeight) == pindex) {
 71                 setBlockIndexCandidates.insert(pindex);
 72             }
 73         }
 74@@ -4416,14 +4421,6 @@ bool ChainstateManager::LoadBlockIndex()
 75         bool ret{m_blockman.LoadBlockIndexDB()};
 76         if (!ret) return false;
 77 
 78-        // If we already have 2 chainstates, then we need to update the
 79-        // snapshot base to point to the correct block entry.
 80-        if (IsSnapshotActive()) {
 81-            CBlockIndex *entry = m_blockman.LookupBlockIndex(*SnapshotBlockhash());
 82-            m_ibd_chainstate->UpdateSnapshotBaseEntry(entry);
 83-            m_snapshot_chainstate->UpdateSnapshotBaseEntry(entry);
 84-        }
 85-
 86         m_blockman.ScanAndUnlinkAlreadyPrunedFiles();
 87 
 88         std::vector<CBlockIndex*> vSortedByHeight{m_blockman.GetAllBlockIndices()};
 89@@ -5142,8 +5139,6 @@ bool ChainstateManager::ActivateSnapshot(
 90             m_snapshot_chainstate->CoinsTip().DynamicMemoryUsage() / (1000 * 1000));
 91 
 92         this->MaybeRebalanceCaches();
 93-
 94-        m_ibd_chainstate->m_snapshot_entry = m_snapshot_chainstate->m_snapshot_entry;
 95     }
 96     return true;
 97 }
 98@@ -5623,8 +5618,6 @@ Chainstate& ChainstateManager::ActivateExistingSnapshot(CTxMemPool* mempool, uin
 99         std::make_unique<Chainstate>(mempool, m_blockman, *this, base_blockhash);
100     LogPrintf("[snapshot] switching active chainstate to %s\n", m_snapshot_chainstate->ToString());
101     m_active_chainstate = m_snapshot_chainstate.get();
102-    // Note: m_snapshot_entry will be populated after the block index is
103-    // loaded; see ChainstateManager::LoadBlockIndex.
104     return *m_snapshot_chainstate;
105 }
106 

(General note on https://github.com/bitcoin/bitcoin/pull/27746/commits/59b085f4d8bb5120c14b055ce394c06fe129dc60)

Reviewers may be interested to take a look at a related future commit in #27596, https://github.com/bitcoin/bitcoin/pull/27596/commits/906ae54c11d873cfc1803078b9ce7822fc00bfd4, where I propose adding chainstate_role as a simple enum parameter to FindBlockPos() so that we may segment block storage by chainstate type.

It doesn’t seem necessary to me, because the block arrival information is something that seems unrelated to setting up a chainstate (and I don’t see why it would need to be reset in the event of a chainstate activation).

I think if we need to reset the counters it would be for tests, which is the only place outside of init.cpp where we use this… I thought I addressed adding ResetBlockSequenceCounters() where it was necessary, but I should look at that again to see if I missed a case.

In commit “Move block-arrival information / preciousblock counters to ChainstateManager” (471da5f6e74bac71aeffe2ebc5faff145a6cbcea)

The “reload of the block index” comment now seems out of date. And actually I don’t see why this ClearBlockIndexCandidates call is necessary here. After the InitializeChainstate call above, the active chainstate should be newly created and setBlockIndexCandidates should already be empty. So I think it would be good to drop this ClearBlockIndexCandidates call, or add a new comment to explain what it’s doing.

https://github.com/bitcoin/bitcoin/pull/27746/commits/943bd665e2523e5cd483aa2a77a511b967dee509

If you happen to retouch, could ActiveChain().Tip() -> ActiveTip().

https://github.com/bitcoin/bitcoin/pull/27746/commits/943bd665e2523e5cd483aa2a77a511b967dee509

If you happen to retouch, ActiveHeight().

https://github.com/bitcoin/bitcoin/pull/27746/commits/943bd665e2523e5cd483aa2a77a511b967dee509

Probably no need to use ActiveChainstate() for these (vs. this->) given the TODO above, but seems fine.

re: #27746 (review)

Probably no need to use ActiveChainstate() for these (vs. this->) given the TODO above, but seems fine.

Marking resolved since later commit “Move CheckBlockIndex() from Chainstate to ChainstateManager” (f921887c1afdcd333cf71ca25e5efbf4991f806e) drops this.

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896)

It seems like NotifyHeaderTip is a “block-storage-related” function just like other functions that are moving from Chainstate to ChainstateManager so it would be cleaner if NotifyHeaderTip took a ChainstateManager argument instead of a Chainstate one, and if it called ActiveChainstate() internally so NotifyHeaderTip callers don’t have to consider how the UI reflects background validation.

re: #27746 (comment), #27746 (review)

I wasn’t sure if that was something to do in this PR or a future one?

I think it would be a good as a followup. Doing it wouldn’t simplify this PR so it’s probably good not to add it here.

https://github.com/bitcoin/bitcoin/pull/27746/commits/55e27ee995944d188d7453c6e78a6538f6739d64

If you retrouch, I think you could remove this line and just do ... snapshot_entry = Assert(m_chainman.GetSnapshotBaseBlock());

Also could move the snapshot_entry initialization out of the loop if this gets slow for some reason.

https://github.com/bitcoin/bitcoin/pull/27746/commits/55e27ee995944d188d7453c6e78a6538f6739d64

Maybe not necessary in lieu of this != &ActiveChainstate()? Not hurting anything though.

https://github.com/bitcoin/bitcoin/pull/27746/commits/2d20f75b0bad0b680cd45fc057c54704ac2a9d66

Not entirely true since we have a small usage (by way of IsAssumedValid()) in LoadBlockIndex().

Edit: oops nevermind, not as of the fixup commit: https://github.com/bitcoin/bitcoin/pull/27746/commits/78fcc8fbdc510c03dd021e6c34a10e6b35fc2c83

In commit “Move block-arrival information / preciousblock counters to ChainstateManager” (0dbc5805ca4371b7621bee0b7a553dac1a481614)

It would be nice to have a documentation comment for this function saying what it is for, especially since it is only called in a test. If the counter values are basically arbitrary and only used for ordering, I’m not sure why if they actually ever need to be reset, or if that’s just a nice thing to do?

In commit “Move block-arrival information / preciousblock counters to ChainstateManager” (0dbc5805ca4371b7621bee0b7a553dac1a481614)

IIUC removing this line is not a change in behavior because the nBlockSequenceId variable was never used again after the UnloadBlockIndex call, outside of tests. Now there is new ResetBlockSequenceCounters function that tests can use to reset it separately. EDIT: Another review comment #27746 (review) also seems to say resetting nBlockSequenceId is unnecessary outside of tests

It might be good to say explicitly in commit message that this commit is supposed to be a refactoring which does not change behavior.

In commit “test: Clear block index flags when testing snapshots” (e8168cd56c490609f59e1dc89add58b1be18b516)

This seems like a good change that makes test setup more realistic, but the could the commit message be updated to say what is motivating this change? Is it just for realism, or will a future change, or a certain kind of test depend on it?

I’d say mostly for realism. My intuition for how this is designed is that if you don’t reset the HAVE_DATA flags, then you’d immediately expect the background chain to process and validate all the blocks for which we HAVE_DATA and thus arrive at the original tip, as soon as you call ActivateBestChain().

Updated commit message.

In commit “test: Clear block index flags when testing snapshots” (e8168cd56c490609f59e1dc89add58b1be18b516)

Adding BLOCK_ASSUMED_VALID here seems surprising since I would expect ActivateSnapshot below to be setting BLOCK_ASSUMED_VALID, and comment above doesn’t mention this flag. I think it would be good to drop this line or add a comment explaining why it is necessary.

In commit “Move block-arrival information / preciousblock counters to ChainstateManager” (0dbc5805ca4371b7621bee0b7a553dac1a481614)

The test seems to pass without this line, at least in this commit.

If this is needed for a future change, would be good to say that in commit message. Or if this is just done for completeness would be good to say that in a code comment.

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896)

Notes: It seems logical to call GetAll here for all chainstates and let the TryAddBlockIndexCandidate and FindMostWorkChain functions figure out how to use the block and which chain to attach it to.

There is a change in behavior here if there are multiple chainstates, because previously this would only add blocks to setBlockIndexCandidates for the active chain, and now it will add all blocks with enough work to setBlockIndexCandidates for active and background chains.

I think this means it will now add blocks above the snapshot height to the background chainstate, which ChainstateManager::LoadBlockIndex was trying to avoid. This is inefficient, but not really a problem because Chainstate::FindMostWorkChain will ignore and remove the blocks when it finds their ancestors have missing data.

Followup commit f18ab9ee7deb92c25bdc68a5c4bef973f5e995f0 “Tighten requirements for adding elements to setBlockIndexCandidates” will make this more efficient by not adding these blocks to setBlockIndexCandidates

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896)

Note: This change to the CheckBlockIndex() call is temporary and will revert back in later commit fb9c405506d6cfb6896e21b79789d7b6e638f6f2 “Move CheckBlockIndex() from Chainstate to ChainstateManager” when it becomes a ChainstateManager method.

There is not a change in behavior here in this commit, because in both places where Chainstate::AcceptBlock was called previously (in ProcessNewBlock and LoadExternalBlockFile), it was only called on the active chainstate.

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896)

Do you think it would be good if the IsInitialBlockDownload method were moved to ChainstateManager. Is there any place it should be actually called on the background chainstate rather than the active chainstate? Could add a TODO comment if IsInitialBlockDownload method should move like other methods are moving.

I haven’t done a careful review of all the places we use IsInitialBlockDownload() to confirm this, but my intuition would be that if we’re using an assumeutxo snapshot, what we care about is whether our snapshot-based chainstate is caught up or not – basically by definition, the background chainstate will always be “in initial block download” until it finishes and is no longer in use, so it wouldn’t make sense to condition anything on whether it’s behind or not.

So yeah I think moving this to ChainstateManager would probably make sense…

re: #27746 (review)

Thanks, it makes sense that the background chainstate only has one state because it is deleted as soon as it finishes syncing.

Would be great if a followup PR moved IsInitialBlockDownload from Chainstate class to ChainstateManager so the IsInitialBlockDownload implementation could refer directly to the active chainstate, and IsInitialBlockDownload callers wouldn’t need to figure out which chainstate to call it on.

In commit “Fix initialization of setBlockIndexCandidates when working with multiple chainstates” (c2bc5cfd9977838af334844675d77090fe9c3c74)

Can add a comment here explaining the logic? I guess the pindex == GetSnapshotBaseBlock() case needs to be a special case because the snapshot block may not have transactions?

Maybe also would be more efficient to call ActiveSnapshotBase instead of GetSnapshotBaseBlock here. I think I would move the commit introducing the ActiveSnapshotBase method earlier, so it’s also not necessary to replace GetSnapshotBaseBlock with ActiveSnapshotBase in later commit “Tighten requirements for adding elements to setBlockIndexCandidates” (ddb77d838c4cd7a4776c5ec70d96331fc8137900)

We may not have received the snapshot base block, and it’s only when we call ReceivedBlockTransactions on a block that we set VALID_TRANSACTIONS on the block index entry. So the snapshot base block may not otherwise be considered a block index candidate, but the snapshot base block could be the tip of the snapshot chain so we need to be able to call TryAddBlockIndexCandidate().

I’ll push an update that adds a comment explaining this.

4a9f0dae7ff64ba2c2f1eb1b44d0ca563726b776

Nit: feel like it may have been okay to leave in the previous assertion too (although this is sorted of indirectly tested by the expected_assumed_valid comparison). No need to change.

 0diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
 1index a936a96577..f0ed54a521 100644
 2--- a/src/test/validation_chainstatemanager_tests.cpp
 3+++ b/src/test/validation_chainstatemanager_tests.cpp
 4@@ -464,6 +464,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
 5         if (i == last_assumed_valid_idx - 1) {
 6             assumed_base = index;
 7             BOOST_CHECK(index->IsAssumedValid());
 8+        } else if (i == last_assumed_valid_idx) {
 9+            BOOST_CHECK(!index->IsAssumedValid());
10         }
11     }

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896)

It seems like fHasMoreOrSameWork will always be false for blocks that were downloaded to be added to the background chain, because they will have less work than the active snapshot chain.

Is that intentional? It would helpful to have a comment here either that either explains why this ok, or adds a TODO indicating that there will be some change later.

I do understand there is not a change in behavior here in this commit because in both places where Chainstate::AcceptBlock was called previously (in ProcessNewBlock and LoadExternalBlockFile), it was only called on the active chainstate, so it is still only looking at the active chainstate now.

The idea here is that this check is part of a heuristic that we use when we receive a block that was not requested. For anti-DoS reasons, back in #5875 we introduced logic to generally not process unrequested blocks, but we left in an optimization that was intended to not throw away blocks that would immediately advance our tip, in case we had some peer (like Matt’s old fast-relay-network, if I remember right) that was sending such blocks (see eg comment here #5875 (comment)).

My view is that we don’t need to process unrequested blocks in general, and that if we do so it’s just an optimization to make block relay at the tip work better. We could probably add special-case logic to allow processing of unrequested blocks that we haven’t yet validated that are ancestors of the ancestor block, but this doesn’t strike me as something that is at all necessary (and perhaps not desirable?).

re: https://github.com/bitcoin/bitcoin/pull/27746/commits/c8f4a8702851c3097ebd038a814d974dce4dcc78#r1251271941

Thanks, that makes sense, and thanks for linking to #5875.

To make this less confusing, I think “Try to process all requested blocks” code comment above could be updated to mention the active chain and better match the code.

Would suggest: “// Check all requested blocks that we do not already have for validity and save them to disk. Skip processing of unrequested blocks as an anti-Dos measure, unless the blocks have more work than the active chain tip, and aren’t too far ahead of it, so are likely to be attached soon.”

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896)

Note: It seems fine to drop this assert, because the only code that is using this mutex is the ActivateBestChain function called below, and ActivateBestChain already has this same assert at the top of the function.

In commit “Move block-storage-related logic to ChainstateManager” (034f920d1755cff752d46b089ad37bdd703bf896)

I think it would be a little clearer to move the BlockValidationState state; variable declaration down into the for loop so it is obvious the variable is ignored and not used to share state between ActivateBestChain calls

I must either be missing something about this line or it might be unnecessary. If I understand correctly this is updating m_undo_height_in_last_blockfile when the next block is connected after the flush happened. However, why not update it right away when the flush happens? I tried that by removing this line it seems like all tests are still passing without it.

It’s not surprising that there’s no test coverage for this. But to explain: the goal of the code is to flush the undo file when the height of the highest block in the undo file equals the height of the highest block in the corresponding block file. There are two cases where this can happen. The first case is on line 633 above, where the undo file and block file are flushed together because the undo data is not far behind the block data. The second case is on line 737 above when undo data is delayed and undo file gets flushed after the block file. The m_undo_height_in_last_blockfile needs to be set here so the m_blockfile_info[nFile].nHeightLast == m_undo_height_in_last_blockfile check in the first case line 633 can work.

In commit “Tighten requirements for adding elements to setBlockIndexCandidates” (eb8ce95a2551fe521d00a5901ea773514395255b)

Note: test here is changed to add the real block 1, instead of a random block, so the block will not be ignored by the new snapshot_base->GetAncestor(pindex->nHeight) == pindex check in TryAddBlockIndexCandidate

In commit “Tighten requirements for adding elements to setBlockIndexCandidates” (eb8ce95a2551fe521d00a5901ea773514395255b)

Is this comment in the commit message still accurate?

Note that this introduces the new invariant that we can only have an assumeutxo snapshot where the snapshotted blockhash is in our block index. As a followup, unit tests that mock creating a snapshot ought to be updated to reflect this.

The only way I can see that it relates to code changes in this commit is the new Assert on this line, and it’s confusing because I would have not have thought loading a snapshot with an unknown block hash worked before this code change, either. Also I’m not sure what unit tests “ought to be updated” means if tests are already updated this commit.

I think it would be good to drop this comment, or add more specifics about what’s actually changing here and what followup needs to be done later.

EDIT: Rereading this, I guess the commit message is just referring to an internal change that affects unit tests, not a bigger change that affects what snapshots are able to be loaded. Maybe a a clearer commit message would be: “After this change, ActivateExistingSnapshot needs to be passed a valid block hash to avoid assert errors when accepting new blocks.” Maybe it would be good to add an assert to ActivateExistingSnapshot to catch potential problems with this earlier, too.

I agree that it would be good to add an assert to catch this earlier, but we discussed before that due to the different ways that chainstates can be constructed and the order in which snapshot block hashes can be passed in (relative to when the block index is populated on startup), that there wasn’t a clear way to do this at an earlier point in the code.

I’ll update the code comment though to make it more clear what will break if an invalid hash is passed in.

In commit “Tighten requirements for adding elements to setBlockIndexCandidates” (eb8ce95a2551fe521d00a5901ea773514395255b)

Note: IIUC, it’s necessary to load the genesis block here so the ActivateExistingSnapshot call below can be passed a known block hash of a block in m_block_index, not a random block.

In commit “Tighten requirements for adding elements to setBlockIndexCandidates” (eb8ce95a2551fe521d00a5901ea773514395255b)

Seems like it might be useful to check BOOST_CHECK_EQUAL(exp_tip, exp_tip2); now to verify ActivateBestChain actually did arrive at the expected CBlockIndex*, not just the right block block hash.

In commit “Tighten requirements for adding elements to setBlockIndexCandidates” (b0b48b3e986319fe8cd5a80052099070279ce5c0)

Note: It seems like this LoadGenesis/SetBestBlock step was never necessary, so good to drop now. Dropping it maybe makes the test a little less realistic, but simplifies things and keeps the test more focused on verifying cache sizes.

In commit “Fix initialization of setBlockIndexCandidates when working with multiple chainstates” (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

It would be good to remove the reliesOnAssumedValid() function since it is no longer used after this.

In commit “Fix initialization of setBlockIndexCandidates when working with multiple chainstates” (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

Note: the reason why all entries were added previously was because pindex->nHeight < first_assumed_valid_height check was always true, because first_assumed_valid_height was std::numeric_limits<int>::max()