From 6f37260131dac972e0137a767c96ed08afc6226c Mon Sep 17 00:00:00 2001 From: Eunovo Date: Tue, 7 Jan 2025 11:30:56 +0000 Subject: [PATCH 1/2] validation.cpp: use assumevalid during reindex -reindex deletes the block tree db and attempts to rebuild it based on local block files, without attempting to re-request headers for which we don't have the full block yet This might cause: - the best known chain to have a chainwork less than the minimumchainwork - the assumedvalid block to not be in the block index if the previous IBD was interrupted before downloading it --- src/node/blockstorage.cpp | 33 +++++++++++++++++++++------------ src/node/blockstorage.h | 1 + src/validation.cpp | 10 ++++++++-- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 6f1a96c8279..654fa9338a9 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -1192,6 +1192,21 @@ void ImportBlocks(ChainstateManager& chainman, std::span 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; @@ -1215,6 +1230,11 @@ void ImportBlocks(ChainstateManager& chainman, std::span 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"); @@ -1237,18 +1257,7 @@ void ImportBlocks(ChainstateManager& chainman, std::span 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 } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 8a34efadfea..6ae4ebe9739 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -353,6 +353,7 @@ public: static constexpr auto PRUNE_TARGET_MANUAL{std::numeric_limits::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(); diff --git a/src/validation.cpp b/src/validation.cpp index 64588e802d7..66b07740aec 100644 --- a/src/validation.cpp +++ b/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; } } From 98dce62e11fe3b75b8f354f607ad2852e0caf861 Mon Sep 17 00:00:00 2001 From: Eunovo Date: Mon, 6 Jan 2025 18:44:17 +0000 Subject: [PATCH 2/2] test: Check that reindex always uses -assumevalid --- test/functional/feature_reindex.py | 98 +++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py index 1ebfe82da54..c0961716120 100755 --- a/test/functional/feature_reindex.py +++ b/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__':