mining: add applySolution() to interface

Sjors commented at 11:48 am on September 12, 2025: member

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:

#33372

DrahtBot added the label Mining on Sep 12, 2025

DrahtBot commented at 11:48 am on September 12, 2025: contributor

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 force-pushed on Sep 12, 2025

Sjors commented at 5:05 pm on September 12, 2025: member

Fixed typo.

in src/ipc/capnp/mining.capnp:35 in 9b8d95f0e3 outdated

31@@ -32,6 +32,7 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
32     getWitnessCommitmentIndex @7 (context: Proxy.Context) -> (result: Int32);
33     getCoinbaseMerklePath @8 (context: Proxy.Context) -> (result: List(Data));
34     submitSolution @9 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
35+    applySolution @11 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Data);

ajtowns commented at 6:46 am on September 15, 2025:

The existing entries seem to be in order of the @n value rather than alphabetically.

Sjors commented at 7:48 am on September 15, 2025:

Before now* we were able to make breaking changes such as renumbering easily, which is why they were numbered this way.

We could order them alphabetically, thematically or based on when they were introduced. I think the first two are better, but no strong preference. @ryanofsky what do you think is better?

= it’s still not a big deal at this stage to break clients

ryanofsky commented at 12:16 pm on September 15, 2025:

re: #33374 (review)

I think it makes sense if method order order in .capnp files just matches the method order in .h files so the declarations are easy to compare.

Since we haven’t needed binary compatibility before, we’ve just renumbered when making any changes, so numbers have been in order up to this point. But if we want to start having binary compatibility, we won’t be able to reassign numbers anymore, and it’ll be normal for them not to be in order in the file.

= it’s still not a big deal at this stage to break clients

Makes sense, but to be sure, there are different ways of breaking ciients. For example if we just make changes like renumbering mining methods, or fields, or changing default values, results could be a little chaotic if an old client connects to a new node or vice versa, because client IPC calls could appear to work but invoke the wrong methods and send invalid parameter values.

A way to make cleaner breaks is to update the Init.makeMining method. Give it a new number so only new clients will be able to call it, and add a Init.makeMiningRemoved or init.makeMiningV1 method with the old number that returns null or triggers an error. This way an old client connecting to a new node, or new cient connecting to an old node will fail early instead of sending requests later that would not be interpreted correctly:

 0--- a/src/ipc/capnp/init.capnp
 1+++ b/src/ipc/capnp/init.capnp
 2@@ -19,5 +19,8 @@ using Mining = import "mining.capnp";
 3 interface Init $Proxy.wrap("interfaces::Init") {
 4     construct [@0](/bitcoin-bitcoin/contributor/0/) (threadMap: Proxy.ThreadMap) -> (threadMap :Proxy.ThreadMap);
 5     makeEcho [@1](/bitcoin-bitcoin/contributor/1/) (context :Proxy.Context) -> (result :Echo.Echo);
 6-    makeMining [@2](/bitcoin-bitcoin/contributor/2/) (context :Proxy.Context) -> (result :Mining.Mining);
 7+    makeMining [@3](/bitcoin-bitcoin/contributor/3/) (context :Proxy.Context) -> (result :Mining.Mining);
 8+
 9+    # Previous declarations for binary compatibility, no longer supported.
10+    makeMiningV1 [@2](/bitcoin-bitcoin/contributor/2/) (context :Proxy.Context) -> (result :Mining.Mining);
11 }
12--- a/src/interfaces/init.h
13+++ b/src/interfaces/init.h
14@@ -38,6 +38,9 @@ public:
15     virtual std::unique_ptr<Echo> makeEcho() { return nullptr; }
16     virtual Ipc* ipc() { return nullptr; }
17     virtual bool canListenIpc() { return false; }
18+
19+    // For IPC compatibility, no longer supported.
20+    virtual std::unique_ptr<Mining> makeMiningV1() { return nullptr; }
21 };
22 
23 //! Return implementation of Init interface for the node process. If the argv

ryanofsky commented at 12:36 pm on September 15, 2025:

re: #33374 (comment)

Looking at the PR more, since this is just adding a new method and should be backwards compatible, probably nothing in my comment above applies here, other than that I think it’s reasonable for applySolution to follow submitSolution in the .capnp file like it does in the .h file, and for the methods to not be renumbered or sorted by number.

in src/node/interfaces.cpp:921 in 9b8d95f0e3 outdated

