diff --git a/doc/release-notes-34672.md b/doc/release-notes-34672.md new file mode 100644 index 00000000000..060d91428f0 --- /dev/null +++ b/doc/release-notes-34672.md @@ -0,0 +1,11 @@ +IPC Interface +------------- + +- `BlockTemplate.submitSolution` now returns `reason` and `debug` rejection + details in addition to the boolean result. Clients must regenerate IPC + bindings from the updated `mining.capnp` schema to use the new method. The + previous `@7` method now returns an error directing clients to update. (#34672) + +- `BlockTemplate.submitSolution` now reports duplicate blocks as failures with + `reason="duplicate"`, matching `Mining.submitBlock`, instead of returning + success for duplicate submissions. (#34672) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 884a1e16578..c8e969711b3 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -59,6 +60,8 @@ public: * @param[in] timestamp time block header field (unix timestamp) * @param[in] nonce nonce block header field * @param[in] coinbase complete coinbase transaction (including witness) + * @param[out] reason failure reason (BIP22) + * @param[out] debug more detailed rejection reason * * @note Unlike the submitblock RPC, this method does not call * UpdateUncommittedBlockStructures to add a missing coinbase witness @@ -69,13 +72,16 @@ public: * is only one byte long, so the coinbase scriptSig needs at least * one additional byte of data to avoid bad-cb-length. * - * @returns if the block was processed, does not necessarily indicate validity. - * - * @note Returns true if the block is already known, which can happen if - * the solved block is constructed and broadcast by multiple nodes - * (e.g. both the miner who constructed the template and the pool). + * @returns true if the block was accepted as a new block */ - virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) = 0; + virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase, std::string& reason, std::string& debug) = 0; + + //! Deprecated older method preserved to return an explicit error for IPC + //! clients using mining.capnp @7. + virtual bool submitSolutionOld7(uint32_t, uint32_t, uint32_t, CTransactionRef) + { + throw std::runtime_error("Old submitSolution (@7) not supported. Please update your client!"); + } /** * Waits for fees in the next block to rise, a new tip or the timeout. diff --git a/src/ipc/capnp/mining.capnp b/src/ipc/capnp/mining.capnp index 5f0347fc3c9..e8da538f9ab 100644 --- a/src/ipc/capnp/mining.capnp +++ b/src/ipc/capnp/mining.capnp @@ -38,9 +38,12 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") { getTxSigops @4 (context: Proxy.Context) -> (result: List(Int64)); getCoinbaseTx @5 (context: Proxy.Context) -> (result: CoinbaseTx); getCoinbaseMerklePath @6 (context: Proxy.Context) -> (result: List(Data)); - submitSolution @7 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool); + submitSolution @10 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (reason: Text, debug: Text, result: Bool); waitNext @8 (context: Proxy.Context, options: BlockWaitOptions) -> (result: BlockTemplate); interruptWait @9() -> (); + + # DEPRECATED: older version of submitSolution which returns an error. + submitSolutionOld7 @7 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool); } struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 7df9f86f53c..afb6ea48e93 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -917,13 +917,11 @@ public: return TransactionMerklePath(m_block_template->block, 0); } - bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override + bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase, std::string& reason, std::string& debug) override { if (!coinbase) return false; AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce); - std::string reason; - std::string debug; - return SubmitBlock(chainman(), std::make_shared(m_block_template->block), /*new_block=*/nullptr, reason, debug); + return SubmitBlock(chainman(), std::make_shared(m_block_template->block), reason, debug); } std::unique_ptr waitNext(BlockWaitOptions options) override @@ -1026,13 +1024,7 @@ public: 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(); + return SubmitBlock(chainman(), std::make_shared(block_in), reason, debug); } std::vector getTransactionsByTxID(const std::vector& txids) override diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 32a21440c4e..06611a4a2f4 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -385,7 +385,7 @@ protected: }; } // namespace -bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr& block, bool* new_block, std::string& reason, std::string& debug) +bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr& block, std::string& reason, std::string& debug) { reason.clear(); debug.clear(); @@ -395,27 +395,34 @@ bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr(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); + bool new_block; + bool accepted = chainman.ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/&new_block); + // No queue drain is needed. The BlockChecked notification used above is + // emitted synchronously by ProcessNewBlock, unlike most validation signals. CHECK_NONFATAL(chainman.m_options.signals)->UnregisterSharedValidationInterface(sc); - if (new_block && !*new_block && accepted) { + if (!new_block && accepted) { reason = "duplicate"; + } else if (!accepted && (!sc->m_found || sc->m_state.IsValid())) { + // ProcessNewBlock can fail without a validation result, for example + // from an activation or system error. It can also fail after a valid + // BlockChecked result. In these cases the validation result is + // inconclusive. + reason = "inconclusive"; } 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. + // The block was accepted but not connected, for example if it does not + // have more work than the current tip. reason = "inconclusive"; } else if (!sc->m_state.IsValid()) { reason = sc->m_state.GetRejectReason(); debug = sc->m_state.GetDebugMessage(); } - return accepted; + const bool result{accepted && new_block && reason.empty()}; + CHECK_NONFATAL(result == reason.empty()); + return result; } void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait) diff --git a/src/node/miner.h b/src/node/miner.h index af327307329..7b5702ac594 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -131,8 +131,8 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman); 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); +//! Returns whether the block was accepted as a new valid block. +bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr& 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 8e7943ed85e..90df73b4567 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -884,9 +884,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) } // Alternate calls between submitBlock and submitSolution via the // Mining interface. + std::string reason{"stale reason"}; + std::string debug{"stale debug"}; if (current_height % 2 == 0) { - 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, ""); @@ -897,7 +897,14 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) BOOST_REQUIRE_EQUAL(reason, "duplicate"); BOOST_REQUIRE_EQUAL(debug, ""); } else { - BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase))); + reason = "stale reason"; + debug = "stale debug"; + BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase), reason, debug)); + BOOST_REQUIRE_EQUAL(reason, ""); + BOOST_REQUIRE_EQUAL(debug, ""); + BOOST_CHECK_THROW(block_template->submitSolutionOld7(block.nVersion, block.nTime, block.nNonce, + MakeTransactionRef(txCoinbase)), + std::runtime_error); } { LOCK(cs_main); diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py index 94ccc1e5615..c36d73ab0ad 100755 --- a/test/functional/interface_ipc_mining.py +++ b/test/functional/interface_ipc_mining.py @@ -111,10 +111,15 @@ 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.""" + async def build_candidate_block(self, template, ctx, extra_nonce=b""): + """Build a complete block from a remote BlockTemplate. + + The returned block replaces the dummy coinbase from CreateNewBlock() + with one constructed from getCoinbaseTx(). + """ block = await mining_get_block(template, ctx) - coinbase = await self.build_coinbase_test(template, ctx, self.miniwallet) + coinbase = await self.build_coinbase_test( + template, ctx, self.miniwallet, extra_nonce=extra_nonce) # Reduce payout for balance comparison simplicity. coinbase.vout[0].nValue = COIN block.vtx[0] = coinbase @@ -532,8 +537,10 @@ class IPCMiningTest(BitcoinTestFramework): 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) + result = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) + assert_equal(result.result, False) + assert_equal(result.reason, "bad-version(0x00000000)") + assert_equal(result.debug, "rejected nVersion=0x00000000 block") self.log.debug("submitBlock should reject a bad-version block") await self.assert_submit_block( mining2, @@ -566,8 +573,10 @@ class IPCMiningTest(BitcoinTestFramework): 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) + result = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness()) + assert_equal(result.result, False) + assert_equal(result.reason, "bad-witness-nonce-size") + assert_equal(result.debug, "CheckWitnessMalleation : invalid witness reserved value size") self.log.debug("Even a rejected submitSolution() mutates the template's block") # Can be used by clients to download and inspect the (rejected) @@ -586,8 +595,10 @@ class IPCMiningTest(BitcoinTestFramework): ) 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) + result = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) + assert_equal(result.result, True) + assert_equal(result.reason, "") + assert_equal(result.debug, "") self.log.debug("Submit a valid complete block through the disconnected node") await self.assert_submit_block(mining2, ctx2, block, result=True) @@ -616,7 +627,7 @@ class IPCMiningTest(BitcoinTestFramework): 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") + self.log.debug("submitSolution should return 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] @@ -624,9 +635,55 @@ class IPCMiningTest(BitcoinTestFramework): 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.log.debug("submitSolution should reject the duplicate block") + result = await template2.submitSolution(ctx2, duplicate_block.nVersion, duplicate_block.nTime, duplicate_block.nNonce, duplicate_coinbase.serialize()) + assert_equal(result.result, False) + assert_equal(result.reason, "duplicate") + assert_equal(result.debug, "") + self.sync_all() + + self.log.debug( + "submitSolution and submitBlock should report inconclusive for valid stale blocks") + async with AsyncExitStack() as stack: + active_template = await mining_create_block_template( + mining2, stack, ctx2, self.default_block_create_options) + solution_template = await mining_create_block_template( + mining2, stack, ctx2, self.default_block_create_options) + submit_block_template = await mining_create_block_template( + mining2, stack, ctx2, self.default_block_create_options) + assert active_template is not None + assert solution_template is not None + assert submit_block_template is not None + + active_block = await self.build_candidate_block( + active_template, ctx2, extra_nonce=b"\x01") + solution_block = await self.build_candidate_block( + solution_template, ctx2, extra_nonce=b"\x02") + submit_block = await self.build_candidate_block( + submit_block_template, ctx2, extra_nonce=b"\x03") + active_block.solve() + solution_block.solve() + submit_block.solve() + + # All three templates share a parent. The first block becomes + # active, so the remaining valid blocks are accepted as stale. + await self.assert_submit_block( + mining2, ctx2, active_block, result=True) + + solution_coinbase = solution_block.vtx[0] + result = await solution_template.submitSolution( + ctx2, + solution_block.nVersion, + solution_block.nTime, + solution_block.nNonce, + solution_coinbase.serialize(), + ) + assert_equal(result.result, False) + assert_equal(result.reason, "inconclusive") + assert_equal(result.debug, "") + + await self.assert_submit_block( + mining2, ctx2, submit_block, result=False, reason="inconclusive") self.sync_all() self.log.debug("Submit the same invalid block twice") @@ -740,8 +797,10 @@ class IPCMiningTest(BitcoinTestFramework): block.vtx[0] = coinbase block.hashMerkleRoot = block.calc_merkle_root() block.solve() - submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result - assert_equal(submitted, True) + result = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) + assert_equal(result.result, True) + assert_equal(result.reason, "") + assert_equal(result.debug, "") assert_equal(node.getblockcount(), height) asyncio.run(capnp.run(async_routine()))