BIP 54 test vectors improvements following review in Inquisition#2122
Merged
murchandamus merged 7 commits intobitcoin:masterfrom Apr 15, 2026
Merged
BIP 54 test vectors improvements following review in Inquisition#2122murchandamus merged 7 commits intobitcoin:masterfrom
murchandamus merged 7 commits intobitcoin:masterfrom
Conversation
…cation The paragraph in its entirety is already unambiguous that all signature-checking operations *present* in the Script (as opposed to *executed*) are counted. However i received feedback that the "potentially executed" language in the first sentence of this paragraph may be confusing. This is because it is in theory possible to have a more accurate upper bound by analyzing the possible spending paths and use the maximum number of signature-checking operations in either to check against the limit. This commit rewords the first sentence to use the word "present" to be extra-clear before even describing how the accounting is performed in later sentences.
Making sure that the coinbase is timelocked is a neat simplification in reasoning about duplicates, but it has caused some confusions. For instance here: https://gnusha.org/pi/bitcoindev/e758af9b-72fc-4fdd-8e07-e1126635780an@googlegroups.com/ Fix this by explicitly stating the implications.
This is a batch update to the tests vectors for the limit on legacy signature-checking operations introduced in BIP 54, following feedack received on the Bitcoin Inquisition PR and from Chris Stewart's implementation in Bitcoin-S.
This is part of feedback received during the Bitcoin Inquisition PR review.
This was pointed out during the Bitcoin Inquisition PR review
murchandamus
added
the
BIP Update by Owner
ariard
reviewed
Mar 16, 2026
ariard
left a comment
ariard
left a comment
There was a problem hiding this comment.
Ongoing review, up to 1f21139
Seen the commit message, I’m still thinking the change can be simplified by not enforcing non-finality of the nSequence field due to the order of checks in IsFinalTx(), while still ensuring uniqueness. Fair game, on me to go to write test vectors pre / post bip54 to verify the consensus logic and make the demonstration.
|
|
||
| A limit is set on the number of potentially executed signature operations in validating a | ||
| A limit is set on the number of signature operations present in the scripts used to validate a | ||
| transaction. It applies to all transactions in the block except the coinbase transaction[^1]. For |
There was a problem hiding this comment.
minor: find the commit message being clear enough that it would be actually valuable to mention that the sum (or union ?) of all the non-executed spending paths is the value on which the limit is applied.
Member
Author
There was a problem hiding this comment.
The following sentence describes exactly this, so i think that is enough for now.
The test vectors were initially designed to maximally simple, which led to much redundancy. That was probably too close to one extreme on the spectrum between simplicity and efficiency. Here we shave off 20k lines by simply representing the header chains as a tree instead of list of lists by duplicating all common headers.
Ariard mentioned he would like to see a test case for a 64-byte transaction spending a Taproot output with an annex. I took the opportunity to also make the output be an OP_RETURN with a 2-byte push, as another semi-realistic transaction.
darosior
force-pushed
the
2603_bip54_improvements
branch
from
bd71c54 to
fa60898
Compare
Member
|
@darosior, is this ready to be merged? |
Member
Author
|
@murchandamus yes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This addresses the feedback received on the test vectors following the merge of #2015, the review of the Bitcoin Inquisition implementation (bitcoin-inquisition/bitcoin#99), and Chris Stewart's implementation for Bitcoin-s (bitcoin-s/bitcoin-s#6170).
There are 4 categories of changes here:
See commit messages for details.