Merge bitcoin/bitcoin#34644: mining: add submitBlock to IPC Mining interface

3962138cc0 test: add IPC submitBlock functional test (woltx)
5b60f69e40 mining: add submitBlock IPC method to Mining interface (woltx)
813b4a80d7 refactor: introduce SubmitBlock helper (w0xlt)

Pull request description:

  This PR adds a `submitBlock` method to the IPC Mining interface, equivalent to the `submitblock` RPC. It accepts a serialized block over IPC, validates/processes it via the normal block-processing path.

  The method uses the same result shape as `checkBlock`: `bool` + `reason/debug out-params`. It reports duplicate, inconclusive, and invalid-block rejection details, and initializes reason/debug on every call.

  Closes #34626

ACKs for top commit:
  Sjors:
    ACK 3962138cc0
  optout21:
    reACK 3962138cc0
  ryanofsky:
    Code review ACK 3962138cc0. Just rebased since and made suggested changes since last review.

Tree-SHA512: 705cbb89972a80b6ff0ab75a78f686983d6077c97f1758795efe5b8968f01065ebef664ac850eae2bc86af8964efa2a68e8dfc677209c312856650f9387ed006

src/interfaces/mining.h

@@ -60,8 +60,10 @@ public:
      * @param[in] nonce nonce block header field
      * @param[in] coinbase complete coinbase transaction (including witness)
      *
-     * @note unlike the submitblock RPC, this method does NOT add the
-     *       coinbase witness automatically.
+     * @note Unlike the submitblock RPC, this method does not call
+     *       UpdateUncommittedBlockStructures to add a missing coinbase witness
+     *       reserved value. Callers must provide a complete coinbase transaction,
+     *       including the witness when a witness commitment is present.
      *
      * @note for heights <= 16, the BIP34 height push in getCoinbaseTx().script_sig_prefix
      *       is only one byte long, so the coinbase scriptSig needs at least
@@ -157,6 +159,27 @@ public:
      */
     virtual bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) = 0;
 
+    /**
+     * Process a fully assembled block.
+     *
+     * Similar to the submitblock RPC. Accepts a complete block, validates
+     * it, and if accepted as new, processes it into chainstate. Accepted
+     * blocks may then be announced to peers through normal validation signals.
+     *
+     * @param[in]  block  the complete block to submit
+     * @param[out] reason failure reason (BIP22)
+     * @param[out] debug  more detailed rejection reason
+     * @returns           true if the block was accepted as a new block. Returns
+     *                    false and sets reason if the block is a duplicate or
+     *                    the validation result is inconclusive.
+     *
+     * @note Unlike the submitblock RPC, this method does not call
+     *       UpdateUncommittedBlockStructures to add a missing coinbase witness
+     *       reserved value. Callers must submit a fully formed block, including
+     *       the coinbase witness when a witness commitment is present.
+     */
+    virtual bool submitBlock(const CBlock& block, std::string& reason, std::string& debug) = 0;
+
     //! Get internal node context. Useful for RPC and testing,
     //! but not accessible across processes.
     virtual const node::NodeContext* context() { return nullptr; }

src/ipc/capnp/mining.capnp

