diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index a95bc220185..ff4f87109f9 100644 --- a/src/interfaces/mining.h +++ b/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; } diff --git a/src/ipc/capnp/mining.capnp b/src/ipc/capnp/mining.capnp index 64cad4d49f2..a6dd8d71f31 100644 --- a/src/ipc/capnp/mining.capnp +++ b/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") { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index edb73628484..2f68f414f0d 100644 --- a/src/node/interfaces.cpp +++ b/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(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(m_block_template->block), /*new_block=*/nullptr, reason, debug); } std::unique_ptr 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(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); } diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 5c0df502e83..ccd9cc7c57a 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include @@ -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& 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& 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(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); diff --git a/src/node/miner.h b/src/node/miner.h index 14780833c02..af327307329 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -18,6 +18,7 @@ #include #include #include +#include #include 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& block, bool* new_block, std::string& reason, std::string& debug); /* Interrupt a blocking call. */ void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 2ddc1307cba..fd9559b543c 100644 --- a/src/test/miner_tests.cpp +++ b/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 shared_pblock = std::make_shared(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))); } diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py index b59ffaab653..4cd9c17c99f 100755 --- a/test/functional/interface_ipc_mining.py +++ b/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())) diff --git a/test/functional/test_framework/ipc_util.py b/test/functional/test_framework/ipc_util.py index 5ce582372f8..a4ebd091e8e 100644 --- a/test/functional/test_framework/ipc_util.py +++ b/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