Skip to content

Use improved getCoinbaseTx() if available#59

Merged
Sjors merged 2 commits intostratum-mining:masterfrom
Sjors:2025/11/coinbase
Feb 16, 2026
Merged

Use improved getCoinbaseTx() if available#59
Sjors merged 2 commits intostratum-mining:masterfrom
Sjors:2025/11/coinbase

Conversation

@Sjors
Copy link
Collaborator

@Sjors Sjors commented Nov 7, 2025 •
edited
Loading

Take advantage of the improved getCoinbaseTx() introduced in bitcoin/bitcoin#33819 which provides a CoinbaseTemplate struct.

This lets us avoid parsing Bitcoin Core's dummy transaction. For backwards compatibility when using the deprecated getCoinbaseRawTx() the ExtractCoinbaseTxTemplate still does that. This can be dropped after a Bitcoin Core release ships with the new method.

Builds on:

@Sjors Sjors mentioned this pull request Nov 7, 2025
Merged
@Sjors Sjors marked this pull request as draft November 7, 2025 17:07
@Sjors
Copy link
Collaborator Author

Sjors commented Nov 11, 2025 •
edited
Loading

Rebased after #57 and bumped getCoinbase to @12.

xyephy
xyephy reviewed Nov 17, 2025
View reviewed changes
Copy link
Contributor

@xyephy xyephy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, this is a much cleaner way to handle coinbase templates. I've highlighted the Assert fix in the comments below , then this is ready to merge (after #58 is fixed).

@Sjors
Copy link
Collaborator Author

Sjors commented Nov 17, 2025 •
edited
Loading

Updated against the latest changes in bitcoin/bitcoin#33819 (e.g. renaming inputSequence to sequence).

@Sjors
Copy link
Collaborator Author

Sjors commented Nov 18, 2025

Empty push after #58 was merged so Github sees it's only one commit now.

@Sjors Sjors mentioned this pull request Nov 18, 2025
Open
xyephy
xyephy reviewed Nov 27, 2025
View reviewed changes
src/sv2/messages.cpp
Comment on lines +25 to +26
for (const auto& output : coinbase.required_outputs) {
m_coinbase_tx_outputs.push_back(output);
Copy link
Contributor

@xyephy xyephy Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new constructor the loop just pushes all required_outputs:
This trusts that getCoinbase() already filtered to OP_RETURN outputs. The fallback path does
the filtering in ExtractCoinbaseTemplate(). This is correct assuming Bitcoin Core's
getCoinbase() returns pre-filtered outputs - just worth double-checking in Bitcoin Core.

Copy link
Collaborator Author

@Sjors Sjors Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trusts that getCoinbase() already filtered to OP_RETURN outputs.

Indeed. I think it's better that Bitcoin Core handles this filtering. This means we no longer have to assume only OP_RETURN outputs are possible.

Copy link
Contributor

@xyephy xyephy Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can say the PR is ready for merging.

Copy link
Collaborator Author

@Sjors Sjors Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I plan to do that after bitcoin/bitcoin#33819 lands.

@Sjors Sjors changed the title Use getCoinbase() if available Use improved getCoinbase() if available Dec 3, 2025
@Sjors Sjors changed the title Use improved getCoinbase() if available Use improved getCoinbaseTx() if available Dec 3, 2025
@Sjors
Copy link
Collaborator Author

Sjors commented Dec 3, 2025 •
edited
Loading

Updated to match the latest Bitcoin Core PR version. The interface changes in that latest update only rename methods and variables, which doesn't impact IPC, so you can still use ece49d3.

ryanofsky added a commit to bitcoin/bitcoin that referenced this pull request Jan 13, 2026
@ryanofsky
Merge #33819: mining: getCoinbase() returns struct instead of raw tx
62557c9 
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
Sjors added 2 commits February 10, 2026 13:11
@Sjors
mining: rename getCoinbaseTx() to ..RawTx()
56895dd 
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
Use CoinbaseTemplate if available
ee016be 
Avoid the brittle process of constructing the template
coinbase fields from Bitcoin Core's dummy coinbase transaction.

This uses the new getCoinbaseTx() interface method. Fall back to the
old approach if that method is not available.
@Sjors
Copy link
Collaborator Author

Sjors commented Feb 10, 2026 •
edited
Loading

Rebased. Renamed CoinbaseTxTemplate to CoinbaseTx like upstream.

bitcoin/bitcoin#33819 was merged so this is ready.

For testing: use the master branch from Bitcoin Core

@Sjors Sjors marked this pull request as ready for review February 10, 2026 12:39
@Sjors Sjors requested a review from xyephy February 10, 2026 13:24
xyephy
xyephy approved these changes Feb 11, 2026
View reviewed changes
Copy link
Contributor

@xyephy xyephy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK.

The CoinbaseTx struct, ExtractCoinbaseTx fallback, and the try/catch backward compatibility mechanism all work correctly. Ready to merge.

@Sjors Sjors merged commit f9d2382 into stratum-mining:master Feb 16, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@xyephy xyephy xyephy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Sjors @xyephy