Add `SigVersion` parameter to `IsOpSuccess()` #29265

Christewart · 2024-01-17T20:43:59Z

This PR adds a SigVersion parameter to IsOpSuccess(). This is needed to make continued progress on #29221 .

Problem

Given that OP_SUCCESSx even causes potentially unparseable scripts to pass, it can be used to introduce multi-byte opcodes, or even a completely new scripting language when prefixed with a specific OP_SUCCESSx opcode.

This is occurring on #29221, in script_assets_test.json there exists a witness redeemScript de4c. In #29221 this translates to OP_GREATHERTHAN64 OP_PUSHDATA1.

When looking at parsing logic for GetScriptOp we see that OP_PUSHDATA1 requires a data element following it to specify the push size. Since this is not available, the script fails to parse and thus fails the entire script.

Solution

Make IsOpSuccess() dependent on SigVersion. Now for SigVersion::TAPSCRIPT leaf versions, we retain the OP_SUCCESSx semantics, thus trivially parsing and passing the witness redeemScript de4c

If you want to see how this works with #29221, please see this commit that shows this logic working in tandem with the new op codes proposed in the 64 bit arithmetic BIP:

DrahtBot · 2024-01-17T20:44:01Z

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.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)
#29270 (Implement OP_CHECKSIGFROMSTACK(VERIFY) by reardencode)
#29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
#29247 (Reenable OP_CAT by 0xBEEFCAF3)
#29221 (Implement 64 bit arithmetic op codes in the Script interpreter by Christewart)
#29198 (OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE) by reardencode)
#29134 (Compressed Bitcoin Transactions by TomBriar)
#29123 (build: Remove HAVE_CONSENSUS_LIB by TheCharlatan)
#29050 (Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes by stevenroose)
#28550 (Covenant tools softfork by jamesob)

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.

src/script/script.cpp

sipa · 2024-01-17T21:43:11Z

I think you'd want to pass SigVersion rather than leaf_version. The SigVersion defines what script/signature rules are active, while the leaf_version is one of several ways of encoding that information in scripts. It also answers your comment: we don't care about unknown leaf_versions because they will never occur (under an unknown leaf version, no script execution happens at all).

That said, I think a change like this belongs in the PR that implements a consensus change that needs it. It's generally better not to touch consensus code unless necessary.

Christewart · 2024-01-17T21:51:25Z

That said, I think a change like this belongs in the PR that implements a consensus change that needs it. It's generally better not
to touch consensus code unless necessary.

Small quibble here, there are other consensus PRs out there that probably would need this change ( #29247 , #29050 , #28550 ).

Its unclear to me right now if I was just "lucky" with my opcode byte selection such that I ran into a test case that failed with witness redeemScript (4edc). Perhaps the test vectors are exhaustive though, I have not looked closely.

That said, if you don't find that argument compelling i will close this in the next few days and add it to #29221

DrahtBot · 2024-01-17T22:12:58Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20590106339}

ajtowns · 2024-01-18T04:13:23Z

