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 (comment)

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 (comment) 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.

how about inserting and returning here in the !is_bg_chainstate case, so that we can skip the GetAncestor() call when dealing with the snapshot chainstate?

Agreed, good idea.

55e27ee

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

Good idea -- and it looks like my code here is wrong anyway, because if we're a disabled background chainstate, then we'll be adding blocks to setBlockIndexCandidates as long as they have more work than the tip!

In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (b0b48b3)

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

In commit "Explicitly track snapshot block header when using multiple chainstates" (d2ccbd3)

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 think I concur with @ryanofsky here. I'll have to double check, but in actual usage in #27596, I think I found that the one usage of this had another (existing) way of getting at this data. I'm afk for a few days, but will verify when I get back.

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 (comment)

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:

--- a/src/validation.h
+++ b/src/validation.h
@@ -528,12 +528,8 @@ protected:
     //! is set to true on the snapshot chainstate.
     bool m_disabled GUARDED_BY(::cs_main) {false};
 
-    void UpdateSnapshotBaseEntry(const CBlockIndex *entry) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
-        { m_snapshot_entry = entry; }
-
-    //! Track the snapshot entry
-    const CBlockIndex* m_snapshot_entry GUARDED_BY(::cs_main) {nullptr};
-
+    //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash)
+    const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main) {nullptr};
 
 public:
     //! Reference to a BlockManager instance which itself is shared across all
@@ -586,6 +582,13 @@ public:
      */
     const std::optional<uint256> m_from_snapshot_blockhash;
 
+    /**
+     * The base of the snapshot this chainstate was created from.
+     *
+     * nullptr if this chainstate was not created from a snapshot.
+     */
+    const CBlockIndex* SnapshotBase() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
     //! Return true if this chainstate relies on blocks that are assumed-valid. In
     //! practice this means it was created based on a UTXO snapshot.
     bool reliesOnAssumedValid() { return m_from_snapshot_blockhash.has_value(); }
@@ -1093,6 +1096,11 @@ public:
         return m_snapshot_chainstate && m_ibd_chainstate && m_ibd_chainstate->m_disabled;
     }
 