915@@ -916,6 +916,12 @@ class BlockTemplateImpl : public BlockTemplate
916         return chainman().ProcessNewBlock(std::make_shared<const CBlock>(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
917     }
918 
919+    CBlock applySolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
920+    {
921+        AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);

ajtowns commented at 7:00 am on September 15, 2025:

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?

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.

Sjors commented at 7:53 am on September 15, 2025:

That could be a problem indeed.

Another thing that’s not great here is that m_block_template->block is mutated. Will look into it. My assumption is that in general a client will call submitSolution first, since it’s time sensitive, and then this method. So it should be fine if it copies to block rather than mutates the original even if that’s marginally slower.

Will look into this.

Sjors commented at 7:55 am on September 15, 2025:

is being run against mainnet params for ~100 blocks

It uses mainnet in order to have difficulty adjustment, but we should probably also run against regtest for segwit coverage.

ajtowns commented at 9:08 am on September 15, 2025:

An OP_TRUE signet would presumably get both for whatever that’s worth

ryanofsky commented at 1:05 pm on September 15, 2025:

Another thing that’s not great here is that m_block_template->block is mutated.

It does seem nicer to not to mutate the block template. And as long as entire block is being serialized and sent over the socket anyway, probably the cost of copying it is not significant.

But one thought is that maybe serializing whole block is not necessary, and this could just return CBlockHeader?

Another idea is that this could be changed to use output parameters, and the client could decide what information it wants returned. For example:

0virtual void applySolution(
1    uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase,
2    CBlockHeader* out_header=nullptr, CBlock* out_block=nullptr, std::string* out_invalid_reason=nullptr) = 0;

The cap’n proto declaration corresponding to this would look like:

0applySolution [@11](/bitcoin-bitcoin/contributor/11/) (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data, wantHeader: Bool, wantBlock :Bool, wantInvalidReason :Bool) 
1    -> (header: Data, block: Data, invalidReason: Text);

(libmultimultiprocess uses wantFoo parameters and foo return values to support c++ methods with optional output parameters.)

The idea with the out_invalid_reason parameter would be to provide the debug information #33372 was previously logging.

Not sure if this interface would actually be more useful in practice, but wanted to point out it is possible to return information selectively like this.

Sjors commented at 4:18 pm on September 15, 2025:

But one thought is that maybe serializing whole block is not necessary, and this could just return CBlockHeader?

I think it’s easier to use if we just return the block. Otherwise a client has the call getBlock(), then applySolution() and then swap out the header in the block they got. In which case they might as well call getBlock() just apply the solution themselves.

I think the two main use cases for this are:

Testing mining (pool) software, which doesn’t need to be performant
Debugging an invalid mainnet block

The alternative approach of having applySolution() optionally return the block itself seems fine too, but then maybe we should just make a breaking change to submitSolution.

Though in terms of performance vs simplicity, I image the client has a single thread that wants to get a new template ASAP after submitSolution(), and then has a separate lower priority thread that calls applySolution() and archives that. That’s an argument in favour of having two separate methods.

ajtowns commented at 7:29 am on September 15, 2025: contributor

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

ryanofsky approved

ryanofsky commented at 1:09 pm on September 15, 2025: contributor

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

Sjors marked this as a draft on Sep 16, 2025

Sjors commented at 2:49 pm on September 16, 2025: member

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():

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

Sjors force-pushed on Sep 16, 2025

plebhash commented at 3:32 pm on September 16, 2025: none

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 commented at 4:38 pm on September 16, 2025: member

@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.

mining: add applySolution() to interface

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.

45fb350a6b

doc: submitSolution doesn't add a witness 7249cbfebc

Sjors force-pushed on Sep 17, 2025

Sjors marked this as ready for review on Sep 17, 2025

Sjors commented at 8:53 am on September 17, 2025: member

Rebased after #33380 landed and updated the PR description.

in src/test/miner_tests.cpp:762 in 45fb350a6b outdated

757@@ -758,7 +758,15 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
758         if (current_height % 2 == 0) {
759             BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
760         } else {
761-            BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase)));
762+            const CTransactionRef coinbase{MakeTransactionRef(txCoinbase)};
763+            BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, coinbase));

ryanofsky commented at 6:16 pm on September 19, 2025:

In commit “mining: add applySolution() to interface” (45fb350a6be50049bc2a85f4b4d9036199bfe392)

Is there a reason this is calling submitSolution twice? Would be good to have a comment if this is necessary.

Sjors commented at 2:02 pm on September 20, 2025:

This test only calls submitSolution() once, unless I’m missing something?

ryanofsky commented at 4:04 pm on September 22, 2025:

re: #33374 (review)

This test only calls submitSolution() once, unless I’m missing something?

Sorry I just misread the diff, didn’t notice the first call was being deleted

ryanofsky commented at 8:03 pm on September 19, 2025: contributor

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.

Sjors commented at 1:58 pm on September 20, 2025: member

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 closed this on Sep 20, 2025

mining: add applySolution() to interface #33374

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)