Small quibble here, there are other consensus PRs out there that probably would need this change ( #29247 , #29050 , #28550 ).

While they're just drafts, they can just each include the commit; DrahtBot will report them as conflicting, but that's not big deal.

If/when it becomes apparent that there's wide consensus in favour of adopting/merging/activating one or more of the changes, we can pull that commit out as a preparatory step prior to the consensus changes. Compare with #16902 or #18002 perhaps.

Christewart · 2024-01-18T18:51:43Z

I think you'd want to pass SigVersion rather than leaf_version. The SigVersion defines what script/signature rules are active, while the leaf_version is one of several ways of encoding that information in scripts. It also answers your comment: we don't care about unknown leaf_versions because they will never occur (under an unknown leaf version, no script execution happens at all).

done in 5600629 , however I still have the return true; catch call, perhaps i'm missing something obvious but i'm unsure of how to get around this.

src/script/script.cpp

Christewart · 2024-01-18T19:17:43Z

src/script/script.cpp

@@ -7,6 +7,7 @@

 #include <crypto/common.h>
 #include <hash.h>
+#include <script/interpreter.h>


This triggers the linter

A new circular dependency in the form of "script/interpreter -> script/script -> script/interpreter" appears to have been introduced.

Is there an easy way to get around this? The include is for SigVersion

seems like putting SigVersion in its own file would fix this, is that the correct approach?

maflcko · 2024-01-18T19:53:20Z

src/script/script.cpp

+    case SigVersion::BASE:
+    case SigVersion::WITNESS_V0:
+    case SigVersion::TAPROOT:
+    default:


Using default in a switch will disable compiler warnings and make code more brittle. See the developer notes on how to handle this.

I attempted to fix this in c1fe710

Is that what things should look like? From Pieter's original comment, it seemed like asserting false in the case of non tapscript sig versions are was desirable.

#29265 (comment)

Rework IsOpSuccess() to use SigVersion rather than leaf_version Use switch based impl for IsOpSuccess() Remove default in switch statement Fix compiler warning Move SigVersion to sigversion.h Fix includes try to fix compile Add sigversion.h Add include guards

Christewart · 2024-02-01T19:35:46Z

That said, I think a change like this belongs in the PR that implements a consensus change that needs it. It's generally better not to touch consensus code unless necessary.

Closing this PR on this recommendation.

For future readers that might find this useful, you can cherry-pick this commit: 50b7286 or find the latest in #29221

Christewart commented Jan 17, 2024

View reviewed changes

src/script/script.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Jan 17, 2024

ajtowns mentioned this pull request Jan 18, 2024

Re enable OP_CAT bitcoin-inquisition/bitcoin#39

Merged

4 tasks

This was referenced Jan 18, 2024

Add OP_INTERNALKEY for Tapscript #29269

Draft

Reenable OP_CAT #29247

Open

Christewart force-pushed the 2024-01-17-isopsuccess-leafver branch from 1498198 to 5600629 Compare January 18, 2024 18:45

Christewart changed the title ~~Add leaf_version parameter to IsOpSuccess()~~ Add SigVersion parameter to IsOpSuccess() Jan 18, 2024

sipa reviewed Jan 18, 2024

View reviewed changes

src/script/script.cpp Outdated Show resolved Hide resolved

Christewart commented Jan 18, 2024

View reviewed changes

Christewart force-pushed the 2024-01-17-isopsuccess-leafver branch from 5600629 to fd9776d Compare January 18, 2024 19:45

maflcko reviewed Jan 18, 2024

View reviewed changes

Christewart force-pushed the 2024-01-17-isopsuccess-leafver branch from fd9776d to b84c861 Compare January 18, 2024 20:06

DrahtBot mentioned this pull request Jan 18, 2024

Implement OP_CHECKSIGFROMSTACK(VERIFY) #29270

Draft

DrahtBot removed the CI failed label Jan 23, 2024

This was referenced Jan 23, 2024

Implement OP_CHECKTEMPLATEVERIFY #29280

Draft

RFC: Deprecate libconsensus #29189

Merged

Compressed Bitcoin Transactions #29134

Closed

build: Remove HAVE_CONSENSUS_LIB #29123

Closed

Christewart force-pushed the 2024-01-17-isopsuccess-leafver branch from 6126c56 to f4907a9 Compare January 23, 2024 12:21

Christewart force-pushed the 2024-01-17-isopsuccess-leafver branch from f4907a9 to 50b7286 Compare February 1, 2024 19:08

Christewart closed this Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SigVersion` parameter to `IsOpSuccess()` #29265

Add `SigVersion` parameter to `IsOpSuccess()` #29265

Christewart commented Jan 17, 2024 •

edited

DrahtBot commented Jan 17, 2024 •

edited

sipa commented Jan 17, 2024

Christewart commented Jan 17, 2024

DrahtBot commented Jan 17, 2024

ajtowns commented Jan 18, 2024

Christewart commented Jan 18, 2024 •

edited

Christewart Jan 18, 2024 •

edited

maflcko Jan 18, 2024

Christewart Jan 18, 2024

Christewart commented Feb 1, 2024 •

edited

Add SigVersion parameter to IsOpSuccess() #29265

Add SigVersion parameter to IsOpSuccess() #29265

Conversation

Christewart commented Jan 17, 2024 • edited

Problem

Solution

DrahtBot commented Jan 17, 2024 • edited

Code Coverage

Reviews

Conflicts

sipa commented Jan 17, 2024

Christewart commented Jan 17, 2024

DrahtBot commented Jan 17, 2024

ajtowns commented Jan 18, 2024

Christewart commented Jan 18, 2024 • edited

Christewart Jan 18, 2024 • edited

Choose a reason for hiding this comment

maflcko Jan 18, 2024

Choose a reason for hiding this comment

Christewart Jan 18, 2024

Choose a reason for hiding this comment

Christewart commented Feb 1, 2024 • edited

Add `SigVersion` parameter to `IsOpSuccess()` #29265

Add `SigVersion` parameter to `IsOpSuccess()` #29265

Christewart commented Jan 17, 2024 •

edited

DrahtBot commented Jan 17, 2024 •

edited

Christewart commented Jan 18, 2024 •

edited

Christewart Jan 18, 2024 •

edited

Christewart commented Feb 1, 2024 •

edited