script: create V1SigVersion for functions which should only accept taproot/tapscript #26101

1. 
   theuni commented at 3:28 pm on September 15, 2022: member

   This is intended to be a safer way for functions to accept only a subset of sigversion values.

   It’s intended to solve the uninitialized value warning seen here: #25972 (comment)

   Now that all possible values in the enum class are tested, there’s no need for a default case and thus the compiler should be able to deduce that there’s no way for this value to be used uninitialized.

   Sadly it’s not possible to do any type of inheritance with an enum class, so instead it’s faked by using std::underlying_type_t<SigVersion>, which should ensure that it’s in sync with SigVersion even if it changes in the future.

   Imo this is safe enough that there’s no need for a default case as suggested here: #25972 (comment) , but that’s up for discussion here.

   The second commit is simply a cleanup that removes the need to pass a sigversion to EvalChecksigTapscript because it’s only intended to ever be used with… tapscript.

2. 
   theuni commented at 3:30 pm on September 15, 2022: member

   ping @sipa . Curious if you dislike this as it ties these functions to this subset of sigversions, though it could easily be ranamed/expanded later to mean “non-v0” if necessary.
3. 
   theuni commented at 3:46 pm on September 15, 2022: member

   Whoops, apparently I missed a few cases. Converting to draft for now.

   Concept ACKs/NACKs welcome.

4. theuni marked this as a draft on Sep 15, 2022
5. DrahtBot added the label Consensus on Sep 15, 2022
6. script: define explicit V1ScriptVersion and use it to constrain

   Some functions only accept a subset of script versions, leaving them with
   undefined behavior when called with v0 scripts. Instead, define a type that
   is a subset of ScriptVersion and use it when applicable.

   b931891360
7. script: no need to pass down implied script version 0ba2daa9b8
8. theuni force-pushed on Sep 15, 2022
9. theuni marked this as ready for review on Sep 16, 2022
10. 
    theuni commented at 12:29 pm on September 16, 2022: member

    Fixed up now.

    Ironically this doesn’t actually eliminate the warning as I’d hoped. I will leave this open in case anyone finds it interesting, but no need to think of it in terms of #25972 (comment).

11. 
    DrahtBot commented at 4:21 am on September 17, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24149 (Signing support for Miniscript Descriptors by darosior)
    • #23502 (wallet: Avoid underpaying transaction fees when signing taproot spends by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

12. DrahtBot added the label Needs rebase on Feb 16, 2023
13. 
    DrahtBot commented at 11:13 am on February 16, 2023: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

14. achow101 closed this on Apr 25, 2023

    ---

15. bitcoin locked this on Apr 24, 2024

Contributors
theuni DrahtBot

Labels
Consensus Needs rebase