[TESTS] Update Transaction Tests to permit setting a flag as always on and disabling the exhaustive failure test #22876
Conversation
|
This particular test failure is because If the test passes when adding another script verification flag, it probably means that it was unnecessary to exclude this flag, the test should be updated to be more specific, or this flag isn't a soft fork. |
|
After patch #22871 then CSV will also have a DISCOURAGE_UPGRADABLE_NOP path. DISCOURAGE_UPGRADABLE can only ever be a soft fork since it's a non-consensus flag. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
@MarcoFalke is their an automated tool to run to fix this PR? |
|
@JeremyRubin Personally I use three-way-diff, which allows to resolve conflicts in a mechanical way (though, manual). It might be possible to write your own program and set it as mergetool: https://git-scm.com/docs/git-config#Documentation/git-config.txt-mergetool (assuming the mergetool is also used for rebases?) |
…n and disabling the exhaustive failure test
2877ebe to
782a1b6
Compare
|
rebased! |
|
Concept NACK, we either have the property of restrict-only script verification flags or we don't. Along the lines of the discussion in #22865 (comment), I'd encourage you to write an implementation of CTV that doesn't break this property and disable tests. |
|
Can this be closed? Seems to be too controversial based on the previous NACK. |
|
No it can't be closed -- I didn't see the nack till now. Gloria is incorrect that CTV "breaks" this property. Any opcode upgrade would break this property in the future if it is correctly implemented. @sipa has approach ACK'd this approach #22865 (comment) here. |
|
Oh indeed I misunderstood that comment. It looks like gating the opcode under DISCOURAGE_UPGRADABLE_NOPS is needed for its usage to remain nonstandard before activation. I can't think of another way to do it either. Perhaps breaking the restrict-only property is fine if we're repurposing a nop. However, I don't believe that this change is desirable on its own. |
|
It's not just a NOP, this type of change, is also required for e.g. OP_SUCCESSX, or for new key types in Taproot as used in APO, or for doing anything with the Annex (edit: annex may be able to 'cheat' like taproot did, will need to think on that). It'd be good to make the testing changes required in core now since ideally we'd have one approach that fits all these types of discouragable upgrades. |
Fixes #22865. Required for #21702 tests.
The basic issue is that that the current exclude flag doesn't play nicely with flags like DISCOURAGE_UPGRADABLE_NOPS + SOME_OPCODE_VERIFY.
When you have a valid TX under SOME_OPCODE_VERIFY, then the tests will try disabling that flag and it will trip over DISCOURAGE_UPGRADABLE_NOPS.
When you exclude DISCOURAGE_UPGRADABLE_NOPS, then it gets re-included when the test checks all cominations of the excluded flags being re-enabled to show failure, but then it doesn't fail when SOME_OPCODE_VERIFY is set.
To address this, we add two optional fields: 1, a list of flags to always enable; 2, a bool of if to disable the one-by-one checker.
By then testing the same vector with:
we get around the one flag at a time checker.
An alternative approach, that would provide better testing coverage, would allow a more abritrary relationship checker that can be specificed by the test writer (e.g., a map of flag to implied flag) to be used during the one-by-one checker.
We cannot simply apply these rules in FillFlags/TrimFlags because if we have a transaction that should fail normally with VERIFY_SOME_OPCODE and DISCOURAGE_UPGRADABLE_OPCODES we can't support both cases.
This gap arose when implementing test vectors for BIP-119. Along the way I also discovered that, e.g., CHECKSEQUENCEVERIFY's Upgradable NOP treatment is improperly asserted as per:
which does not assert the proper DISCOURAGE_UPGRADABLE_NOP behavior. I beleive that lack of discouragement to be mildly worrying but not a major security concern. Fixing the testing harnesses would allow us to properly implement DISCOURAGE_UPGRADABLE_NOPS for CSV and add the corresponding test cases.