-
Notifications
You must be signed in to change notification settings - Fork 43
clarify CoinbaseOutputConstraints
#163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clarify CoinbaseOutputConstraints
#163
Conversation
fc69c3a to
94a4b26
Compare
Sjors
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to expand this.
a423ea1 to
f7d6ab2
Compare
7d6a9ea to
75ba96b
Compare
07-Template-Distribution-Protocol.md
Outdated
| In summary, the Template Provider MUST reserve: | ||
| - 866 + 4*`coinbase_output_max_additional_size` weight units for SegWit blocks | ||
| - 636 + 4*`coinbase_output_max_additional_size` weight units for legacy blocks | ||
| - `coinbase_output_max_additional_sigops` sigops for either case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still suggest some padding in case the above calculation or the implementation is wrong. It's very likely you won't find out for years because block templates are almost never full to the very last byte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can say that is recommended to add padding of x so that would be impossible to create an invalid block cause we calculated a too little weight? IMO if we quantify x we should say why we choose such number. Or maybe even better recommend to test against core to be sure that do not produce invalid blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a new paragraph along the lines of:
Additionally, the Template Provider SHOULD reserve additional X weight units as a safety margin against unexpected edge cases.
The question is: how much is an ideal value for X?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 400? That's 0.01% of the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommend to test against core
There's no easy way to make Core generate the absolute maximum size template, you'd have to populate the mempool in a specific way and deal with any weirdness in how Bitcoin constructs block from the mempool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little bit less. Bitcoin Core reserves 2000 for coinbase, without knowing what's in it. The calculations here show that there is at least 1168 weight units, plus ~132 for a single taproot payout, so a safety margin of 700. A stratum v1 miner that wants to include lots of outputs has to be more careful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep make sense to me we can add it in the spec and be happy from what concern me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still digesting the numbers provided by @Sjors on his latest comments
I'm having the opportunity for some IRL chats with @ismaelsadeeq about this, I'll report back here soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adapted the PR.
I'm summarizing everything into one simple note:
Please note that Bitcoin Core establishes a floor value of 2000 weight units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I added this floor. The rationale is to prevent the creation of an invalid block template because we’re not sure what to use to finish the computation of the reserved weight, since coinbase_output_max_additional_size is unknown. If coinbase_output_max_additional_size is not calculated correctly, you risk creating an invalid block just to squeeze in a few more transactions.
75ba96b to
7fced64
Compare
|
Relevant to SegWit block and transaction serialization: https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki |
|
See above, just haggling over a few remaining WU, but we're pretty close. |
8ca984e to
d5a1794
Compare
|
ACK d5a1794 Though someone should double check the numbers. |
d5a1794 to
8c2a576
Compare
|
ACK 8c2a576 |
52e3b0d to
67d5807
Compare
67d5807 to
6bca4aa
Compare
recently @Fi3 created stratum-mining/sv2-tp#47 with some questions about how to interpret
CoinbaseOutputConstraintsthe lengthy discussions that unfolded there made it clear that the current definition of the message is not clearly understandable
this PR aims to clarify it, hopefully leaving no room for ambiguity or confusion
for a better visualization of the tables, reviewers can refer to this rendering on my fork