@@ -25,6 +25,7 @@ interface Mining $Proxy.wrap("interfaces::Mining") {
     createNewBlock @4 (context :Proxy.Context, options: BlockCreateOptions, cooldown: Bool = true) -> (result: BlockTemplate);
     checkBlock @5 (context :Proxy.Context, block: Data, options: BlockCheckOptions) -> (reason: Text, debug: Text, result: Bool);
     interrupt @6 () -> ();
+    submitBlock @7 (context :Proxy.Context, block: Data) -> (reason: Text, debug: Text, result: Bool);
 }
 
 interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {

src/node/interfaces.cpp

@@ -920,7 +920,9 @@ public:
     bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
     {
         AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
-        return chainman().ProcessNewBlock(std::make_shared<const CBlock>(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
+        std::string reason;
+        std::string debug;
+        return SubmitBlock(chainman(), std::make_shared<const CBlock>(m_block_template->block), /*new_block=*/nullptr, reason, debug);
     }
 
     std::unique_ptr<BlockTemplate> waitNext(BlockWaitOptions options) override
@@ -1021,6 +1023,17 @@ public:
         return state.IsValid();
     }
 
+    bool submitBlock(const CBlock& block_in, std::string& reason, std::string& debug) override
+    {
+        auto block = std::make_shared<const CBlock>(block_in);
+        bool new_block;
+        const bool accepted = SubmitBlock(chainman(), block, &new_block, reason, debug);
+        // ProcessNewBlock() can accept and store a block before it is checked
+        // for validity. Treat duplicates as errors for mining clients, and only
+        // return success when validation completed without setting a reason.
+        return accepted && new_block && reason.empty();
+    }
+
     const NodeContext* context() override { return &m_node; }
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
     KernelNotifications& notifications() { return *Assert(m_node.notifications); }

src/node/miner.cpp

@@ -38,6 +38,7 @@
 #include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
+#include <validationinterface.h>
 #include <versionbits.h>
 
 #include <algorithm>
@@ -357,6 +358,62 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
     block.fChecked = false;
 }
 
+namespace {
+class SubmitBlockStateCatcher final : public CValidationInterface
+{
+public:
+    uint256 m_hash;
+    bool m_found{false};
+    BlockValidationState m_state;
+
+    explicit SubmitBlockStateCatcher(const uint256& hash) : m_hash{hash} {}
+
+protected:
+    void BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state) override
+    {
+        if (block->GetHash() != m_hash) return;
+        // ProcessNewBlock emits BlockChecked synchronously while holding cs_main,
+        // so SubmitBlock can read these fields after ProcessNewBlock returns
+        // without extra synchronization.
+        m_found = true;
+        m_state = state;
+    }
+};
+} // namespace
+
+bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& block, bool* new_block, std::string& reason, std::string& debug)
+{
+    reason.clear();
+    debug.clear();
+
+    // This follows the submitblock RPC's validation-state capture pattern, but
+    // is intentionally kept separate from the RPC implementation. The RPC entry
+    // point decodes hex, formats BIP22/JSONRPC results, and calls
+    // UpdateUncommittedBlockStructures() for legacy witness handling. IPC
+    // callers submit already-formed blocks and need bool + reason/debug
+    // results, while submitSolution() preserves its duplicate-as-success
+    // behavior.
+    auto sc = std::make_shared<SubmitBlockStateCatcher>(block->GetHash());
+    CHECK_NONFATAL(chainman.m_options.signals)->RegisterSharedValidationInterface(sc);
+    bool accepted = chainman.ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/new_block);
+    CHECK_NONFATAL(chainman.m_options.signals)->UnregisterSharedValidationInterface(sc);
+
+    if (new_block && !*new_block && accepted) {
+        reason = "duplicate";
+    } else if (!sc->m_found) {
+        // A block can be accepted and stored without being connected, for
+        // example if it does not have more work than the current tip. In that
+        // case no BlockChecked callback is emitted, so the validation result is
+        // inconclusive. Mining::submitBlock treats this as an error for mining
+        // clients, but it does not mean the block is invalid.
+        reason = "inconclusive";
+    } else if (!sc->m_state.IsValid()) {
+        reason = sc->m_state.GetRejectReason();
+        debug = sc->m_state.GetDebugMessage();
+    }
+    return accepted;
+}
+
 void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait)
 {
     LOCK(kernel_notifications.m_tip_block_mutex);

src/node/miner.h

@@ -18,6 +18,7 @@
 #include <cstdint>
 #include <memory>
 #include <optional>
+#include <string>
 #include <vector>
 
 class CBlockIndex;
@@ -129,6 +130,9 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
 /* Compute the block's merkle root, insert or replace the coinbase transaction and the merkle root into the block */
 void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce);
 
+//! Submit a block and capture the validation state via the BlockChecked callback.
+//! Returns whether ProcessNewBlock accepted the block.
+bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& block, bool* new_block, std::string& reason, std::string& debug);
 
 /* Interrupt a blocking call. */
 void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait);

src/test/miner_tests.cpp

@@ -858,12 +858,20 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
             block.hashMerkleRoot = BlockMerkleRoot(block);
             block.nNonce = bi.nonce;
         }
