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

jamesob · 2025-04-10T19:41:43Z

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, yuvicc, delta1, pinheadmz
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:

#32998 (Bump SCRIPT_VERIFY flags to 64 bit by ajtowns)
#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 arminsabouri)
#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.

jonatack · 2025-04-11T15:34:58Z

Concept ACK

JeremyRubin · 2025-04-16T01:37:11Z

src/script/interpreter.h

    // Making unknown public key versions (in BIP 342 scripts) non-standard
    SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE = (1U << 20),

+    // Validating OP_CHECKSIGFROMSTACK(VERIFY)


should maybe use bits 24 and 25

JeremyRubin · 2025-04-16T01:45:31Z

src/pubkey.cpp

 }

-bool XOnlyPubKey::VerifySchnorr(const uint256& msg, std::span<const unsigned char> sigbytes) const
+bool XOnlyPubKey::VerifySchnorr(


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.

src/script/interpreter.cpp

src/pubkey.cpp

JeremyRubin · 2025-04-16T02:07:52Z

src/script/interpreter.cpp

+        }
+    }
+    if (pubkey_in.size() == 0) {
+        return set_error(serror, SCRIPT_ERR_PUBKEYTYPE);


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

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?

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?

src/script/interpreter.cpp

shahsb

I agree with @JeremyRubin comments..!

shahsb · 2025-04-17T07:04:51Z

Concept ACK

JeremyRubin · 2025-04-17T13:25:51Z

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.

instagibbs · 2025-04-17T19:47:01Z

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 · 2025-04-26T14:23:58Z

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

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 <[email protected]>

yuvicc · 2025-05-22T15:14:16Z

Concept ACK

delta1 · 2025-06-09T11:59:06Z

Concept ACK

pinheadmz · 2025-06-10T17:04:26Z

In an effort to keep pull requests focused on technical discussion, I invite all contributors of conceptual review to post here:

bitcoin-core/meta#28

Please try to keep pull request comments focused on the code changes, and move all other comments including "concept N/ACK" to the discussion page.

benthecarman · 2025-06-22T07:56:03Z

src/script/interpreter.cpp

+                    // OP_CHECKSIGFROMSTACK is only available in Tapscript
+                    if (sigversion == SigVersion::BASE || sigversion == SigVersion::WITNESS_V0) {
+                        return set_error(serror, SCRIPT_ERR_BAD_OPCODE);
+                    }


https://github.com/bitcoin/bips/blob/master/bip-0348.md

benthecarman · 2025-07-30T17:57:29Z

src/script/interpreter.cpp

    return true;
 }

+static bool EvalChecksigFromStack(const valtype& sig, const valtype& msg, const valtype& pubkey_in, ScriptExecutionData& execdata, unsigned int flags, SigVersion sigversion, ScriptError* serror, bool& success)


nit: this is between EvalChecksigPreTapscript and EvalChecksigTapscript which can be a little confusing/annoying

DrahtBot · 2025-08-11T11:28:31Z

🐙 This pull request conflicts with the target branch and needs rebase.

652424a test: additional test coverage for script_verify_flags (Anthony Towns) 417437e script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns) 3cbbcb6 script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns) bddcade script/verify_flags: make script_verify_flags type safe (Anthony Towns) a5ead12 script/interpreter: introduce script_verify_flags typename (Anthony Towns) 4577fb2 rpc: have getdeploymentinfo report script verify flags (Anthony Towns) a398693 validation: export GetBlockScriptFlags() (Anthony Towns) 5db8cd2 Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns) Pull request description: We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](https://github.com/benthecarman/bitcoin/blob/d4a86277ed8a0712e03fbbce290e9209165e049c/src/script/interpreter.h#L175-L195). Therefore, bump this to 64 bits. In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo. ACKs for top commit: instagibbs: reACK 652424a achow101: ACK 652424a darosior: ACK 652424a theStack: Code-review ACK 652424a 🎏 Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f

laanwj added the Consensus label Apr 11, 2025

jamesob force-pushed the 2025-04-csfs branch from 1554828 to cb0c9f6 Compare April 11, 2025 15:07

This was referenced Apr 11, 2025

cmake, guix: Skip building tests in subtrees for releases #32054

Closed

build: Embedded ASMap [3/3]: Build binary dump header file #28792

Open

Remove the legacy wallet and BDB dependency #28710

Merged

fjahr mentioned this pull request Apr 14, 2025

Topics for FFM Socratic Meetup #3 btcffm/website-btcffm#21

Closed