assumeutxo: Remove BLOCK_ASSUMED_VALID flag

Flag adds complexity and is not currently used for anything.

4 files changed, 14 insertions, 62 deletions

diff --git a/src/chain.h b/src/chain.h
index 7faeb25088..bb70dbd8bc 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -128,21 +128,8 @@ enum BlockStatus : uint32_t {
 
     BLOCK_OPT_WITNESS        =   128, //!< block data in blk*.dat was received with a witness-enforcing client
 
-    /**
-     * 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 may have
-     * descendant blocks with VALID_SCRIPTS set, because they can be validated
-     * based on an assumeutxo snapshot.
-     *
-     * When an assumeutxo snapshot is loaded, the ASSUMED_VALID flag is added to
-     * unvalidated blocks at the snapshot height and below. 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,
+    BLOCK_STATUS_RESERVED    =   256, //!< Unused flag that was previously set on assumeutxo snapshot blocks and their
+                                      //!< ancestors before they were validated, and unset when they were validated.
 };
 
 /** The block chain is a tree shaped structure starting with the
@@ -316,14 +303,6 @@ public:
         return ((nStatus & BLOCK_VALID_MASK) >= nUpTo);
     }
 
-    //! @returns true if the block is assumed-valid; this means it is queued to be
-    //!   validated by a background chainstate.
-    bool IsAssumedValid() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
-    {
-        AssertLockHeld(::cs_main);
-        return nStatus & BLOCK_ASSUMED_VALID;
-    }
-
     //! Raise the validity level of this block index entry.
     //! Returns true if the validity was changed.
     bool RaiseValidity(enum BlockStatus nUpTo) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
@@ -333,12 +312,6 @@ public:
         if (nStatus & BLOCK_FAILED_MASK) return false;
 
         if ((nStatus & BLOCK_VALID_MASK) < nUpTo) {
-            // If this block had been marked assumed-valid and we're raising
-            // its validity to a certain point, there is no longer an assumption.
-            if (nStatus & BLOCK_ASSUMED_VALID && nUpTo >= BLOCK_VALID_SCRIPTS) {
-                nStatus &= ~BLOCK_ASSUMED_VALID;
-            }
-
             nStatus = (nStatus & ~BLOCK_VALID_MASK) | nUpTo;
             return true;
         }
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 26f9ab59a6..4bf66a55eb 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -276,9 +276,6 @@ struct SnapshotTestSetup : TestChain100Setup {
             BOOST_CHECK_EQUAL(
                 *node::ReadSnapshotBaseBlockhash(found),
                 *chainman.SnapshotBlockhash());
-
-            // Ensure that the genesis block was not marked assumed-valid.
-            BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid());
         }
 
         const auto& au_data = ::Params().AssumeutxoForHeight(snapshot_height);
@@ -410,7 +407,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, SnapshotTestSetup)
 //! - First, verify that setBlockIndexCandidates is as expected when using a single,
 //!   fully-validating chainstate.
 //!
-//! - Then mark a region of the chain BLOCK_ASSUMED_VALID and introduce a second chainstate
+//! - Then mark a region of the chain as missing data and introduce a second chainstate
 //!   that will tolerate assumed-valid blocks. Run LoadBlockIndex() and ensure that the first
 //!   chainstate only contains fully validated blocks and the other chainstate contains all blocks,
 //!   except those marked assume-valid, because those entries don't HAVE_DATA.
@@ -421,7 +418,6 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
     Chainstate& cs1 = chainman.ActiveChainstate();
 
     int num_indexes{0};
-    int num_assumed_valid{0};
     // Blocks in range [assumed_valid_start_idx, last_assumed_valid_idx) will be
     // marked as assumed-valid and not having data.
     const int expected_assumed_valid{20};
@@ -456,37 +452,30 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
     reload_all_block_indexes();
     BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), 1);
 
-    // Mark some region of the chain assumed-valid, and remove the HAVE_DATA flag.
+    // Reset some region of the chain's nStatus, removing the HAVE_DATA flag.
     for (int i = 0; i <= cs1.m_chain.Height(); ++i) {
         LOCK(::cs_main);
         auto index = cs1.m_chain[i];
 
-        // Blocks with heights in range [91, 110] are marked ASSUMED_VALID
+        // Blocks with heights in range [91, 110] are marked as missing data.
         if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) {
-            index->nStatus = BlockStatus::BLOCK_VALID_TREE | BlockStatus::BLOCK_ASSUMED_VALID;
+            index->nStatus = BlockStatus::BLOCK_VALID_TREE;
             index->nTx = 0;
             index->nChainTx = 0;
         }
 
         ++num_indexes;
-        if (index->IsAssumedValid()) ++num_assumed_valid;
 
         // Note the last fully-validated block as the expected validated tip.
         if (i == (assumed_valid_start_idx - 1)) {
             validated_tip = index;
-            BOOST_CHECK(!index->IsAssumedValid());
         }
         // Note the last assumed valid block as the snapshot base
         if (i == last_assumed_valid_idx - 1) {
             assumed_base = index;
-            BOOST_CHECK(index->IsAssumedValid());
-        } else if (i == last_assumed_valid_idx) {
-            BOOST_CHECK(!index->IsAssumedValid());
         }
     }
 
-    BOOST_CHECK_EQUAL(expected_assumed_valid, num_assumed_valid);
-
     // Note: cs2's tip is not set when ActivateExistingSnapshot is called.
     Chainstate& cs2 = WITH_LOCK(::cs_main,
         return chainman.ActivateExistingSnapshot(*assumed_base->phashBlock));
diff --git a/src/validation.cpp b/src/validation.cpp
index f27f47aea0..a01b3da9ca 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5048,7 +5048,6 @@ void ChainstateManager::CheckBlockIndex()
     CBlockIndex* pindexFirstNotTransactionsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TRANSACTIONS (regardless of being valid or not), since assumeutxo snapshot if used.
     CBlockIndex* pindexFirstNotChainValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_CHAIN (regardless of being valid or not), since assumeutxo snapshot if used.
     CBlockIndex* pindexFirstNotScriptsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_SCRIPTS (regardless of being valid or not), since assumeutxo snapshot if used.
-    CBlockIndex* pindexFirstAssumeValid = nullptr; // Oldest ancestor of pindex which has BLOCK_ASSUMED_VALID
 
     // After checking an assumeutxo snapshot block, reset pindexFirst pointers
     // to earlier blocks that have not been downloaded or validated yet, so
@@ -5068,7 +5067,6 @@ void ChainstateManager::CheckBlockIndex()
 
     while (pindex != nullptr) {
         nNodes++;
-        if (pindexFirstAssumeValid == nullptr && pindex->nStatus & BLOCK_ASSUMED_VALID) pindexFirstAssumeValid = pindex;
         if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;
         if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
             pindexFirstMissing = pindex;
@@ -5115,7 +5113,7 @@ void ChainstateManager::CheckBlockIndex()
             if (pindex->nStatus & BLOCK_HAVE_DATA) assert(pindex->nTx > 0);
         }
         if (pindex->nStatus & BLOCK_HAVE_UNDO) assert(pindex->nStatus & BLOCK_HAVE_DATA);
-        if (pindex->IsAssumedValid()) {
+        if (snap_base && snap_base->GetAncestor(pindex->nHeight) == pindex) {
             // Assumed-valid blocks should connect to the main chain.
             assert((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_TREE);
         }
@@ -5271,7 +5269,6 @@ void ChainstateManager::CheckBlockIndex()
             if (pindex == pindexFirstNotTransactionsValid) pindexFirstNotTransactionsValid = nullptr;
             if (pindex == pindexFirstNotChainValid) pindexFirstNotChainValid = nullptr;
             if (pindex == pindexFirstNotScriptsValid) pindexFirstNotScriptsValid = nullptr;
-            if (pindex == pindexFirstAssumeValid) pindexFirstAssumeValid = nullptr;
             // Find our parent.
             CBlockIndex* pindexPar = pindex->pprev;
             // Find which child we just visited.
@@ -5757,22 +5754,14 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
     // Fake various pieces of CBlockIndex state:
     CBlockIndex* index = nullptr;
 
-    // Don't make any modifications to the genesis block.
-    // This is especially important because we don't want to erroneously
-    // apply BLOCK_ASSUMED_VALID to genesis, which would happen if we didn't skip
-    // it here (since it apparently isn't BLOCK_VALID_SCRIPTS).
+    // Don't make any modifications to the genesis block since it shouldn't be
+    // neccessary, and since the genesis block doesn't have normal flags like
+    // BLOCK_VALID_SCRIPTS set.
     constexpr int AFTER_GENESIS_START{1};
 
     for (int i = AFTER_GENESIS_START; i <= snapshot_chainstate.m_chain.Height(); ++i) {
         index = snapshot_chainstate.m_chain[i];
 
-        // Mark unvalidated block index entries beneath the snapshot base block as assumed-valid.
-        if (!index->IsValid(BLOCK_VALID_SCRIPTS)) {
-            // This flag will be removed once the block is fully validated by a
-            // background chainstate.
-            index->nStatus |= BLOCK_ASSUMED_VALID;
-        }
-
         // Fake BLOCK_OPT_WITNESS so that Chainstate::NeedsRedownload()
         // won't ask to rewind the entire assumed-valid chain on startup.
         if (DeploymentActiveAt(*index, *this, Consensus::DEPLOYMENT_SEGWIT)) {
diff --git a/src/validation.h b/src/validation.h
index 71aac46f81..57e0777a2a 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -583,9 +583,10 @@ public:
     const CBlockIndex* SnapshotBase() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /**
-     * The set of all CBlockIndex entries with either BLOCK_VALID_TRANSACTIONS (for
-     * itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if using background
-     * chainstates) and as good as our current tip or better. Entries may be failed,
+     * The set of all CBlockIndex entries that have as much work as our current
+     * tip or more, and transaction data needed to be validated (with
+     * BLOCK_VALID_TRANSACTIONS for each block and its parents back to the
+     * genesis block or an assumeutxo snapshot block). Entries may be failed,
      * though, and pruning nodes may be missing the data for the block.
      */
     std::set<CBlockIndex*, node::CBlockIndexWorkComparator> setBlockIndexCandidates;