Merge e6e170cf6c67a56b9c14cece66fdc4fab5f3ec6b into 5f4422d68dc3530c353af1f87499de1c864b60ad

3301d2cbe8c3b76c97285d75fa59637cb6952d0b qa: Wait for txindex to avoid race condition (Hodlinator)
9bfb0d75ba10591cc6c9620f9fd1ecc0e55e7a48 qa: Remove unnecessary -txindex args (Hodlinator)
7ac281c19cd3d11f316dbbb3308eabf1ad4f26d6 qa: Add missing coverage of corrupt indexes (Hodlinator)

Pull request description:

  - Add synchronization in 3 places where if the Transaction Index happens to be slow, we get rare test failures when querying it for transactions (one such case experienced on Windows, prompting investigation).
  - Remove unnecessary TxIndex initialization in some tests.
  - Add some test coverage where TxIndex aspect could be tested in feature_init.py.

ACKs for top commit:
  fjahr:
    re-ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
  mzumsande:
    Code Review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
  furszy:
    Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
  Prabhat1308:
    Concept ACK [`3301d2c`](3301d2cbe8)

Tree-SHA512: 7c2019e38455f344856aaf6b381faafbd88d53dc88d13309deb718c1dcfbee4ccca7c7f1b66917395503a6f94c3b216a007ad432cc8b93d0309db9805f38d602

src/interfaces/mining.h

@@ -7,7 +7,7 @@
 
 #include <consensus/amount.h>       // for CAmount
 #include <interfaces/types.h>       // for BlockRef
-#include <node/types.h>             // for BlockCreateOptions, BlockWaitOptions
+#include <node/types.h>             // for BlockCreateOptions, BlockWaitOptions, BlockCheckOptions
 #include <primitives/block.h>       // for CBlock, CBlockHeader
 #include <primitives/transaction.h> // for CTransactionRef
 #include <stdint.h>                 // for int64_t
@@ -106,6 +106,20 @@ public:
      */
     virtual std::unique_ptr<BlockTemplate> createNewBlock(const node::BlockCreateOptions& options = {}) = 0;
 
+    /**
+     * Checks if a given block is valid.
+     *
+     * @param[in] block       the block to check
+     * @param[in] options     verification options: the proof-of-work check can be
+     *                        skipped in order to verify a template generated by
+     *                        external software.
+     * @param[out] reason     failure reason (BIP22)
+     * @returns               whether the block is valid
+     *
+     * For signets the challenge verification is skipped when check_pow is false.
+     */
+    virtual bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason) = 0;
+
     //! Get internal node context. Useful for RPC and testing,
     //! but not accessible across processes.
     virtual node::NodeContext* context() { return nullptr; }

src/ipc/capnp/mining-types.h

@@ -14,13 +14,7 @@
 #include <validation.h>
 
 namespace mp {
-// Custom serialization for BlockValidationState.
-void CustomBuildMessage(InvokeContext& invoke_context,
-                        const BlockValidationState& src,
-                        ipc::capnp::messages::BlockValidationState::Builder&& builder);
-void CustomReadMessage(InvokeContext& invoke_context,
-                       const ipc::capnp::messages::BlockValidationState::Reader& reader,
-                       BlockValidationState& dest);
+// Custom serializations
 } // namespace mp
 
 #endif // BITCOIN_IPC_CAPNP_MINING_TYPES_H

src/ipc/capnp/mining.capnp

@@ -18,6 +18,7 @@ interface Mining $Proxy.wrap("interfaces::Mining") {
     getTip @2 (context :Proxy.Context) -> (result: Common.BlockRef, hasResult: Bool);
     waitTipChanged @3 (context :Proxy.Context, currentTip: Data, timeout: Float64) -> (result: Common.BlockRef);
     createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate);
+    checkBlock @5 (block: Data, options: BlockCheckOptions) -> (reason: Text, result: Bool);
 }
 
 interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
@@ -45,12 +46,7 @@ struct BlockWaitOptions $Proxy.wrap("node::BlockWaitOptions") {
     feeThreshold @1 : Int64 $Proxy.name("fee_threshold");
 }
 
