test: test OP_CSV empty stack fail in feature_csv_activation.py #17921
Conversation
instagibbs
left a comment
There was a problem hiding this comment.
Your changes look ok I guess, but the magic numbers in mining are throwing me off.
I think in general this test needs some loving, but I think if the magic mining numbers are explained and we check for specific reject reasons this is an improvement
|
@instagibbs: Thanks for reviewing! Before I get into action: should I really put in all those suggested changes (magic numbers elimination/explanation, reject reason checking, adding empty stack test case) in one single PR? |
|
Minimally I want to know that it's being rejected for the right reasons, not "oops the script is wrong in a different way". Secondary is the magic numbers floating around, but that can be held off for now(though I'd really appreciate it, magic numbers in tests make them very brittle and hard to understand). |
e8f1227 to
3330bce
Compare
jimmysong
left a comment
There was a problem hiding this comment.
Would like some documentation on some of the magic numbers. Otherwise, looks good.
|
@jimmysong: Thanks for reviewing! Feels funny to have ones code reviewed by the author of a book that I am just reading right now 😂 |
|
glad you're reading it! |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
| Needs rebase |
With BIP112 activated, the operation OP_CHECKSEQUENCEVERIFY (former OP_NOP3)
leads to script interpreter termination with an error if one of the following
conditions is true:
-> stack is empty
-> top item on stack is negative (< 0)
-> top item on stack has disable flag unset and at least one of
four other conditions is true (contains the core CSV logic)
This commits adds the missing empty stack failure test to the functional test
by prepending a valid scriptSig with just OP_CHECKSEQUENCEVERIFY. If BIP112 is
inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and
the transaction remains valid -- if it is active, the tx is invalid due to an
empty stack (for both tx versions 1 and 2, as well).
93f5ee0 to
5ffaf88
Compare
|
Rebased. |
…sv_activation.py 54be4e7 test: check specific reject reasons in feature_csv_activation.py (Sebastian Falbesoner) Pull request description: This is kind of a prequel to bitcoin#17921: increases the general quality of the functional test `feature_csv_activation.py` by checking for the specific reject reasons whenever the sending of a block fails. To get the reason, we have to limit the script threads to 1 via the parameter `-par=1`, like it is also done in `feature_cltv.py`: https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/test/functional/feature_cltv.py#L57-L61 The commit also fixes a bug that was uncovered with this checks: for the BIP112 version 1 tx tests, txs from `bip112txs_vary_OP_CSV_v1` have been add twice to the list `failed_txs`: https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/test/functional/feature_csv_activation.py#L396-L397 leading also to a block rejection as expected but for the wrong reason. It seems one of those two tx lists was meant to be `bip112txs_vary_OP_CSV_v1` (without the `_9`) and it was a typo. ACKs for top commit: MarcoFalke: ACK 54be4e7 📶 Tree-SHA512: 9aac11aee3f53f1ae95ddb346a2f268872038f4d118c8dcf81b8201dee869774c9f3c3f1c326e370b8fd4eaf8e0673371689a96d9b1cb91be4286c88824725c3
These changes in the test documentation reflect the changes introduced in bitcoin#17921
9053b88 update docstring in feature_csv_activation.py (Pierre K) Pull request description: These changes in the test documentation reflect the changes introduced in #17921. ACKs for top commit: MarcoFalke: review ACK 9053b88 Tree-SHA512: 17fb954baded8dab1c869dd48b76b516150bae616c792c573e4114d4adfdd40195745c56570aa3050cc0015ee496acd7ec178df8ba14831dd22f9722fda84da2
…tion.py 9053b88 update docstring in feature_csv_activation.py (Pierre K) Pull request description: These changes in the test documentation reflect the changes introduced in bitcoin#17921. ACKs for top commit: MarcoFalke: review ACK 9053b88 Tree-SHA512: 17fb954baded8dab1c869dd48b76b516150bae616c792c573e4114d4adfdd40195745c56570aa3050cc0015ee496acd7ec178df8ba14831dd22f9722fda84da2
These changes in the test documentation reflect the changes introduced in bitcoin#17921
Adds an empty stack failure check for OP_CSV (BIP112) to the functional test
feature_csv_activation.pyby prepending a valid scriptSig withOP_CHECKSEQUENCEVERIFY.If BIP112 is inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and the transaction remains valid -- if it is active, the tx is invalid due to an empty stack (for both tx versions 1 and 2, as well).