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

pull jamesob wants to merge 3 commits into bitcoin:master from jamesob:2025-04-csfs changing 20 files +296 −12
  1. 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:

    See also:

  2. DrahtBot commented at 7:41 pm on April 10, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    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
    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:

    • #32415 (scripted-diff: adapt script error constant names in feature_taproot.py by theStack)
    • #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)
    • #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.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    new code should not do anything but failing the script execution. → new code should not do anything but fail the script execution.
    tapleaves → tap leaves
    fails immediately with sig for wrong data → fails immediately with a sig for wrong data

  3. laanwj added the label Consensus on Apr 11, 2025
  4. jamesob force-pushed on Apr 11, 2025
  5. jonatack commented at 3:34 pm on April 11, 2025: member
    Concept ACK
  6. 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
  7. 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.
  8. 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.

  9. 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!
  10. 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?

  11. 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?
  12. 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.
  13. 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
  14. shahsb commented at 7:04 am on April 17, 2025: none
    I agree with @JeremyRubin comments..!
  15. shahsb commented at 7:04 am on April 17, 2025: none
    Concept ACK
  16. 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.

  17. DrahtBot requested review from jonatack on Apr 17, 2025
  18. DrahtBot requested review from shahsb on Apr 17, 2025
  19. 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

  20. jamesob force-pushed on Apr 26, 2025
  21. 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.
  22. DrahtBot added the label CI failed on Apr 29, 2025
  23. DrahtBot added the label Needs rebase on Apr 29, 2025
  24. 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>
    d3e568f25f
  25. consensus: add a CSFS deployment for regtest only b49dac5a08
  26. test: add BIP-348 coverage to feature_taproot 8c000e7f38
  27. jamesob force-pushed on Apr 30, 2025
  28. DrahtBot removed the label Needs rebase on May 1, 2025
  29. DrahtBot removed the label CI failed on May 1, 2025
  30. DrahtBot added the label Needs rebase on May 6, 2025
  31. DrahtBot commented at 11:17 pm on May 6, 2025: contributor

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


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-08 12:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me