-# Note: serialization of the BlockValidationState C++ type is somewhat fragile
-# and using the struct can be awkward. It would be good if testBlockValidity
-# method were changed to return validity information in a simpler format.
-struct BlockValidationState {
-    mode @0 :Int32;
-    result @1 :Int32;
-    rejectReason @2 :Text;
-    debugMessage @3 :Text;
+struct BlockCheckOptions $Proxy.wrap("node::BlockCheckOptions") {
+    checkMerkleRoot @0 :Bool $Proxy.name("check_merkle_root");
+    checkPow @1 :Bool $Proxy.name("check_pow");
 }

src/ipc/capnp/mining.cpp

@@ -8,40 +8,4 @@
 #include <mp/proxy-types.h>
 
 namespace mp {
-void CustomBuildMessage(InvokeContext& invoke_context,
-                        const BlockValidationState& src,
-                        ipc::capnp::messages::BlockValidationState::Builder&& builder)
-{
-    if (src.IsValid()) {
-        builder.setMode(0);
-    } else if (src.IsInvalid()) {
-        builder.setMode(1);
-    } else if (src.IsError()) {
-        builder.setMode(2);
-    } else {
-        assert(false);
-    }
-    builder.setResult(static_cast<int>(src.GetResult()));
-    builder.setRejectReason(src.GetRejectReason());
-    builder.setDebugMessage(src.GetDebugMessage());
-}
-
-void CustomReadMessage(InvokeContext& invoke_context,
-                       const ipc::capnp::messages::BlockValidationState::Reader& reader,
-                       BlockValidationState& dest)
-{
-    if (reader.getMode() == 0) {
-        assert(reader.getResult() == 0);
-        assert(reader.getRejectReason().size() == 0);
-        assert(reader.getDebugMessage().size() == 0);
-    } else if (reader.getMode() == 1) {
-        dest.Invalid(static_cast<BlockValidationResult>(reader.getResult()), reader.getRejectReason(), reader.getDebugMessage());
-    } else if (reader.getMode() == 2) {
-        assert(reader.getResult() == 0);
-        dest.Error(reader.getRejectReason());
-        assert(reader.getDebugMessage().size() == 0);
-    } else {
-        assert(false);
-    }
-}
 } // namespace mp

src/node/interfaces.cpp

@@ -1093,6 +1093,11 @@ public:
         return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
     }
 
+    bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason) override
+    {
+        return chainman().TestBlockValidity(block, reason, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root);
+    }
+
     NodeContext* context() override { return &m_node; }
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
     KernelNotifications& notifications() { return *Assert(m_node.notifications); }

src/node/miner.cpp

@@ -176,10 +176,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     pblock->nNonce         = 0;
     pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
 
-    BlockValidationState state;
-    if (m_options.test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev,
-                                                            /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
-        throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString()));
+    std::string reason;
+    if (m_options.test_block_validity && !m_chainstate.m_chainman.TestBlockValidity(*pblock, reason, /*check_pow=*/false, /*check_merkle_root=*/false)) {
+        throw std::runtime_error(strprintf("TestBlockValidity failed: %s", reason));
     }
     const auto time_2{SteadyClock::now()};
 

src/node/types.h

@@ -17,6 +17,7 @@
 #include <cstddef>
 #include <policy/policy.h>
 #include <script/script.h>
+#include <uint256.h>
 #include <util/time.h>
 
 namespace node {
@@ -85,6 +86,17 @@ struct BlockWaitOptions {
     CAmount fee_threshold{MAX_MONEY};
 };
 
+struct BlockCheckOptions {
+    /**
+     * Set false to omit the merkle root heck
+     */
+    bool check_merkle_root{true};
+
+    /**
+     * Set false to omit the proof-of-work check
+     */
+    bool check_pow{true};
+};
 } // namespace node
 
 #endif // BITCOIN_NODE_TYPES_H

src/rpc/mining.cpp

@@ -386,9 +386,9 @@ static RPCHelpMan generateblock()
         block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
         RegenerateCommitments(block, chainman);
 
-        BlockValidationState state;
-        if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
-            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
+        std::string reason;
+        if (!miner.checkBlock(block, {.check_merkle_root = false, .check_pow = false}, reason)) {
+            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", reason));
         }
     }
 
@@ -740,13 +740,9 @@ static RPCHelpMan getblocktemplate()
                 return "duplicate-inconclusive";
             }
 
