Merge bitcoin#31981: Add checkBlock() to Mining interface

a18e572 test: more template verification tests (Sjors Provoost)
10c9088 test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8 Add checkBlock to Mining interface (Sjors Provoost)
6077157 ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4 validation: refactor TestBlockValidity (Sjors Provoost)

Pull request description:

  This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.

  In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.

  The new Mining interface method is used in `miner_tests`.

  It's not used by the `getblocktemplate` and `generateblock` RPC calls, see bitcoin#31981 (comment)

  The `inconclusive-not-best-prevblk` check is moved from RPC
  code to `TestBlockValidity`.

  Test coverage is increased by `mining_template_verification.py`.

  Superseedes bitcoin#31564

  ## Background

  ### Verifying block templates (no PoW)

  Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].

  The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.

  (although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of bitcoin#31098)

  [^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
  [^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
  [^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196

ACKs for top commit:
  davidgumberg:
    reACK bitcoin@a18e572
  achow101:
    ACK a18e572
  TheCharlatan:
    ACK a18e572
  ryanofsky:
    Code review ACK a18e572 just adding another NONFATAL_UNREACHABLE since last review

Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1

Author: achow101

src/interfaces/mining.h

@@ -114,6 +114,21 @@ class Mining
      */
     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

@@ -984,6 +984,15 @@ class MinerImpl : public Mining
         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
+    {
+        LOCK(chainman().GetMutex());
+        BlockValidationState state{TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root)};
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
+        return state.IsValid();
+    }
+
     NodeContext* context() override { return &m_node; }
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
     KernelNotifications& notifications() { return *Assert(m_node.notifications); }

src/node/miner.cpp

@@ -179,10 +179,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()));
+    if (m_options.test_block_validity) {
+        if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
+            throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
+        }
     }
     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

@@ -388,8 +388,7 @@ 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)) {
+        if (BlockValidationState state{TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
             throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
         }
     }
@@ -745,13 +744,7 @@ 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);
+            return BIP22ValidationResult(TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/true));
         }
 
         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));