From 83f3bc002d08006996ab100313fa2ddcd6568a80 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Thu, 28 May 2026 13:06:39 -0700 Subject: [PATCH] mining: clarify SubmitBlock result handling Make the submitBlock return value explicit and check that it stays consistent with the BIP22 reason string, so future changes do not return success with a reason or failure without one. Report "inconclusive" when no specific block rejection reason is available. This covers blocks accepted without being connected, and processing failures where ProcessNewBlock returns false without an invalid BlockChecked result, for example when ActivateBestChain fails after BlockChecked reported a valid block. Also document why no validation-interface queue drain is needed before unregistering: BlockChecked is emitted synchronously by ProcessNewBlock, unlike most validation signals. --- src/node/interfaces.cpp | 4 ++- src/node/miner.cpp | 15 +++++++---- test/functional/interface_ipc_mining.py | 36 ++++++++++++++++++++++--- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 2f68f414f0d..6747dd60aaa 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1031,7 +1031,9 @@ public: // 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 bool result{accepted && new_block && reason.empty()}; + CHECK_NONFATAL(result == reason.empty()); + return result; } const NodeContext* context() override { return &m_node; } diff --git a/src/node/miner.cpp b/src/node/miner.cpp index ccd9cc7c57a..66ae870f844 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -396,16 +396,21 @@ 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); + // 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) { 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(); diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py index 4cd9c17c99f..8d2ee85964c 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 @@ -623,6 +628,31 @@ class IPCMiningTest(BitcoinTestFramework): assert_equal(submitted, True) self.sync_all() + self.log.debug("submitBlock should report inconclusive for a valid stale block") + async with AsyncExitStack() as stack: + active_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 submit_block_template is not None + + active_block = await self.build_candidate_block( + active_template, ctx2, extra_nonce=b"\x01") + submit_block = await self.build_candidate_block( + submit_block_template, ctx2, extra_nonce=b"\x02") + active_block.solve() + submit_block.solve() + + # Both templates share a parent. The first block becomes active, + # so the remaining valid block is accepted as stale. + await self.assert_submit_block( + mining2, ctx2, active_block, result=True) + + 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") 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)