-            // TestBlockValidity only supports blocks built on the current Tip
-            if (block.hashPrevBlock != tip) {
-                return "inconclusive-not-best-prevblk";
-            }
-            BlockValidationState state;
-            TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/true);
-            return BIP22ValidationResult(state);
+            std::string reason;
+            bool res{miner.checkBlock(block, {.check_pow = false}, reason)};
+            return res ? UniValue::VNULL : UniValue{reason};
         }
 
         const UniValue& aClientRules = oparam.find_value("rules");

src/test/ipc_test.capnp

@@ -19,6 +19,5 @@ interface FooInterface $Proxy.wrap("FooImplementation") {
     passUniValue @2 (arg :Text) -> (result :Text);
     passTransaction @3 (arg :Data) -> (result :Data);
     passVectorChar @4 (arg :Data) -> (result :Data);
-    passBlockState @5 (arg :Mining.BlockValidationState) -> (result :Mining.BlockValidationState);
-    passScript @6 (arg :Data) -> (result :Data);
+    passScript @5 (arg :Data) -> (result :Data);
 }

src/test/ipc_test.cpp

@@ -102,25 +102,6 @@ void IpcPipeTest()
     std::vector<char> vec2{foo->passVectorChar(vec1)};
     BOOST_CHECK_EQUAL(std::string_view(vec1.begin(), vec1.end()), std::string_view(vec2.begin(), vec2.end()));
 
-    BlockValidationState bs1;
-    bs1.Invalid(BlockValidationResult::BLOCK_MUTATED, "reject reason", "debug message");
-    BlockValidationState bs2{foo->passBlockState(bs1)};
-    BOOST_CHECK_EQUAL(bs1.IsValid(), bs2.IsValid());
-    BOOST_CHECK_EQUAL(bs1.IsError(), bs2.IsError());
-    BOOST_CHECK_EQUAL(bs1.IsInvalid(), bs2.IsInvalid());
-    BOOST_CHECK_EQUAL(static_cast<int>(bs1.GetResult()), static_cast<int>(bs2.GetResult()));
-    BOOST_CHECK_EQUAL(bs1.GetRejectReason(), bs2.GetRejectReason());
-    BOOST_CHECK_EQUAL(bs1.GetDebugMessage(), bs2.GetDebugMessage());
-
-    BlockValidationState bs3;
-    BlockValidationState bs4{foo->passBlockState(bs3)};
-    BOOST_CHECK_EQUAL(bs3.IsValid(), bs4.IsValid());
-    BOOST_CHECK_EQUAL(bs3.IsError(), bs4.IsError());
-    BOOST_CHECK_EQUAL(bs3.IsInvalid(), bs4.IsInvalid());
-    BOOST_CHECK_EQUAL(static_cast<int>(bs3.GetResult()), static_cast<int>(bs4.GetResult()));
-    BOOST_CHECK_EQUAL(bs3.GetRejectReason(), bs4.GetRejectReason());
-    BOOST_CHECK_EQUAL(bs3.GetDebugMessage(), bs4.GetDebugMessage());
-
     auto script1{CScript() << OP_11};
     auto script2{foo->passScript(script1)};
     BOOST_CHECK_EQUAL(HexStr(script1), HexStr(script2));

src/test/miner_tests.cpp

@@ -22,6 +22,7 @@
 #include <util/translation.h>
 #include <validation.h>
 #include <versionbits.h>
+#include <pow.h>
 
 #include <test/util/setup_common.h>
 
@@ -666,7 +667,38 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
-    std::unique_ptr<BlockTemplate> block_template;
+
+    // Create and check a simple template
+    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
+    BOOST_REQUIRE(block_template);
+    {
+        CBlock block{block_template->getBlock()};
+        {
+            std::string reason;
+            BOOST_REQUIRE(!mining->checkBlock(block, {.check_pow = false}, reason));
+            BOOST_REQUIRE_EQUAL(reason, "bad-txnmrklroot");
+        }
+
+        block.hashMerkleRoot = BlockMerkleRoot(block);
+
+        {
+            std::string reason;
+            BOOST_REQUIRE(mining->checkBlock(block, {.check_pow = false}, reason));
+            BOOST_REQUIRE_EQUAL(reason, "");
+        }
+
+        {
+            // A block template does not have proof-of-work, but it might pass
+            // verifcation by coincidence. Grind the nonce if needed:
+            while (CheckProofOfWork(block.GetHash(), block.nBits, Assert(m_node.chainman)->GetParams().GetConsensus())) {
+                block.nNonce++;
+            }
+
+            std::string reason;
+            BOOST_REQUIRE(!mining->checkBlock(block, {.check_pow = true}, reason));
+            BOOST_REQUIRE_EQUAL(reason, "high-hash");
+        }
+    }
 
     // We can't make transactions until we have inputs
     // Therefore, load 110 blocks :)

