8000 [Refactor]: Rename Script methods that only work on PreTapScript scripts by sanket1729 · Pull Request #22338 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

sanket1729
Copy link
Contributor
@sanket1729 sanket1729 commented Jun 24, 2021 •
edited
Loading
- 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.

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.

@ghost
Copy link
ghost commented Jun 24, 2021

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

@sanket1729
Copy link
Contributor Author

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.

@benthecarman
Copy link
Contributor

Concept ACK

1 similar comment
@theStack
Copy link
Contributor

Concept ACK

@JeremyRubin
Copy link
Contributor

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

laanwj
laanwj reviewed Nov 10, 2021
View reviewed changes
src/script/script.h Outdated
@@ -205,7 +205,7 @@ enum opcodetype
};

// Maximum value that an opcode can be
static const unsigned int MAX_OPCODE = OP_NOP10;
static const unsigned int MAX_OPCODE = OP_CHECKSIGADD;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context:

OP_NOP10 = 0xb9,
OP_CHECKSIGADD = 0xba,

@laanwj
Copy link
Member
laanwj commented Nov 10, 2021

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 90f6a62

@DrahtBot
Copy link
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.

@ajtowns
Copy link
Contributor
ajtowns commented Jan 19, 2022

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)

maflcko
maflcko reviewed Jan 19, 2022
View reviewed changes
Copy link
Member
@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fanquake
Copy link
Member

@sanket1729 are you interested in following up here?

@sanket1729
Copy link
Contributor Author

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

@sanket1729
Copy link
Contributor Author

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.

@sanket1729
Copy link
Contributor Author

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.

@sanket1729 sanket1729 changed the title Change MAX_OPCODE to OP_CHECKSIGADD [Refactor]: Rename Script methods that only work on PreTapScript scripts Apr 28, 2022
@DrahtBot DrahtBot mentioned this pull request May 19, 2022
Merged
@sanket1729
Rename script operations that only work on pretapscripts
4b6caf8 
Constant:
MAX_OPCODE -> MAX_OPCODE_PRE_TAPSCRIPT

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

class:
OpCodeParser -> PreTapScriptOpCodeParser
@DrahtBot
Copy link
Contributor
DrahtBot commented May 20, 2022 •
edited
Loading

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

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.

@DrahtBot DrahtBot mentioned this pull request Jun 5, 2022
Closed
9 tasks
@DrahtBot DrahtBot mentioned this pull request Aug 11, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Oct 4, 2022
Closed
@achow101
Copy link
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.

@achow101 achow101 closed this Oct 12, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@maflcko maflcko maflcko left review comments

Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@sanket1729 @benthecarman @theStack @JeremyRubin @laanwj @DrahtBot @ajtowns @fanquake @achow101 @maflcko
0