refactor: Appropriate re-naming of MAX_OPCODE after tapscript #30953

pull itornaza wants to merge 1 commits into bitcoin:master from itornaza:ion-max-script-opcodes changing 5 files +8 −8
  1. itornaza commented at 5:51 pm on September 23, 2024: contributor

    In src/script/script.h:216 we define MAX_OPCODE to denote the maximum opcode that can be used in script.

    However, after the addition of the OP_CHECKSIGADD with taproot in the opcode set, the MAX_OPCODE naming looks somehow misleading.

    Renaming the variable to MAX_SCRIPT_OPCODE clearly denotes that the variable is intended for and referring exclusively to script (and not tapscript).

  2. refactor: Appropriate re-naming of MAX_OPCODE after tapscript 30e78d7c3d
  3. DrahtBot commented at 5:51 pm on September 23, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept NACK darosior

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. DrahtBot added the label Refactoring on Sep 23, 2024
  5. darosior commented at 7:53 am on October 1, 2024: member
    Concept NACK. Not that this change makes things worse, but i don’t think it meets the bar for refactorings.
  6. itornaza commented at 9:04 am on October 1, 2024: contributor

    Concept NACK. Not that this change makes things worse, but i don’t think it meets the bar for refactorings.

    Thank you for the clarification @darosior, and I will make sure to aim higher in the future to match the standards. However, the name of this variable after the introduction of tapscript is misleading and especially to new contributors like myself that read through the code and try to understand it. With the assumption that is very difficult to differentiate tapscript opcodes from script opcodes in a more structured way, this renaming despite being humble as the refactoring bar is concerned, made sense to me.

  7. darosior commented at 12:32 pm on October 1, 2024: member
    I understand your motivation. I’m not making a value judgement on whether this change is good or bad. It is just my personal opinion that the limited resources of the project are better allocated to something else than reviewing a bikeshed-prone (inbefore someone suggests renaming the variable to MAX_OPCODE_BEFORE_WITNESS_V1 instead) refactoring of a complicated and consensus critical part of the software.
  8. itornaza commented at 9:59 am on October 2, 2024: contributor
    @darosior, highly respecting your opinion and taking note of your suggestions, I am close this pr.
  9. itornaza closed this on Oct 2, 2024


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-10-08 16:12 UTC

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