Skip to content

BIP 54 test vectors improvements following review in Inquisition #2122

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.

Sign up for GitHub

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

Open
darosior wants to merge 7 commits into
base: master
Choose a base branch
from
+4,276 −24,364
Open

BIP 54 test vectors improvements following review in Inquisition #2122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aef4d9e
bip54: reword "potentially executed" language in sigops limit specifi…
darosior Mar 13, 2026
1f21139
bip54: expand on rationale for non-final coinbase nSequence
darosior Mar 13, 2026
ff39733
bip54: update sigops test vectors following Inquisition review
darosior Mar 13, 2026
78126a5
bip54: move comment as first element in txsize test vectors
darosior Mar 13, 2026
4f94c5f
bip54: make test vectors POSIX-compliant (newline at EOF)
darosior Mar 13, 2026
977300d
bip54: restructure timestamp test vectors into a tree
darosior Mar 13, 2026
fa60898
bip54: add another transaction size test case
darosior Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions bip-0054.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ Given a block at height `N`:
or equal to the value of the timestamp of the block at height `N-2015` (T<sub>N</sub> &ge;
T<sub>N−2015</sub>).

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
Copy link

@ariard ariard Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

each input in the transaction, count the number of `CHECKSIG` and `CHECKMULTISIG` in the input
scriptSig and previous output's scriptPubKey, including the P2SH redeemScript. If the total summed
Expand Down Expand Up @@ -123,7 +123,10 @@ validation will need to be re-activated from block 1,983,702. A simple way to pr
mandate that future coinbase transactions vary from coinbase transactions before [bip-0034][BIP34]
activation. There are multiple ways of achieving this, but setting and enforcing the timelock for
the coinbase transaction makes it so all coinbase transactions past Consensus Cleanup activation
could not have been valid before this height and therefore cannot be a duplicate[^11].
could not have been valid before this height and therefore cannot be a duplicate[^11]. This
simplifies both reasoning and client implementation, since the [bip-0030][BIP30] check can be
skipped entirely past Consensus Cleanup activation, regardless of the [bip-0034][BIP34] activation
status[^12].

## Backward compatibility

Expand Down Expand Up @@ -218,6 +221,10 @@ bip-0034 height commitment and the corresponding future block height.
coinbase transactions as not having duplicate past Consensus Cleanup activation would be consistent
for any implementation which enforces `nLockTime` from the genesis block, which is the behaviour
notably of Bitcoin Core but also of all other implementations the authors are aware of.
[^12]: For instance Bitcoin Core only disables [bip-0030][BIP30] validation for a specific chain
Copy link

@ariard ariard Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the footnote can be improved here. something something like, “as a matter of performance optimizations, Bitcoin Core since $HASH_COMMIT_OR_RELEASE has only been enforcing BIP34, which for guaranteeing uniqueness of txid is a superset of BIP30 due to the mandatory inclusion of the height in the coinbase transaction’s scriptSig.. Without enforcing the setting of the nLocktime field to be equal to the block height for the coinbase transaction, this would have to be perpetuated after the Consensus Cleanup activation”.

Copy link
Member Author

@darosior darosior Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without enforcing the setting of the nLocktime field to be equal to the block height for the coinbase transaction, this would have to be perpetuated after the Consensus Cleanup activation”.

That would be inaccurate. You need the full timelocking of the coinbase transaction (nLockTime + nSequence), or you would need to do the same thing for Consensus Cleanup as was done for BIP 34.

where [bip-0034][BIP34] violations have been manually inspected (see [here][Core validation.cpp
BIP34]). Without the guarantee given by enforcing the timelock on coinbase transactions, this would
have to be perpetuated for the Consensus Cleanup.
Copy link

@ariard ariard Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above, but i think it should say “after the Consensus Cleanup activation”, the usage of the for do not denotates any temporal referential.

Copy link
Member Author

@darosior darosior Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my response above, the same thing would have to be done for the Consensus Cleanup #2122 (comment). Using for in this sentence is therefore appropriate, as far as i can tell.


[BIP30]: https://github.com/bitcoin/bips/blob/master/bip-0030.mediawiki
[BIP-XXXX]: https://github.com/TheBlueMatt/bips/blob/7f9670b643b7c943a0cc6d2197d3eabe661050c2/bip-XXXX.mediawiki
Expand All @@ -242,3 +249,4 @@ notably of Bitcoin Core but also of all other implementations the authors are aw
[Core 29.0]: https://bitcoincore.org/en/releases/29.0
[inquisition-implem]: https://github.com/darosior/bitcoin/tree/2509_inquisition_consensus_cleanup
[Core 30.0]: https://bitcoincore.org/en/releases/30.0
[Core validation.cpp BIP34]: https://github.com/bitcoin/bitcoin/blob/390e7d61bd531505bb3d13f38316c282b85ed1dd/src/validation.cpp#L2401-L2459
3 changes: 2 additions & 1 deletion bip-0054/test_vectors/coinbases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,5 @@
"valid": false,
"comment": "Block at height 4 with coinbase's nLockTime set to block's nTime minus 1 and maximum non-final nSequence."
}
]
]

Loading