-
Notifications
You must be signed in to change notification settings - Fork 5.6k
BIP 54: Consensus Cleanup #1800
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
base: master
Are you sure you want to change the base?
Conversation
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.
First pass of review, mostly copy editing suggestions so far.
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.
Concept ACK on the timewarp and Murch-Zawy fix, as well as the 64 byte transaction ban.
I still need to think about the worst case validation fix more, though the approach seems reasonable.
With respect to the nLockTime
change, I need to get a sense of what pool software should take into account, especially since Bitoin Core does not construct the coinbase transaction.
We don't strictly need this change until height 1,983,702, but it seems better to enforce it at the same time as the rest of these changes. Otherwise we might end up with accidental forks several decades from now with nobody remembering this BIP.
I also still need to see and study the implementation(s), outside the timewarp fix, which I'm fairly familiar with by now.
There might be a problem with the math in this BIP. Exposing said problem clearly might be a $5k a day wreck the whole LN style of exploit. Asking for a friend who is curious on how to handle said problem. |
@ariard i made sure you are on the private thread, if you want to discuss potentially sensitive details please do so there. Alternatively feel free to contact me privately. |
e3a557f
to
404010f
Compare
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.
Just a quick first look and some editorial comments as I see that this is still being edited and already getting quite a bit of feedback.
8d16f01
to
625937e
Compare
Thanks everyone for the review thus far. I think i've addressed everything. |
625937e
to
21a9cd5
Compare
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.
This proposal looks mostly complete from an editorial standpoint. In a few segments, the style could be even more prosaic—the document could be a bit clearer by avoiding passive voice and directly referencing subjects and objects of sentences than referencing via noun phrases and pronouns. Some abstract ideas are referenced with different terms. It would be preferable if the same term is used for the same concept throughout.
bip-cc.md
Outdated
|
||
Transactions whose witness-stripped serialized size is exactly 64 bytes are made invalid. | ||
|
||
The coinbase transaction's `nLockTime` field must be set to the height of the block minus 1[^2] |
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.
nLockTime is the most ideal location to put an extranonce, so it seems like a bad idea to burn it for no good reason.
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.
@luke-jr why would miners switch to something different from the current extraNonce?
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.
It would have been better to bring up conceptual feedback on the mailing list. "For no good reason" is unnecessarily aggressive, the point here is not to make things invalid just for the sake of it. Please engage charitably to foster productive discussions.
To your point, it's my understanding that nLockTime
makes for a good extranonce because it's serialized at the very end of the coinbase transaction. Rolling it does not require re-hashing the whole transaction as you can just cache a midstate for the first 64 bytes. Could you provide more details about how you expect it to be used exactly, and under what circumstances the operation it improves may become a bottleneck?
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.
@luke-jr why would miners switch to something different from the current extraNonce?
There is no "current extraNonce". Extranonces can be anywhere in theory. But this particular location, as @darosior says, minimises the need to re-hash a lot of data to roll it. That means it can more practically be done on ASICs or MCUs with nearly no latency.
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.
Can you do the same with an output? Since a coinbase doesn't have any inputs to sign (which commit to outputs), rolling an OP_RETURN
output should be just as easy and there's no space limit?
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.
@luke-jr i see how it might be useful in theory but i don't think it would be in realistic conditions. Could you describe (ideally on the ML or Delving thread) the specifics of a future ASIC design where it would matter?
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 moving this discussion to the Delving thread. Please @luke-jr provide more details there so your objection can be addressed.
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 don't use Delving
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.
Then can you please share details about your objection somewhere? Anywhere. Here, the dev mailing list, not Delving, 0bin, whatever.
Just sharing vague concerns without details makes it really hard to address your objection. And i trust it is not your intention to prevent having an objective discussion about the tradeoff being made here.
21a9cd5
to
797984c
Compare
The friend said the potential problem is not related on the timewarp fix. |
Hi @ariard, if you don’t intend to contribute constructively, could you please refrain from posting in this repository? |
Let’s call this BIP 54. Please update the table entry preamble and title correspondingly. |
@ariard I've removed the off-topic comment. Better to air those kinds of issues in a personal blog post. Let's remain focused here on technical review, please. |
For what it's worth i have already addressed the suggestion to split this BIP into 4 twice on the mailing list. Last occurrence is here. Unless compelling arguments in favour of doing so are given i do not plan on addressing it a third time. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment has been minimized.
This comment has been minimized.
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 did another full review of this proposal. Most of my comments are nits, please feel free to take or leave those as you choose.
It is my impression that iterating over the four separate issues several times in each section makes it harder to put together the full picture for the proposal. It might be easier to read this proposal if the parts of each fix were grouped more closely.
bip-0054.md
Outdated
- if `N % 2016` is equal to 0, the timestamp of the block must be set to a value higher than or | ||
equal to the value of the timestamp of block at height `N-1` minus 7200; |
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.
Written out this just sounds a bit ambiguous to me. Perhaps add a formula for a precise statement:
- if `N % 2016` is equal to 0, the timestamp of the block must be set to a value higher than or | |
equal to the value of the timestamp of block at height `N-1` minus 7200; | |
- if `N % 2016` is equal to 0, the timestamp of the block must be set to a value higher than or | |
equal to the value of the timestamp of block at height `N-1` minus 7200; | |
i.e., `t(N) ≥ t(N−1) − 7200` |
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.
(here and below)
I think that both writing it out and writing it in pseudocode is unnecessarily heavy. I would choose the written out version over pseudocode since a BIP is a text document. Therefore i will keep the existing version.
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.
Mildly disagree. BIPs are technical documentation, not prose. The main purpose is to convey complex information accurately and concisely. I suggested adding the inequation, because I felt that it presents the information more accessibly than the sentence. It’s not a blocker for me, though.
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.
Agree with @murchandamus.
bip-0054.md
Outdated
- if `N % 2016` is equal to 0, the timestamp of the block must be set to a value higher than or | ||
equal to the value of the timestamp of block at height `N-1` minus 7200; | ||
- if `N % 2016` is equal to 2015, the timestamp of the block must be set to a value higher than | ||
or equal to the value of the timestamp of the block at height `N-2015`. |
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.
or equal to the value of the timestamp of the block at height `N-2015`. | |
or equal to the value of the timestamp of the block at height `N-2015`; | |
i.e., `t(N) ≥ t(N−2015)` |
b1a59ca
to
f0ca398
Compare
I took another look. I am obviously biased but to me it seems pretty clear and easy to follow. It is a short, concise, document which discusses 4 small fixes to consensus rules. In every section each small issue is discussed in its own paragraph. The ordering is conserved across sections. Suggestions welcome of course, but at this point i am not interested in making much further changes. |
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.
It is my impression that iterating over the four separate issues several times in each section makes it harder to put together the full picture for the proposal. It might be easier to read this proposal if the parts of each fix were grouped more closely.
Agree with @murchandamus here.
I'd prefer to see separate, focused BIPs, as recommended in both BIP 2 and BIP 3, as that would be much clearer. Failing which, then clearer separation in this draft but -0.5 to -0.9 on that.
Reference code would be better as well, as highlighted by @murchandamus in https://github.com/bitcoin/bips/pull/1800/files#r2056793498 and related.
bip-0054.md
Outdated
|
||
## Acknowledgements | ||
|
||
This document builds upon an [earlier proposal][BIP-XXXX] by one of the authors. |
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.
It seems odd not to name the author here.
This document builds upon an [earlier proposal][BIP-XXXX] by one of the authors. | |
This document builds upon an [earlier proposal][BIP-XXXX] by Matt Corallo. |
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 curious odd as in reading odd, or as in out of place like lacking attribution? Because i've seen this being used before and that's why i used it here.
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.
(Took your suggestion either way.)
bip-0054.md
Outdated
- if `N % 2016` is equal to 0, the timestamp of the block must be set to a value higher than or | ||
equal to the value of the timestamp of block at height `N-1` minus 7200; |
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.
Agree with @murchandamus.
It would be useful to mention the implication of reaching block IIUC (if we don't deal with this along with the 2106 problem) it just means that in the far future coinbase transactions are timelocked to 1985. This is fine, they're still spendable and they're still unique since we require the exact height. At this point there's almost 200 comments on this PR, making it practically impossible for a reviewer to see if all feedback has been addressed. And then every time someone comments on a "resolved" thread, you have to scroll all the way up and unfold them all to find. That slightly increases my preference for having a separate BIP for each change and one to combine them (as a fresh PR). |
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.
Hey, I feel that I should ensure that I made myself clear. This proposal fulfills the editorial criteria, and I’m happy to merge it as is. Thank you for working on this project.
To be precise, my suggestion was to keep it as one document, but to consider swapping the sections and subsection ordering, i.e.:
Current:
Motivation
- Timewarp
- Block validation time
- Merkle tree weakness
- Coinbase duplication
Specification
- Timewarp
- Block validation time
- Merkle tree weakness
- Coinbase duplication
Rationale
- Timewarp
- Block validation time
- Merkle tree weakness
- Coinbase duplication
Miner forward compatibility
- Timewarp
- Block validation time
- Merkle tree weakness
- Coinbase duplication
Alternative:
Timewarp
- Motivation
- Specification
- Rationale
- Miner forward compatibility
Block validation time
- Motivation
- Specification
- Rationale
- Miner forward compatibility
Merkle tree weakness
- Motivation
- Specification
- Rationale
- Miner forward compatibility
Coinbase duplication
- Motivation
- Specification
- Rationale
- Miner forward compatibility
It seems to me that this has a similar benefit as what people may be trying to achieve when they suggest splitting it into several BIPs. Given that multiple reviewers made similar suggestions, I figured it might be worth seeing what the document with the alternative order would look like instead of just speculating about it.
I reordered the content of this document (with minimal other changes to capture the sentences that applied to all fixes) in murchandamus@6f2b322. Please see the preview of the document with the alternative ordering, and the preview of the current document for comparison.
I prefer the alternative order as it seems more natural to me. I believe that it presents the information in a more accessible manner as the topics are grouped rather than the topic switching with every paragraph. I think it would make the document easier to read and to reference. I don’t think it is necessary to split it to four documents, but wouldn’t be opposed to that either.
To reiterate, this document already fulfills the editorial requirements, so I’m happy to merge it as is if that’s your preference whenever you have had a chance to address existing review.
Murch's structure makes sense too. Perhaps another way to deal with the hard-to-review-ness of the current PR is to merge it at some stage and then continue editing via issues and pull requests? Those could be grouped in a Project. |
Thank you @murchandamus for the clarification and for putting together your suggested alternative structure. Unfortunately i'm not going to take it, in my opinion having 4 sections with each a motivation-specs-rationale is confusing and makes it unnecessarily longer. |
f0ca398
to
8ac9f53
Compare
8ac9f53
to
1ee4351
Compare
Good point. I could have something akin to bitcoin/bitcoin#32155 (comment) in the rationale? But i don't think this should hold up this PR. |
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.
Alright. LGTM, then.
I think it's fine to merge the current version as a Draft and then have others edit it further. I'm happy to review pull requests that do so, maybe add me as a Deputy? |
This is a BIP draft for a Consensus Cleanup soft fork.
This proposal builds upon Matt Corallo's 2019 proposal, which i set to revive at the end of 2023. I eventually shared my research in a Delving Bitcoin post in early 2024 (along with a semi-private companion post for the redacted sensitive details). A number of people contributed ideas, testing and otherwise fruitful discussion. This led to settling on a list of mitigations, which i updated the mailing list about in February 2025.
I think it's now ready to be officially published as a BIP.