Rework validation logic for assumeutxo

[sdaftuar]

This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation.

This PR also fixes a bug in how a chainstate's setBlockIndexCandidates was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating setBlockIndexCandidates in some scenarios involving multiple chainstates.

Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well.

[jamesob]

Great! Thanks for this. I'm active in these neighborhoods today as well, since adding some -stopatheight/restart logic to the functional tests in #27596 has turned up some issues with CheckBlockIndex(), which are maybe related to some of the changes here. So I'll be looking at this shortly.

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, jamesob, Sjors, ryanofsky
Concept ACK	dergoegge
Stale ACK	fjahr, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28052 (blockstorage: XOR blocksdir *.dat files by MarcoFalke)
• #27866 (blockstorage: Return on fatal flush errors by TheCharlatan)
• #27460 (rpc: Add importmempool RPC by MarcoFalke)

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.

[Sjors]

The first two commits up to f50fa24 look good to me.

How do you see the division of labor between ChainstateManager and BlockManager? The ChainState doc currently says:

Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in BlockManager.

In any case moving AcceptBlock from Chainstate to ChainstateManager seems an improvement.

[Sjors]

If you can split 534b6f9 in ~half that would make it a bit easier to review. Maybe start with anything that's trivial to move, and then do the more involved changes in the second commit.

[Sjors]

534b6f9 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?

[sdaftuar]

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.

[ryanofsky]

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.

[sdaftuar]

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.

[sdaftuar]

How do you see the division of labor between ChainstateManager and BlockManager? The ChainState doc currently says:

Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in BlockManager.

I think of BlockManager as being a generic backend database of block (and undo) data, while any validation-related logic should live in Chainstate (if it relies on chainstate-specific information) or somewhere else (either a static function in validation or in ChainstateManager as I propose here) if not.

I think AcceptBlock could also be a static function in validation.cpp; I mostly just wanted to get it out of Chainstate because I think that having block storage be coupled to a particular chainstate is a confusing idea, but I can go either way on whether it belongs in ChainstateManager or not.

[sdaftuar]

@Sjors @achow101 Thanks for the review! I split up the big commit as @Sjors suggested, and with the test fixes I think this is ready to be moved out of Draft status.

[mzumsande]

Concept ACK

[mzumsande]

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?

[sdaftuar]

Agreed, good idea.

[jamesob]

55e27ee

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

[sdaftuar]

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!

[achow101]

ACK f02125f

[ryanofsky]

Partial code review ACK f02125f. I've looked at most of this, but not the whole thing yet, and all the changes seem very nice. This should help avoid bugs and make assumeutxo code more maintainable. I think it would be good to clean up documentation as part of this too. Here are documentation changes I would suggest:

--- a/doc/design/assumeutxo.md
+++ b/doc/design/assumeutxo.md
@@ -17,10 +17,9 @@ respectively generate and load UTXO snapshots. The utility script
 
 - A new block index `nStatus` flag is introduced, `BLOCK_ASSUMED_VALID`, to mark block
   index entries that are required to be assumed-valid by a chainstate created
-  from a UTXO snapshot. This flag is mostly used as a way to modify certain
+  from a UTXO snapshot. This flag is used as a way to modify certain
   CheckBlockIndex() logic to account for index entries that are pending validation by a
-  chainstate running asynchronously in the background. We also use this flag to control
-  which index entries are added to setBlockIndexCandidates during LoadBlockIndex().
+  chainstate running asynchronously in the background.
 
 - The concept of UTXO snapshots is treated as an implementation detail that lives
   behind the ChainstateManager interface. The external presentation of the changes
diff --git a/src/chain.h b/src/chain.h
index f5dd0fd31514..cb8bc7f04792 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -113,10 +113,10 @@ enum BlockStatus : uint32_t {
     BLOCK_VALID_TRANSACTIONS =    3,
 
     //! Outputs do not overspend inputs, no double spends, coinbase output ok, no immature coinbase spends, BIP30.
-    //! Implies all parents are also at least CHAIN.
+    //! Implies all parents are either at least VALID_CHAIN, or are ASSUMED_VALID
     BLOCK_VALID_CHAIN        =    4,
 
-    //! Scripts & signatures ok. Implies all parents are also at least SCRIPTS.
+    //! Scripts & signatures ok. Implies all parents are either at least VALID_SCRIPTS, or are ASSUMED_VALID.
     BLOCK_VALID_SCRIPTS      =    5,
 
     //! All validity bits.
@@ -134,10 +134,18 @@ enum BlockStatus : uint32_t {
     BLOCK_OPT_WITNESS        =   128, //!< block data in blk*.dat was received with a witness-enforcing client
 
     /**
-     * If set, this indicates that the block index entry is assumed-valid.
-     * Certain diagnostics will be skipped in e.g. CheckBlockIndex().
-     * It almost certainly means that the block's full validation is pending
-     * on a background chainstate. See `doc/design/assumeutxo.md`.
+     * If ASSUMED_VALID is set, it means that this block has not been validated
+     * and has validity status less than VALID_SCRIPTS. Also that it has
+     * descendant blocks with VALID_SCRIPTS set, because they were validated
+     * based on an assumeutxo snapshot.
+     *
+     * When an assumeutxo snapshot is loaded, the ASSUMED_VALID flag is added to
+     * unvalidated blocks below the snapshot height. Then, as the background
+     * validation progresses, and these blocks are validated, the ASSUMED_VALID
+     * flags are removed. See `doc/design/assumeutxo.md` for details.
+     *
+     * This flag is only used to implement checks in CheckBlockIndex() and
+     * should not be used elsewhere.
      */
     BLOCK_ASSUMED_VALID      =   256,
 };

---

re: PR description #27746 (comment)

base block is: the block hash

Grammar here seems off, maybe replace with "base block, and the base block hash"

---

re: PR description #27746 (comment)

This code needs a lot of cleanup and test fixes and so I've marked it as a draft -- help from reviewers would be welcome!

Could drop this part since PR seems to be a good state to review and merge.

---

re: #27746 (comment)

I think AcceptBlock could also be a static function in validation.cpp

IMO, would be nice make it a standalone function (static or not). Unless a function is really tied to one particular class or needs access to its private state, better for it just be standalone and use the public interface.

[ryanofsky]

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.

[ryanofsky]

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.

[jamesob]

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.

[sdaftuar]

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!

[ryanofsky]

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;
 }
 

