#36455 Test: Add legacy script edge cases to taproot test
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36455 Test: Add legacy script edge cases to taproot test | Waiting on author | 1 | Positive | 0 + 32 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Adds explicit test cases to feature_taproot.py covering legacy script edge cases that alternative implementations have historically implemented incorrectly. These include OP_CHECKSEQUENCEVERIFY with negative values, 5-byte operands whose values fit in fewer bytes, OP_IFDUP on non-minimal or negative zeros, OP_ROLL on stacks with duplicate values, OP_16 within P2SH scriptSigs, and P2SH scriptSig stack item count boundaries.
Problem: Bitcoin Core's test vectors do not explicitly exercise several legacy script interpreter edge cases. When generating test asset dumps for downstream and alternative implementations, coverage-guided minimizers omit these cases because existing tests hit the relevant code branches without isolating the specific edge conditions.
Evidence of importance:
- "These were discovered by finding gaps in alternate implementations (bitcoin-s and rbitcoin)" (PR description)
- "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." (commit 660c159220)
Risk of a mistake: low (tests). Touches only functional test cases in feature_taproot.py.
Changed since the previous assessment: Added a boundary test case for P2SH scriptSig stack sizes checking the 1,000-item stack limit.
Reviewability
Paused · Waiting on author
Open objections from darosior, instagibbs with no author reply for 10 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: address comment removal suggestion from instagibbs and test organization feedback from darosior 2026-10-22
Agreement
Positive
support without stated reasons, no open objection (instagibbs, instagibbs)
Thread notes: Added darosior's non-blocking suggestion regarding testing legacy script cases outside the Taproot test suite.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| instagibbs (member) | question, maintenance | whether these test cases will still be dropped on regeneration | accepted · author confirmed they survive regeneration and instagibbs reACKed | answered | yes | 2026-10-19: 'Though IIUC these will still be dropped on regeneration?' Status from: 2026-10-21: instagibbs: 'reACK 660c1592205fb80b258e9926db80575d5e7957ae' |
| sedited (member) | suggestion, approach | consider whether these test cases should go into the functional tests at all if dropped on regeneration | answered · author clarified that the tests survive regeneration and add coverage | answered | yes | 2026-10-19: 'Should these even go into the functional tests then?' Status from: 2026-10-21: benthecarman: 'Since they survive regeneration and add coverage, I don't see why not.' |
| instagibbs (member) | suggestion, style | remove the unneeded comment | open | answered | no | 2026-10-21: 'dont think we need this comment' |
| darosior (member) | suggestion, approach | export the test framework to non-Taproot tests or add legacy cases to script_tests.json rather than adding legacy-only cases to the Taproot test | open | answered | no | 2026-10-22: 'i dislike that this adds legacy-only cases to a test supposed to test Taproot logic.' |
Support:
- instagibbs (member) (ACK, 2026-10-19): (no reason given) [not substantive]
- instagibbs (member) (ACK, 2026-10-21): (no reason given) [not substantive]
Participants:
- fanquake (member, neutral, 1 statements 2026-10-16 to 2026-10-16): pinged reviewers
- instagibbs (member, support, 3 statements 2026-10-19 to 2026-10-21): reACKed after author clarified regeneration behavior, suggested removing a comment
- sedited (member, question, 1 statements 2026-10-19 to 2026-10-19): questioned whether these cases belong in the functional tests
- darosior (member, neutral, 1 statements 2026-10-22 to 2026-10-22): suggested moving legacy script cases to non-Taproot tests or script_tests.json, though noted it is not a blocker
Dependencies
Enables:
Files
32 lines under test/bench/ci.
- test/functional/feature_taproot.py +32/-0
Card
The short text the ranking pass compares across PRs.
Adds explicit test spenders to feature_taproot.py for legacy script edge cases: OP_CHECKSEQUENCEVERIFY with negative numbers, 5-byte numeric inputs, OP_IFDUP on non-minimal or negative zeros, OP_ROLL with identical stack values, OP_16 in P2SH scriptSigs, and P2SH scriptSig 1,000-item stack size boundaries. These edge cases were identified from discrepancies in alternative Bitcoin implementations such as bitcoin-s and rbitcoin. Existing test assets lacked these vectors because automated minimizers dropped them when broader code paths were already touched. Merging this enables updated test dumps to be exported to bitcoin-core/qa-assets#297.
Data
thread read through 2026-10-22 (7 statements), head 660c159220
labels: Tests
Categories
Test infrastructure
P3 · test coverage
Adds explicit test coverage for legacy script interpreter edge cases that alternative implementations have gotten wrong, ensuring coverage-guided minimizers retain them when generating QA test assets. The underlying Bitcoin Core code already executes these branches correctly, making these regression tests valuable but reasonably deferrable.
Membership: Labeled Tests and modifies test/functional/feature_taproot.py to add explicit coverage for consensus script interpreter edge cases.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1