Add max coinbase out sigops to CoinbaseOutputDataSize in TP protocol#86
Conversation
The template provide need a way to know how many sigops the pool want to include in the coinbase outupts, otherwise is not able to create a template that if mined will result in a valid block.
|
I can implement this on the template provider side. Let's recommend 400 as the default, as that's what Bitcoin Core has been using for ages ( This will need some coordination in deployment, especially if we don't change the protocol version number (see #82). |
|
@Sjors I addressed your comments. When we have consensus on this I can implement it in SRI and you can add it to TP. I think the most contentious point here is how to handle versioning |
|
I'll let you know here once I have a branch with this change in it for the Template Provider. If you can open a pull request on the SRI repo with the same change, we can test if they're compatible. |
This is still missing. Perhaps worded as:
|
not sure if we should recommend a default at all. If so I would go with 0. |
|
0 will create an invalid block if you your coinbase output is non-taproot. And you won't find out until someone starts spamming the mempool with bare multisig: https://bitcoinexplainedpodcast.com/@nado/episodes/bitcoin-explained-76-stamps-and-the-invalid-block-caused-by-it The reason I suggest 400 is because that's what |
| | Field Name | Data Type | Description | | ||
| | ----------------------------------- | --------- | ----------------------------------------------------------------------------------------------- | | ||
| | coinbase_output_max_additional_size | U32 | The maximum additional serialized bytes which the pool will add in coinbase transaction outputs | | ||
| | coinbase_output_max_sigops | U16 | The maximum additional sigops which the pool will add in coinbase transaction outputs | |
There was a problem hiding this comment.
Should we add _additional for consistency?
what about: legacy pools should use 400, new pools should use 0 ? |
That assumes new pools want to use Taproot, that seems too DEMANDing :-) |
|
Here you go, completely untested: Sjors/bitcoin#45 (branch |
…ptions c504b69 refactor: add coinbase constraints to BlockCreateOptions (Sjors Provoost) 6b4c817 refactor: pass BlockCreateOptions to createNewBlock (Sjors Provoost) 323cfed refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost) Pull request description: When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs. At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually [produced an invalid block](https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426) which exceeded the sigops limit. The existince of such patches suggests it may be useful to make this value configurable. This PR would make such a change easier. However, the main motivation is that in the Stratum v2 spec requires the pool to communicate the maximum bytes they intend to add to the coinbase outputs. Specifically the `CoinbaseOutputDataSize` message which is part of the [Template Distribution Protocol](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---server) has a field `coinbase_output_max_additional_size`. A proposed change to the spec adds the max additional sigops as well: stratum-mining/sv2-spec#86. Whether that change makes it into the spec is not important though, as adding both to `BlockAssembler::Options` makes sense. The first commit is a test refactor followup for #30335, related to the code that's changed here, but not required. The second commit introduces BlockCreateOptions, with just `use_mempool`. The thirds commit adds `coinbase_max_additional_weight` and `coinbase_output_max_additional_sigops` to `BlockCreateOptions`. They use the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432. ACKs for top commit: itornaza: tested ACK c504b69 ryanofsky: Code review ACK c504b69 ismaelsadeeq: Code review ACK c504b69 Tree-SHA512: de2fa085f47048c91d95524e03f909f6f27f175c1fefa3d6106445e7eb5cf5b710eda6ea5b641cf3b4704a4e4e0181a0c829003b9fd35465f2a46167e5d64487
|
@Fi3 how a JDC could know how many sigops has to signal to its TP? Shouldn't it receive them in the |
if it have the outputs it can parse them I'm wondering how it communicate it to the TP btw |
JDS informs JDC about then, JDC informs TP about this PR proposes introducing this means JDC is going to inform TP about @GitGab19 question is: how is JDC informed about to me, the most logical answer simply follows the same pattern as JDS informs JDC about therefore, IMO this PR should also propose adding a new |
That makes sense. The Mining interface in Bitcoin Core already supports passing passing So this spec change won't require a change to the interface, which means there's no rush to get it finalised before the v29 Bitcoin Core release (which hopefully ships this interface). |
|
Related to this, I realized a while ago that |
I'm pretty sure that is setted by TP. That field should contain the output with the segwit commitment (only the TP know it if we don't request txs data) |
Actually it's not (at least from my findings) because they are never communicated to the TP. How could it know them by itself? |
I'm pretty sure that you are wrong. Try to link that a JDC to a TP that is synced on testnet or mainnet and print that field |
|
The TP protocol is designed to be as unidirectional as possible - it doesn't think it just dumps work. It isn't fully unidirectional as it considers the size of coinbase the client wants, but that's the best we can do. Passing the coinbase tx output set from the JDS to the TP just to have the TP echo then back to the client doesn't accomplish anything, IMO, and reduces the unidirectionality goal. It's trivial to just have the TP+JD client figure it out. |
Makes sense, thanks for your answer! |
I don't think it needs to be the last one, just needs to have the right magic bytes. However it may still be a sane default to put it last:
|
|
Yea, the design of Sv2 basically assumes that all things that require commitments (future soft forks and/or merged-mined chains) will do the same (or use the segwit commitment). |
|
I rebased Sjors/bitcoin#45, so you can test it against a modified SRI. |
|
I added a commit that makes the Template Provider backwards compatible, it will just assume max additional sigops is 400 if the field is missing. Suggested deployment strategy:
|
Co-authored-by: plebhash <147345153+plebhash@users.noreply.github.com>
Co-authored-by: plebhash <147345153+plebhash@users.noreply.github.com>
|
Should I go ahead and include Sjors/bitcoin#45 in the next TP release? Probably in the next few weeks. |
|
Sounds good to me 👍 |
|
Done. I'll let you know when the new release is out. Conversely, please let me know which release of SRI will have this field, so I can add a code comment about that. |
|
Will add this as a discussion point of our next dev call and get back to you. |
|
It's fine to just ping me once it happens, no need to plan in advance. |
|
The new release https://github.com/Sjors/bitcoin/releases/tag/sv2-tp-0.1.14 supports the new |
|
Changes introduced on SRI by PR stratum-mining/stratum#1456 ✅ Instructions on our website about version of TP to use --> updated by PR stratum-mining/stratumprotocol.org#246 ✅ |
|
Nice! Just let me know when there is a new SRI release, so I can also make a note in the TP code. |
…ockAssembler::Options 1291cbb refactor: add coinbase constraints to BlockCreateOptions (Sjors Provoost) c9796c9 refactor: pass BlockCreateOptions to createNewBlock (Sjors Provoost) 6217ffa refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost) Pull request description: When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs. At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually [produced an invalid block](https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426) which exceeded the sigops limit. The existince of such patches suggests it may be useful to make this value configurable. This PR would make such a change easier. However, the main motivation is that in the Stratum v2 spec requires the pool to communicate the maximum bytes they intend to add to the coinbase outputs. Specifically the `CoinbaseOutputDataSize` message which is part of the [Template Distribution Protocol](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---server) has a field `coinbase_output_max_additional_size`. A proposed change to the spec adds the max additional sigops as well: stratum-mining/sv2-spec#86. Whether that change makes it into the spec is not important though, as adding both to `BlockAssembler::Options` makes sense. The first commit is a test refactor followup for #30335, related to the code that's changed here, but not required. The second commit introduces BlockCreateOptions, with just `use_mempool`. The thirds commit adds `coinbase_max_additional_weight` and `coinbase_output_max_additional_sigops` to `BlockCreateOptions`. They use the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432. ACKs for top commit: itornaza: tested ACK 1291cbb ryanofsky: Code review ACK 1291cbb ismaelsadeeq: Code review ACK 1291cbb Tree-SHA512: de2fa085f47048c91d95524e03f909f6f27f175c1fefa3d6106445e7eb5cf5b710eda6ea5b641cf3b4704a4e4e0181a0c829003b9fd35465f2a46167e5d64487
…ockAssembler::Options bbf6256 refactor: add coinbase constraints to BlockCreateOptions (Sjors Provoost) 9c183e1 refactor: pass BlockCreateOptions to createNewBlock (Sjors Provoost) b166fc7 refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost) Pull request description: When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs. At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually [produced an invalid block](https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426) which exceeded the sigops limit. The existince of such patches suggests it may be useful to make this value configurable. This PR would make such a change easier. However, the main motivation is that in the Stratum v2 spec requires the pool to communicate the maximum bytes they intend to add to the coinbase outputs. Specifically the `CoinbaseOutputDataSize` message which is part of the [Template Distribution Protocol](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---server) has a field `coinbase_output_max_additional_size`. A proposed change to the spec adds the max additional sigops as well: stratum-mining/sv2-spec#86. Whether that change makes it into the spec is not important though, as adding both to `BlockAssembler::Options` makes sense. The first commit is a test refactor followup for #30335, related to the code that's changed here, but not required. The second commit introduces BlockCreateOptions, with just `use_mempool`. The thirds commit adds `coinbase_max_additional_weight` and `coinbase_output_max_additional_sigops` to `BlockCreateOptions`. They use the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432. ACKs for top commit: itornaza: tested ACK bbf6256 ryanofsky: Code review ACK bbf6256 ismaelsadeeq: Code review ACK bbf6256 Tree-SHA512: de2fa085f47048c91d95524e03f909f6f27f175c1fefa3d6106445e7eb5cf5b710eda6ea5b641cf3b4704a4e4e0181a0c829003b9fd35465f2a46167e5d64487
5 participants
The template provide need a way to know how many sigops the pool want to include in the coinbase outupts, otherwise is not able to create a template that if mined will result in a valid block.