mining: add applySolution() to interface #33374

Sjors · 2025-09-12T11:48:32Z

Unlike the submitblock RPC which takes a fully serialized block, when a block solution is received via IPC the client only provides the nonce, coinbase and a few other fields. It may not have all the information it needs to reconstruct the block. This makes debugging difficult when the block is invalid.

This PR adds applySolution() which returns the reconstructed block and can be used by the client for debugging. It's assigned @11 in the .cap file, and will not break existing client software.

It can also be used by the client for an alternative broadcast mechanism.

The tests cover both mainnet (unit tests) and regtest (functional tests), because the latter has SegWit active.

The second commit documents the fact that applySolution() as well as the (already existing) submitSolution are slightly different from the submitsolution RPC in that they expect a complete coinbase transaction and will not automatically add a witness. This behavior is also covered in the functional test of the first commit.

This is alternative or complimentary approach to:

mining: log failed blocks in submitSolution() #33372

DrahtBot · 2025-09-12T11:48:37Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33374.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

submitsolution -> submitSolution [references a method/RPC name; correct capitalization makes the reference unambiguous]

^{drahtbot_id_5_m}

Sjors · 2025-09-12T17:05:16Z

Fixed typo.

ajtowns

I like this better than just logging stuff, for whatever that's worth.

src/ipc/capnp/mining.capnp

src/node/interfaces.cpp

ryanofsky

Concept ACK, this does seem like a more general solution than 33372.

src/ipc/capnp/mining.capnp

src/node/interfaces.cpp

Sjors · 2025-09-16T14:49:27Z

On second thought I don't think mutating the block is a problem.

