Skip to content

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

Closed
sanket1729 wants to merge 1 commit intobitcoin:masterfrom
sanket1729:opcode_test
Closed

[Refactor]: Rename Script methods that only work on PreTapScript scripts#22338
sanket1729 wants to merge 1 commit intobitcoin:masterfrom
sanket1729:opcode_test

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

sanket1729 commented Jun 24, 2021

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

benthecarman commented Jun 25, 2021

Concept ACK

1 similar comment
@theStack
Copy link
Contributor

theStack commented Jun 28, 2021

Concept ACK

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Jun 28, 2021

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
Copy link
Member

@laanwj laanwj Nov 10, 2021

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 10, 2021

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

fanquake commented Apr 26, 2022

@sanket1729 are you interested in following up here?

@sanket1729
Copy link
Contributor Author

sanket1729 commented Apr 26, 2022

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

@sanket1729
Copy link
Contributor Author

sanket1729 commented Apr 28, 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.

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

sanket1729 commented Apr 28, 2022

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

achow101 commented Oct 12, 2022

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

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

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