BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)

jamesob commented at 7:41 pm on April 10, 2025: contributor

This implements BIP-348 (OP_CHECKSIGFROMSTACK), but only specifies a regtest deployment. There is no effective policy change, since the SCRIPT_VERIFY_* flags (as used) result in the same OP_SUCCESS-like behavior.

This change can be composed with other opcode specifications (e.g. CTV, see #31989) and bundled into the same deployment (yet to be specified).

I encourage more general, conceptual discussion to happen on Delving Bitcoin and not on this pull request.

Some related discussion on Delving Bitcoin here:

https://delvingbitcoin.org/t/ctv-csfs-can-we-reach-consensus-on-a-first-step-towards-covenants/

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32247.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jonatack

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32080 (OP_CHECKCONTRACTVERIFY by bigspider)
#31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
#29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
#29247 (CAT in Tapscript (BIP-347) by 0xBEEFCAF3)
#29039 (versionbits refactoring by ajtowns)
#26201 (Remove Taproot activation height by Sjors)

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.

laanwj added the label Consensus on Apr 11, 2025

Implement OP_CHECKSIGFROMSTACK

Some code and ideas from Elements by stevenroose, and sanket1729
Porting help from moonsettler.

Tests added to the transaction tests framework.

Co-authored-by: James O'Beirne <github@au92.org>

19674ccf3b

consensus: add a CSFS deployment for regtest only cb0c9f6389

jamesob force-pushed on Apr 11, 2025

jonatack commented at 3:34 pm on April 11, 2025: member

Concept ACK

in src/script/interpreter.h:146 in 19674ccf3b outdated

142@@ -143,6 +143,12 @@ enum : uint32_t {
143     // Making unknown public key versions (in BIP 342 scripts) non-standard
144     SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE = (1U << 20),
145 
146+    // Validating OP_CHECKSIGFROMSTACK(VERIFY)

JeremyRubin commented at 1:37 am on April 16, 2025:

should maybe use bits 24 and 25

in src/pubkey.cpp:230 in 19674ccf3b outdated

226@@ -227,12 +227,13 @@ bool XOnlyPubKey::IsFullyValid() const
227     return secp256k1_xonly_pubkey_parse(secp256k1_context_static, &pubkey, m_keydata.data());
228 }
229 
230-bool XOnlyPubKey::VerifySchnorr(const uint256& msg, std::span<const unsigned char> sigbytes) const
231+bool XOnlyPubKey::VerifySchnorr(

JeremyRubin commented at 1:45 am on April 16, 2025:

i think it might be better to introduce a new function, VerifySchnorrArbitrary here, that way any existing use is identical API. Or use multi dispatch.

in src/script/interpreter.cpp:1286 in 19674ccf3b outdated

1281+                    // Note that (as with CHECKSIG) if a signature was supplied and its
1282+                    // verification fails, we do _not_ push a "false" result to the stack.
1283+                    // Rather, we terminate script execution immediately. This might be
1284+                    // surprising if you're reading this for the first time.
1285+                    if (!EvalChecksigFromStack(vchSigIn, vchMsg, vchPubKey, execdata, flags, sigversion, serror, fSuccess)) {
1286+                        return false;

JeremyRubin commented at 1:46 am on April 16, 2025:

usually we return via set_error, right?

or I guess EvalCheckSigFromStack sets it for us – maybe worth a comment.

in src/pubkey.cpp:231 in 19674ccf3b outdated

226@@ -227,12 +227,13 @@ bool XOnlyPubKey::IsFullyValid() const
227     return secp256k1_xonly_pubkey_parse(secp256k1_context_static, &pubkey, m_keydata.data());
228 }
229 
230-bool XOnlyPubKey::VerifySchnorr(const uint256& msg, std::span<const unsigned char> sigbytes) const
231+bool XOnlyPubKey::VerifySchnorr(
232+    const std::span<const unsigned char> msg, std::span<const unsigned char> sigbytes) const

JeremyRubin commented at 1:57 am on April 16, 2025:

needs to be asserting that msg.size() > 0

JeremyRubin commented at 1:57 am on April 16, 2025:

secp256k1_schnorrsig_verify will assert otherwise

JeremyRubin commented at 2:00 am on April 16, 2025:

nvm misread the ARGCHECK –> it’s only checked that if msg is NULL, then length must be 0, which… makes sense!

in src/script/interpreter.cpp:365 in 19674ccf3b outdated

360+        if (execdata.m_validation_weight_left < 0) {
361+            return set_error(serror, SCRIPT_ERR_TAPSCRIPT_VALIDATION_WEIGHT);
362+        }
363+    }
364+    if (pubkey_in.size() == 0) {
365+        return set_error(serror, SCRIPT_ERR_PUBKEYTYPE);

JeremyRubin commented at 2:07 am on April 16, 2025:

should we just update the BIP for this to just be the internalkey? At the very least, there isn’t a hard reason I can think of to burn this keytype (other than checksig keytype parity)

we’d still be bound to the bytes per sigop restrictions, and it seems harmless to allow?

cc @instagibbs @reardencode

in src/script/interpreter.cpp:370 in 19674ccf3b outdated

365+        return set_error(serror, SCRIPT_ERR_PUBKEYTYPE);
366+    } else if (pubkey_in.size() == 32) {
367+        if (!success) {
368+            return true;
369+        }
370+        if (sig.size() != 64) {

JeremyRubin commented at 2:09 am on April 16, 2025:

code wise, it shouldn’t matter, but I’d set success = false here anyways?

in src/script/interpreter.cpp:376 in 19674ccf3b outdated

371+            return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_SIZE);
372+        }
373+
374+        XOnlyPubKey pubkey{pubkey_in};
375+
376+        if (!pubkey.VerifySchnorr(msg, sig)) {

JeremyRubin commented at 2:10 am on April 16, 2025:

same – it doesn’t matter, but I’d set success = false.

in src/script/interpreter.cpp:381 in 19674ccf3b outdated

376+        if (!pubkey.VerifySchnorr(msg, sig)) {
377+            return set_error(serror, SCRIPT_ERR_SCHNORR_SIG);
378+        }
379+    } else {
380+        /*
381+         *  New public key version softforks should be defined before this `else` block.

JeremyRubin commented at 2:30 am on April 16, 2025:

on L354 we do set success variable, and according to current BIP text, we should be setting success =true explicitly in this case.

JeremyRubin commented at 2:30 am on April 16, 2025:

further, wer’e erronenously setting success=false for unknown key types, so definitely have to fix that!

JeremyRubin commented at 3:09 am on April 16, 2025:

thought about this more; we do want success=false in all cases when sig = null, i was wrong

BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only) #32247

Code Coverage & Benchmarks

Reviews

Conflicts