[TESTS] Update Transaction Tests to permit setting a flag as always on and disabling the exhaustive failure test #22876

pull JeremyRubin wants to merge 1 commits into bitcoin:master from JeremyRubin:update-txvalid-json changing 2 files +21 −10
  1. JeremyRubin commented at 8:37 am on September 3, 2021: 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:

    0...'DISCOURAGE_UPGRADABLE_NOPS', 'NONE', true]
    1...'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:

    0                   // To provide for future soft-fork extensibility, if the
    1                    // operand has the disabled lock-time flag set,
    2                    // CHECKSEQUENCEVERIFY behaves as a NOP.
    3                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0)
    4                        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.

  2. fanquake added the label Tests on Sep 3, 2021
  3. glozow commented at 11:33 am on September 3, 2021: member

    This particular test failure is because SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS should only be applied for NOPs that aren’t upgraded: See #21702 (review).

    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.

  4. JeremyRubin commented at 4:45 pm on September 3, 2021: contributor

    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.

  5. DrahtBot commented at 4:33 pm on September 12, 2021: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

  6. DrahtBot added the label Needs rebase on Dec 1, 2021
  7. JeremyRubin commented at 6:14 pm on December 12, 2021: contributor
    @MarcoFalke is their an automated tool to run to fix this PR?
  8. maflcko commented at 7:59 am on December 13, 2021: member

    @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?)

  9. bitcoin deleted a comment on Dec 13, 2021
  10. [TESTS] Update Transaction Tests to permit setting a flag as always on and disabling the exhaustive failure test 782a1b6585
  11. JeremyRubin force-pushed on Dec 17, 2021
  12. DrahtBot removed the label Needs rebase on Dec 17, 2021
  13. JeremyRubin commented at 8:36 pm on December 17, 2021: contributor
    rebased!
  14. glozow commented at 7:53 pm on April 22, 2022: member
    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.
  15. maflcko commented at 12:50 pm on May 12, 2022: member
    Can this be closed? Seems to be too controversial based on the previous NACK.
  16. JeremyRubin commented at 2:14 pm on May 12, 2022: contributor

    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.

  17. glozow commented at 7:19 pm on May 12, 2022: member

    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.

  18. JeremyRubin commented at 10:00 pm on May 12, 2022: contributor

    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.

  19. JeremyRubin closed this on Dec 16, 2022

  20. bitcoin locked this on Dec 16, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me