test: bug fix in transaction_tests #21280
Merged
Conversation
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
Member
Author
|
@MarcoFalke thanks for catching the bug - this should fix it |
maflcko
reviewed
Feb 23, 2021
glozow
force-pushed
the
2021-02-test-bug
branch
from
1fde7a4 to
0f6d0ff
Compare
Member
Author
|
Cirrus failure is in feature_blockfilterindex_prune.py, which I'm pretty sure is unrelated, given that this is only touching the unit tests |
maflcko
reviewed
Mar 30, 2021
glozow
force-pushed
the
2021-02-test-bug
branch
from
c0295c4 to
f5e150a
Compare
This exact test is also already in tx_invalid.json#L29 It will also test with no-P2SH flags, so duplicating with different flags is not necessary.
glozow
force-pushed
the
2021-02-test-bug
branch
from
f5e150a to
a944a21
Compare
jnewbery
reviewed
Apr 1, 2021
Contributor
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
utACK a944a21be41d1e5469822b1858c304c983010880
maflcko
reviewed
Apr 1, 2021
glozow
force-pushed
the
2021-02-test-bug
branch
2 times, most recently
from
f61876c to
24f2951
Compare
Member
Author
|
Updates: |
jnewbery
reviewed
Apr 7, 2021
Contributor
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
ACK 24f2951e11d0aa88e3b45271ec457abaadc6fde0
A few small style suggestions inline if you retouch this.
PR bitcoin#19168 introduced this function but it always returns an empty vector.
Add missing script verify flags to mapFlagNames. iterate through mapFlagNames values instead of bits. BOOST_CHECK_MESSAGE better reports which test failed exactly, whereas BOOST_ERROR was just incrementing the error counter.
Similar to 19db590, which removed these for the valid tests. Not removing ones that cause a false/empty stack error because these tests should fail due to being invalid with CSV/CLTV
There is no way to iterate through all script verification flags, and it's not guaranteed that every power of 2 is used. Just make sure that all flags in STANDARD_SCRIPT_VERIFY_FLAGS are present in mapFlagNames; this covers all consensus and policy flags. If mapFlagNames has more flags than STANDARD_SCRIPT_VERIFY_FLAGS, that's okay. Nonexistent flags will be caught by the compiler.
Member
Author
|
Addressed the compiler warning about unused variable and @jnewbery's comments - thanks! |
Contributor
|
utACK b109bde |
maflcko
changed the title
maflcko
reviewed
Apr 19, 2021
| [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000001 CHECKLOCKTIMEVERIFY"]], | ||
| "01000000010001000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000065cd1d", "CHECKLOCKTIMEVERIFY"], | ||
| [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967295 CHECKLOCKTIMEVERIFY 1"]], | ||
| [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967295 CHECKLOCKTIMEVERIFY"]], |
Member
There was a problem hiding this comment.
It might be nice to check the exact script reject reason instead of just failure
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 19, 2021
b109bde [test] check that mapFlagNames is up to date (glozow) 5d3ced7 [test] remove unnecessary OP_1s from invalid tests (glozow) 5aee73d [test] minor improvements / followups (glozow) 8a365df [test] fix bug in ExcludeIndividualFlags (glozow) 8cac292 [test] remove invalid test from tx_valid.json (glozow) Pull request description: This is a followup to bitcoin#19698. - There was a bug in the `ExcludeIndividualFlags` function which is fixed here. - Fixing this bug also showed that there is a test that's supposed to fail (already existing in tx_invalid.json) in tx_valid.json, so I removed it. Other than that, the tests should all pass. - Also implements a few suggestions I received offline: removing the `OP_1`s from the invalid tests (similar to bitcoin@19db590), comments, and style. - A few other small fixes, like adding asserts, putting all the flags in `mapFlagNames`, better error messages ACKs for top commit: jnewbery: utACK b109bde Tree-SHA512: 7233a8c0f1ae1172fac8000ea6e05384ecf79074c39948d118464868505c7f02f17e96503c81bd05c07adb2087648a5d93d9899e16fdefa6b7efcb51319444a9
gwillen
pushed a commit
to ElementsProject/elements
that referenced
this pull request
Jun 1, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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 is a followup to #19698.
ExcludeIndividualFlagsfunction which is fixed here.OP_1s from the invalid tests (similar to 19db590), comments, and style.mapFlagNames, better error messages