Merge bitcoin/bitcoin#27746: Rework validation logic for assumeutxo

a733dd79e29068ad1e0532ac42a45188a040a7b9 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar)
d4a11abb1972b54f0babdccfbb2fde97ab885933 Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar)
3556b850221bc0e597d7dd749d4d47ab58dc8083 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar)
0ce805b632dcb98944a931f758f76f530f5ce5f2 Documentation improvements for assumeutxo (Ryan Ofsky)
768690b7ce551cd403f8e2a099372915f6022ad4 Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar)
d43a1f1a2fa35d377c7a9ad7ab92d1ae325bde3d Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar)
d0d40ea9a6478d81d7531b7cfc52a8bdaa0883d6 Move block-storage-related logic to ChainstateManager (Suhas Daftuar)
3cfc75366e6596942cbc84f354f42dfd7fc5c073 test: Clear block index flags when testing snapshots (Suhas Daftuar)
272fbc370c4e133d31d9f1d34e327cc265c5fad2 Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar)
10c05710ce1602d932037f72dc6c4bbc3f6f34ba Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar)
471da5f6e74bac71aeffe2ebc5faff145a6cbcea Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar)
1cfc887d00c5d1d4281107e3b3ff4641c6c34631 Remove CChain dependency in node/blockstorage (Suhas Daftuar)
fe86a7cd480b32463da900db764d2d11a2bea095 Explicitly track maximum block height stored in undo files (Suhas Daftuar)

Pull request description:

  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.

ACKs for top commit:
  achow101:
    ACK a733dd79e29068ad1e0532ac42a45188a040a7b9
  jamesob:
    reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic))
  Sjors:
    Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9.
  ryanofsky:
    Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. 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.

Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675

3 files changed, 25 insertions, 11 deletions

diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 78416ec576..0d25c798ce 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -618,7 +618,7 @@ fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const
     return BlockFileSeq().FileName(pos);
 }
 
-bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown)
+bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown)
 {
     LOCK(cs_LastBlockFile);
 
@@ -644,7 +644,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
             // when the undo file is keeping up with the block file, we want to flush it explicitly
             // when it is lagging behind (more blocks arrive than are being connected), we let the
             // undo block write case handle it
-            finalize_undo = (m_blockfile_info[nFile].nHeightLast == (unsigned int)active_chain.Tip()->nHeight);
+            finalize_undo = (m_blockfile_info[nFile].nHeightLast == m_undo_height_in_last_blockfile);
             nFile++;
             if (m_blockfile_info.size() <= nFile) {
                 m_blockfile_info.resize(nFile + 1);
@@ -660,6 +660,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
         }
         FlushBlockFile(!fKnown, finalize_undo);
         m_last_blockfile = nFile;
+        m_undo_height_in_last_blockfile = 0; // No undo data yet in the new file, so reset our undo-height tracking.
     }
 
     m_blockfile_info[nFile].AddBlock(nHeight, nTime);
@@ -749,8 +750,9 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid
         // the FindBlockPos function
         if (_pos.nFile < m_last_blockfile && static_cast<uint32_t>(block.nHeight) == m_blockfile_info[_pos.nFile].nHeightLast) {
             FlushUndoFile(_pos.nFile, true);
+        } else if (_pos.nFile == m_last_blockfile && static_cast<uint32_t>(block.nHeight) > m_undo_height_in_last_blockfile) {
+            m_undo_height_in_last_blockfile = block.nHeight;
         }
-
         // update nUndoPos in block index
         block.nUndoPos = _pos.nPos;
         block.nStatus |= BLOCK_HAVE_UNDO;
@@ -839,7 +841,7 @@ bool BlockManager::ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatF
     return true;
 }
 
-FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const FlatFilePos* dbp)
+FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, const FlatFilePos* dbp)
 {
     unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION);
     FlatFilePos blockPos;
@@ -852,7 +854,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CCha
         // we add BLOCK_SERIALIZATION_HEADER_SIZE only for new blocks since they will have the serialization header added when written to disk.
         nBlockSize += static_cast<unsigned int>(BLOCK_SERIALIZATION_HEADER_SIZE);
     }
-    if (!FindBlockPos(blockPos, nBlockSize, nHeight, active_chain, block.GetBlockTime(), position_known)) {
+    if (!FindBlockPos(blockPos, nBlockSize, nHeight, block.GetBlockTime(), position_known)) {
         error("%s: FindBlockPos failed", __func__);
         return FlatFilePos();
     }
@@ -905,7 +907,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
                     break; // This error is logged in OpenBlockFile
                 }
                 LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
-                chainman.ActiveChainstate().LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
+                chainman.LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
                 if (chainman.m_interrupt) {
                     LogPrintf("Interrupt requested. Exit %s\n", __func__);
                     return;
@@ -924,7 +926,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
             FILE* file = fsbridge::fopen(path, "rb");
             if (file) {
                 LogPrintf("Importing blocks file %s...\n", fs::PathToString(path));
-                chainman.ActiveChainstate().LoadExternalBlockFile(file);
+                chainman.LoadExternalBlockFile(file);
                 if (chainman.m_interrupt) {
                     LogPrintf("Interrupt requested. Exit %s\n", __func__);
                     return;
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index c2e903e470..eb40d45aba 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -24,7 +24,6 @@ class BlockValidationState;
 class CBlock;
 class CBlockFileInfo;
 class CBlockUndo;
-class CChain;
 class CChainParams;
 class Chainstate;
 class ChainstateManager;
@@ -94,7 +93,7 @@ private:
         EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     void FlushBlockFile(bool fFinalize = false, bool finalize_undo = false);
     void FlushUndoFile(int block_file, bool finalize = false);
-    bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown);
+    bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown);
     bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize);
 
     FlatFileSeq BlockFileSeq() const;
@@ -128,6 +127,19 @@ private:
     RecursiveMutex cs_LastBlockFile;
     std::vector<CBlockFileInfo> m_blockfile_info;
     int m_last_blockfile = 0;
+
+    // Track the height of the highest block in m_last_blockfile whose undo
+    // data has been written. 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 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.
+    unsigned int m_undo_height_in_last_blockfile = 0;
+
     /** Global flag to indicate we should check to see if there are
      *  block/undo files that should be deleted.  Set on startup
      *  or if we allocate more file space when we're in prune mode
@@ -202,7 +214,7 @@ public:
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /** Store block on disk. If dbp is not nullptr, then it provides the known position of the block within a block file on disk. */
-    FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const FlatFilePos* dbp);
+    FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, const FlatFilePos* dbp);
 
     /** Whether running in -prune mode. */
     [[nodiscard]] bool IsPruneMode() const { return m_prune_mode; }
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 255d8be0ec..0828f64856 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -221,7 +221,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
 
         // A reload of the block index is required to recompute setBlockIndexCandidates
         // for the fully validated chainstate.
-        chainman.ActiveChainstate().UnloadBlockIndex();
+        chainman.ActiveChainstate().ClearBlockIndexCandidates();
 
         auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options);
         if (init_status != ChainstateLoadStatus::SUCCESS) {