Built on top of #34644
Preferred to open a separate PR to keep the other one focused on submitBlock.
This PR:
ProcessBlock helper that wraps ProcessNewBlock with SubmitBlockStateCatcher to capture BlockValidationStatereason and debug output parameters to submitSolution, matching submitBlock
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34672.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible places where comparison-specific test macros should replace generic comparisons:
BOOST_CHECK_THROW(block_template->submitSolutionOld7(...), std::runtime_error) -> Prefer BOOST_CHECK_EXCEPTION with a matcher that verifies the error message, so the test checks the specific failure reason instead of only the exception type.<sup>2026-06-16 18:08:41</sup>
35 | @@ -35,7 +36,7 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") { 36 | getTxSigops @4 (context: Proxy.Context) -> (result: List(Int64)); 37 | getCoinbaseTx @5 (context: Proxy.Context) -> (result: CoinbaseTx); 38 | getCoinbaseMerklePath @6 (context: Proxy.Context) -> (result: List(Data)); 39 | - submitSolution @7 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool); 40 | + submitSolution @7 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (reason: Text, debug: Text, result: Bool);
For this not to be a breaking change, I suspect the new arguments need to go last?
Yes, exactly. To avoid a breaking IPC schema change, the existing result field should stay first and the new reason/debug fields should be appended after it.
The issue is that current libmultiprocess code generation does not handle this schema order when result is the C++ return value, but appears first in the Cap’n Proto result fields, before reason/debug output parameters.
I added d425c20 here so this PR can be reviewed together with the corresponding libmultiprocess change in bitcoin-core/libmultiprocess#282.
If that libmultiprocess change is accepted, this PR can use a proper subtree update instead of the temporary direct subtree change.
Why are we trying to avoid a breaking change here?
I forgot I already commented here :-)
I think a breaking change is fine: #34672 (comment)
re: #34672 (review)
Why are we trying to avoid a breaking change here?
A breaking change should be ok, but a silently breaking change is not ok. Specifically, a version bump, or an error when you call the old method would be ok. But returning corrupt data when the old method is called would not be good.
It is also not hard to provide compatibility in cases like this with just a few lines of code: https://github.com/bitcoin-core/libmultiprocess/pull/282#issuecomment-4575261500
silently breaking change is not ok.
Agreed, if we make this a breaking change, makeMining should be bumped from @3 to @4 (and @3 should throw, like @2 does)
Updated the code following Ryan’s suggestion in https://github.com/bitcoin-core/libmultiprocess/pull/282#issuecomment-4575261500.
The libmultiprocess/mpgen change was removed. The new submitSolution return fields now use a new capnp ordinal (@10), while the old @7 method is kept as a deprecated compatibility wrapper returning only the bool result. This avoids a silent wire/schema break.
The intentional duplicate-handling behavior change is documented in the release note.
is kept as a deprecated compatibility wrapper returning only the bool result.
I think it's fine to make the old method throw.
I think it's fine to make the old method throw.
Agreed. Done. Thanks.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/26611966904/job/78419634172</sub>
<sub>LLM reason (✨ experimental): CI failed due to the lint “subtree” check detecting a subtree directory was modified without a corresponding subtree merge (“FAIL: subtree directory was touched without subtree merge”).</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Rebased onto master, which now includes #34644.
The current CI/lint failure is known: this PR temporarily modifies the src/ipc/libmultiprocess subtree directly. See #34672 (review) for more details.
If bitcoin-core/libmultiprocess#282 is accepted, this can be replaced with a proper subtree update.
The changes introduced here are ready to be reviewed and discussed, so I am moving this out of draft.
Concept ACK
It's a useful for IPC clients to get failure reasons. Currently they only get a false result and then people have to dig through Bitcoin Core's debug log to find the actual reason.
I suggest treating duplicate blocks the same as submitBlock(). Just document the breaking change in a release note.
This PR should be draft until after the https://github.com/bitcoin-core/libmultiprocess/pull/282 subtree update. Would be good to link that in the PR description.
It seems that the subtree update can be avoided by making a breaking change in the parameter order. That would also make it consistent with submitBlock(). In that case you need to bump makeMining from @3 and @4.
The libmultiprocess/mpgen change has been removed, and submitSolution now follows the compatibility approach suggested in https://github.com/bitcoin-core/libmultiprocess/pull/282#issuecomment-4575261500.
Specifically, the updated submitSolution method uses a new capnp ordinal, while the old ordinal is kept as a deprecated compatibility wrapper returning only the bool result. This avoids a silent wire/schema break for old clients.
See #34672 (review) for more details.
40 | + submitSolution @10 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (reason: Text, debug: Text, result: Bool); 41 | waitNext @8 (context: Proxy.Context, options: BlockWaitOptions) -> (result: BlockTemplate); 42 | interruptWait @9() -> (); 43 | + 44 | + # DEPRECATED: older version of submitSolution which only returns a bool. 45 | + submitSolutionOld7 @7 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
Isn't this interface still experimental and breaking changes are supposed to be expected?
re: #34672 (review)
Isn't this interface still experimental and breaking changes are supposed to be expected?
A breaking change would be ok but in order to implement it you would either need to either bump the mining interface version like 9453c153612ae9b30308362048099bc53afcde6f or return an error from this method like:
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -78,9 +78,7 @@ public:
//! Deprecated older method preserved for IPC clients using mining.capnp [@7](/bitcoin-bitcoin/contributor/7/).
virtual bool submitSolutionOld7(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase)
{
- std::string reason;
- std::string debug;
- return submitSolution(version, timestamp, nonce, coinbase, reason, debug);
+ throw std::runtime_error("Old submitSolution (@7) not supported. Please update your client!");
}
/**
so clients know they are being broken, otherwise they will send and receive corrupt data.
Capnproto is lower level than JSON or gRPC. No field names or IDs are sent over the wire so when you add or remove fields you don't get missing field errors, you get data corruption. You need to follow https://capnproto.org/language.html#evolving-your-protocol rules to avoid this.
(Also, it can be nice to avoid immediately breaking old clients when making minor feature improvements. But this is more subjective.)
See also #34672 (review)
Changed the old method to throw.
400 | + // emitted synchronously by ProcessNewBlock, unlike most validation signals. 401 | CHECK_NONFATAL(chainman.m_options.signals)->UnregisterSharedValidationInterface(sc); 402 | 403 | if (new_block && !*new_block && accepted) { 404 | reason = "duplicate"; 405 | + } else if (!accepted && (!sc->m_found || sc->m_state.IsValid())) {
In commit "mining: clarify SubmitBlock result handling" (https://github.com/bitcoin/bitcoin/pull/34672/changes/3f65137444dc45a5f352f31b1f42b424c8d99108)
This branch seems to be catching scenarios where ProcessNewBlock failed, but BlockChecked reported valid. I think this is a good improvement,, as this could happen when ActivateBestChain fails
Since this new !accepted && (!sc->m_found || sc->m_state.IsValid()) branch fixes this crash path, I think it should be mentioned in the commit description as well
Added in 83f3bc002d . Thanks
Added. Thanks.
Claude suggested this to cover the else if (!accepted && (!sc->m_found || sc->m_state.IsValid())) branch:
diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 5de893768d..7e98992c82 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -384,3 +384,48 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index)
BOOST_CHECK_EQUAL(GetWitnessCommitmentIndex(pblock), 2);
}
+
+// submitBlock() reports "inconclusive" when ProcessNewBlock() fails for a
+// non-validity reason (e.g. an activation/system error) rather than because the
+// submitted block is invalid. This exercises the
+// `!accepted && !sc->m_found` branch in SubmitBlock(), which has no other
+// coverage.
+BOOST_AUTO_TEST_CASE(submit_block_inconclusive_on_activation_failure)
+{
+ bool ignored;
+ BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(std::make_shared<CBlock>(Params().GenesisBlock()), true, true, &ignored));
+
+ auto mining{interfaces::MakeMining(m_node)};
+ const uint256 genesis{Params().GenesisBlock().GetHash()};
+ std::string reason, debug;
+
+ // Block A extends genesis and becomes the active tip.
+ const auto block_a{GoodBlock(genesis)};
+ BOOST_REQUIRE(mining->submitBlock(*block_a, reason, debug));
+
+ // Block B1 is a valid sibling of A: accepted and stored, but not connected
+ // (equal work). This is the existing "accepted but not connected" path.
+ const auto block_b1{GoodBlock(genesis)};
+ BOOST_REQUIRE(block_b1->GetHash() != block_a->GetHash());
+ BOOST_REQUIRE(!mining->submitBlock(*block_b1, reason, debug));
+ BOOST_CHECK_EQUAL(reason, "inconclusive");
+
+ // Corrupt B1's stored location so it can no longer be read from disk.
+ WITH_LOCK(::cs_main, {
+ CBlockIndex* index{m_node.chainman->m_blockman.LookupBlockIndex(block_b1->GetHash())};
+ BOOST_REQUIRE(index);
+ index->nDataPos = 1 << 30; // past end of file
+ });
+
+ // Block B2 builds on B1, so the B-branch now has more work than A.
+ // Submitting it triggers a reorg that must read B1 back from disk to connect
+ // it; the read fails with a fatal (non-invalid) error, so ProcessNewBlock()
+ // returns false without ever emitting BlockChecked for B2. The result is
+ // accepted == false and m_found == false: branch 2's !sc->m_found case.
+ const auto block_b2{GoodBlock(block_b1->GetHash())};
+ reason = "stale";
+ debug = "stale";
+ BOOST_CHECK(!mining->submitBlock(*block_b2, reason, debug));
+ BOOST_CHECK_EQUAL(reason, "inconclusive");
+ BOOST_CHECK_EQUAL(debug, "");
+}
BOOST_AUTO_TEST_SUITE_END()
(wording needs some tweaking; it's referring to what the functional tests are currently doing)
922 | {
923 | AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
924 | - std::string reason;
925 | - std::string debug;
926 | - return SubmitBlock(chainman(), std::make_shared<const CBlock>(m_block_template->block), /*new_block=*/nullptr, reason, debug);
927 | + bool new_block;
In commit "mining: add reason and debug output to submitSolution" (https://github.com/bitcoin/bitcoin/pull/34672/changes/b6b601ac9913c1c7aa9679a759ac7e9bdff46ead)
Stylistic nit. I think this should be initialized to false.
If this commit needs to be retouched, I can initialize it there. But the follow-up refactor already removes this local variable: BlockTemplateImpl::submitSolution() now delegates directly to SubmitBlock().
80 | */ 81 | - virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) = 0; 82 | + virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase, std::string& reason, std::string& debug) = 0; 83 | + 84 | + //! Deprecated older method preserved for IPC clients using mining.capnp @7. 85 | + virtual bool submitSolutionOld7(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase)
In commit "mining: add reason and debug output to submitSolution" (https://github.com/bitcoin/bitcoin/pull/34672/changes/b6b601ac9913c1c7aa9679a759ac7e9bdff46ead)
In 057336b, the release notes mention that BlockTemplate.submitSolution now reports duplicate blocks as failures, but this submitSolutionOld7 now also returns false for duplicates.
Should the release notes also state that this deprecated method also changes duplicate behaviour? A client reading the current release notes that say
"The previous @7 method is still accepted for compatibility, but is deprecated and only returns the boolean result."
Might think they can safely continue using it without any changes.
Addressed this by making the old @7 entry point return an explicit error instead of preserving the bool-only wrapper. Thanks.
405 | + } else if (!accepted && (!sc->m_found || sc->m_state.IsValid())) { 406 | + // ProcessNewBlock can fail without a validation result, for example 407 | + // from an activation or system error. It can also fail after a valid 408 | + // BlockChecked result. In these cases the validation result is 409 | + // inconclusive. 410 | + reason = "inconclusive";
In commit "mining: clarify SubmitBlock result handling" (3f65137)
Should the "inconclusive" reason for ActivateBestChain failures be a different string? The same "inconclusive" is used for both "accepted but not connected" and "system error during activation."
These are very different conditions. Should these be distinguishable?
I think keeping reason="inconclusive" for both cases is preferable because the reason field is BIP22-style and this matches the existing submitblock RPC behavior when validation does not produce a conclusive BlockChecked result.
The cases are internally different, but neither has a specific block rejection reason to report. The code keeps them as separate branches with comments, following up on earlier feedback in #34644 (review)
Concept ACK
IIUC, this PR is extending the submitSolution to return reason and debug rejection details, aligning behaviour with submitBlock and centralizing the result handling logic, while preserving the deprecated IPC method.
This is a nice improvement. There are still some future refactors and deduplications that I spotted, but those can be done subsequently.
Most of the changes made here look good to me. I left some notes and suggestions
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.
Add reason and debug output parameters to submitSolution, matching
submitBlock. This relays the specific failure reason (e.g.
"bad-version(...)", "bad-witness-nonce-size", "duplicate") to callers
instead of just a bool.
Use a new capnp ordinal for the updated method and keep the old @7 method
as a deprecated entry point returning an explicit error, so old clients do
not decode corrupt result fields and are directed to update.
Move the accepted/new-block/reason consistency check into SubmitBlock()
so submitBlock() and submitSolution() use the same success criteria.
This keeps duplicate and inconclusive handling in one place, removes the
new_block output parameter from the helper, and makes the helper return
whether the submitted block was accepted as a new valid block.
402 | // No queue drain is needed. The BlockChecked notification used above is 403 | // emitted synchronously by ProcessNewBlock, unlike most validation signals. 404 | CHECK_NONFATAL(chainman.m_options.signals)->UnregisterSharedValidationInterface(sc); 405 | 406 | - if (new_block && !*new_block && accepted) { 407 | + if (!new_block && accepted) {
In ed75d70fdb82042c9f44f3bd51b33c8b415f50aa refactor: centralize SubmitBlock result handling: maybe preserve a bit of the comment above:
// Treat duplicates as errors for mining clients.