BIP 54 test vectors improvements following review in Inquisition#2122
BIP 54 test vectors improvements following review in Inquisition#2122darosior wants to merge 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/[email protected]/ 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
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.
| 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 |
There was a problem hiding this comment.
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”.
There was a problem hiding this comment.
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.
| [^12]: For instance Bitcoin Core only disables [bip-0030][BIP30] validation for a specific chain | ||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
3 participants
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.