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

Christewart commented at 8:43 PM on January 17, 2024: contributor

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 commented at 8:44 PM on January 17, 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. 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.

in src/script/script.cpp:350 in 1498198042 outdated

 347 | +        return opcode == 80 || opcode == 98 || (opcode >= 126 && opcode <= 129) ||
 348 |             (opcode >= 131 && opcode <= 134) || (opcode >= 137 && opcode <= 138) ||
 349 |             (opcode >= 141 && opcode <= 142) || (opcode >= 149 && opcode <= 153) ||
 350 |             (opcode >= 187 && opcode <= 254);
 351 | +    }
 352 | +    return true;

Christewart commented at 8:52 PM on January 17, 2024:

For unknown leaf_versions, do we want to return true by default?

Christewart commented at 6:50 PM on January 18, 2024:

I don't see how this is better with SigVersion, perhaps I'm missing some pattern matching features in c++ that would enforce exhaustiveness checks?

sipa commented at 6:52 PM on January 18, 2024:

There simply wouldn't exist a SigVersion for unknown leaf version.

Christewart commented at 6:56 PM on January 18, 2024:

So when you say that I assume you want this

bool IsOpSuccess(const opcodetype& opcode, SigVersion sigversion)
{
    if (sigversion == SigVersion::TAPSCRIPT) {
        return opcode == 80 || opcode == 98 || (opcode >= 126 && opcode <= 129) ||
           (opcode >= 131 && opcode <= 134) || (opcode >= 137 && opcode <= 138) ||
           (opcode >= 141 && opcode <= 142) || (opcode >= 149 && opcode <= 153) ||
           (opcode >= 187 && opcode <= 254);
    }
}

However this gives compiler warnings

script/script.cpp: In function ‘bool IsOpSuccess(const opcodetype&, SigVersion)’:
script/script.cpp:368:1: warning: control reaches end of non-void function [-Wreturn-type]
  368 | }
      | ^
script/script.cpp: In function ‘bool IsOpSuccess(const opcodetype&, SigVersion)’:
script/script.cpp:368:1: warning: control reaches end of non-void function [-Wreturn-type]
  368 | }
      | ^

The compiler warning makes sense to me - it wants a 'catch all' or 'default' case.

sipa commented at 9:43 PM on January 17, 2024: member

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 commented at 9:51 PM on January 17, 2024: contributor

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 added the label CI failed on Jan 17, 2024

DrahtBot commented at 10:12 PM on January 17, 2024: contributor

🚧 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.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/20590106339</sub>

ajtowns commented at 4:13 AM on January 18, 2024: contributor

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 force-pushed on Jan 18, 2024

Christewart renamed this:
~~Add leaf_version parameter to `IsOpSuccess()`~~
Add `SigVersion` parameter to `IsOpSuccess()`
on Jan 18, 2024

Christewart commented at 6:51 PM on January 18, 2024: contributor

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 https://github.com/bitcoin/bitcoin/pull/29265/commits/5600629cdb2530699b7da453f398f4bb259c8b04 , 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.

in src/script/script.cpp:344 in 5600629cdb outdated

 338 | @@ -338,12 +339,16 @@ bool GetScriptOp(CScriptBase::const_iterator& pc, CScriptBase::const_iterator en
 339 |      return true;
 340 |  }
 341 |  
 342 | -bool IsOpSuccess(const opcodetype& opcode)
 343 | +bool IsOpSuccess(const opcodetype& opcode, SigVersion sigversion)
 344 |  {
 345 | -    return opcode == 80 || opcode == 98 || (opcode >= 126 && opcode <= 129) ||
 346 | +    if (sigversion == SigVersion::TAPSCRIPT) {

sipa commented at 6:57 PM on January 18, 2024:

I would suggest a switch case, over all possible SigVersions. The non-taproot ones can just contain an assert false, as IsOpSuccess shouldn't be called for anything non-taproot.

Alternatively, there could also be separate IsOpSuccessTapscript and IsOpSuccessFooscript functions, which get called in the respective branches.

Christewart commented at 6:58 PM on January 18, 2024:

Ok, both of these make sense to me. I'll do the switch implementation for now.

in src/script/script.cpp:10 in 5600629cdb outdated

   6 | @@ -7,6 +7,7 @@
   7 |  
   8 |  #include <crypto/common.h>
   9 |  #include <hash.h>
  10 | +#include <script/interpreter.h>

Christewart commented at 7:17 PM on January 18, 2024:

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?

Christewart force-pushed on Jan 18, 2024

in src/script/script.cpp:355 in fd9776d48c outdated

 352 |             (opcode >= 187 && opcode <= 254);
 353 | +        break;
 354 | +    case SigVersion::BASE:
 355 | +    case SigVersion::WITNESS_V0:
 356 | +    case SigVersion::TAPROOT:
 357 | +    default:

maflcko commented at 7:53 PM on January 18, 2024:

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

Christewart commented at 8:15 PM on January 18, 2024:

I attempted to fix this in c1fe7109fc989c263c19696229c73c5982ff19e7

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 (review)

Christewart force-pushed on Jan 18, 2024

DrahtBot removed the label CI failed on Jan 23, 2024

Christewart force-pushed on Jan 23, 2024

Add SigVersion parameter to IsOpSuccess()

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

50b72867b3

Christewart force-pushed on Feb 1, 2024

Christewart commented at 7:35 PM on February 1, 2024: contributor

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: https://github.com/bitcoin/bitcoin/pull/29221/commits/50b72867b3ca4c794f8a768b8b2a9de7fbec1508 or find the latest in #29221

Christewart closed this on Feb 1, 2024

bitcoin locked this on Jan 31, 2025