src/validation.cpp

@@ -4641,40 +4641,86 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
     return result;
 }
 
-bool TestBlockValidity(BlockValidationState& state,
-                       const CChainParams& chainparams,
-                       Chainstate& chainstate,
-                       const CBlock& block,
-                       CBlockIndex* pindexPrev,
-                       bool fCheckPOW,
-                       bool fCheckMerkleRoot)
+bool ChainstateManager::TestBlockValidity(const CBlock& block, std::string& reason, const bool check_pow, const bool check_merkle_root)
 {
-    AssertLockHeld(cs_main);
-    assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip());
-    CCoinsViewCache viewNew(&chainstate.CoinsTip());
-    uint256 block_hash(block.GetHash());
-    CBlockIndex indexDummy(block);
-    indexDummy.pprev = pindexPrev;
-    indexDummy.nHeight = pindexPrev->nHeight + 1;
-    indexDummy.phashBlock = &block_hash;
+    ChainstateManager& chainman{ActiveChainstate().m_chainman};
 
-    // NOTE: CheckBlockHeader is called by CheckBlock
-    if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, pindexPrev)) {
-        LogError("%s: Consensus::ContextualCheckBlockHeader: %s\n", __func__, state.ToString());
+    // Lock must be held throughout this function for two reasons:
+    // 1. We don't want the tip to change during several of the validation steps
+    // 2. To prevent a CheckBlock() race condition for fChecked, see ProcessNewBlock()
+    LOCK(chainman.GetMutex());
+
+    BlockValidationState state;
+    CBlockIndex* tip{Assert(ActiveTip())};
+
+    if (block.hashPrevBlock != *Assert(tip->phashBlock)) {
+        reason = "inconclusive-not-best-prevblk";
         return false;
     }
-    if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot)) {
-        LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString());
+
+    // Sanity check
+    Assert(!block.fChecked);
+    // For signets CheckBlock() verifies the challenge iif fCheckPow is set.
+    if (!CheckBlock(block, state, GetConsensus(), /*fCheckPow=*/check_pow, /*fCheckMerkleRoot=*/check_merkle_root)) {
+        reason = state.GetRejectReason();
+        Assume(!reason.empty());
+        return false;
+    } else {
+        // Sanity check
+        Assume(check_pow || !block.fChecked);
+    }
+
+    /**
+     * At this point ProcessNewBlock would call AcceptBlock(), but we
+     * don't want to store the block or its header. Run individual checks
+     * instead:
+     * - skip AcceptBlockHeader() because:
+     *   - we don't want to update the block index
+     *   - we do not care about duplicates
+     *   - we already ran CheckBlockHeader() via CheckBlock()
+     *   - we already checked for prev-blk-not-found
+     *   - we know the tip is valid, so no need to check bad-prevblk
+     * - we already ran CheckBlock()
+     * - do run ContextualCheckBlockHeader()
+     * - do run ContextualCheckBlock()
+     */
+
+    if (!ContextualCheckBlockHeader(block, state, ActiveChainstate().m_blockman, chainman, tip)) {
+        Assume(!state.IsValid());
+        reason = state.GetRejectReason();
+        Assume(!reason.empty());
         return false;
     }
-    if (!ContextualCheckBlock(block, state, chainstate.m_chainman, pindexPrev)) {
-        LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString());
+    Assume(state.IsValid());
+
+    if (!ContextualCheckBlock(block, state, *this, tip)) {
+        Assume(!state.IsValid());
+        reason = state.GetRejectReason();
+        Assume(!reason.empty());
         return false;
     }
