mining: add getCoinbase()

Sjors commented at 2:30 pm on November 7, 2025: member

Deprecate getCoinbaseTx() in favor of a new method that provides a struct with everything clients need to construct a coinbase. This is safer than providing a raw dummy coinbase that clients then have to manipulate.

Also deprecate getCoinbaseCommitment() and getWitnessCommitmentIndex().

A new helper method ExtractCoinbaseTemplate() processes the dummy coinbase transaction generated by BlockAssembler::CreateNewBlock and produces a CoinbaseTemplate.

Expand the interface_ipc.py functional test to document its usage.

Can be tested using:

https://github.com/stratum-mining/sv2-tp/pull/59

DrahtBot added the label Mining on Nov 7, 2025

DrahtBot commented at 2:30 pm on November 7, 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/33819.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	optout21

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33795 (test: Ignore error message give from python because of PYTHON_GIL by kevkevinpal)
#33745 (mining: check witness commitment in submitBlock by Sjors)
#32420 (miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC by Sjors)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Scan outputs from getCoinbase() outputs field for the -> Scan the outputs field from getCoinbase() for the [duplicate word “outputs” makes the sentence confusing]

drahtbot_id_5_m

Sjors commented at 2:31 pm on November 7, 2025: member

Working on a matching sv2-tp pull request. It will first try getCoinbase() and if that fails it falls back to getCoinbaseTx().

in src/interfaces/mining.h:45 in 79e73b0947 outdated

41@@ -42,8 +42,26 @@ class BlockTemplate
42     // Sigop cost per transaction, not including coinbase transaction.
43     virtual std::vector<int64_t> getTxSigops() = 0;
44 
45+    /** Return fields needed to construct a coinbase transaction */

ismaelsadeeq commented at 2:58 pm on November 7, 2025:

Given sv2-tp and a few others are the users of this interface and they are still in development.

I’d rather have any un-useful methods pruned. When we release a stable interface then we can start deprecating things to avoid breaking clients (As you mentioned in the tracking issue it is okay to make breaking changes so all these methods should be pruned and clients should update).

Sjors commented at 3:16 pm on November 7, 2025:

There’s a note in the tracking issue #33777 that we can drop these deprecated methods as soon as there’s an important breaking change.

Even for testing it’s nice that current clients work against both v30 and master.

ismaelsadeeq commented at 3:34 pm on November 7, 2025:

Hmm, do you plan on deprecating it first and then removing it in a later release? On the contrary, I think it should be dropped as soon as we deem it unnecessary. Otherwise, we’ll end up with interface methods that conflict with each other. It would also be easier to review the changes together, rather than keeping them all here with deprecation comments and later deleting them it will save us review cycle.

Even for testing it’s nice that current clients works against both v30 and master.

I think this is not big of deal, since you have a release that is against v30, your master should be in sync with master which can easily update when breaking change occur.

Sjors commented at 3:52 pm on November 7, 2025:

do you plan on deprecating it first and then removing it in a later release?

No, because the interface is marked experimental I think it’s fine to drop it in any release we want. That might be as soon as v31, but we can also do it later.

your master should be in sync with master

sv2-tp is used by people who only run v30, so it has to support both master and v30.

It can already handle new methods by just trying them and falling back to old methods when they’re missing.

Recent versions of sv2-tp all work against v30, so I haven’t had to push people to switch to a newer version either. But we start making breaking changes, then I have to start instructing users of Bitcoin Core master to use specific newer versions of sv2-tp.

ismaelsadeeq commented at 4:09 pm on November 7, 2025:

Thanks for the context. I still think it’s fine to do it together. You can have a major release of sv2-tp that works with v30 and minor releases that includes additional improvements to that the major releases that is compatible with v30. sv2-tp master should just be in sync with bitcoin/bitcoin master people that want to run that should build master from source.

Sjors commented at 4:50 pm on November 7, 2025:

In the longer run it probably makes sense for sv2-tp to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn’t have backports. Each release is a combination of bug fixes and new features.

ismaelsadeeq commented at 5:14 pm on November 7, 2025:

In the longer run it probably makes sense for sv2-tp to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn’t have backports. Each release is a combination of bug fixes and new features.

Hmm, fwiw I am approach NACK changes that introduce conflicting interface method that deprecate and don’t prune on experimental interface that did not offer interface stability for the reason discussed above. However I am open minded to convincing otherwise and curious to know what other contributors think.

Sjors commented at 5:21 pm on November 7, 2025:

The old methods are redundant, but they’re not conflicting. Clients can use either and both have test coverage.

