Skip to content

Conversation

@JeremyRubin
Copy link
Contributor

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:

...'DISCOURAGE_UPGRADABLE_NOPS', 'NONE', true]
...'NONE', 'SOME_OPCODE_VERIFY']

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:

                   // To provide for future soft-fork extensibility, if the
                    // operand has the disabled lock-time flag set,
                    // CHECKSEQUENCEVERIFY behaves as a NOP.
                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0)
                        break

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.

@maflcko
Copy link
Member

maflcko commented Sep 3, 2021

Please use your own fork for feature branches, not the main repo

@maflcko maflcko closed this Sep 3, 2021
@maflcko
Copy link
Member

maflcko commented Sep 3, 2021

@JeremyRubin You can rename the remote from origin to no_push_origin, or change the protocol to https to avoid this happening in the future by accident.

@JeremyRubin
Copy link
Contributor Author

ah crap i think the gh tool did that, wasn't doing a manually push.

@laanwj laanwj deleted the update-txvalid-json branch September 3, 2021 08:35
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve Transaction Tests Flags

4 participants