+    const CBlockIndex* ActiveSnapshotBase() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+    {
+        return m_active_chainstate ? m_active_chainstate->SnapshotBase() : nullptr;
+    }
+
     /**
      * Import blocks from an external file
      *
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1579,9 +1579,13 @@ Chainstate::Chainstate(
       m_chainman(chainman),
       m_from_snapshot_blockhash(from_snapshot_blockhash)
 {
-    if (m_from_snapshot_blockhash) {
-        m_snapshot_entry = WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(*m_from_snapshot_blockhash));
-    }
+}
+
+const CBlockIndex* Chainstate::SnapshotBase()
+{
+    if (!m_from_snapshot_blockhash) return nullptr;
+    if (!m_cached_snapshot_base) m_cached_snapshot_base = Assert(m_chainman.m_blockman.LookupBlockIndex(*m_from_snapshot_blockhash));
+    return m_cached_snapshot_base;
 }
 
 void Chainstate::InitCoinsDB(
@@ -3423,8 +3427,9 @@ void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex)
             // For the background chainstate, we only consider connecting blocks
             // towards the snapshot base (which can't be nullptr or else we'll
             // never make progress).
-            assert(m_snapshot_entry != nullptr);
-            if (m_snapshot_entry->GetAncestor(pindex->nHeight) == pindex) {
+            const CBlockIndex* snapshot_base{m_chainman.ActiveSnapshotBase()};
+            assert(snapshot_base != nullptr);
+            if (snapshot_base->GetAncestor(pindex->nHeight) == pindex) {
                 setBlockIndexCandidates.insert(pindex);
             }
         }
@@ -4416,14 +4421,6 @@ bool ChainstateManager::LoadBlockIndex()
         bool ret{m_blockman.LoadBlockIndexDB()};
         if (!ret) return false;
 
-        // If we already have 2 chainstates, then we need to update the
-        // snapshot base to point to the correct block entry.
-        if (IsSnapshotActive()) {
-            CBlockIndex *entry = m_blockman.LookupBlockIndex(*SnapshotBlockhash());
-            m_ibd_chainstate->UpdateSnapshotBaseEntry(entry);
-            m_snapshot_chainstate->UpdateSnapshotBaseEntry(entry);
-        }
-
         m_blockman.ScanAndUnlinkAlreadyPrunedFiles();
 
         std::vector<CBlockIndex*> vSortedByHeight{m_blockman.GetAllBlockIndices()};
@@ -5142,8 +5139,6 @@ bool ChainstateManager::ActivateSnapshot(
             m_snapshot_chainstate->CoinsTip().DynamicMemoryUsage() / (1000 * 1000));
 
         this->MaybeRebalanceCaches();
-
-        m_ibd_chainstate->m_snapshot_entry = m_snapshot_chainstate->m_snapshot_entry;
     }
     return true;
 }
@@ -5623,8 +5618,6 @@ Chainstate& ChainstateManager::ActivateExistingSnapshot(CTxMemPool* mempool, uin
         std::make_unique<Chainstate>(mempool, m_blockman, *this, base_blockhash);
     LogPrintf("[snapshot] switching active chainstate to %s\n", m_snapshot_chainstate->ToString());
     m_active_chainstate = m_snapshot_chainstate.get();
-    // Note: m_snapshot_entry will be populated after the block index is
-    // loaded; see ChainstateManager::LoadBlockIndex.
     return *m_snapshot_chainstate;
 }
 

re: #27746 (comment)

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 (comment) 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.

re: #27746 (comment)

GetSnapshotBaseBlock() is nullptr so the special-case won't apply

I may be misunderstanding, but I don't think it's true that GetSnapshotBaseBlock is null for the background chainstate. This is a ChainstateManager method, so GetSnapshotBaseBlock is going to be non-null if any snapshot is loaded, and this code will try to add the snapshot block to both chainstates, even though it's not needed for the background chainstate. It might be clearer if the code narrowed the special case, and only added the snapshot block to the snapshot chainstate where it was actually needed, not to both chainstates. But that might make the code more complicated.

I do think it might be better if the comment said "assumeutxo snapshot chainstate" instead of "assumeutxo-based chainstate" to be more specific about when this special case is needed, but I don't think it need to go into detail about other cases where the check is harmless and not needed.

In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b)

Would be good to directly verify the snapshot block is included:

// block before the snapshot is excluded (as well as earlier blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base->pprev), 0); 
// snapshot block is included (as well as later blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base), 1);

re: #27746 (comment)

768690b: this had me confused, but it's because the test considers a scenario where the snapshot base block has been downloaded. It could be nice to include the moment before it's downloaded where this should be 0.

This review comment does not match my understanding. The snapshot block is assumed_base at height 39 and it does not have data. This line is checking assumed_tip block at height 100 which does have data.

I think all the variable names in this test are pretty confusing, and it would probably be clearer if blocks were referred to directly by height, but that's the was the test style, so it's not changing.

In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b)

I think it's a little misleading to say the other chainstate contains all block except those marked assume-valid because:

• It actually does contain one block marked assumed-valid, which is the snapshot base (block 39)
• It also excludes other blocks which are not assumed-valid because they don't have more work than the tip (blocks 1-19)

I think this comment could incorporate sjors suggestion above and just say "Run LoadBlockIndex() and ensure that setBlockIndexCandidates of the first chainstate only contains blocks leading to the snapshot base, and that setBlockIndexCandidates of the second chainstate only contains blocks following the snapshot base."

Or could leave out the details and say "Run LoadBlockIndex() and check setBlockIndexCandidates of both chainstates."

re: #27746 (comment)

0ce805b: why isn't BLOCK_VALID_TRANSACTIONS also alternatively ASSUMED_VALID?

I'm not sure what this review comment is asking. The code comment above still seems accurate to me, but it maybe it could be improved.

In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b)

It looks like there may be a bug in this code because this pindex == GetSnapshotBaseBlock() special case will add the snapshot block to setBlockIndexCandidates whether or not it has data, but there is also an assert in FindMostWorkChain which asserts that blocks in setBlockIndexCandidates do have data:

I think it's not possible to hit this assert for the snapshot chainstate, but it may be possible to hit this assert for the background chainstate.

For the snapshot chainstate, I think the assert won't be reached because after the snapshot is loaded the Chainstate::LoadChainTip function should immediately set the chain tip to the snapshot block index:

so the m_chain.Contains(pindexTest) check before the assert will be false and the assert will not run:

But for the background chainstate, it seems possible assert could be hit if FindMostWorkChain is called before the snapshot block is downloaded.

In any case, this special pindex == GetSnapshotBaseBlock() case seems overbroad. It seems like it should only apply to the snapshot chainstate, not the background chainstate, and I'm not actually sure why it is necessary to have at all if Chainstate::LoadChainTip is already going to set the snapshot block as the snapshot chainstate tip.

I think the interaction between these checks and the checks inside the TryAddBlockIndexCandidate are also unnecessarily confusing, and it would be nice whether there is a bug or not to move all the move all the checks inside TryAddBlockIndexCandidate and call it unconditionally. This should make it easeir to avoid adding the snapshot block the background chainstate candidate set before it is downloaded. It would also be great to drop the snapshot block special case entirely if it is not needed.

It looks like there may be a bug in this code because this pindex == GetSnapshotBaseBlock() special case will add the snapshot block to setBlockIndexCandidates whether or not it has data, but there is also an assert in FindMostWorkChain which asserts that blocks in setBlockIndexCandidates do have data:

bitcoin/src/validation.cpp
Line 2914 in c5a63ea

assert(pindexTest->HaveTxsDownloaded() || pindexTest->nHeight == 0); 

I think it's not possible to hit this assert for the snapshot chainstate, but it may be possible to hit this assert for the background chainstate.

If I'm reading FindMostWorkChain() correctly, this assert won't be hit for a background chainstate, because the second clause of this condition

while (pindexTest && !m_chain.Contains(pindexTest)) {

will exclude the snapshot base block from consideration without data (since it is not in the background chainstate's m_chain until it is actually downloaded, and so has data). So as far as I can tell, there's no bug here - but I'm glad you're double checking this; the setBlockIndexCandidates code is very confusing indeed, and I'm amenable to any way we can make it more easily understandable.

re: #27746 (comment)

The way I'm reading e code:

If a snapshot is loaded and pindexTest points to the snapshot block and the snapshot block is not yet downloaded, and m_chain is the background chain, then m_chain.Contains(pindexTest) will be false, so the while loop condition will be true, and the assert could be be hit.

You seem to be saying that m_chain.Contains(pindexTest) will be false, so the while condition won't be hit?

I'm sorry, my previous comment was more or less incorrect. Another reason why the assert you mention doesn't pose a problem is because, confusingly, HaveTxsDownloaded() is just an alias for nChainTx != 0. Since we manually set nChainTx for the snapshot base block (315586f) this check (again, very confusingly) passes. At some point long ago I made a note somewhere about fixing that method's misleading name...

So the long story short is that FindMostWorkChain() eventually works for the background chainstate because it detects, correctly, that there are blocks with missing data between the IBD tip and the snapshot base, and correctly rewinds m_chain back to the actual IBD tip. I think when @sdaftuar first proposed this change I was confused about why the LoadBlockIndex() simplification could actually work, and this is the reason.

So the long story short is that FindMostWorkChain() eventually works for the background chainstate because it detects, correctly, that there are blocks with missing data between the IBD tip and the snapshot base, and correctly rewinds m_chain back to the actual IBD tip.

Again I'm wrong: snapshot base block never makes it into background chainstate's m_chain because LoadChainTip() (when called from the init code in src/node/chainstate.cpp) sets the background chainstate's tip to the correct IBD tip (on the basis of the coinscache GetBestBlock()), which is before the snapshot base block.

Thanks, I missed that HaveTxsDownloaded was checking nChainTx not nTx.

I guess the main thing I'm confused about now is why this pindex == GetSnapshotBaseBlock() || special case is needed at all? If LoadChainTip() already sets the snapshot block to be the snapshot tip, why does the snapshot block need to be added to the candidate set?

I guess the main thing I'm confused about now is why this pindex == GetSnapshotBaseBlock() || special case is needed at all? If LoadChainTip() already sets the snapshot block to be the snapshot tip, why does the snapshot block need to be added to the candidate set?

I'm not sure if there's a more fundamental reason, but various assertions fail if this case is stripped out of LoadBlockIndex(); e.g. the !setBlockIndexCandidates.empty() assertion fails during PruneBlockIndexCandidates() if this special case is not included.

The true purpose of setBlockIndexCandidates has always been somewhat mysterious to me, but the most essential use seems to be in FindMostWorkChain(), to supply candidates for activation in ActivateBestChain().

I've added some documentation for HaveTxsDownloaded() (9f9ffb6), and I'm happy to add more for the confusing process discussed above.