From this projects perspective there’s hardly any additional review burden when we prune deprecated methods in a later PR, that should be trivial.

Sjors commented at 6:09 pm on November 7, 2025:

To sum up what needs to happen if we delete instead of deprecate these old methods:

sv2-tp needs a new release that includes https://github.com/stratum-mining/sv2-tp/pull/59 (no problem, will happen anyway)
the SRI TP (draft) https://github.com/stratum-mining/sv2-apps/pull/59 needs to be updated to support both the old and new method (like sv2-tp does), sometime before v31 comes out (support for the old method can be dropped after v31 comes out, at the cost of dropping support for v30). a) https://github.com/2140-dev/bitcoin-capnp-types needs to track 30.x and master
the instructions at https://stratumprotocol.org/developers#run-template-provider need to be updated to specify a minimum template provider version (since it supports “Bitcoin Core v30 or newer”)

Things that should be unaffected:

CI for https://github.com/stratum-mining/sv2-apps pins Bitcoin Core to a release (e.g. v30) so is not impacted by changes to master. As long as they bump the template provider version when bumping Bitcoin Core to v31 nothing should break.
Miners who patch Bitcoin Core (e.g. for merge mining) and stick to the v30.x branch
Any sv2 tutorial or Docker automation that pins Bitcoin Core to v30 (rather than master)

None of these things are hard, but I prefer to avoid the coordination overhead and reserve that for a more important change.

Sjors commented at 1:51 pm on November 11, 2025:

Let’s also wait and see what comes out of #33421 (review) because if I have to modify the signature of submitSolution() (to return a CBlock), that’s the kind of breaking change I think is worth making.

Update: no need to change submitSolution()

Sjors force-pushed on Nov 7, 2025

Sjors commented at 4:55 pm on November 7, 2025: member

It’s ready for testing with:

https://github.com/stratum-mining/sv2-tp/pull/59

I also pushed a small change that switches ExtractCoinbaseTemplate to take CTransactionRef coinbase_tx as its argument rather than a CBlockTemplate. It doesn’t really make a difference here, but lets me copy-paste it to sv2-tp which doesn’t have CBlockTemplate.

Sjors commented at 6:11 pm on November 7, 2025: member

@plebhash can you try to see if it’s easy for you to support both getCoinbase() and getCoinbaseTx() in https://github.com/stratum-mining/sv2-apps/pull/59, preferring the first and falling back to the latter if it doesn’t exist?

in test/functional/interface_ipc.py:226 in 0dfe602281

223             coinbase.vout[0].scriptPubKey = miniwallet.get_output_script()
224+            coinbase.vout[0].nValue = coinbase_template.valueRemaining
225+            # Add SegWit OP_RETURN. This is currently always present even for
226+            # empty blocks, but this may change.
227+            found_witness_op_return = False
228+            print(WITNESS_COMMITMENT_HEADER.hex())

optout21 commented at 1:08 pm on November 11, 2025:

Could you add a short text before the value to the printout? Or if this is a debug-leftover, remove it.

Sjors commented at 1:21 pm on November 11, 2025:

Sorry that’s leftover debugging code.

in test/functional/interface_ipc.py:202 in 0dfe602281

197@@ -188,10 +198,56 @@ async def async_routine():
198             check_opts = self.capnp_modules['mining'].BlockCheckOptions()
199             template = await mining.result.createNewBlock(opts)
200             block = await self.parse_and_deserialize_block(template, ctx)
201-            coinbase = await self.parse_and_deserialize_coinbase_tx(template, ctx)
202-            balance = miniwallet.get_balance()
203+
204+            self.log.debug("Build coinbase transaction using getCoinbase()")

optout21 commented at 1:11 pm on November 11, 2025:

Maybe extract this part into a separate method; that can also serve as an example on how to build the cb tx from the getCoinbase() result, and leave the flow of this test more readable

Sjors commented at 1:38 pm on November 11, 2025:

Done

optout21 commented at 1:16 pm on November 11, 2025: none

There is no direct C++ unit test for getCoinbase/ExtractCoinbaseTemplate, would it make sense to add one? Or is the interface test sufficient (test/functional/interface_ipc.py)?

in test/functional/interface_ipc.py:237 in 0dfe602281

234+                coinbase.vout.append(output)
235+                if output.scriptPubKey == coinbase_commitment:
236+                    found_witness_op_return = True
237+
238+            assert_equal(has_witness, found_witness_op_return)
239+

optout21 commented at 1:34 pm on November 11, 2025:

In case of has_witness, coinbase_commitment and coinbase_template.witness could be directly compared here.

Sjors commented at 1:39 pm on November 11, 2025:

