Support multiple coinbase outputs by xyephy · Pull Request #55 · stratum-mining/sv2-tp

xyephy · 2025-10-28T18:49:04Z

Previously only the first coinbase output was serialized, breaking merge mining and multi-payout pool scenarios.

This change iterates through all outputs in m_coinbase_tx_outputs and serializes them sequentially using VectorWriter.

Also adds debug logging to track the number of outputs and total size for observability.

Fixes #18

Sjors · 2025-10-28T18:56:36Z

Thanks, it would be good to have some tests for this, maybe in https://github.com/Sjors/sv2-tp/blob/master/src/test/sv2_messages_tests.cpp

xyephy · 2025-10-28T19:15:43Z

Thanks, it would be good to have some tests for this, maybe in https://github.com/Sjors/sv2-tp/blob/master/src/test/sv2_messages_tests.cpp

Let me work on that, thank you for the feedback.

Adds Sv2NewTemplate_MultipleOutputs_test which verifies that all coinbase outputs are serialized correctly, not just the first one. The test creates a NewTemplate message with 3 different outputs (100, 200, and 300 sats) and checks that the serialized bytes match the expected format with all three outputs included. Requested by maintainer in PR stratum-mining#55.

xyephy · 2025-10-28T22:09:04Z

@Sjors I have pushed some test updates, looking forward to hear what you think.

Sjors · 2025-10-29T10:03:16Z

Thanks! Will review in more detail soon. I assume the test fails if you omit the first commit?

It would be nice to also (manually) test this in a Bitcoin Core + sv2_tp + SRI integration. I guess we'd need a patch or Bitcoin Core to actually add a second output, it can just be nonsense, on e.g. signet.

xyephy · 2025-10-29T10:39:32Z

Thanks! Will review in more detail soon. I assume the test fails if you omit the first commit?

It would be nice to also (manually) test this in a Bitcoin Core + sv2_tp + SRI integration. I guess we'd need a patch or Bitcoin Core to actually add a second output, it can just be nonsense, on e.g. signet.

I think it should work, the only time it failed is when I did 3 commits so I was forced to squash one and test passed, do you want me to make it one commit instead of two?

I will try do a manual simulation as you have advised and then share my findings, hopefully today or latest tomorrow.

Sjors · 2025-10-29T11:28:06Z

one commit instead of two

No, two is fine. I'll try it.

manual simulation as you have advised and then share my findings

Thanks, please share the steps. I haven't worked on merge mining ever, so not sure what the easiest way is.

xyephy · 2025-10-29T13:40:54Z

Thanks, please share the steps. I haven't worked on merge mining ever, so not sure what the easiest way is.

Let me share the steps once I've figured out the merge mining and patch needed for bitcoin core

Sjors · 2025-10-29T16:24:23Z

CI is finding test failures:

test/sv2_template_provider_tests.cpp(98): fatal error: in "sv2_template_provider_tests/client_tests": Unexpected message size while receiving initial template broadcast

xyephy · 2025-10-29T17:21:40Z

CI is finding test failures:

test/sv2_template_provider_tests.cpp(98): fatal error: in "sv2_template_provider_tests/client_tests": Unexpected message size while receiving initial template broadcast

working on a fix to solve the ci fail

xyephy · 2025-10-31T12:51:14Z

@Sjors I've pushed the new update, here's are the steps I used to test with https://gist.github.com/xyephy/13aff2a54c6286316ce7f8f089862c7f . Let me know if changes are now working.

Sjors

Thanks for the elaborate testing instructions!

It made me realise there's a problem: the node always adds a dummy output script (typically OP_TRUE) with the full coinbase reward. We need to exclude that (or the pool will make an invalid block because it tries to spend that amount again).

I think the most robust solution is to filter the coinbase outputs for OP_RETURN. That includes SegWit and (hopefully) all merge mining schemes.

A cleaner solution would require an update to Bitcoin Core to either:

Stop adding the dummy output; or
Add a method similar to getWitnessCommitmentIndex() to indicate which other inputs are commitments that need to be included.

src/sv2/messages.cpp

Sjors · 2025-10-31T13:06:08Z

src/sv2/messages.h