-        std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(block);
-        // Alternate calls between Chainman's ProcessNewBlock and submitSolution
-        // via the Mining interface. The former is used by net_processing as well
-        // as the submitblock RPC.
+        // Alternate calls between submitBlock and submitSolution via the
+        // Mining interface.
         if (current_height % 2 == 0) {
-            BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
+            std::string reason{"stale reason"};
+            std::string debug{"stale debug"};
+            BOOST_REQUIRE(mining->submitBlock(block, reason, debug));
+            BOOST_REQUIRE_EQUAL(reason, "");
+            BOOST_REQUIRE_EQUAL(debug, "");
+
+            reason = "stale reason";
+            debug = "stale debug";
+            BOOST_REQUIRE(!mining->submitBlock(block, reason, debug));
+            BOOST_REQUIRE_EQUAL(reason, "duplicate");
+            BOOST_REQUIRE_EQUAL(debug, "");
         } else {
             BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase)));
         }

test/functional/interface_ipc_mining.py

@@ -6,9 +6,14 @@
 import asyncio
 import time
 from contextlib import AsyncExitStack
+from copy import deepcopy
 from decimal import Decimal
 from io import BytesIO
-from test_framework.blocktools import NULL_OUTPOINT, script_BIP34_coinbase_height
+from test_framework.blocktools import (
+    NULL_OUTPOINT,
+    script_BIP34_coinbase_height,
+    WITNESS_COMMITMENT_HEADER,
+)
 from test_framework.messages import (
     CBlockHeader,
     COIN,
@@ -58,10 +63,10 @@ class IPCMiningTest(BitcoinTestFramework):
         self.skip_if_no_py_capnp()
 
     def set_test_params(self):
-        self.num_nodes = 2
+        self.num_nodes = 3
 
     def setup_nodes(self):
-        self.extra_init = [{"ipcbind": True}, {}]
+        self.extra_init = [{"ipcbind": True}, {}, {"ipcbind": True}]
         super().setup_nodes()
         # Use this function to also load the capnp modules (we cannot use set_test_params for this,
         # as it is being called before knowing whether capnp is available).
@@ -106,6 +111,22 @@ class IPCMiningTest(BitcoinTestFramework):
         coinbase_tx.nLockTime = coinbase_res.lockTime
         return coinbase_tx
 
+    async def build_candidate_block(self, template, ctx):
+        """Build a complete block from a remote BlockTemplate."""
+        block = await mining_get_block(template, ctx)
+        coinbase = await self.build_coinbase_test(template, ctx, self.miniwallet)
+        # Reduce payout for balance comparison simplicity.
+        coinbase.vout[0].nValue = COIN
+        block.vtx[0] = coinbase
+        block.hashMerkleRoot = block.calc_merkle_root()
+        return block
+
+    async def assert_submit_block(self, mining, ctx, block, *, result, reason="", debug=""):
+        submit = await mining.submitBlock(ctx, block.serialize())
+        assert_equal(submit.result, result)
+        assert_equal(submit.reason, reason)
+        assert_equal(submit.debug, debug)
+
     def run_mining_interface_test(self):
         """Test Mining interface methods."""
         self.log.info("Running Mining interface test")
@@ -167,6 +188,10 @@ class IPCMiningTest(BitcoinTestFramework):
         # Reconnect nodes so next tests are happy
         node.wait_for_rpc_connection()
         self.connect_nodes(1, 0)
+        # Restarting node 0 drops its P2P connection to the rest of the test
+        # chain. Restore the synced 0-1-2 topology before later tests split
+        # node 2 off for submitBlock checks.
+        self.sync_all()
 
     def run_block_template_test(self):
         """Test BlockTemplate interface methods."""
@@ -461,20 +486,31 @@ class IPCMiningTest(BitcoinTestFramework):
 
         async def async_routine():
             ctx, mining = await make_mining_ctx(self)
+            # Node 0 drives the checkBlock() and submitSolution() checks. Node
+            # 2 has a separate IPC interface and starts synced through node 1,
+            # so it can be isolated below to test submitBlock() without
+            # changing node 0's template and chain state first.
+            ctx2, mining2 = await make_mining_ctx(self, node_index=2)
 
             current_block_height = self.nodes[0].getchaintips()[0]["height"]
             check_opts = self.capnp_modules['mining'].BlockCheckOptions()
 
+            # Send a real transaction so the template includes it.
+            self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
+
             async with destroying((await mining.createNewBlock(ctx, self.default_block_create_options)).result, ctx) as template:
-                block = await mining_get_block(template, ctx)
+                block = await self.build_candidate_block(template, ctx)
+                coinbase = block.vtx[0]
+                self.log.debug("Template should include a mempool transaction")
+                assert len(block.vtx) >= 2, "Block should include at least the coinbase and the mempool tx"
                 balance = self.miniwallet.get_balance()
-                coinbase = await self.build_coinbase_test(template, ctx, self.miniwallet)
-                # Reduce payout for balance comparison simplicity
-                coinbase.vout[0].nValue = COIN
-                block.vtx[0] = coinbase
-                block.hashMerkleRoot = block.calc_merkle_root()
                 original_version = block.nVersion
 
+                self.log.debug("Disconnect node 2 before block submission tests")
+                # The default topology is 2 -> 1 -> 0. Splitting the 1-2 edge
+                # lets node 2 accept/reject complete blocks independently.
+                self.disconnect_nodes(1, 2)
+
                 self.log.debug("Submit solution that can't be deserialized")
                 try:
                     await template.submitSolution(ctx, 0, 0, 0, b"")
@@ -488,8 +524,19 @@ class IPCMiningTest(BitcoinTestFramework):
                 check = await mining.checkBlock(ctx, block.serialize(), check_opts)
                 assert_equal(check.result, False)
                 assert_equal(check.reason, "bad-version(0x00000000)")
+                assert_equal(check.debug, "rejected nVersion=0x00000000 block")
+                self.log.debug("submitSolution should reject a bad-version block")
                 submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
                 assert_equal(submitted, False)
+                self.log.debug("submitBlock should reject a bad-version block")
+                await self.assert_submit_block(
+                    mining2,
+                    ctx2,
+                    block,
+                    result=False,
+                    reason="bad-version(0x00000000)",
+                    debug="rejected nVersion=0x00000000 block",
+                )
                 self.log.debug("Submit a valid block")
                 block.nVersion = original_version
                 block.solve()
@@ -503,6 +550,16 @@ class IPCMiningTest(BitcoinTestFramework):
 
                 self.log.debug("Submitted coinbase must include witness")
                 assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex())
