mining: getCoinbase() returns struct instead of raw tx #33819

Sjors · 2025-11-07T14:30:41Z

The first commit renames getCoinbaseTx() to getCoinbaseRawTx() to reflect that it returns a serialised transaction. This does not impact IPC clients, because they do not use the function name.

The second commit then introduces a replacement getCoinbase() that provides a struct with everything clients need to construct a coinbase. This avoids clients having to parse and manipulate our dummy transaction.

Deprecate but don't remove getCoinbaseRawTx(), getCoinbaseCommitment() and getWitnessCommitmentIndex().

After this change we can drop these deprecated methods, which in turn would allow us to clear the dummy transaction from the getBlock() result. But that is left for a followup to keep this PR focussed. See Sjors#106 for an approach.

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

Can be tested using:

Use improved getCoinbaseTx() if available stratum-mining/sv2-tp#59

DrahtBot · 2025-11-07T14:30:51Z

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
ACK	ryanofsky, ismaelsadeeq, vasild
Concept ACK	optout21
Approach ACK	ajtowns

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
#33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
#33965 (mining: fix -blockreservedweight shadows IPC option by Sjors)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33795 (test: Ignore error message give from python because of PYTHON_GIL by kevkevinpal)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#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.

Sjors · 2025-11-07T14:31:33Z

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

src/interfaces/mining.h

Sjors · 2025-11-07T16:55:09Z

It's ready for testing with:

Use improved getCoinbaseTx() if available stratum-mining/sv2-tp#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 · 2025-11-07T18:11:48Z

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

test/functional/interface_ipc.py

optout21 · 2025-11-11T13:16:39Z

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)?

test/functional/interface_ipc.py

src/node/interfaces.cpp

Sjors · 2025-11-11T13:39:55Z

There is no direct C++ unit test

No strong opinion on whether we need one.

Sjors · 2025-11-11T13:42:00Z

src/interfaces/mining.h

+    /**
+     * Return scriptPubKey with SegWit OP_RETURN.
+     *
+     * @note deprecated: use the output field from getCoinbase()


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)

src/interfaces/mining.h

optout21 · 2025-11-11T13:47:56Z

src/node/interfaces.cpp

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.

re: #33819 (comment)

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.

I don't see a clean way to get the index from ExtractCoinbaseTemplate, but even if there were, calling GetWitnessCommitmentIndex seems like the simplest implementation of this method. Changing this might make more sense if the GetWitnessCommitmentIndex function was only called this one place and could be eliminated, but this isn't the case.

The latest push dropped ExtractCoinbaseTemplate entirely.

optout21 · 2025-11-11T13:48:15Z

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.

DrahtBot · 2025-11-11T13:50:18Z

🚧 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.}

Hints

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 · 2025-12-04T08:28:02Z

Applied both suggestions from @ryanofsky: #33819 (review)

ryanofsky

Code review ACK 5761dac2f5481c27550483279a007c2b850bc990, with the two suggestions applied since last review.

Sjors · 2025-12-08T10:15:01Z

Trivial rebase after #29641.

ryanofsky

Code review ACK 1bd6cae. Just rebased with no changes, to avoid trivial merge conflict.

ryanofsky

Code review ACK 1bd6cae. Just rebasing again since last review to avoid minor conflict

This frees up the name getCoinbaseTx() for the next commit. Changing a function name does not impact IPC clients, as they only consider the function signature and sequence number.

Sjors · 2025-12-19T09:43:29Z

Rebased after #34003.

ismaelsadeeq

ACK 979805b

The commit message is helpful for my review.

I still prefer that deprecated methods be removed once we have a replacement for them, for 1. better hygiene for the codebase. 2 Ensures we only maintain what should be used and our perceived valid approach for IPC clients. 3. Will prevent naive users from using deprecated method.

However, others seem to prefer deprecating them first, then batch-removing them later.

#33819 (review) Option 2 is my second preferred option. The method names and numbering are kept, but we return an error when someone tries to use them
It would allow us to remove the implementation code and tests while maintaining the methods and numbering for the transition period (can even be kept indefinitely since no implementation and test to maintain, could also be deleted in batch).

src/node/interfaces.cpp

src/ipc/capnp/mining.capnp

ismaelsadeeq · 2026-01-04T11:10:52Z

src/node/types.h

+     * Currently there are no non-zero required_outputs, so block_reward_remaining
+     * is the entire block reward. See also required_outputs.
+     */
+    CAmount block_reward_remaining;
+    /*
+     * To be included as the last outputs in the coinbase transaction.
+     * Currently this is only the witness commitment OP_RETURN, but future
+     * softforks or a custom mining patch could add more.
+     *
+     * The dummy output that spends the full reward is excluded.
+     */


