Implement BIP 348 validation (CHECKSIGFROMSTACK)

[instagibbs]

Re-applied logic from #72

[benthecarman]

ACK 92d41ec91ab4bfb256f1568aab49bb74b747552f

[ajtowns]

Should be 2024, 3, 2, since it's revision 2 you're implementing?

[instagibbs]

done thanks

[ajtowns]

Testing !sig.empty() or success_out only once seems cleaner:

} else if (size() == 32) {
    if (success_out) {
        if (sig.size() != 64) return SCRIPT_ERR_SCHNORR_SIG_SIZE;
        XOnlyPubKey pubkey{pubkey_in};
        if (!pubkey.VerifySchnorr(msg, sig)) return SCRIPT_ERR_SCHNORR_SIG;
    }
}

[instagibbs]

yes, taken

[instagibbs]

rebased

[ajtowns]

$ ./feature_taproot.py --randomseed 8836805849730300108

errors with "Data push larger than necessary" for me with this PR applied.

[ajtowns]

I believe you could use the five verification result=TRUE cases in test_framework/bip340_test_vectors.csv here to perhaps get a bit more coverage.

Not sure adding this to feature_taproot is a great idea? Seems like it makes it hard to test the behaviour when CSFS isn't activated, and it doesn't benefit from running multiple test cases at once to bring the total time down.

[instagibbs]

I believe you could use the five verification result=TRUE cases in test_framework/bip340_test_vectors.csv here to perhaps get a bit more coverage.

I'll take another look at this soon.

If it were easier to activate softforks, then it's pretty simple to test prior-to-activation behavior. instagibbs@bfda78a#diff-5371490253f356bb2ddeb014bb570c5a422ac73ca6dcd8db0f16b81cafa3c786R1898 as an examplefor TEMPLATEHASH

[ajtowns]

Set -vbparams=checksigfromstack:0:3999999999, mine a block with nVersion=0x62000302 (from getdeploymentinfo) using create_block, and mine 288 or so blocks on top?

[instagibbs]

figured it out and pushed a couple examples

[instagibbs]

@ajtowns changed to have inputs push something that won't be non-minimalpush in non-witness land

[Christewart]

By the specification of BIP348 these pops should happen before any logical evaluation

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

[instagibbs]

that's an implementation detail, not a semantic difference?

[ajtowns]

(lint checker has complaints)

[ajtowns]

ACK fe286d0

Manually checked behaviour with:

$ cat ${BIPS}/bip-0340/test-vectors.csv | tail -n +2  | cut -d, -f3,5,6,7 | (export IFS=,; while read key msg sig succ; do
  (./bitcoin-util -sigversion=tapscript -script_flags=STANDARD evalscript '0xCC' "$sig" "$msg" "$key" |
  jq .success; echo $succ) | fmt; done) |
  sort | uniq -c
     10 false FALSE
      9 true TRUE

Can't specify it as OP_CHECKSIGFROMSTACK via evalscript because OpCodeParser in core_read.cpp tops out at MAX_OPCODE=OP_NOP10 which is a pre-taproot limit, and also excludes CHECKSIGADD.