-
Notifications
You must be signed in to change notification settings - Fork 5.6k
BIP 53: Disallow 64-byte transactions #1760
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.
I am a little surprised at this pull request, given the frequent coverage of Antoine’s ongoing research on reviving the Great Consensus Cleanup that includes a similar fix.
It seems better to me to package all four fixes in one soft fork. Was it overlooked that this work is being duplicated, or are you disagreeing with the direction of Antoine’s work and want to put forward an alternative? Could you elaborate why you decided to submit this?
HI Murch!
While the GCC is a great name for a disjoint set of bitcoin protocol security fixes, that doesn't mean they all need to be bundled into one BIP and deployed at the same time.
This document takes no opinion on how this enhancement is deployed. Previously we have deployed multiple consensus changes in a single soft fork that contained multiple BIPs. The 2017 Segwit soft fork had BIP141, BIP143. The Taproot soft fork had BIP341 and BIP342.
I was recommended that this was a piece of work that was easily to peel off from the rest of GCC and propose a fix for. Its relatively simple and straight forward, and has been known about for a long time.
I think Antoine's research is great, but just because he is researching the topic doesn't mean he gets to monopolize the vulnerabilities. The 64 byte transaction vulnerability has been known since at least 2012 - yet no one has taken the time to write up a BIP to fix it 😬 . I've put forth the first BIP draft in the 13 years since this vulnerability has been known. I encourage Antoine to write up his proposal (hopefully sooner rather than later) as he doesn't seem to be interested in working with me. Other vulnerabilities in GCC have also been known about for a long time - such as worse case block validation time. Antoine has done a lot of novel research there and I think his work product has been excellent on delving. There is plenty of meat left on the GCC bone, this was a small piece I decided to carve off. |
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 does seem like a reasonable start for a BIP. A few parts of the content could perhaps be organized a bit differently, and some sections could be expanded. Especially the Specification could be a bit more precise in what an implementation should do to be compliance with this BIP. I left you a few editorial nits that you could perhaps take a look at.
It seems likely to me that this submission may be duplicating some effort on all of our ends, assuming that the other anticipated BIP gets submitted near-term, but fair enough, we have not received another BIP on this topic. I guess I can’t fault multiple people for being interested in the same topic.
bip-XXXX.mediawiki
Outdated
construct the funding transaction whose coins will be spent by this one) is able | ||
to fool an SPV client in this way.]</ref> of work. This also reduces implementation complexity of SPV wallets<ref>[https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710/43 The steps needed to make sure a merkle proof for a transaction is secure.]</ref>. | ||
|
||
This could be mitigated by knowing the depth of the merkle tree. Requiring SPV clients to request both the coinbase transaction could mitigate this attack. |
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.
The area around "both the coinbase transaction" seems to be missing something
bip-XXXX.mediawiki
Outdated
|
||
==Backward compatibility== | ||
|
||
There have been 5 64 byte transactions that have occcurred in the bitcoin blockchain as of this |
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.
Could you expand on how you are suggesting that implementers should mitigate this incompatibility? What about the potential existence of a pre-signed transaction that serializes to 64 bytes?
bip-XXXX.mediawiki
Outdated
|
||
This BIP disallows bitcoin transactions that are serialized to 64 bytes in length without it's witness. | ||
|
||
==Rationale== |
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 lot of the content of this section would maybe fit better under Motivation. Rationale usually explores the design decisions, alternate designs, and related work. For example you could expand on the approach of tracking the height of the Merkle branch to the coinbase transaction in the Rationale.
66b353a
to
87ecfc4
Compare
87ecfc4
to
9f670fa
Compare
I believe the CI failure is unrelated to 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.
Looks quite good. A few minor suggestions follow. Suggest rebasing to current master for a green CI if you repush.
bip-XXXX.mediawiki
Outdated
/** | ||
* We want to enforce certain rules (specifically the 64-byte transaction check) | ||
* before we call CheckBlock to check the merkle root. This allows us to enforce | ||
* malleability checks which may interact with other CheckBlock checks. |
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.
* malleability checks which may interact with other CheckBlock checks. | |
* malleability checks that may interact with other CheckBlock checks. |
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 going to avoid changing these nits unless they change in the reference implementation unless you have a super strong opinion on this.
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 see. It wasn't clear to me that this is extracted from the open pull in bitcoin-inquisition.
bip-XXXX.mediawiki
Outdated
* malleability checks which may interact with other CheckBlock checks. | ||
* This is currently called both in AcceptBlock prior to writing the block to | ||
* disk and in ConnectBlock. | ||
* Note that as this is called before merkle-tree checks so must never return a |
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.
* Note that as this is called before merkle-tree checks so must never return a | |
* Note that this is called before merkle-tree checks and so must never return a |
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.
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Co-authored-by: Jon Atack <[email protected]>
Co-authored-by: Jon Atack <[email protected]>
Co-authored-by: Jon Atack <[email protected]>
Co-authored-by: Jon Atack <[email protected]>
Done in 23e01bf |
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.
The document seems fairly complete content-wise, but the presentation could use more attention. Capitalization is a bit random: some acronyms are capitalized ("SPV"), but others are uncapitalized (e.g. "utxos") and proper nouns are sometimes capitalized (Merkle, Bitcoin Core), sometimes not (merkle, bitcoin). Please either retitle one of the two Rationale sections or combine them.
It would be good if the proposal got more commentary or review from other contributors, but otherwise seems close to ready for a merge.
Please also add the table entry to the README. |
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 assume you are still working on my other suggestions, so I’ll take another look later.
Yes I am. Child nap time only allows for so many things to get done at 1
time 😅. I’ll try to address the rest later today
…On Tue, Apr 22, 2025 at 12:12 PM murchandamus ***@***.***> wrote:
***@***.**** commented on this pull request.
I assume you are still working on my other suggestions, so I’ll take
another look later.
------------------------------
In bip-0053.mediawiki
<#1760 (comment)>:
> @@ -6,7 +6,7 @@
Comments-URI: https://github.com/bitcoin/bips/wiki/Comments:BIP-0053
Status: Draft
Type: Standards Track
- Created: ?
+ Created: 2024-12-20
According to BIP2
<https://github.com/bitcoin/bips/blob/master/bip-0002.mediawiki#bip-header-preamble>,
the "Created" header records the date a proposal was assigned a number,
which is why I suggested 2025-04-11.
image.png (view on web)
<https://github.com/user-attachments/assets/5ac62de5-00b6-455e-940f-f22c88d228e1>
—
Reply to this email directly, view it on GitHub
<#1760 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA22ETNRE5UYZ32HHFCPFTD22ZZ6DAVCNFSM6AAAAABW3SF3JSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOOBUG43TEOJWG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Ok think this is addressed in f797b9f, bd815e9, acda401
Apologies, I see what you are talking about now and this is fixed in 5a89036. Also I missed some code review due to github.com collapsing some comments, hopefully everything is addressed now with 5a89036 |
Yeah, I think I see what happened: when you started by moving the file, it marked all of my review comments as outdated, because they had been left on the old file. ;) |
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.
Reviewed the changes, looks much better, thanks. :)
What’s your plan for this proposal? Do you see it as a standalone soft fork that can be merged as is on its own, or are you working on additional parts? What is the intended deployment strategy?
This is possible if the community wants to do this.
I am not at this time.
I don't have one at this time. Recent precedent with BIP148 and BIP343 details activating the last 2 soft forks that contained a variety of BIPs. These activation BIPs were written by a different set of people than the BIP authors for taproot and segwit. I assume something like this will have to happen again as it seems like there isn't really another realistic path to deploying soft forks IMO. |
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.
Okay, is there anything else that is missing from your perspective before this can be merged?
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
I thought about this a bit more, there are 2 things that could potentially be addressed. I would like to hear your opinion. How BIP3 defines the
One concern that was raised on the mailing list by Jeremy Rubin and agreed by Eric Voskuil was that disallowing 64 byte transaction
Do you think this is adequately addressed by this section of the BIP? I'm on the fence. One other concern shared on the delving thread by Antoine Poinsot was sidechains may be vulnerable to the same merkle tree weakeness that SPV clients are. I haven't investigated this further, but it could potentially merit a section the This currently isn't at the top of my bitcoin research priorities, so if you think these things should be done I'll address them next week. |
This BIP makes 64 byte bitcoin transactions serialized without the witness consensus invalid
Mailing list post: https://groups.google.com/g/bitcoindev/c/rf3QOlzg230/m/eOOC8pkOAgAJ
Delving discussion: https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710
Side note, wasn't sure how to best handle the 2-Bitcoin-Merkle.pdf as it isn't hosted anywhere easily accessible AFAICT? For now I just added it into the git repo. Lmk if you have other suggestions