+                VectorWriter{outputs_bytes, outputs_bytes.size(), output};
+            }
+
+            LogPrintLevel(BCLog::SV2, BCLog::Level::Debug, "Serialized %zu coinbase output(s), total size: %zu bytes\n",


nit: maybe downgrade to trace?

Sjors · 2025-10-31T13:11:48Z

src/test/sv2_template_provider_tests.cpp

-        4 +                 // coinbase_tx_outputs_count (0)
-        2 +                 // B0_64K length for outputs array (0)
+        4 +                 // coinbase_tx_outputs_count (1 - mock creates 1 output)
+        2 + 10 +            // B0_64K: length(10) + 1 output (8 bytes nValue + 2 bytes script)


I was initially confused why this changed, but it's because we're now including all coinbase outputs.

Let's make the mock more realistic add give it:

a dummy anyone-can-spend output, e.g. OP_TRUE with 50BTC

a (fake) segwit commitment rather than the current OP_RETURN, and have the mock getWitnessCommitmentIndex return that position.

a (fake) merge mining commitment

Sjors · 2025-10-31T13:18:13Z

src/test/sv2_messages_tests.cpp

+    // U32              ffffffff            coinbase tx input sequence
+    // U64              0040075af0750700    coinbase tx value remaining
+    // U32              03000000            coinbase tx outputs count (3 outputs)
+    // B0_64K           2100                coinbase_tx_outputs (33 bytes total)


Let's add an anyone-can-spend output here, because the node will provide one and we need to omit it.

Sjors · 2025-10-31T13:19:26Z

src/sv2/messages.cpp

-    if (witness_commitment_index != NO_WITNESS_COMMITMENT) {
-        m_coinbase_tx_outputs_count = 1;
-        m_coinbase_tx_outputs = {coinbase_tx->vout[witness_commitment_index]};
+    // Extract ALL coinbase outputs to support merge mining and other use cases


I think we need to filter this to cover only OP_RETURNs, otherwise we include the dummy output added by the node and since the spends the full reward, the pool would make an invalid block.

xyephy · 2025-10-31T14:16:05Z

@Sjors thank you for the detailed review, let me look into it and add the recommendations you've made so as to have a much better solution.

Implement PR stratum-mining#55 review feedback from Sjors: - Changed NewTemplate to only include OP_RETURN outputs instead of all coinbase outputs. Bitcoin Core adds a dummy output with the full block reward that must be filtered out, otherwise pools would create invalid blocks trying to spend that amount again. - Removed unused witness_commitment_index parameter from Sv2NewTemplateMsg constructor. - Changed output serialization log from Debug to Trace level to reduce log noise. - Updated test mocks to create realistic coinbase transactions with multiple outputs (1 reward + 2 OP_RETURN) to properly test filtering. - Updated all test expectations to reflect filtered OP_RETURN behavior. Testing: - All 19 unit tests pass - End-to-end tested with SRI mining-device: 397 blocks mined successfully - Zero coinbase validation errors across all mined blocks

Sjors · 2025-11-03T10:14:30Z

Thanks, that looks better at first glance. Can you squash both commits?

xyephy · 2025-11-03T10:17:26Z

@Sjors I have pushed new fixes as you recommended, I've managed to solo mine with Sri mining-device without getting any invalid blocks. Let me know if this works on your end and if there are any further changes I need to add, thank you.

xyephy · 2025-11-03T10:18:10Z

Thanks, that looks better at first glance. Can you squash both commits?

let me do that

This change enables sv2-tp to handle block templates with multiple coinbase outputs, which is required for merge mining and other use cases where additional coinbase outputs are needed. Implementation: - Changed NewTemplate to only include OP_RETURN outputs instead of all coinbase outputs. Bitcoin Core adds a dummy output with the full block reward that must be filtered out, otherwise pools would create invalid blocks trying to spend that amount again. - Removed unused witness_commitment_index parameter from Sv2NewTemplateMsg constructor. - Changed output serialization log from Debug to Trace level to reduce log noise. - Updated test mocks to create realistic coinbase transactions with multiple outputs (1 reward + 2 OP_RETURN) to properly test filtering. - Updated all test expectations to reflect filtered OP_RETURN behavior. Testing: - All 19 unit tests pass - End-to-end tested with SRI mining-device: 397 blocks mined successfully - Zero coinbase validation errors across all mined blocks Fixes: stratum-mining#18

Sjors · 2025-11-03T10:41:02Z

Looks great, thanks!

Sjors · 2025-11-03T12:38:22Z

It would be good to round-trip test this in SRI: stratum-mining/sv2-apps#58

xyephy · 2025-11-03T19:26:58Z

It would be good to round-trip test this in SRI: stratum-mining/sv2-apps#58

let me do that.

xyephy force-pushed the support-multiple-coinbase-outputs branch from 3d24164 to 1adcfcf Compare October 28, 2025 21:47

xyephy force-pushed the support-multiple-coinbase-outputs branch from b202a40 to b8363fc Compare October 29, 2025 14:02

xyephy force-pushed the support-multiple-coinbase-outputs branch from b8363fc to 3ccbe50 Compare October 29, 2025 17:14

xyephy force-pushed the support-multiple-coinbase-outputs branch from 3ccbe50 to 820bd71 Compare October 31, 2025 12:12

Sjors reviewed Oct 31, 2025

View reviewed changes

Sjors mentioned this pull request Oct 31, 2025

mining: check witness commitment in submitBlock bitcoin/bitcoin#33745

Merged

xyephy force-pushed the support-multiple-coinbase-outputs branch from 36a7ed1 to 9e68692 Compare November 3, 2025 10:23

Sjors merged commit d812921 into stratum-mining:master Nov 3, 2025
19 checks passed

Sjors mentioned this pull request Nov 3, 2025

Integration tests against TP with multiple coinbase outputs stratum-mining/sv2-apps#58

Open

xyephy mentioned this pull request Nov 3, 2025

Integration tests should use sv2-tp via IP and Bitcoin Core v30 stratum-mining/sv2-apps#57

Closed

Sjors mentioned this pull request Jan 5, 2026

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

Merged

Conversation

xyephy commented Oct 28, 2025

Uh oh!

Sjors commented Oct 28, 2025

Uh oh!

xyephy commented Oct 28, 2025

Uh oh!

xyephy commented Oct 28, 2025

Uh oh!

Sjors commented Oct 29, 2025

Uh oh!

xyephy commented Oct 29, 2025

Uh oh!

Sjors commented Oct 29, 2025

Uh oh!

xyephy commented Oct 29, 2025

Uh oh!

Sjors commented Oct 29, 2025

Uh oh!

xyephy commented Oct 29, 2025

Uh oh!

xyephy commented Oct 31, 2025

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Sjors Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Sjors Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

xyephy commented Oct 31, 2025

Uh oh!

Sjors commented Nov 3, 2025

Uh oh!

xyephy commented Nov 3, 2025

Uh oh!

xyephy commented Nov 3, 2025

Uh oh!

Uh oh!

Sjors commented Nov 3, 2025

Uh oh!

Sjors commented Nov 3, 2025

Uh oh!

xyephy commented Nov 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sjors left a comment •

edited

Loading

Sjors Oct 31, 2025 •

edited

Loading