Merge 98dce62e11fe3b75b8f354f607ad2852e0caf861 into db2c57ae9eebdb75c58cd165ac929919969c19a9

src/node/blockstorage.cpp

@@ -1190,6 +1190,21 @@ void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_
 {
     ImportingNow imp{chainman.m_blockman.m_importing};
 
+    auto update_all_chainstate = [&]() {
+        // scan for better chains in the block chain database, that are not yet connected in the active best chain
+
+        // We can't hold cs_main during ActivateBestChain even though we're accessing
+        // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
+        // the relevant pointers before the ABC call.
+        for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
+            BlockValidationState state;
+            if (!chainstate->ActivateBestChain(state, nullptr)) {
+                chainman.GetNotifications().fatalError(strprintf(_("Failed to connect best block (%s)."), state.ToString()));
+                return;
+            }
+        }
+    };
+
     // -reindex
     if (!chainman.m_blockman.m_blockfiles_indexed) {
         int nFile = 0;
@@ -1213,6 +1228,11 @@ void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_
             }
             nFile++;
         }
+
+        // Call ActivateBestChain here so we can skip script verification
+        // during reindex if assumevalid is enabled
+        update_all_chainstate();
+
         WITH_LOCK(::cs_main, chainman.m_blockman.m_block_tree_db->WriteReindexing(false));
         chainman.m_blockman.m_blockfiles_indexed = true;
         LogPrintf("Reindexing finished\n");
@@ -1235,18 +1255,7 @@ void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_
         }
     }
 
-    // scan for better chains in the block chain database, that are not yet connected in the active best chain
-
-    // We can't hold cs_main during ActivateBestChain even though we're accessing
-    // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
-    // the relevant pointers before the ABC call.
-    for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
-        BlockValidationState state;
-        if (!chainstate->ActivateBestChain(state, nullptr)) {
-            chainman.GetNotifications().fatalError(strprintf(_("Failed to connect best block (%s)."), state.ToString()));
-            return;
-        }
-    }
+    update_all_chainstate();
     // End scope of ImportingNow
 }
 

src/node/blockstorage.h

@@ -353,6 +353,7 @@ public:
     static constexpr auto PRUNE_TARGET_MANUAL{std::numeric_limits<uint64_t>::max()};
 
     [[nodiscard]] bool LoadingBlocks() const { return m_importing || !m_blockfiles_indexed; }
+    [[nodiscard]] bool IsReindexing() const { return m_importing && !m_blockfiles_indexed; }
 
     /** Calculate the amount of disk space the block & undo files currently use */
     uint64_t CalculateCurrentUsage();

src/validation.cpp

@@ -2499,7 +2499,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
         if (it != m_blockman.m_block_index.end()) {
             if (it->second.GetAncestor(pindex->nHeight) == pindex &&
                 m_chainman.m_best_header->GetAncestor(pindex->nHeight) == pindex &&
-                m_chainman.m_best_header->nChainWork >= m_chainman.MinimumChainWork()) {
+                (m_chainman.m_best_header->nChainWork >= m_chainman.MinimumChainWork() || m_chainman.m_blockman.IsReindexing())) {
                 // This block is a member of the assumed verified chain and an ancestor of the best header.
                 // Script verification is skipped when connecting blocks under the
                 // assumevalid block. Assuming the assumevalid block is valid this
@@ -2513,9 +2513,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
                 //  that are hardly doing any signature verification at all in testing without having to
                 //  artificially set the default assumed verified block further back.
                 // The test against the minimum chain work prevents the skipping when denied access to any chain at
-                //  least as good as the expected chain.
+                // least as good as the expected chain.
+                // During a reindex, skip the minimumchainwork check because the previous IBD run may have been interrupted
+                // before it could connect enough blocks to reach the minimumchainwork
                 fScriptChecks = (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= 60 * 60 * 24 * 7 * 2);
             }
+        } else if (m_chainman.m_blockman.IsReindexing()) {
+            // During a reindex, the assumed valid block may not be in the index
+            // if the previous IBD run was interrupted before it downloaded the assume valid block.
+            fScriptChecks = false;
         }
     }
 

test/functional/feature_reindex.py

