Merge c1939c43c3 into c5e44a0435

32d55e28af test: Use the correct node for doubled keypath test (Ava Chow)

Pull request description:

  #29124 had a silent merge conflict with #32350 which resulted in it using the wrong node. Fix the test to use the correct v22 node.

ACKs for top commit:
  maflcko:
    lgtm ACK 32d55e28af
  rkrux:
    ACK 32d55e28af
  BrandonOdiwuor:
    Code Review ACK 32d55e28af

Tree-SHA512: 1e0231985beb382b16e1d608c874750423d0502388db0c8ad450b22d17f9d96f5e16a6b44948ebda5efc750f62b60d0de8dd20131f449427426a36caf374af92

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
@@ -114,6 +114,21 @@ 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)
+     * @param[out] debug      more detailed rejection reason
+     * @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, std::string& debug) = 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, debug: 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

@@ -1110,6 +1110,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, std::string& debug) override
+    {
+        return TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root, reason, debug);
+    }
+
     NodeContext* context() override { return &m_node; }
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
     KernelNotifications& notifications() { return *Assert(m_node.notifications); }

src/node/miner.cpp

@@ -173,10 +173,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     pblock->nBits          = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());
     pblock->nNonce         = 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;
+    std::string debug;
+    if (m_options.test_block_validity && !TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false, reason, debug)) {
+        throw std::runtime_error(strprintf("TestBlockValidity failed: %s - %s", reason, debug));
     }
     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 check
+     */
+    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

@@ -387,9 +387,10 @@ 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;
+        std::string debug;
+        if (!miner.checkBlock(block, {.check_merkle_root = false, .check_pow = false}, reason, debug)) {
+            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s - %s", reason, debug));
         }
     }
 
@@ -741,13 +742,12 @@ 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;
+            std::string debug;
+            bool res{miner.checkBlock(block, {.check_pow = false}, reason, debug)};
+            if (res) return UniValue::VNULL;
+            LogDebug(BCLog::RPC, "Invalid block: %s", debug);
+            return 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,44 @@ 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;
+            std::string debug;
+            BOOST_REQUIRE(!mining->checkBlock(block, {.check_pow = false}, reason, debug));
+            BOOST_REQUIRE_EQUAL(reason, "bad-txnmrklroot");
+            BOOST_REQUIRE_EQUAL(debug, "hashMerkleRoot mismatch");
+        }
+
+        block.hashMerkleRoot = BlockMerkleRoot(block);
+
+        {
+            std::string reason;
+            std::string debug;
+            BOOST_REQUIRE(mining->checkBlock(block, {.check_pow = false}, reason, debug));
+            BOOST_REQUIRE_EQUAL(reason, "");
+            BOOST_REQUIRE_EQUAL(debug, "");
+        }
+
+        {
+            // A block template does not have proof-of-work, but it might pass
+            // verification by coincidence. Grind the nonce if needed:
+            while (CheckProofOfWork(block.GetHash(), block.nBits, Assert(m_node.chainman)->GetParams().GetConsensus())) {
+                block.nNonce++;
+            }
+
+            std::string reason;
+            std::string debug;
+            BOOST_REQUIRE(!mining->checkBlock(block, {.check_pow = true}, reason, debug));
+            BOOST_REQUIRE_EQUAL(reason, "high-hash");
+            BOOST_REQUIRE_EQUAL(debug, "proof of work failed");
+        }
+    }
 
     // We can't make transactions until we have inputs
     // Therefore, load 110 blocks :)

src/validation.cpp

@@ -4648,40 +4648,82 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
     return result;
 }
 
-bool TestBlockValidity(BlockValidationState& state,
-                       const CChainParams& chainparams,
-                       Chainstate& chainstate,
+bool TestBlockValidity(Chainstate& chainstate,
                        const CBlock& block,
-                       CBlockIndex* pindexPrev,
-                       bool fCheckPOW,
-                       bool fCheckMerkleRoot)
+                       const bool check_pow,
+                       const bool check_merkle_root,
+                       std::string& reason,
+                       std::string& debug)
 {
-    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;
+    // 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(chainstate.m_chainman.GetMutex());
 
-    // 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());
+    BlockValidationState state;
+    CBlockIndex* tip{Assert(chainstate.m_chain.Tip())};
+
+    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());
+
+    // For signets CheckBlock() verifies the challenge iff fCheckPow is set.
+    if (!CheckBlock(block, state, chainstate.m_chainman.GetConsensus(), /*fCheckPow=*/check_pow, /*fCheckMerkleRoot=*/check_merkle_root)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
         return false;
     }
-    if (!ContextualCheckBlock(block, state, chainstate.m_chainman, pindexPrev)) {
-        LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString());
+
+    /**
+     * 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, chainstate.m_blockman, chainstate.m_chainman, tip)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
         return false;
     }
-    if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) {
+
+    if (!ContextualCheckBlock(block, state, chainstate.m_chainman, tip)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
+        return false;
+    }
+
+    // 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(&chainstate.CoinsTip());
+    CCoinsView blockCoins;
+    CCoinsViewCache view(&blockCoins);
+    view.SetBackend(tip_view);
+
+    // Set fJustCheck to true in order to update, and not clear, validation caches.
+    if(!chainstate.ConnectBlock(block, state, &index_dummy, view, /*fJustCheck=*/true)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
+        return false;
+    }
+    if (!Assume(state.IsValid())) {
+        LogError("Unexpected invalid validation state");
         return false;
     }
-    assert(state.IsValid());
 
     return true;
 }

src/validation.h

@@ -383,14 +383,26 @@ 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,
+/**
+ * Verify a block, including transactions.
+ *
+ * @param[in]   block       The block we want to process. Must connect to the
+ *                          current tip.
+ * @param[in]   chainstate    The chainstate to connect to.
+ * @param[out]  reason      rejection reason (BIP22)
+ * @param[out]  debug       more detailed rejection reason
+ * @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(Chainstate& chainstate,
                        const CBlock& block,
-                       CBlockIndex* pindexPrev,
-                       bool fCheckPOW = true,
-                       bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+                       const bool check_pow,
+                       const bool check_merkle_root,
+                       std::string& reason,
+                       std::string& debug);
 
 /** 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);

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/test_runner.py

@@ -211,6 +211,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',

test/functional/wallet_backwards_compatibility.py

@@ -87,7 +87,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
         # Make sure that this is being automatically cleaned up by migration
         node_master = self.nodes[1]
-        node_v22 = self.nodes[self.num_nodes - 5]
+        node_v22 = self.nodes[self.num_nodes - 3]
         wallet_name = "bad_deriv_path"
         node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
         bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)