BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only) #32247
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32247. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
| @@ -143,6 +143,12 @@ enum : uint32_t { | |||
| // Making unknown public key versions (in BIP 342 scripts) non-standard | |||
| SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE = (1U << 20), | |||
|
|
|||
| // Validating OP_CHECKSIGFROMSTACK(VERIFY) | |||
There was a problem hiding this comment.
should maybe use bits 24 and 25
| @@ -227,12 +227,13 @@ bool XOnlyPubKey::IsFullyValid() const | |||
| return secp256k1_xonly_pubkey_parse(secp256k1_context_static, &pubkey, m_keydata.data()); | |||
| } | |||
|
|
|||
| bool XOnlyPubKey::VerifySchnorr(const uint256& msg, std::span<const unsigned char> sigbytes) const | |||
| bool XOnlyPubKey::VerifySchnorr( | |||
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| if (pubkey_in.size() == 0) { | ||
| return set_error(serror, SCRIPT_ERR_PUBKEYTYPE); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
|
Concept ACK |
|
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. |
|
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 |
|
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 <github@au92.org>
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
In an effort to keep pull requests focused on technical discussion, I invite all contributors of conceptual review to post here: 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. |
| // OP_CHECKSIGFROMSTACK is only available in Tapscript | ||
| if (sigversion == SigVersion::BASE || sigversion == SigVersion::WITNESS_V0) { | ||
| return set_error(serror, SCRIPT_ERR_BAD_OPCODE); | ||
| } |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| @@ -343,6 +343,53 @@ static bool EvalChecksigPreTapscript(const valtype& vchSig, const valtype& vchPu | |||
| 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) | |||
There was a problem hiding this comment.
nit: this is between EvalChecksigPreTapscript and EvalChecksigTapscript which can be a little confusing/annoying
|
🐙 This pull request conflicts with the target branch and needs rebase. |
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:
See also: