Implement OP_CHECKSIGFROMSTACK(VERIFY) #29270

1. 
   reardencode commented at 6:18 am on January 18, 2024: none

   This pull request forks from #29198 and includes only OP_CHECKSIGFROMSTACK(VERIFY) and their tests.
2. 
   DrahtBot commented at 6:19 am on January 18, 2024: 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/29270.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	moonsettler

   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:

   • #31519 (refactor: Use std::span over Span by maflcko)
   • #29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
   • #29247 (CAT in Tapscript (BIP-347) by 0xBEEFCAF3)

   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.

3. DrahtBot added the label CI failed on Jan 18, 2024
4. 
   moonsettler commented at 1:14 pm on January 19, 2024: none

   Concept ACK!
5. winlover32 approved
6. glozow added the label Consensus on Feb 5, 2024
7. reardencode force-pushed on Apr 25, 2024
8. reardencode force-pushed on Apr 25, 2024
9. Implement OP_CHECKSIGFROMSTACK

   Some code and ideas from Elements by stevenroose, and sanket1729
   Porting help from moonsettler
   
   Tests added to the transaction tests framework.

   b49c23258a
10. reardencode force-pushed on Nov 26, 2024
11. 
    fanquake commented at 5:49 pm on February 20, 2025: member

    Closing for now, given #29198 was closed.
12. fanquake closed this on Feb 20, 2025

    ---

13. 
    1440000bytes commented at 6:59 pm on February 20, 2025: none

    Concept ACK

    Quoting a line from a recent post written by AJ as a soft forking guide:

    Open a PR for the changes to core, along with thorough tests

    https://ajtowns.github.io/bfg/finalization.html

14. in src/script/interpreter.cpp:369 in b49c23258a

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

    ---

    ---

    

    1440000bytes commented at 6:11 am on February 21, 2025:

    Maybe named constants can be used instead of 0,32 and 64. I am not sure if this works according to coding practices used in this repository.

    https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

    Or a comment.

    ---

    

    moonsettler commented at 10:43 am on February 21, 2025:

    There is nothing wrong with named constants when a value is reused a lot and can change in the future. But they don’t really make the code more readeable in most cases.
15. 
    reardencode commented at 8:13 pm on March 6, 2025: none

    @fanquake please reopen this PR.

    I’ve rebased on master and ensured that tests pass locally.

    #29198 was closed because this (and other PRs I opened subsequently) superseded it by splitting its contents up into smaller units.

16. 
    fanquake commented at 9:26 am on March 7, 2025: member

    @reardencode It’s no-longer possible to re-open:

    

    You’ll need to open a new PR.

17. 
    darosior commented at 1:49 pm on March 12, 2025: member

    @reardencode could you start by getting it merged and deployed on inquisition first? I’m not sure how another PR to Core for this would be useful at this point.

Contributors
reardencode DrahtBot moonsettler winlover32 fanquake 1440000bytes darosior

Labels
Consensus CI failed