+                missing_witness_block = deepcopy(block)
+                missing_witness_block.vtx[0].wit.vtxinwit = []
+                has_witness_commitment = any(
+                    len(o.scriptPubKey) >= 38 and o.scriptPubKey[2:6] == WITNESS_COMMITMENT_HEADER
+                    for o in missing_witness_block.vtx[0].vout
+                )
+                assert has_witness_commitment, "Coinbase should have a witness commitment output"
+                missing_witness_block.hashMerkleRoot = missing_witness_block.calc_merkle_root()
+                missing_witness_block.solve()
+                self.log.debug("submitSolution should reject a coinbase missing witness")
                 submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness())).result
                 assert_equal(submitted, False)
 
@@ -512,14 +569,34 @@ class IPCMiningTest(BitcoinTestFramework):
                 remote_block_after = await mining_get_block(template, ctx)
                 assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex())
 
+                self.log.debug("submitBlock should reject a block missing coinbase witness")
+                await self.assert_submit_block(
+                    mining2,
+                    ctx2,
+                    missing_witness_block,
+                    result=False,
+                    reason="bad-witness-nonce-size",
+                    debug="CheckWitnessMalleation : invalid witness reserved value size",
+                )
+
                 self.log.debug("Submit again, with the witness")
                 submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
                 assert_equal(submitted, True)
 
+                self.log.debug("Submit a valid complete block through the disconnected node")
+                await self.assert_submit_block(mining2, ctx2, block, result=True)
+                assert_equal(self.nodes[2].getchaintips()[0]["height"], current_block_height + 1)
+                self.log.debug("submitBlock should reject the duplicate complete block")
+                await self.assert_submit_block(mining2, ctx2, block, result=False, reason="duplicate")
+
             self.log.debug("Block should propagate")
             # Check that the IPC node actually updates its own chain
             assert_equal(self.nodes[0].getchaintips()[0]["height"], current_block_height + 1)
             # Stalls if a regression causes submitSolution() to accept an invalid block:
