#36280 validation: throw when a compressed script can't be decompressed
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36280 validation: throw when a compressed script can't be decompressed | Waiting on author | 1 | Mild | 7 + 89 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
Throws a serialization error in `ScriptCompression::Unser` when `DecompressScript` fails instead of silently ignoring the failure. Also adds `[[nodiscard]]` to the script compression helpers and expands unit tests and fuzzing for off-curve and invalid-pubkey script decompression.
Problem: `ScriptCompression::Unser` ignores the return value of `DecompressScript`. When decompression fails (reachable via data corruption), it leaves the script empty without throwing an error, causing what should be an unspendable script (such as an off-curve P2PK) to silently turn into an anyone-can-spend empty script.
Evidence of importance:
- PR description and commit 5500521f8b: "`ScriptCompression::Unser` ignores the `DecompressScript` return value and leaves the script empty on failure, without notifying upper layers about the error. In the worst-case scenario, which is currently only reachable through corruption, an unspendable off-curve P2PK turns into an anyone-can-spend empty script."
Scope: A small core change in compressor.h accompanied by unit test and fuzz test additions.
Reviewability
Paused · Waiting on author
Open objections from fjahr, l0rinc with no author reply for 7 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: respond to l0rinc's change request regarding unreadable coins handling in rollback snapshots and review suggestions 2026-09-29
- author: response to l0rinc's changes requested review and comments on unreadable coin handling in dumptxoutset/scantxoutset 2026-09-29
Agreement
Mild
objection that needs an answer has none yet (l0rinc)
Thread notes: Added fametrano's Tested ACK based on extensive test vector validation and regtest testing. / Added fametrano's Tested ACK based on extensive test vector analysis and regression testing. l0rinc's objection regarding dumptxoutset silently skipping unreadable coins remains open and awaits author response.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| fjahr (member) | suggestion, approach | Cover each type in the fuzz test so test coverage is not weakened. | open | answered | no | 2026-09-26: 'Would you mind covering each type here already? It doesn't seem to be that complex and otherwise the test is weaker than it needs to be.' |
| fjahr (member) | suggestion, approach | Avoid dropping the call to DecompressScript with different nSize. | accepted · reviewer dropped the point noting the coverage was not that important, and author further clarified coverage of raw scripts | answered | yes | 2026-09-26: 'I guess this coverage wasn't that interesting but this is dropping the call of `DecompressScript` with different `nSize`.' Status from: 2026-09-28: fjahr: 'as I said, not that important coverage I guess' |
| fjahr (member) | suggestion, approach | Run the check once instead of 1,000 times in the unit test, or move it to fuzz testing. | open | answered | no | 2026-09-26: 'Do we really need to check this 1000 times? If we do, then maybe this should rather be a fuzz test? For a unit test doing this once should suffice I think.' |
| l0rinc (member) | suggestion, approach | Extract off-curve and wrong-Y raw-storage assertions into a shared helper function. | open | answered | no | 2026-09-29: 'Seems to me the off-curve and wrong-Y raw-storage assertions are the same, so could we extract them into a helper?' |
| l0rinc (member) | suggestion, approach | Round-trip compression classes 2 and 3 as well for completeness. | open | answered | no | 2026-09-29: 'For completeness we could round-trip 2/3 as well' |
| l0rinc (member) | suggestion, interface | Include the compression class and a hex dump of the payload in the decompression failure exception message. | open | answered | no | 2026-09-29: 'For diagnostics we could include the compression class and a hex dump of the payload as well.' |
| l0rinc (member) | suggestion, approach | Simplify the decodability check in the fuzz test by using XOnlyPubKey. | open | answered | no | 2026-09-29: 'nit: I think we can simplify this a bit' |
| l0rinc (member) | suggestion, maintenance | Clarify comment to state that trailing data follows the script in undo records and snapshot files rather than Coin records. | open | answered | no | 2026-09-29: 'nit: the script is the last field of a `Coin`, so nothing follows it in a chainstate value. Undo records and snapshot files are where more data follows.' |
| l0rinc (member) | suggestion, approach | Add unit tests verifying that Coin and CTxUndo deserialization propagate the new script decompression failure. | open | answered | no | 2026-09-29: 'Given the concern about exceptions being caught along the way (hinted at in the PR description and discussed in several related PRs and issues recently), could we add simple higher-level checks that `' |
| l0rinc (member) | suggestion, approach | Cap raw-payload consumption in the script fuzz test to leave more fuzzed input for trailing checks. | open | answered | no | 2026-09-29: 'nit: could we cap raw-payload consumption to leave more fuzz input for later checks?' |
| l0rinc (member) | objection, correctness | dumptxoutset rollback snapshots skip unreadable coins on decompression failure and can succeed with coins missing. | open | answered | no | 2026-09-29: 'When `dumptxoutset` creates a rollback snapshot, the copy loop skips coins on read failure. The cursor catches the new decompression exception and returns false, so this PR adds another trigger for th' |
| l0rinc (member) | suggestion, interface | Return a UTXO read error in scantxoutset when reading a coin fails instead of returning partial results with success: false. | open | answered | no | 2026-09-29: '`scantxoutset` uses the same cursor reader, but a failed read currently looks like cancellation (`success: false` with partial results). Could we return the UTXO read error there as well?' |
| l0rinc (member) | suggestion, approach | Perform a round-trip comparison for special scripts in the fuzz test to ensure they re-serialize unchanged. | open | answered | no | 2026-09-29: 'Indeed, special-scripts were already checked to be decodable and should re-serialize to the same bytes, so a round-trip comparison would check that the script hasn't changed.' |
Support:
- sedited (member) (Approach ACK, 2026-09-16): (no reason given) [not substantive]
- fjahr (member) (Concept ACK, 2026-09-26): Looks already pretty solid but I left a few, mostly minor comments. [not substantive]
- purpleKarrot (member) (ACK, 2026-09-28) [read]: I see that all decoding errors are reported through exceptions, which is not great. But it means that the error handling in this PR is aligned.
- l0rinc (member) (Concept ACK, 2026-09-29) [read]: we have lots of similar findings that we should slowly plug, this seems like a good next step
- fametrano (contributor) (Tested ACK, 2026-10-25) [both: CTxUndo, Coin, DecompressScript, Unser, coins_tests, compress_tests, loadtxoutset, script fuzz target]: tested against 4636 vectors across bare script, Coin, and CTxUndo deserialization paths, and tested loadtxoutset on regtest with corrupted snapshot coins
Participants:
- sedited (member, support, 1 statements 2026-09-16 to 2026-09-16): Approach ACK
- fjahr (member, support, 7 statements 2026-09-26 to 2026-09-28): Concept ACK with review comments on test coverage
- purpleKarrot (member, support, 1 statements 2026-09-28 to 2026-09-28): ACK; error handling aligns with existing decoding exception handling
- l0rinc (member, objection, 10 statements 2026-09-29 to 2026-09-29): Concept ACK with changes requested on callers skipping unreadable coins and multiple test suggestions
- fametrano (contributor, support, 1 statements 2026-10-25 to 2026-10-25): Tested ACK with 4,636 test vectors, regtest snapshot tests, and unit and fuzz test runs
Files
95 lines under test/bench/ci.
- src/test/fuzz/script.cpp +43/-6
- src/test/compress_tests.cpp +46/-0
- src/compressor.h +7/-3
Card
The short text the ranking pass compares across PRs.
Throws an exception in ScriptCompression::Unser when DecompressScript fails instead of silently leaving the script empty. Previously, deserialization ignored decompression failure, which in corrupt database or stream states could turn an unspendable off-curve P2PK script into an anyone-can-spend empty script without alerting calling code. The PR also marks script compression and decompression functions as nodiscard and adds test coverage for unspendable off-curve pubkeys and invalid-Y coordinates. No dependencies are noted.
Data
thread read through 2026-10-25 (22 statements), head 5500521f8b
labels: Validation
Categories
Validation
P2 · bug fix (ranking pass: Moved up: corrupted compressed script silently becoming anyone-can-spend is a fund-safety-adjacent correctness hazard)
Fixes a silent failure during script decompression that could allow corrupted data to turn unspendable scripts into anyone-can-spend scripts. As noted in the description, `ScriptCompression::Unser` previously ignored the return value of `DecompressScript`, leaving the script empty on failure rather than raising an error.
Membership: Carries the Validation label and alters `src/compressor.h`, which handles UTXO script compression for chainstate storage.
Factors: security/stability 2, bug 2, performance 0, user value 0, leverage 1