I think the comparison below should be enough, and I want to make it as simple as possible to later drop the deprecated method.

optout21 commented at 1:52 pm on November 11, 2025:

My reasoning was to add extra assurance that the deprecated and new method gives the same result, it’s a simple test. But it’s fine as it is.

Sjors force-pushed on Nov 11, 2025

in src/node/interfaces.cpp:911 in 0dfe602281

optout21 commented at 1:39 pm on November 11, 2025:

As a progression towards getCoinbase & deprecation, this implementation could invoke ExtractCoinbaseTemplate, and return the witness field (after uint256 -> unsigned char array type conversion)

Sjors commented at 1:44 pm on November 11, 2025:

Refactoring these deprecated methods would add to the review burden. And there’s still ongoing discussion above about whether this PR should even keep them

optout21 commented at 1:47 pm on November 11, 2025:

As a progression towards getCoinbase & deprecation, this implementation could internally also do what is suggested in the deprecation node: invoke ExtractCoinbaseTemplate, scan outputs for the SegWit marker, and return the index.

Sjors commented at 1:39 pm on November 11, 2025: member

There is no direct C++ unit test

No strong opinion on whether we need one.

in src/interfaces/mining.h:56 in 0dfe602281 outdated

51+     **/
52     virtual CTransactionRef getCoinbaseTx() = 0;
53+    /**
54+     * Return scriptPubKey with SegWit OP_RETURN.
55+     *
56+     * @note deprecated: use the output field from getCoinbase()

Sjors commented at 1:42 pm on November 11, 2025:

getCoinbase() returns a CoinbaseTemplate struct which has an outputs field, which itself is an array with zero or more elements.

(I’ll rename output to outputs)

optout21 commented at 1:42 pm on November 11, 2025:

0     * [@note](/bitcoin-bitcoin/contributor/note/) deprecated: use the outputs field from getCoinbase()

optout21 commented at 1:43 pm on November 11, 2025:

The witness field could also be used, isn’t it?

Sjors commented at 1:48 pm on November 11, 2025:

This returns the full scriptPubKey as generated by GenerateCoinbaseCommitment. Although you can derive that from the witness field, I don’t think one should.

in src/interfaces/mining.h:62 in 0dfe602281 outdated

57+     */
58     virtual std::vector<unsigned char> getCoinbaseCommitment() = 0;
59+    /**
60+     * Return which output in the dummy coinbase contains the SegWit OP_RETURN.
61+     *
62+     * @note deprecated. Scan outputs from getCoinbase() output field for the

optout21 commented at 1:44 pm on November 11, 2025:

0     * [@note](/bitcoin-bitcoin/contributor/note/) deprecated. Scan outputs from getCoinbase() outputs field for the

optout21 commented at 1:48 pm on November 11, 2025: none

Concept ACK (d926d31603316a10e3210f10480dd02bb34f667a)

The added new method makes sense. The deprecating methods are only being marked in the header comments, they are kept, there is no formal plan for deprecation. But there is a fair plan for switching users, and keeping them is not an extra burden.

Sjors force-pushed on Nov 11, 2025

DrahtBot added the label CI failed on Nov 11, 2025

DrahtBot commented at 1:50 pm on November 11, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19267405324/job/55086515663 LLM reason (✨ experimental): Python lint failure (ruff): unused import detected in test/interface_ipc.py causing CI to fail.

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.

Sjors force-pushed on Nov 11, 2025

Sjors commented at 2:51 pm on November 11, 2025: member

CI failed because it runs against master and #33575 has been merged, which added interruptWait at position @11. I rebased and bumped getCoinbase() to @12.

fanquake commented at 4:18 pm on November 11, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/19269384275/job/55093373232?pr=33819#step:7:3549:

0 Error:  node0 2025-11-11T15:38:48.659039Z [unknown] [validation.cpp:4535] [bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock> &, bool, bool, bool *)] [error] ProcessNewBlock: AcceptBlock FAILED (bad-version(0x00000000), rejected nVersion=0x00000000 block)

Sjors commented at 4:45 pm on November 11, 2025: member

Odd, the invalid-version error is happening at the bottom of this block:

 0self.log.debug("Submit a block with a bad version")
 1block.nVersion = 0
 2block.solve()
 3res = await mining.result.checkBlock(block.serialize(), check_opts)
 4assert_equal(res.result, False)
 5assert_equal(res.reason, "bad-version(0x00000000)")
 6res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
 7assert_equal(res.result, False)
 8self.log.debug("Submit a valid block")
 9block.nVersion = original_version
10block.solve()
11res = await mining.result.checkBlock(block.serialize(), check_opts)
12assert_equal(res.result, True)

It’s about the block nVersion, not the coinbase, so this PR shouldn’t have changed that.

Actually FAILED (bad-version(0x00000000) is not the reason the test fails, it’s an expected error log message emitted by the node.

The actual error is a little further down: bad-cb-height, which seems more likely to be related to this PR.

mining: add getCoinbase()

Deprecate getCoinbaseTx() in favor of a new method that provides a
struct with everything clients need to construct a coinbase. This
is safer than providing a raw dummy coinbase that clients then have to
manipulate.

Also deprecate getCoinbaseCommitment() and getWitnessCommitmentIndex().

A new helper method ExtractCoinbaseTemplate() processes the dummy
coinbase transaction generated by BlockAssembler::CreateNewBlock
and produces a CoinbaseTemplate.

Expand the interface_ipc.py functional test to document its usage.

80acabaf93

Sjors force-pushed on Nov 11, 2025

Sjors commented at 6:02 pm on November 11, 2025: member

It turns out that using the getCoinbase() result directly in the Python test led to memory management related errors. Parts of the coinbase were dropped by the time of the second checkBlock().

This probably happened once I extracted the coinbase specific checks into build_coinbase_test, which returns the coinbase CTransaction but presumably let go of the getCoinbase() result.

Rather than debug (our use of) PyCapnp, I just introduced a @dataclass for CoinbaseTemplateData, so we fully own (everything in) it.

plebhash commented at 6:56 pm on November 11, 2025: none

@plebhash can you try to see if it’s easy for you to support both getCoinbase() and getCoinbaseTx() in stratum-mining/sv2-apps#59, preferring the first and falling back to the latter if it doesn’t exist? @Sjors we’re not using getCoinbaseTx on that code

we do getBlock and that provides everything we need, including relevant coinbase info

DrahtBot removed the label CI failed on Nov 11, 2025

Sjors commented at 8:23 am on November 12, 2025: member

we do getBlock and that provides everything we need

Ok, that’s not really how you’re supposed to use it :-)

It does make a breaking change potentially unpainful.

However there’s another gotcha if we remove a method from the interface: there’s no gaps allowed in the .capnp method numbering. This could be worked around as follows:

 0interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
 1    destroy [@0](/bitcoin-bitcoin/contributor/0/) (context :Proxy.Context) -> ();
 2    getBlockHeader [@1](/bitcoin-bitcoin/contributor/1/) (context: Proxy.Context) -> (result: Data);
 3    getBlock [@2](/bitcoin-bitcoin/contributor/2/) (context: Proxy.Context) -> (result: Data);
 4    getTxFees [@3](/bitcoin-bitcoin/contributor/3/) (context: Proxy.Context) -> (result: List(Int64));
 5    getTxSigops [@4](/bitcoin-bitcoin/contributor/4/) (context: Proxy.Context) -> (result: List(Int64));
 6    getCoinbase [@5](/bitcoin-bitcoin/contributor/5/) (context: Proxy.Context) -> (result: CoinbaseTemplate);
 7    # getCoinbaseTx [@5](/bitcoin-bitcoin/contributor/5/) (context: Proxy.Context) -> (result: Data);
 8    # getCoinbaseCommitment [@6](/bitcoin-bitcoin/contributor/6/) (context: Proxy.Context) -> (result: Data);
 9    getWitnessCommitmentIndex [@7](/bitcoin-bitcoin/contributor/7/) (context: Proxy.Context) -> (result: Int32);
10    getCoinbaseMerklePath [@8](/bitcoin-bitcoin/contributor/8/) (context: Proxy.Context) -> (result: List(Data));
11    submitSolution [@9](/bitcoin-bitcoin/contributor/9/) (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
12    waitNext [@10](/bitcoin-bitcoin/contributor/10/) (context: Proxy.Context, options: BlockWaitOptions) -> (result: BlockTemplate);
13    interruptWait [@6](/bitcoin-bitcoin/contributor/6/)() -> ();
14}

So @5 is assigned from getCoinbaseTx to getCoinbase, @6 is changed from getCoinbaseCommitment to interruptWait (not in v30 and not yet in an sv2-tp release), but getWitnessCommitmentIndex is kept (but still deprecated and made to error).

But such an intermediate state seems needlessly confusing, so my first preference is still to not make a breaking change here, my second preference to completely break things, not this intermediate approach.

DrahtBot added the label Needs rebase on Nov 12, 2025

DrahtBot commented at 4:56 pm on November 12, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

mining: add getCoinbase() #33819

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)