+            self.sync_blocks(self.nodes[:2])
+            # Rejoin node 2 and verify the submitBlock result converges with
+            # the submitSolution result from node 0.
+            self.connect_nodes(1, 2)
             self.sync_all()
             # Check that the other node accepts the block
             assert_equal(self.nodes[0].getchaintips()[0], self.nodes[1].getchaintips()[0])
@@ -530,6 +607,54 @@ class IPCMiningTest(BitcoinTestFramework):
             check = await mining.checkBlock(ctx, block.serialize(), check_opts)
             assert_equal(check.result, False)
             assert_equal(check.reason, "inconclusive-not-best-prevblk")
+            self.log.debug("submitBlock on the same node should fail with duplicate after submitSolution succeeds")
+            await self.assert_submit_block(mining, ctx, block, result=False, reason="duplicate")
+
+            self.log.debug("submitSolution should still return True for a duplicate after submitBlock succeeds")
+            async with destroying((await mining2.createNewBlock(ctx2, self.default_block_create_options)).result, ctx2) as template2:
+                duplicate_block = await self.build_candidate_block(template2, ctx2)
+                duplicate_coinbase = duplicate_block.vtx[0]
+                duplicate_block.solve()
+                self.log.debug("Submit a valid complete block before duplicate submitSolution")
+                await self.assert_submit_block(mining2, ctx2, duplicate_block, result=True)
+                self.nodes[2].waitforblockheight(current_block_height + 2)
+                self.log.debug("submitSolution should accept the duplicate block")
+                submitted = (await template2.submitSolution(ctx2, duplicate_block.nVersion, duplicate_block.nTime, duplicate_block.nNonce, duplicate_coinbase.serialize())).result
+                assert_equal(submitted, True)
+            self.sync_all()
+
+            self.log.debug("Submit the same invalid block twice")
+            async with destroying((await mining2.createNewBlock(ctx2, self.default_block_create_options)).result, ctx2) as template2:
+                invalid_block = await self.build_candidate_block(template2, ctx2)
+                invalid_block.vtx[0].nLockTime = 2**32 - 1
+                invalid_block.hashMerkleRoot = invalid_block.calc_merkle_root()
+                invalid_block.solve()
+                self.log.debug("submitBlock should reject the non-final block")
+                await self.assert_submit_block(
+                    mining2,
+                    ctx2,
+                    invalid_block,
+                    result=False,
+                    reason="bad-txns-nonfinal",
+                    debug="non-final transaction",
+                )
+                self.log.debug("submitBlock should report duplicate-invalid for the same block")
+                await self.assert_submit_block(
+                    mining2,
+                    ctx2,
+                    invalid_block,
+                    result=False,
+                    reason="duplicate-invalid",
+                    debug=f"block {invalid_block.hash_hex} was previously marked invalid",
+                )
+
+            self.log.debug("Submit a malformed complete block")
+            try:
+                await mining2.submitBlock(ctx2, block.serialize()[:-15])
+                raise AssertionError("submitBlock unexpectedly succeeded")
+            except capnp.lib.capnp.KjException as e:
+                assert_capnp_failed(e, "remote exception: std::exception: SpanReader::read(): end of data:")
+            assert_equal(self.nodes[2].is_node_stopped(), False)
 
         asyncio.run(capnp.run(async_routine()))
 

test/functional/test_framework/ipc_util.py

@@ -94,8 +94,8 @@ def load_capnp_modules(config):
     }
 
 
-async def make_capnp_init_ctx(self):
-    node = self.nodes[0]
+async def make_capnp_init_ctx(self, node_index=0):
+    node = self.nodes[node_index]
     # Establish a connection, and create Init proxy object.
     connection = await capnp.AsyncIoStream.create_unix_connection(node.ipc_socket_path)
     client = capnp.TwoPartyClient(connection)
@@ -152,9 +152,9 @@ async def mining_get_coinbase_tx(block_template, ctx) -> CoinbaseTxData:
         lockTime=int(template_capnp.lockTime),
     )
 
-async def make_mining_ctx(self):
+async def make_mining_ctx(self, node_index=0):
     """Create IPC context and Mining proxy object."""
-    ctx, init = await make_capnp_init_ctx(self)
+    ctx, init = await make_capnp_init_ctx(self, node_index)
     self.log.debug("Create Mining proxy object")
     mining = init.makeMining(ctx).result
     return ctx, mining