[Refactor]: Rename Script methods that only work on PreTapScript scripts #22338

pull sanket1729 wants to merge 1 commits into bitcoin:master from sanket1729:opcode_test changing 15 files +44 −39
  1. sanket1729 commented at 7:33 PM on June 24, 2021: contributor

    <s> - This change is not really necessary as MAX_OPCODE check is only really used while decoding transactions, but makes the code more consistent(and correct)

    • The tests that use MAX_OPCODE are not failing because they are under P2SH context(where the OP_CHECKSIGADD is invalid anyways). The comment in the test has been updated to accurately reflect what is going on.</s>

    Based on Aj's comment, I have changed the PR so that it renames the constants/functions that operate on PreTapScript Scripts.

    I don't know if this is the ideal way to separate pre-tapscripts and tapscripts. If the new PR is not useful, feel free to close it.

  2. DrahtBot added the label Consensus on Jun 24, 2021
  3. ghost commented at 9:16 PM on June 24, 2021: none

    Concept ACK

    Since BIP 342 is implemented in Bitcoin Core, OP_CHECKSIGADD is the last executable opcode now according to my understanding.

    This change is not really necessary as MAX_OPCODE check is only really used while decoding transactions

    Not sure about this part

  4. sanket1729 commented at 11:35 PM on June 24, 2021: contributor

    This change is not really necessary as MAX_OPCODE check is only really used while decoding transactions

    To elaborate, right now this is used while parsing scrpitPubkey and ScrpitSig in a Tx. It is impossible for CHECKSIGADD to end up in scriptSig or scriptPubkey in taproot. Secondly, the opcode execution is only valid for taproot scripts, but according to the current design, the functions checking validity of scripts don't have access to appropriate scriptContext.

    So, the ideal solution would be to somehow pass the script context and enforce that the CHECKSIGADD is only used in tapscripts, but that probably requires more discussion and is out of scope for this PR.

  5. benthecarman commented at 6:23 PM on June 25, 2021: contributor

    Concept ACK

  6. theStack commented at 10:16 PM on June 28, 2021: contributor

    Concept ACK

  7. JeremyRubin commented at 10:58 PM on June 28, 2021: contributor

    Would it be a better idea to split the baby now and make the handling of pre-v1 and v1 scripts different here?

  8. in src/script/script.h:212 in 90f6a623b3 outdated 
     204 | @@ -205,7 +205,7 @@ enum opcodetype
 205 |  };
 206 |  
 207 |  // Maximum value that an opcode can be
 208 | -static const unsigned int MAX_OPCODE = OP_NOP10;
 209 | +static const unsigned int MAX_OPCODE = OP_CHECKSIGADD;
    laanwj commented at 3:45 PM on November 10, 2021:

    context:

    OP_NOP10 = 0xb9,
OP_CHECKSIGADD = 0xba,
  9. laanwj commented at 3:56 PM on November 10, 2021: member

    Concept ACK

    So, the ideal solution would be to somehow pass the script context and enforce that the CHECKSIGADD is only used in tapscripts, but that probably requires more discussion and is out of scope for this PR.

    The main thing to review here is then that this doesn't create any risk for non-tapscript verification. It seems that MAX_OPCODE is used in:

    • ParseOpCode parsing from text (non-consensus critical)
    • CScript::HasValidOps() (used only in CheckTxScriptsSanity, in core_read, and tests/fuzz, so non-consensus-critical)
    • ConsumeOpcodeType in the fuzz tests (non-consensus critical)

    I don't think this should even be labeled "consensus"? Or am I missing something.

    Code review ACK 90f6a623b394e65a34a1918ec67420c899c52bf4

  10. DrahtBot removed the label Consensus on Nov 10, 2021
  11. DrahtBot added the label Utils/log/libs on Nov 10, 2021
  12. DrahtBot commented at 4:13 PM on November 10, 2021: contributor

    I don't think this should even be labeled "consensus"? Or am I missing something.

    I am still learning. Not mixing consensus code with utility code would help me.

  13. ajtowns commented at 7:47 PM on January 19, 2022: contributor

    I don't think this makes sense? For tapscript, the highest valid opcode is OP_SUCCESS254, and for non-tapscript the highest valid opcode is still OP_NOP10.

    Maybe it would be better to rename MAX_OPCODE and HasValidOps to explicitly refer to "OG" script? (maybe also ParseScript in io_core.h)

  14. maflcko commented at 7:51 PM on January 19, 2022: member

    test fails? Maybe needs rebase?

    test/script_parse_tests.cpp(53): error: in "script_parse_tests/parse_script": exception std::runtime_error expected but not raised

    https://cirrus-ci.com/task/5542171511095296?logs=ci#L6208

  15. fanquake commented at 2:06 PM on April 26, 2022: member

    @sanket1729 are you interested in following up here?

  16. sanket1729 commented at 7:08 PM on April 26, 2022: contributor

    @fanquake, sorry I missed this. I will follow up on this in a couple of days.

  17. sanket1729 force-pushed on Apr 28, 2022
  18. sanket1729 commented at 10:11 AM on April 28, 2022: contributor

    I don't think this makes sense? For tapscript, the highest valid opcode is OP_SUCCESS254, and for non-tapscript the highest valid opcode is still OP_NOP10.

    Indeed

    Maybe it would be better to rename MAX_OPCODE and HasValidOps to explicitly refer to "OG" script? (maybe also ParseScript in io_core.h)

    Agreed, this seems the cleaner way to solve this.

  19. sanket1729 force-pushed on Apr 28, 2022
  20. sanket1729 commented at 10:43 AM on April 28, 2022: contributor

    Changed the PR description. I think there might be other places where we implicitly assume script as PreTapScript. For example, there is also a constant MAX_SCRIPT_SIZE that only holds for Pretaproot scripts.

  21. sanket1729 renamed this:
    Change MAX_OPCODE to OP_CHECKSIGADD
    [Refactor]: Rename Script methods that only work on PreTapScript scripts
    on Apr 28, 2022
  22. DrahtBot added the label Needs rebase on May 19, 2022
  23. Rename script operations that only work on pretapscripts
    Constant:
MAX_OPCODE -> MAX_OPCODE_PRE_TAPSCRIPT

Functions:
ParseScript -> ParseScriptPreTapScript
ParseOpcode -> ParseOpCodePreTapScript
HasValidOps -> HasValidOpsPreTapScript

class:
OpCodeParser -> PreTapScriptOpCodeParser
    4b6caf858d
  24. sanket1729 force-pushed on May 19, 2022
  25. DrahtBot removed the label Needs rebase on May 19, 2022
  26. DrahtBot commented at 2:41 AM on May 20, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26246 (refactor: Remove duplicated test code by aureleoules)

    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.

  27. achow101 commented at 6:33 PM on October 12, 2022: member

    Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

  28. achow101 closed this on Oct 12, 2022
  29. bitcoin locked this on Oct 12, 2023
Contributors
sanket1729ghostbenthecarmantheStackJeremyRubinlaanwjDrahtBotajtownsmaflckofanquakeachow101
Labels
Utils/log/libs
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-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me