Test: Add legacy script edge cases to taproot test #36455

pull benthecarman wants to merge 1 commits into bitcoin:master from benthecarman:taproot-legacy-edge-spenders changing 1 files +32 −0

benthecarman commented at 7:18 PM on October 7, 2026: contributor

Add feature_taproot spenders for script edge cases that other implementations have gotten wrong: CSV with -2, a 5-byte operand whose value fits in 1 byte, IFDUP on negative or non-minimal zero, ROLL with duplicate values, OP_16 in a P2SH scriptSig, and a P2SH scriptSig that leaves 1,000 items for the scriptPubKey to run on.

None of the current vectors catch an interpreter that gets these wrong. Merging their dumps into the script assets adds new coverage, and a regeneration of the script assets keeps them.

These were discovered by finding gaps in alternate implementations (bitcoin-s and rbitcoin)

Script assets: bitcoin-core/qa-assets#297
DrahtBot added the label Tests on Oct 7, 2026
DrahtBot commented at 7:19 PM on October 7, 2026: contributor


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



External sites
- Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36455
- Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36455.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36455#author-ralph


Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK instagibbs

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
fanquake commented at 6:20 AM on October 16, 2026: member

cc @instagibbs

cc @marcofleon can you look at https://github.com/bitcoin-core/qa-assets/pull/297.
instagibbs commented at 1:11 PM on October 19, 2026: member

ACK fcfd8aa09b482531b61c9481d169a54b66ea0cee

Though IIUC these will still be dropped on regeneration?
sedited commented at 1:28 PM on October 19, 2026: contributor

Though IIUC these will still be dropped on regeneration?

Should these even go into the functional tests then?

Type	Reviewers
ACK	instagibbs

Test: Add legacy script edge cases to taproot test

Add feature_taproot spenders for script edge cases that other
implementations have gotten wrong: CSV with -2, a 5-byte operand whose
value fits in 1 byte, IFDUP on negative or non-minimal zero, ROLL with
duplicate values, OP_16 in a P2SH scriptSig, and a P2SH scriptSig that
leaves 1,000 items for the scriptPubKey to run on.

None of the current vectors catch an interpreter that gets these
wrong. Merging their dumps into the script assets adds new coverage,
and a regeneration of the script assets keeps them.

660c159220

benthecarman force-pushed on Oct 21, 2026
benthecarman commented at 5:25 PM on October 21, 2026: contributor

Though IIUC these will still be dropped on regeneration?

I tested this and they don't.

Should these even go into the functional tests then?

Since they survive regeneration and add coverage, I don't see why not. The dumps are how they get into the script assets.

I also moved the P2SH stack-size spender here from #36465, as you suggested there. I'll update qa-assets#297 as well

in test/functional/feature_taproot.py:1261 in 660c159220

1255 | @@ -1248,6 +1256,30 @@ def zero_fn(h):
1256 |                  standard = hashtype in VALID_SIGHASHES_ECDSA and (p2sh or witv0)
1257 |                  add_spender(spenders, "compat/nocsa", hashtype=hashtype, p2sh=p2sh, witv0=witv0, standard=standard, script=CScript([OP_IF, OP_11, pubkey1, OP_CHECKSIGADD, OP_12, OP_EQUAL, OP_ELSE, pubkey1, OP_CHECKSIG, OP_ENDIF]), key=eckey1, sigops_weight=4-3*witv0, inputs=[getter("sign"), b''], failure={"inputs": [getter("sign"), b'\x01']}, **ERR_BAD_OPCODE)
1258 |  
1259 | +    # == Legacy script edge cases ==
1260 | +
1261 | +    # These cover script interpreter edge cases that other implementations have gotten wrong.

instagibbs commented at 5:29 PM on October 21, 2026:

dont think we need this comment

instagibbs approved
instagibbs commented at 5:29 PM on October 21, 2026: member

reACK 660c1592205fb80b258e9926db80575d5e7957ae
fanquake referenced this in commit de96f3ae6d on Oct 21, 2026
darosior commented at 3:31 PM on October 22, 2026: member

Test coverage for surprising consensus-critical logic should take precedence, so not a blocker, but i dislike that this adds legacy-only cases to a test supposed to test Taproot logic. If we find the framework of this test is useful, which clearly is the case, then i think it should be exported to other non-Taproot specific tests. But also non-Taproot script test cases need less structure, so if you can just add those to script_tests.json & co as part of #36458 that's also a refactor we can punt down to another time.

Contributors

Labels

Linked (view graph)

#36458 Test: Add script vectors for interpreter edges #36465 Test: Add P2SH scriptSig stack-size boundary tests

Test: Add legacy script edge cases to taproot test #36455

External sites

Reviews