@@ -11,12 +11,36 @@
 """
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.messages import MAGIC_BYTES
+from test_framework.blocktools import (
+    COINBASE_MATURITY,
+    create_block,
+    create_coinbase,
+)
+from test_framework.messages import (
+    MAGIC_BYTES,
+    CBlockHeader,
+    COutPoint,
+    CTransaction,
+    CTxIn,
+    CTxOut,
+    msg_block,
+    msg_headers,
+)
+from test_framework.p2p import P2PInterface
+from test_framework.script import (
+    CScript,
+    OP_TRUE,
+)
 from test_framework.util import (
     assert_equal,
     util_xor,
 )
 
+class BaseNode(P2PInterface):
+    def send_header_for_blocks(self, new_blocks):
+        headers_message = msg_headers()
+        headers_message.headers = [CBlockHeader(b) for b in new_blocks]
+        self.send_message(headers_message)
 
 class ReindexTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -97,6 +121,77 @@ class ReindexTest(BitcoinTestFramework):
             node.wait_for_rpc_connection(wait_for_import=False)
         node.stop_node()
 
+    # Check that reindex uses -assumevalid
+    def assume_valid(self):
+        self.log.info("Testing reindex with -assumevalid")
+        self.start_node(0)
+        node = self.nodes[0]
+        self.generate(node, 1)
+        coinbase_to_spend = node.getblock(node.getbestblockhash())['tx'][0]
+        # Generate COINBASE_MATURITY blocks to make the coinbase spendable
+        self.generate(node, COINBASE_MATURITY)
+        tip = int(node.getbestblockhash(), 16)
+        block_info = node.getblock(node.getbestblockhash())
+        block_time = block_info['time'] + 1
+        height = block_info['height'] + 1
+        blocks = []
+
+        # Create a block with an invalid signature
+        tx = CTransaction()
+        tx.vin.append(CTxIn(COutPoint(int(coinbase_to_spend, 16), 0), scriptSig=b""))
+        tx.vout.append(CTxOut(49 * 10000, CScript([OP_TRUE])))
+        tx.calc_sha256()
+        invalid_block = create_block(tip, create_coinbase(height), block_time, txlist=[tx])
+        invalid_block.solve()
+        tip = invalid_block.sha256
+        blocks.append(invalid_block)
+
+        # Bury that block with roughly two weeks' worth of blocks (2100 blocks) and an extra 100 blocks
+        for _ in range(2200):
+            block_time += 1
+            height += 1
+            block = create_block(tip, create_coinbase(height), block_time)
+            block.solve()
+            blocks.append(block)
+            tip = block.sha256
+
+        # Start node0 with -assumevalid pointing to the block with invalid signature
+        node.stop_node()
+        self.start_node(0, extra_args=["-assumevalid=" + blocks[-1].hash])
+
+        # Send blocks to node0
+        p2p0 = node.add_p2p_connection(BaseNode())
+        p2p0.send_header_for_blocks(blocks[0:2000])
+        p2p0.send_header_for_blocks(blocks[2000:])
+        # Only send the first 2100 blocks so the assumevalid block is not saved to disk before we reindex
+        # This allows us to test that assumevalid is used in reindex even if the assumevalid block was not loaded during reindex
+        for block in blocks[:2101]:
+            p2p0.send_message(msg_block(block))
+        p2p0.sync_with_ping(timeout=960)
+
+        expected_height = height - 100
+        # node0 should sync to tip of the chain, ignoring the invalid signature
+        assert_equal(node.getblock(node.getbestblockhash())['height'], expected_height)
+
+        self.log.info("Testing reindex with assumevalid block in block index and minchainwork > best_header chainwork")
+        # Restart node0 but raise minimumchainwork to that of a chain 32256 blocks long
+        node.stop_node()
+        self.start_node(0, extra_args=["-reindex", "-assumevalid=" + blocks[2100].hash, "-minimumchainwork=00000000000000000000000000000000000000000000000000007e01acd42dd2"])
+
+        # node0 should sync to tip of the chain, ignoring the invalid signature
+        assert_equal(node.getblock(node.getbestblockhash())['height'], expected_height)
+        assert_equal(int(node.getbestblockhash(), 16), blocks[2100].sha256)
+
+        self.log.info("Testing reindex with assumevalid block not in block index")
+        node.stop_node()
+        self.start_node(0, extra_args=["-reindex", "-assumevalid=" + blocks[-1].hash])
+
+        # node0 should still sync to tip of the chain, ignoring the invalid signature
+        assert_equal(node.getblock(node.getbestblockhash())['height'], expected_height)
+        assert_equal(int(node.getbestblockhash(), 16), blocks[2100].sha256)
+
+        self.log.info("Success")
+
     def run_test(self):
         self.reindex(False)
         self.reindex(True)
@@ -105,6 +200,7 @@ class ReindexTest(BitcoinTestFramework):
 
         self.out_of_order()
         self.continue_reindex_after_shutdown()
+        self.assume_valid()
 
 
 if __name__ == '__main__':