#36386 compressor: oversized script decoding can leave a spendable script
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36386 compressor: oversized script decoding can leave a spendable script | Ready | 4 (+1) | Strong | 1 + 16 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
Replaces the destination script with a fresh OP_RETURN script instead of appending to it when decompressing an oversized script. A unit test is added to verify that decompressing an oversized script into a reused destination containing an incomplete push produces an unspendable script.
Problem: When decoding an oversized script into a reused script buffer, appending OP_RETURN with operator<< retains prior buffer contents. If the buffer has an incomplete push, the OP_RETURN byte is treated as push data instead of an unspendable opcode, potentially leaving the script spendable.
Evidence of importance:
- PR description: 'Found while reviewing #36280. This predates that PR and appears latent because oversized outputs are excluded from the UTXO set, but it could affect malformed stored records decoded into a reused destination.'
Risk of a mistake: low (validation). Touches UTXO script decompression logic, but oversized scripts are prevented from entering the UTXO database by standard validation rules and the change strictly enforces unspendability.
Changed since the previous assessment: No functional changes to the fix or test; commits appear reorganized to separate the test characterization from the implementation fix.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: answer optout21's review comments 2026-10-27
- author: consider hodlinator's compile-time hex literal nit 2026-10-26
Agreement
Strong
substantive support, no open objection (jcastros, fametrano, optout21)
Thread notes: Added ACKs from maflcko and optout21. Recorded optout21's review comments regarding destination independence in DecompressScript, with the question on clearing script open and the codepath adjustment suggestion dropped.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko (member) | suggestion, approach | Drop the test or clarify in a comment that it does not test real production behavior | answered · fix pushed · author pointed to existing destination reuse in snapshot export and added clarifying comment to the test | answered | yes | 2026-09-30: 'I am half suggesting to drop the test in a third commit again, or at least clarify in the comment that this isn't supposed to test any real production behavior.' Status from: 2026-09-30: l0rinc: 'Regardless, added a comment clarifying that this tests the fallback directly, since normal UTXO insertion excludes overs' |
| hodlinator (member) | suggestion, style | Simplify the test stream expression and avoid the redundant CScript constructor call | accepted · dropped in approving review r:5650692444 | answered | no | 2026-10-14: 'This has a bit too much happening at once IMO (+ redundant `CScript`-ctor-call). Would rather have something closer to' Status from: 2026-10-26: hodlinator: 'ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd' |
| hodlinator (member) | suggestion, approach | Verify restored starts out spendable before the compression round-trip makes it unspendable | accepted · fix pushed · implemented in push and accepted in approving review r:5650692444 | answered | no | 2026-10-14: 'nit: Could verify that `restored` starts out spendable before the compression round-trip makes it unspendable?' Status from: 2026-10-26: hodlinator: 'ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd' |
| hodlinator (member) | suggestion, approach | Pin down the exact resulting script in the test with BOOST_CHECK_EQUAL_COLLECTIONS | accepted · dropped in approving review r:5650692444 | answered | no | 2026-10-14: 'Ralph suggested also pinning down the resulting script (https://git.fish.foo/bitcoin/bitcoin/pulls/36386#issuecomment-608275), which seems fair enough.' Status from: 2026-10-26: hodlinator: 'ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd' |
| hodlinator (member) | suggestion, style | Compute the incomplete push script at compile time using hex literal | open | answered | no | 2026-10-26: 'nit: Could compute at compile time' |
| optout21 (contributor) | suggestion, scope | Adjust the 0x04/0x05 error codepath in DecompressScript or soften the commit message claim that the result is independent of prior contents | accepted · dropped in rc:4389807813 upon realizing #36280 addresses it, though c:6444909520 noted answers are still appreciated | answered | no | 2026-10-27: 'Either adjust this codepath as well (reset it in case of error), or soften the claim.' Status from: 2026-10-27: optout21: 'I've just realized that #36280 addresses exactly this, so I think it's fine as it is in this PR.' |
| optout21 (contributor) | question, approach | Whether the script output variable could be cleared at the beginning of DecompressScript | open · unanswered; optout21 noted in c:6444909520 that answers to comments would still be appreciated | answered | no | 2026-10-27: 'Would it be possible to clear the `script` output variable at the beginning of this method? That would ensure the claim (independence of previous value).' |
Support:
- maflcko (member) (2026-09-30): lgtm either way [not substantive]
- jcastros (contributor) (Tested ACK, 2026-10-05) [tested]: built test_bitcoin, ran compress_tests on both commits, and verified reverting the change causes compress_oversized_script to fail
- hodlinator (member) (Concept ACK, 2026-10-14): Concept ACK f313faff443e5cdbd858a54c6bb939799d7ce714 [not substantive]
- fametrano (none) (Tested ACK, 2026-10-25) [tested]: decoded 1307 compressed scripts into fresh and reused targets against an independent library and verified all raw and oversized decodes match
- hodlinator (member) (ACK, 2026-10-26): ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd [not substantive]
- maflcko (member) (ACK, 2026-10-27): review ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd [not substantive]
- optout21 (contributor) (ACK, 2026-10-27) [read]: This change is a net improvement, independent of #36280
Participants:
- maflcko (member, support, 2 statements 2026-09-30 to 2026-10-27): ACK; previously suggested dropping test or clarifying destination reuse comment
- jcastros (contributor, support, 1 statements 2026-10-05 to 2026-10-05): Tested ACK; verified compress_tests pass and fail when reverting fix
- hodlinator (member, support, 6 statements 2026-10-14 to 2026-10-26): ACK; suggested compile-time hex literal nit after earlier suggestions were addressed or dropped
- fametrano (none, support, 1 statements 2026-10-25 to 2026-10-25): Tested ACK; tested differential decoding of 1307 compressed scripts across fresh and reused targets
- optout21 (contributor, support, 5 statements 2026-10-27 to 2026-10-27): ACK; asked whether script output variable could be cleared at the beginning of DecompressScript
Files
16 lines under test/bench/ci.
- src/test/compress_tests.cpp +16/-0
- src/compressor.h +1/-1
Card
The short text the ranking pass compares across PRs.
Fixes a latent bug in script decompression where handling an oversized script appends OP_RETURN to the destination buffer rather than replacing it. If the destination buffer is reused and contains an unfinished data push, the appended opcode is treated as push data rather than an unspendable opcode, leaving the script spendable. The author notes that oversized scripts are excluded from the UTXO set during validation, making the issue latent unless malformed records are decoded into reused buffers.
Data
thread read through 2026-10-27 (16 statements), head 426f29c3f1
labels: none