-    if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) {
+    Assume(state.IsValid());
+
+    // We don't want ConnectBlock to update the actual chainstate, so create
+    // a cache on top of it, along with a dummy block index.
+    CBlockIndex index_dummy{block};
+    uint256 block_hash(block.GetHash());
+    index_dummy.pprev = tip;
+    index_dummy.nHeight = tip->nHeight + 1;
+    index_dummy.phashBlock = &block_hash;
+    CCoinsViewCache tip_view(&ActiveChainstate().CoinsTip());
+    CCoinsView blockCoins;
+    CCoinsViewCache view(&blockCoins);
+    view.SetBackend(tip_view);
+
+    // Set fJustCheck to true in order to update, and not clear, validation caches.
+    if(!ActiveChainstate().ConnectBlock(block, state, &index_dummy, view, /*fJustCheck=*/true)) {
+        Assume(!state.IsValid());
+        reason = state.GetRejectReason();
+        Assume(!reason.empty());
         return false;
     }
-    assert(state.IsValid());
+    Assume(state.IsValid());
 
     return true;
 }

src/validation.h

@@ -383,15 +383,6 @@ public:
 /** Context-independent validity checks */
 bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
 
-/** Check a block is completely valid from start to finish (only works on top of our current best block) */
-bool TestBlockValidity(BlockValidationState& state,
-                       const CChainParams& chainparams,
-                       Chainstate& chainstate,
-                       const CBlock& block,
-                       CBlockIndex* pindexPrev,
-                       bool fCheckPOW = true,
-                       bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
-
 /** Check with the proof of work on each blockheader matches the value in nBits */
 bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams);
 
@@ -1182,6 +1173,20 @@ public:
         FlatFilePos* dbp = nullptr,
         std::multimap<uint256, FlatFilePos>* blocks_with_unknown_parent = nullptr);
 
