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, shahsb, i-am-yuvi
Stale ACK	JeremyRubin

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:

#32473 (Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts by sipa)
#32453 ([Policy] Discourage Unsigned Annexes by JeremyRubin)
#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)
#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

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

instagibbs commented at 6:41 pm on April 16, 2025:

https://gnusha.org/pi/bitcoindev/CAD5xwhi6DYVm3sONub0x4s=Ef0TupA4j4KxY616RnacXr1GsLA@mail.gmail.com/

I think I agree with 2022-you; separate opcode for key is cleaner and strictly better. Leaving it undefined might be fine?

JeremyRubin commented at 7:56 pm on April 16, 2025:

I’m fine with that – if we want INTERNALKEY it can be it’s own opcode later on.

follow up would be: does symmetry with CHECKSIG mean we should make OP_0 permanently a non-key? v.s. preserving the upgrade path?

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

shahsb commented at 7:04 am on April 17, 2025: none

I agree with @JeremyRubin comments..!

shahsb commented at 7:04 am on April 17, 2025: none

Concept ACK

JeremyRubin commented at 1:25 pm on April 17, 2025: contributor

cr ACK cb0c9f6

This matches the BIP’s semantics, and the implementation is reasonable. Minor nits above to reduce possibility of behavior changes for other users of Schnorr APIs.

DrahtBot requested review from jonatack on Apr 17, 2025

DrahtBot requested review from shahsb on Apr 17, 2025

instagibbs commented at 7:47 pm on April 17, 2025: member

I spent a bit of time writing tests (reading directly from the BIP) for feature_taproot.py: https://github.com/instagibbs/bitcoin/tree/2025-04-bip348_tests

This should hopefully give some nice coverage of border conditions, including sigops budget and maybe inspired people to add more if they want to contribute in a positive way.

https://github.com/instagibbs/bitcoin/tree/2025-03-bip348-inq-28 this was an alternative implementation of the core logic, but I think it matches here

jamesob force-pushed on Apr 26, 2025

jamesob commented at 2:23 pm on April 26, 2025: contributor

Thanks @instagibbs; I’ve compressed your test changes into a single commit and added that here.

DrahtBot added the label CI failed on Apr 29, 2025

DrahtBot added the label Needs rebase on Apr 29, 2025

jamesob force-pushed on Apr 30, 2025

DrahtBot removed the label Needs rebase on May 1, 2025

DrahtBot removed the label CI failed on May 1, 2025

DrahtBot added the label Needs rebase on May 6, 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>

15d7aa5391

consensus: add a CSFS deployment for regtest only 765a674603

test: add BIP-348 coverage to feature_taproot ae64dcebe1

jamesob force-pushed on May 12, 2025

DrahtBot removed the label Needs rebase on May 12, 2025

i-am-yuvi commented at 3:14 pm on May 22, 2025: contributor

Concept ACK

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

Code Coverage & Benchmarks

Reviews

Conflicts