ipc: breaking but non-urgent changes #106
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
Jassor909
approved these changes
Nov 7, 2025
Sjors
force-pushed
the
2025/11/interface-breaking
branch
5 times, most recently
from
326789a to
e502f2d
Compare
Introduce a new method intended to replace getCoinbaseTx(), 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. 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 and ensure equivalence.
Add default_witness_commitment to CoinbaseTemplate for use in the getblocktemplate RPC.
Drop the following mining interface methods in favor of getCoinbase(): - getCoinbaseTx() - getCoinbaseCommitment() - getWitnessCommitmentIndex()
Clear dummy coinbase from block templates by default so it's not exposed to callers of getBlock(). Have RPC and test code opt-in to keeping the dummy coinbase where needed.
Adding a context parameter ensures that these methods are run in their own thread and don't block other calls. This is especially important for checkBlock() which may receive a slow to validate block from an untrusted origin (the IPC connection itself is assumed to trusted). This is a breaking change both ways: - clients must use the updated capnp file - updated clients can't call these methods on an unupgraded node
Sjors
force-pushed
the
2025/11/interface-breaking
branch
from
e502f2d to
e883903
Compare
Previously the dummy coinbase returned by the Mining IPC interface always had a witness and SegWit OP_RETURN output. This is unnecessary for empty blocks as well as for blocks without any SegWit spends. This commit makes this behavior configurable via always_add_coinbase_commitment on BlockCreateOptions. It defaults to false, making this a potentially breaking change to clients. The presence of this field in requests makes it incompatible with Bitcoin Core v30, so it's a breaking change in any case. Preserve historical getblocktemplate RPC behavior. Although it doesn't return a coinbase transaction, it does always provide a default_witness_commitment. This is set in BlockAssembler::CreateNewBlock() via a call GenerateCoinbaseCommitment(), which has the side-effect of adding a coinbase witness.
Sjors
force-pushed
the
2025/11/interface-breaking
branch
from
e883903 to
40d1886
Compare
Owner
Author
|
I'll rebase after bitcoin#33880 lands to see if that fixes the failing jobs. |
Owner
Author
|
PR bitcoin#33936 fixes an important memory management bug, so it justifies a breaking change imo. If it lands then I might as well PR the other breaking changes here. |
ryanofsky
added a commit
to bitcoin/bitcoin
that referenced
this pull request
Jan 13, 2026
48f57bb mining: add new getCoinbaseTx() returning a struct (Sjors Provoost) d59b4cd mining: rename getCoinbaseTx() to ..RawTx() (Sjors Provoost) Pull request description: 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: - stratum-mining/sv2-tp#59 ACKs for top commit: ryanofsky: Code review ACK 48f57bb. Just rebased and addressed comments and dropped coinbase tx "template" suffix, which is a nice change ismaelsadeeq: code review ACK 48f57bb vasild: ACK 48f57bb Tree-SHA512: c4f1d752777fb3086a1a0b7b8b06e4205dbe2f3adb41f218855ad1dee952adccc263cf82acd3bf9300cc83c2c64cebd2b27f66a69beee32d325b9a85e3643b0d
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I plan to split these into standalone pull requests in due time.
Based on
Non-breaking commits
mining: deprecate getCoinbaseCommitment()
Its value is only used by the
getblocktemplateRPC and is redundant with therequired_outputsfields ofgetCoinbase()Breaking commits
mining: drop getCoinbaseTx(), etc
This drops three deprecated methods:
getCoinbaseTx()getCoinbaseCommitment()getWitnessCommitmentIndex()Pass context to
createNewBlock()andcheckBlock()Adding a
contextparameter ensures that these methods are run in their own thread and don't block other calls.This is especially important for checkBlock() which may receive a slow to validate block from an untrusted origin (the IPC connection itself is assumed to trusted).
This is a breaking change both ways:
Make non-mandatory coinbase commitment opt-in
This adds
always_add_coinbase_commitmenttoBlockCreateOptionsIt could be carefully documented to avoid making it a breaking change, by telling clients to omit it for older Bitcoin Core versions. But it's easier as a breaking change.
Part of the test is commented out pending the newly proposed
getCoinbase()method