[ryanofsky]

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.

[Sjors]

Comments around 768690b.

It would be nice if the documentation for setBlockIndexCandidates and m_blocks_unlinked explained their purpose (as opposed to merely describing what's in them).

/** 
* The set of all CBlockIndex entries that are - or recently
* were - candidates for a new tip. These are as good as our
* current tip or better, and must either have BLOCK_VALID_TRANSACTIONS
* (for itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if using
* background chainstates). Entries may be failed, though, and
* pruning nodes may be missing the data for the block.
*/
… setBlockIndexCandidates

/**
* CBlockIndex entries removed from setBlockIndexCandidates due
* to pruned data for themselves or their ancestors.
*/
… m_blocks_unlinked`

(The above still doesn't satisfyingly explain why we hold them in m_blocks_unlinked rather than treat them as any other block index entry. I assume it's because we use the list to proactively aks for these blocks?)

Suggested documentation for TryAddBlockIndexCandidate:

/**
* Add CBlockIndex entry to setBlockIndexCandidates if it has more
* work than the current tip. For the background chainstate, we only
* consider connecting blocks towards the snapshot base 
*/

[Sjors]

768690b: Maybe clarify that initially this only applies to the snapshot chainstate, which uses the base block as its starting point. For the background chainstate, as well an active chainstate not created from a snapshot, GetSnapshotBaseBlock() is nullptr so the special-case won't apply. But the background state will consider it a candidate once it's been downloaded.

[ryanofsky]

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.

[Sjors]

768690b

//!
//! - Run LoadBlockIndex() and ensure that setBlockIndexCandidates of the first ...

[Sjors]

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.

[ryanofsky]

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.

[Sjors]

Code review ACK a733dd7.

My comments above can all wait for a followup, if any.

I'd like to take this for a spin in a rebased #27596 with -checkblockindex to see if we didn't miss anything (without pruning, since that has a clear TODO).

[Sjors]

d0d40ea Maybe add: Background validation always ignores unrequested blocks.

[Sjors]

d0d40ea: // We don't relay background validation blocks.

[ryanofsky]

re: #27746 (review)

(The above still doesn't satisfyingly explain why we hold them in m_blocks_unlinked rather than treat them as any other block index entry. I assume it's because we use the list to proactively aks for these blocks?)

I also found this pretty confusing when I was reviewing it and found a helpful explanation here: https://en.bitcoin.it/wiki/Bitcoin_Core_0.11_(ch_6):_The_Blockchain. The description of m_blocks_unlinked (which used to be mapBlocksUnlinked) is:

Multimap containing "all pairs A->B, where A (or one if its ancestors) misses transactions, but B has transactions." (comment at main.cpp:125).

The purpose of mapBlocksUnlinked is to quickly attach blocks we've already received to the blockchain when we receive a missing, intermediate block.

The alternative would be to search the entire mapBlockIndex; however, it is more efficient to keep track of unlinked blocks in a separate data structure.

So I think the map just exists for efficiency, not to proactively do anything. It would be great to improve documentation in the actual code, though.

[ryanofsky]

Code review ACK a733dd7. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.

[ryanofsky]

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.

[ryanofsky]

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);

[ryanofsky]

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.

[ryanofsky]

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."

[ryanofsky]

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.

[TheCharlatan]

Post-merge ACK a733dd7

Very happy with the separation of concerns between blockstorage and validation.

[fjahr]

Post-merge code review ACK a733dd7

I also synced and kept a node running for a few days with the status of my previous ACK and didn't see anything unusual.

[ryanofsky]

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:

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.

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:

bitcoin/src/validation.cpp

Line 4153 in c5a63ea

m_chain.SetTip(*pindex);

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

bitcoin/src/validation.cpp

Lines 2913 to 2914 in c5a63ea

	while (pindexTest && !m_chain.Contains(pindexTest)) {
	assert(pindexTest->HaveTxsDownloaded() || pindexTest->nHeight == 0);

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.

[jamesob]

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.

[ryanofsky]

re: #27746 (comment)

The way I'm reading e code:

bitcoin/src/validation.cpp

Lines 2913 to 2914 in c5a63ea

	while (pindexTest && !m_chain.Contains(pindexTest)) {
	assert(pindexTest->HaveTxsDownloaded() || pindexTest->nHeight == 0);

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?

[jamesob]

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.

[jamesob]

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.

[ryanofsky]

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?

[jamesob]

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

[jamesob]

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