I think this assumes that the provided coinbase tx includes the coinbase witness, which is different to how submitblock behaves. (In particular submitblock includes a call to chainman.UpdateUncommittedBlockStructures, whereas AddMerkleRootAndCoinbase doesn't) That seems like it might be a source of bugs -- might at least be worth adding a check and having an explicit log message if the coinbase requires a witness but doesn't have it?

IIUC only the submitblock RPC adds the coinbase witness, whereas submitSolution() and applySolution() IPC calls both don't.

This has been a source of confusion:

So far the SRI software has been giving us a complete coinbase, including the coinbase witness. But this involved a hardcoded assumption about its value. The current ambition is to have the Template Provider communicate this value (eventually, no rush).

But it seems the approach in RPC land was different: we don't communicate the coinbase witness to pool software and just fix it when they submit the solution. The description of says "This is safe for submitted blocks.", but that's only true if the pool took our SegWit OP_RETURN.

The latter is not unreasonable, but I would prefer that we pick one approach:

Give pools both the OP_RETURN and coinbase witness; or
The pool figures out both values

I prefer (1).

cc @plebhash.

FWIW I think miner_tests is being run against mainnet params for ~100 blocks, which means that segwit is never active (it activates at block 481824), so the witness-specific code isn't being tested there.

I rebased this on #33380 and added a test on regtest. This checks that dropping the coinbase witness has an effect. You can verify this test by modifying applySolution():

AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
LOCK(cs_main);
# yolo
chainman().UpdateUncommittedBlockStructures(m_block_template->block, chainman().ActiveTip());
return m_block_template->block;

plebhash · 2025-09-16T15:32:19Z

The current ambition is to have the Template Provider communicate this value (eventually, no rush).

this would require a change to the Sv2 spec so that NewTemplate message carries a new field called coinbase_witness .

but unfortunately, changing Sv2 spec is usually received with resistance from the current players.

if/when a softfork happens and we start committing stuff to the coinbases' witness reserved value, that could be stronger motivation.

but until that day comes, this spec change would be merely motivated for making Sv2 "future-proof" with regards to this hypothetical soft fork.

so this is all to put some extra emphasis on the (eventually, no rush) quoted above.

Give pools both the OP_RETURN and coinbase witness; or

The pool figures out both values

I prefer (1).

In Sv2 terms, option 2. would incur in extra round trips (RequestTransactionData/.Success) and computation of witness root, which would translate into undesired delays.

So I'd prefer 1 as well. Even if Bitcoin Core continues to redundantly provide 0x000...000 as coinbase witness and Sv2 spec continues sticking to the assumption that coinbase witness always has this same value.

So whenever we get consensus a Sv2 spec change (either preemptively or forced by a soft-fork), the Sv2 TP would already have means to fill NewTemplate.coinbase_witness with appropriate values coming from Bitcoin Core via IPC.

But AFAIU, that's somewhat different from the proposal on this PR, right?

IIUC this PR is only adding a new applySolution, and the discussion point now is simply the assumptions about how such solution would be crafted by Sv2 software.

Sjors · 2025-09-16T16:38:31Z

@plebhash indeed this PR just introduces the extra method and clarifies in the documentation that submitSolution and applySolution are slightly different from the submitsolution RPC.

Unlike the submitblock RPC which takes a fully serialized block, when a block solution is received via IPC the client only provides the nonce, coinbase and a few other fields. It may not have all the information it needs to reconstruct the block. This makes debugging difficult when the block is invalid. This commit adds applySolution() which returns the reconstructed block and can be used by the client for debugging. It can also be used by the client for an alternative broadcast mechanism.

Sjors · 2025-09-17T08:53:16Z

Rebased after #33380 landed and updated the PR description.

ryanofsky · 2025-09-19T20:03:55Z

Have a few questions and comments:

Main question is why is this new applySolution method needed at all if submitSolution() modifies the block and there is already a getBlock() method?
I think it would be good if submitSolution documented fact that calling it changes getBlockHeader, getBlock, getCoinbaseTx values.
New comments say that coinbase witness is not added automatically, but it's not really clear what happens if client doesn't add it. Block is rejected or invalid? Behavior is undefined? Would be helpful to indicate.
If result is not well defined, I think it would be good for submitSolution to raise an error in this case since it seems it seems easy to detect. Code could just throw std::runtime_exception, since libmultiprocess can pass along standard exceptions as Text return values
I'm not actually sure why the IPC interface being different than the RPC interface is good, and what the downside is of filling in the OP_RETURN and witness like the RPC does. Doing this seems basically the same as your option (1) "Give pools both the OP_RETURN and coinbase witness" exceptit gives the values after submitting instead of before? If you wanted to fully support option 1 would you just add some extra accessors here that could be called before submitSolution?
It might be a good idea to add a BlockSubmitOptions struct passed to SubmitSolution, similar to existing BlockCreateOptions, BlockWaitOptions, BlockCheckOptions structs. A "pretend" option might be useful for only updating the block without submitting it, like applySolution. It might also be useful to have options controlling the coinbase withness behavior and whether to return validation errors.

src/test/miner_tests.cpp

Sjors · 2025-09-20T13:58:41Z

Main question is why is this new applySolution method needed at all if submitSolution() modifies the block and there is already a getBlock() method?

This is a great point. Let's just embrace the fact that submitSolution() modifies the block.

I'll extract some of the documentation improvements, and add some based on your comments, and PR those later. Ditto for the extra test coverage.

Sjors · 2025-10-30T12:04:24Z

@ryanofsky wrote:

New comments say that coinbase witness is not added automatically, but it's not really clear what happens if client doesn't add it. Block is rejected or invalid? Behavior is undefined? Would be helpful to indicate.

It turns out we accept such an invalid block. The reason for that is a cached m_checked_witness_commitment. Fixed in #33745 which also adds documentation from this PR.

PR bitcoin#33374 proposed a new Mining IPC method applySolution() which could be used by clients to obtain the reconstructed block for inspection, especially in the case of a rejected block. However it was pointed out during review that submitBlock() modified the template CBlock in place, so the client can just call getBlock() and no new method is needed. This commit adds a test to document that (now intentional) behavior.

Sjors · 2025-10-31T15:58:56Z

Github (not the moderators) completely disappeared the (very useful) earlier feedback from Plebhash. Hopefully his account gets restored, but meanwhile see the @0xB10C backup: https://mirror.b10c.me/bitcoin-bitcoin/33374#issuecomment-3299309843

0xB10C · 2025-10-31T16:50:51Z

Note that the @plebhash comments were actually remove from the archive (as it just pulls the GitHub API. Gone from the GitHub API means gone from the latest version of the archive and mirroring) and might thus be missing from the mirroring after the next rebuild..

See bitcoin-data/github-metadata-backup-bitcoin-bitcoin@9ca6ff3#diff-a4bfd29817b2ed302dbaf2c9bd4514ace8aa1ffeacf02046400d93b526b8ab02L847

Sjors · 2025-11-03T10:43:25Z

@plebhash's account has been restored and his comments and issues came back.

6eaa00f test: clarify submitBlock() mutates the template (Sjors Provoost) 862bd43 mining: ensure witness commitment check in submitBlock (Sjors Provoost) 00d1b6e doc: clarify UpdateUncommittedBlockStructures (Sjors Provoost) Pull request description: When an IPC client requests a new block template via the Mining interface, we hold on to its `CBlock`. That way when they call `submitSolution()` we can modify it in place, rather than having to reconstruct the full block like the `submitblock` RPC does. Before this commit however we forgot to invalidate `m_checked_witness_commitment`, which we should since the client brings a new coinbase. This would cause us to accept an invalid chaintip. Fix this and add a test to confirm that we now reject such a block. As a sanity check, we add a second node to the test and confirm that will accept our mined block. As first noticed in #33374 the IPC code takes the coinbase as provided, unlike the `submitblock` RPC which calls `UpdateUncommittedBlockStructures()` and adds witness commitment to the coinbase if it was missing. Although that could have been an alternative fix, we instead document that IPC clients are expected to provide the full coinbase including witness commitment. Patch to produce the original issue: ```diff diff --git a/src/node/miner.cpp b/src/node/miner.cpp index b988e28..28e9048 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -450,15 +450,10 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t } block.nVersion = version; block.nTime = timestamp; block.nNonce = nonce; block.hashMerkleRoot = BlockMerkleRoot(block); - - // Reset cached checks - block.m_checked_witness_commitment = false; - block.m_checked_merkle_root = false; - block.fChecked = false; } std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman, KernelNotifications& kernel_notifications, CTxMemPool* mempool, diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index cce56e3..bf1b7048ab 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -216,22 +216,22 @@ class IPCInterfaceTest(BitcoinTestFramework): assert_equal(res.result, True) # The remote template block will be mutated, capture the original: remote_block_before = await self.parse_and_deserialize_block(template, ctx) - self.log.debug("Submitted coinbase must include witness") + self.log.debug("Submitted coinbase with missing witness is accepted") assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex()) res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness()) - assert_equal(res.result, False) + assert_equal(res.result, True) self.log.debug("Even a rejected submitBlock() mutates the template's block") # Can be used by clients to download and inspect the (rejected) # reconstructed block. remote_block_after = await self.parse_and_deserialize_block(template, ctx) assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex()) - self.log.debug("Submit again, with the witness") + self.log.debug("Submit again, with the witness - does not replace the invalid block") res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) assert_equal(res.result, True) self.log.debug("Block should propagate") assert_equal(self.nodes[1].getchaintips()[0]["height"], current_block_height + 1) ``` ACKs for top commit: ryanofsky: Code review ACK 6eaa00f. Just documentation updates and test clarifications since last review, also splitting up a commit. TheCharlatan: Re-ACK 6eaa00f ismaelsadeeq: Code review and tested ACK 6eaa00f Tree-SHA512: 3a6280345b0290fe8300ebc63c13ad4058d24ceb35b7d7a784b974d5f04f420860ac03a9bf2fc6a799ef3fc55552ce033e879fa369298f976b9a01d72bd55d9e

DrahtBot added the Mining label Sep 12, 2025

Sjors mentioned this pull request Sep 12, 2025

mining: log failed blocks in submitSolution() #33372

Closed

Sjors force-pushed the 2025/08/applySolution branch from 1025446 to 9b8d95f Compare September 12, 2025 17:05

Sjors mentioned this pull request Sep 12, 2025

Can't reproduce: signet template without coinbase outputs stratum-mining/sv2-tp#19

Closed

ajtowns reviewed Sep 15, 2025

View reviewed changes

src/ipc/capnp/mining.capnp Outdated Show resolved Hide resolved

src/node/interfaces.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Sep 15, 2025

View reviewed changes

src/ipc/capnp/mining.capnp Outdated Show resolved Hide resolved

src/node/interfaces.cpp Outdated Show resolved Hide resolved

Sjors marked this pull request as draft September 16, 2025 13:41

Sjors force-pushed the 2025/08/applySolution branch from 9b8d95f to e3f0686 Compare September 16, 2025 14:49

Sjors added 2 commits September 17, 2025 10:51

doc: submitSolution doesn't add a witness

7249cbf

Sjors force-pushed the 2025/08/applySolution branch from e3f0686 to 7249cbf Compare September 17, 2025 08:51

Sjors marked this pull request as ready for review September 17, 2025 08:52

ryanofsky reviewed Sep 19, 2025

View reviewed changes

src/test/miner_tests.cpp Show resolved Hide resolved

Sjors closed this Sep 20, 2025

ryanofsky mentioned this pull request Sep 19, 2025

Multiprocess tracking issue #28722

Open

Sjors mentioned this pull request Sep 23, 2025

Write block to disk after submitting stratum-mining/sv2-tp#20

Merged

Sjors mentioned this pull request Oct 30, 2025

mining: check witness commitment in submitBlock #33745

Merged

0xB10C mentioned this pull request Oct 31, 2025

Consider making the backup events append-only to avoid banned GH users comments being removed 0xB10C/github-metadata-backup#8

Open

Sjors referenced this pull request Nov 11, 2025

interfaces: create block template via block template cache

9500dca

Sjors mentioned this pull request Nov 11, 2025

node: add BlockTemplateCache #33421

Open

mining: add applySolution() to interface #33374

mining: add applySolution() to interface #33374

Uh oh!

Conversation

Sjors commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)

Uh oh!

Sjors commented Sep 12, 2025

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plebhash commented Sep 16, 2025

Uh oh!

Sjors commented Sep 16, 2025

Uh oh!

Sjors commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Sep 19, 2025

Uh oh!

Uh oh!

Sjors commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Oct 30, 2025

Uh oh!

Sjors commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xB10C commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sjors commented Sep 12, 2025 •

edited

Loading

DrahtBot commented Sep 12, 2025 •

edited

Loading

Sjors commented Sep 16, 2025 •

edited

Loading

Sjors commented Sep 17, 2025 •

edited

Loading

Sjors commented Sep 20, 2025 •

edited

Loading

Sjors commented Oct 31, 2025 •

edited

Loading

0xB10C commented Oct 31, 2025 •

edited

Loading