[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:

    ...'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.

  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. JeremyRubin cross-referenced this on Sep 7, 2021 from issue Improve Transaction Tests Flags by JeremyRubin
  6. DrahtBot cross-referenced this on Sep 12, 2021 from issue [Tests] Compute the Power Set of all flags instead of one by one exclusion by JeremyRubin
  7. DrahtBot commented at 4:33 PM on September 12, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  8. DrahtBot cross-referenced this on Nov 18, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments by maflcko
  9. DrahtBot cross-referenced this on Nov 18, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) by maflcko
  10. DrahtBot added the label Needs rebase on Dec 1, 2021
  11. JeremyRubin commented at 6:14 PM on December 12, 2021: contributor

    @MarcoFalke is their an automated tool to run to fix this PR?

  12. 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?)

  13. bitcoin deleted a comment on Dec 13, 2021
  14. [TESTS] Update Transaction Tests to permit setting a flag as always on and disabling the exhaustive failure test 782a1b6585
  15. JeremyRubin force-pushed on Dec 17, 2021
  16. DrahtBot removed the label Needs rebase on Dec 17, 2021
  17. JeremyRubin commented at 8:36 PM on December 17, 2021: contributor

    rebased!

  18. DrahtBot cross-referenced this on Dec 18, 2021 from issue docs: avoid C-style casts; use modern C++ casts by PastaPastaPasta
  19. JeremyRubin cross-referenced this on Jan 3, 2022 from issue Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin
  20. 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.

  21. 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.

  22. 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.

  23. laanwj added this to the "Chasing Concept ACK" column in a project

  24. 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.

  25. 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.

  26. DrahtBot cross-referenced this on Jun 5, 2022 from issue test: Unit tests for taproot/tapscript coverage in `interpreter.cpp` by david-bakin
  27. DrahtBot cross-referenced this on Oct 4, 2022 from issue refactor: Remove duplicated test code by aureleoules
  28. JeremyRubin closed this on Dec 16, 2022

  29. 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: 2026-05-19 06:53 UTC