In 979805b "mining: add new getCoinbaseTx() returning a struct"

nit: Current comment is a bit speculative. I think we should just document existing rules and requirements.

The reason it's a bit more elaborate is that otherwise people will be confused why this is an array.

The part about merged mining is not speculative, only the part about future soft-forks. But from the point of view of a client implementer this speculation is very important to keep in mind. They should not be lazy and only take the first element from the array. And by "they" I mean myself :-) stratum-mining/sv2-tp#55

re: #33819 (comment)

nit: Current comment is a bit speculative. I think we should just document existing rules and requirements.

Agree with sjors it is important to document things that may be required in the future even if they are not required now. But in case this is updated, it would be good to make the "Currently..." sentence a separate final paragraph. This way, all the fields would be documented in a consistent way with descriptions & requirements first, and information about specific values they currently contain last.

src/node/miner.h

Introduce a new method intended to replace getCoinbaseRawTx(), which 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. The CoinbaseTx data is populated during the dummy transaction generation and stored in struct CBlockTemplate. Expand the interface_ipc.py functional test to document its usage and ensure equivalence.

ryanofsky

Code review ACK 48f57bb. Just rebased and addressed comments and dropped coinbase tx "template" suffix, which is a nice change

ryanofsky · 2026-01-07T19:15:08Z

src/node/types.h

+     * Currently there are no non-zero required_outputs, so block_reward_remaining
+     * is the entire block reward. See also required_outputs.
+     */
+    CAmount block_reward_remaining;
+    /*
+     * To be included as the last outputs in the coinbase transaction.
+     * Currently this is only the witness commitment OP_RETURN, but future
+     * softforks or a custom mining patch could add more.
+     *
+     * The dummy output that spends the full reward is excluded.
+     */


re: #33819 (comment)

nit: Current comment is a bit speculative. I think we should just document existing rules and requirements.

Agree with sjors it is important to document things that may be required in the future even if they are not required now. But in case this is updated, it would be good to make the "Currently..." sentence a separate final paragraph. This way, all the fields would be documented in a consistent way with descriptions & requirements first, and information about specific values they currently contain last.

ismaelsadeeq

code review ACK 48f57bb

vasild

ACK 48f57bb

doc/release-notes-33819.md

vasild · 2026-01-09T13:11:58Z

src/node/miner.cpp

+    coinbaseTx.vout[0].nValue = block_reward;
+    coinbase_tx.block_reward_remaining = block_reward;


Looks messy to have two variables named coinbaseTx and coinbase_tx. I do not have a better suggestion though. Feel free to ignore.

vasild · 2026-01-09T13:22:23Z

src/node/types.h

+    /* nVersion */
+    uint32_t version;
+    /* nSequence for the only coinbase transaction input */
+    uint32_t sequence;
+    /**


Change /* to /** in order to get doxygen link the comment with the variable that follows it, like the comments for the subsequent members. Same for the required_outputs member.

Will do if I need to retouch.

DrahtBot added the Mining label Nov 7, 2025

Sjors mentioned this pull request Nov 7, 2025

Mining interface tracking issue #33777

Open

13 tasks

ismaelsadeeq reviewed Nov 7, 2025

View reviewed changes

src/interfaces/mining.h Outdated Show resolved Hide resolved

Sjors mentioned this pull request Nov 7, 2025

Use improved getCoinbaseTx() if available stratum-mining/sv2-tp#59

Draft

Sjors force-pushed the 2025/11/coinbase-template branch from 79e73b0 to 0dfe602 Compare November 7, 2025 16:52

Sjors mentioned this pull request Nov 7, 2025

ipc: breaking but non-urgent changes Sjors/bitcoin#106

Open

1 task

Sjors mentioned this pull request Nov 7, 2025

Add 30.x branch and/or v30.0 tag 2140-dev/bitcoin-capnp-types#4

Open

This was referenced Nov 8, 2025

test: Ignore error message give from python because of PYTHON_GIL #33795

Open

mining: check witness commitment in submitBlock #33745

Merged

miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC #32420

Open