+    /**
+     * Verify a block, including transactions.
+     *
+     * @param[in]   block       The block we want to process. Must connect to the
+     *                          current tip.
+     * @param[out]  reason      rejection reason (BIP22)
+     * @param[in]   check_pow   perform proof-of-work check, nBits in the header
+     *                          is always checked
+     * @param[in]   check_merkle_root check the merkle root
+     *
+     * For signets the challenge verification is skipped when check_pow is false.
+     */
+    bool TestBlockValidity(const CBlock& block, std::string& reason, const bool check_pow = true, const bool check_merkle_root = true);
+
     /**
      * Process an incoming block. This only returns after the best known valid
      * block is made active. Note that it does not, however, guarantee that the

test/functional/feature_init.py

@@ -88,7 +88,7 @@ class InitTest(BitcoinTestFramework):
 
         args = ['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1']
         for terminate_line in lines_to_terminate_after:
-            self.log.info(f"Starting node and will exit after line {terminate_line}")
+            self.log.info(f"Starting node and will terminate after line {terminate_line}")
             with node.busy_wait_for_debug_log([terminate_line]):
                 if platform.system() == 'Windows':
                     # CREATE_NEW_PROCESS_GROUP is required in order to be able
@@ -108,12 +108,22 @@ class InitTest(BitcoinTestFramework):
             'blocks/index/*.ldb': 'Error opening block database.',
             'chainstate/*.ldb': 'Error opening coins database.',
             'blocks/blk*.dat': 'Error loading block database.',
+            'indexes/txindex/MANIFEST*': 'LevelDB error: Corruption: CURRENT points to a non-existent file',
+            # Removing these files does not result in a startup error:
+            # 'indexes/blockfilter/basic/*.dat', 'indexes/blockfilter/basic/db/*.*', 'indexes/coinstats/db/*.*',
+            # 'indexes/txindex/*.log', 'indexes/txindex/CURRENT', 'indexes/txindex/LOCK'
         }
 
         files_to_perturb = {
             'blocks/index/*.ldb': 'Error loading block database.',
             'chainstate/*.ldb': 'Error opening coins database.',
             'blocks/blk*.dat': 'Corrupted block database detected.',
+            'indexes/blockfilter/basic/db/*.*': 'LevelDB error: Corruption',
+            'indexes/coinstats/db/*.*': 'LevelDB error: Corruption',
+            'indexes/txindex/*.log': 'LevelDB error: Corruption',
+            'indexes/txindex/CURRENT': 'LevelDB error: Corruption',
+            # Perturbing these files does not result in a startup error:
+            # 'indexes/blockfilter/basic/*.dat', 'indexes/txindex/MANIFEST*', 'indexes/txindex/LOCK'
         }
 
         for file_patt, err_fragment in files_to_delete.items():
@@ -135,9 +145,10 @@ class InitTest(BitcoinTestFramework):
             self.stop_node(0)
 
         self.log.info("Test startup errors after perturbing certain essential files")
+        dirs = ["blocks", "chainstate", "indexes"]
         for file_patt, err_fragment in files_to_perturb.items():
-            shutil.copytree(node.chain_path / "blocks", node.chain_path / "blocks_bak")
-            shutil.copytree(node.chain_path / "chainstate", node.chain_path / "chainstate_bak")
+            for dir in dirs:
+                shutil.copytree(node.chain_path / dir, node.chain_path / f"{dir}_bak")
             target_files = list(node.chain_path.glob(file_patt))
 
             for target_file in target_files:
@@ -151,10 +162,9 @@ class InitTest(BitcoinTestFramework):
 
             start_expecting_error(err_fragment)
 
-            shutil.rmtree(node.chain_path / "blocks")
-            shutil.rmtree(node.chain_path / "chainstate")
-            shutil.move(node.chain_path / "blocks_bak", node.chain_path / "blocks")
-            shutil.move(node.chain_path / "chainstate_bak", node.chain_path / "chainstate")
+            for dir in dirs:
+                shutil.rmtree(node.chain_path / dir)
+                shutil.move(node.chain_path / f"{dir}_bak", node.chain_path / dir)
 
     def init_pid_test(self):
         BITCOIN_PID_FILENAME_CUSTOM = "my_fancy_bitcoin_pid_file.foobar"

test/functional/mempool_accept.py

@@ -45,6 +45,7 @@ from test_framework.util import (
     assert_equal,
     assert_greater_than,
     assert_raises_rpc_error,
+    sync_txindex,
 )
 from test_framework.wallet import MiniWallet
 from test_framework.wallet_util import generate_keypair
@@ -270,6 +271,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
 
         self.log.info('A coinbase transaction')
         # Pick the input of the first tx we created, so it has to be a coinbase tx
+        sync_txindex(self, node)
         raw_tx_coinbase_spent = node.getrawtransaction(txid=node.decoderawtransaction(hexstring=raw_tx_in_block)['vin'][0]['txid'])
         tx = tx_from_hex(raw_tx_coinbase_spent)
         self.check_mempool_result(

test/functional/mining_template_verification.py

@@ -0,0 +1,274 @@
+#!/usr/bin/env python3
+# Copyright (c) 2024-Present The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test getblocktemplate RPC in proposal mode
+
+Generate several blocks and test them against the getblocktemplate RPC.
+"""
+
+from concurrent.futures import ThreadPoolExecutor
+
+import copy
+
+from test_framework.blocktools import (
+    create_block,
+    create_coinbase,
+    add_witness_commitment,
+)
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+)
+
+from test_framework.messages import (
+    COutPoint,
+    CTxIn,
+    uint256_from_compact,
+)
+
+from test_framework.wallet import (
+    MiniWallet,
+)
+
+
+class MiningTemplateVerificationTest(BitcoinTestFramework):
+
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def run_test(self):
+        node = self.nodes[0]
+
+        block_0_hash = node.getbestblockhash()
+        block_0_height = node.getblockcount()
+        self.generate(node, sync_fun=self.no_op, nblocks=1)
+        block_1 = node.getblock(node.getbestblockhash())
+        block_2 = create_block(
+            int(block_1["hash"], 16),
+            create_coinbase(block_0_height + 2),
+            block_1["mediantime"] + 1,
+        )
+
+        # Block must build on the current tip
+        bad_block_2 = copy.deepcopy(block_2)
+        bad_block_2.hashPrevBlock = int(block_0_hash, 16)
+        bad_block_2.solve()
+
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": bad_block_2.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            "inconclusive-not-best-prevblk",
+        )
+
+        self.log.info("Lowering nBits should make the block invalid")
+        bad_block_2 = copy.deepcopy(block_2)
+        bad_block_2.nBits = bad_block_2.nBits - 1
+        bad_block_2.solve()
+
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": bad_block_2.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            "bad-diffbits",
+        )
+
+        self.log.info("Generate a block")
+        target = uint256_from_compact(block_2.nBits)
+        # Ensure that it doesn't meet the target by coincidence
+        while block_2.sha256 <= target:
+            block_2.nNonce += 1
+            block_2.rehash()
+
+        self.log.info("A block template doesn't need PoW")
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": block_2.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            None,
+        )
+
+        self.log.info("Add proof of work")
+        block_2.solve()
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": block_2.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            None,
+        )
+
+        self.log.info("getblocktemplate does not submit the block")
+        assert_equal(node.getblockcount(), block_0_height + 1)
+
+        self.log.info("Submitting this block should succeed")
+        assert_equal(node.submitblock(block_2.serialize().hex()), None)
+        node.waitforblockheight(2)
+
+        self.log.info("Generate a transaction")
+        tx = MiniWallet(node).create_self_transfer()
+        block_3 = create_block(
+            int(block_2.hash, 16),
+            create_coinbase(block_0_height + 3),
+            block_1["mediantime"] + 1,
+            txlist=[tx["hex"]],
+        )
+        assert_equal(len(block_3.vtx), 2)
+        add_witness_commitment(block_3)
+        block_3.solve()
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": block_3.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            None,
+        )
+
+        # Call again to ensure the UTXO set wasn't updated
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": block_3.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            None,
+        )
+
+        self.log.info("Add an invalid transaction")
+        bad_tx = copy.deepcopy(tx)
+        bad_tx["tx"].vout[0].nValue = 10000000000
+        bad_tx_hex = bad_tx["tx"].serialize().hex()
+        assert_equal(
+            node.testmempoolaccept([bad_tx_hex])[0]["reject-reason"],
+            "bad-txns-in-belowout",
+        )
+        block_3 = create_block(
+            int(block_2.hash, 16),
+            create_coinbase(block_0_height + 3),
+            block_1["mediantime"] + 1,
+            txlist=[bad_tx_hex],
+        )
+        assert_equal(len(block_3.vtx), 2)
+        add_witness_commitment(block_3)
+        block_3.solve()
+
+        self.log.info("This can't be submitted")
+        assert_equal(
+            node.submitblock(block_3.serialize().hex()), "bad-txns-in-belowout"
+        )
+
+        self.log.info("And should also not pass getblocktemplate")
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": block_3.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            "duplicate-invalid",
+        )
+
+        self.log.info("Can't spend coins out of thin air")
+        bad_tx = copy.deepcopy(tx)
+        bad_tx["tx"].vin[0] = CTxIn(
+            outpoint=COutPoint(hash=int("aa" * 32, 16), n=0), scriptSig=b""
+        )
+        bad_tx_hex = bad_tx["tx"].serialize().hex()
+        assert_equal(
+            node.testmempoolaccept([bad_tx_hex])[0]["reject-reason"], "missing-inputs"
+        )
+        block_3 = create_block(
+            int(block_2.hash, 16),
+            create_coinbase(block_0_height + 3),
+            block_1["mediantime"] + 1,
+            txlist=[bad_tx_hex],
+        )
+        assert_equal(len(block_3.vtx), 2)
+        add_witness_commitment(block_3)
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": block_3.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            "bad-txns-inputs-missingorspent",
+        )
+
+        self.log.info("Can't spend coins twice")
+        tx_hex = tx["tx"].serialize().hex()
+        tx_2 = copy.deepcopy(tx)
+        tx_2_hex = tx_2["tx"].serialize().hex()
+        # Nothing wrong with these transactions individually
+        assert_equal(node.testmempoolaccept([tx_hex])[0]["allowed"], True)
+        assert_equal(node.testmempoolaccept([tx_2_hex])[0]["allowed"], True)
+        # But can't be combined
+        assert_equal(
+            node.testmempoolaccept([tx_hex, tx_2_hex])[0]["package-error"],
+            "package-contains-duplicates",
+        )
+        block_3 = create_block(
+            int(block_2.hash, 16),
+            create_coinbase(block_0_height + 3),
+            block_1["mediantime"] + 1,
+            txlist=[tx_hex, tx_2_hex],
+        )
+        assert_equal(len(block_3.vtx), 3)
+        add_witness_commitment(block_3)
+        assert_equal(
+            node.getblocktemplate(
+                template_request={
+                    "data": block_3.serialize().hex(),
+                    "mode": "proposal",
+                    "rules": ["segwit"],
+                }
+            ),
+            "bad-txns-inputs-missingorspent",
+        )
+
+        # Ensure that getblocktemplate can be called concurrently by many threads.
+        self.log.info("Check blocks in parallel")
+        check_50_blocks = lambda n: [
+            assert_equal(
+                n.getblocktemplate(
+                    template_request={
+                        "data": block_3.serialize().hex(),
+                        "mode": "proposal",
+                        "rules": ["segwit"],
+                    }
+                ),
+                "bad-txns-inputs-missingorspent",
+            )
+            for _ in range(50)
+        ]
+        rpcs = [node.cli for _ in range(6)]
+        with ThreadPoolExecutor(max_workers=len(rpcs)) as threads:
+            list(threads.map(check_50_blocks, rpcs))
+
+
+if __name__ == "__main__":
+    MiningTemplateVerificationTest(__file__).main()

test/functional/rpc_rawtransaction.py

@@ -34,6 +34,7 @@ from test_framework.util import (
     assert_equal,
     assert_greater_than,
     assert_raises_rpc_error,
+    sync_txindex,
 )
 from test_framework.wallet import (
     getnewdestination,
@@ -70,7 +71,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         self.num_nodes = 3
         self.extra_args = [
             ["-txindex"],
-            ["-txindex"],
+            [],
             ["-fastprune", "-prune=1"],
         ]
         # whitelist peers to speed up tx relay / mempool sync
@@ -109,6 +110,7 @@ class RawTransactionsTest(BitcoinTestFramework):
             self.log.info(f"Test getrawtransaction {'with' if n == 0 else 'without'} -txindex")
 
             if n == 0:
+                sync_txindex(self, self.nodes[n])
                 # With -txindex.
                 # 1. valid parameters - only supply txid
                 assert_equal(self.nodes[n].getrawtransaction(txId), tx['hex'])

test/functional/rpc_txoutproof.py

@@ -12,6 +12,7 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
+    sync_txindex,
 )
 from test_framework.wallet import MiniWallet
 
@@ -77,6 +78,7 @@ class MerkleBlockTest(BitcoinTestFramework):
         assert_equal(sorted(self.nodes[0].verifytxoutproof(self.nodes[0].gettxoutproof([txid1, txid2]))), sorted(txlist))
         assert_equal(sorted(self.nodes[0].verifytxoutproof(self.nodes[0].gettxoutproof([txid2, txid1]))), sorted(txlist))
         # We can always get a proof if we have a -txindex
+        sync_txindex(self, self.nodes[1])
         assert_equal(self.nodes[0].verifytxoutproof(self.nodes[1].gettxoutproof([txid_spent])), [txid_spent])
         # We can't get a proof if we specify transactions from different blocks
         assert_raises_rpc_error(-5, "Not all transactions found in specified or retrieved block", self.nodes[0].gettxoutproof, [txid1, txid3])

test/functional/test_framework/util.py

@@ -592,3 +592,10 @@ def find_vout_for_address(node, txid, addr):
         if addr == tx["vout"][i]["scriptPubKey"]["address"]:
             return i
     raise RuntimeError("Vout not found for address: txid=%s, addr=%s" % (txid, addr))
+
+
+def sync_txindex(test_framework, node):
+    test_framework.log.debug("Waiting for node txindex to sync")
+    sync_start = int(time.time())
+    test_framework.wait_until(lambda: node.getindexinfo("txindex")["txindex"]["synced"])
+    test_framework.log.debug(f"Synced in {time.time() - sync_start} seconds")

test/functional/test_runner.py

@@ -243,6 +243,7 @@ BASE_SCRIPTS = [
     'rpc_decodescript.py',
     'rpc_blockchain.py --v1transport',
     'rpc_blockchain.py --v2transport',
+    'mining_template_verification.py',
     'rpc_deprecated.py',
     'wallet_disable.py',
     'wallet_change_address.py --legacy-wallet',

test/functional/wallet_avoid_mixing_output_types.py

@@ -117,7 +117,6 @@ class AddressInputTypeGrouping(BitcoinTestFramework):
         self.extra_args = [
             [
                 "-addresstype=bech32",
-                "-txindex",
             ],
             [
                 "-addresstype=p2sh-segwit",