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:
BlockTemplateCache by ismaelsadeeq)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-->
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.
<!--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.
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.
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 compatibility wrapper returning only the bool result, so
old clients do not decode corrupt result fields.
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.
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?
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
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.
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.
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?
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