compressor: oversized script decoding can leave a spendable script #36386

l0rinc commented at 1:25 AM on September 30, 2026: contributor

Problem: The oversized-script fallback appends OP_RETURN to the destination instead of replacing it. If the destination already contains an unfinished push, the appended byte becomes pushed data and the result can remain spendable.

Fix: Replace the destination with OP_RETURN rather than appending it.

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.

DrahtBot commented at 1:25 AM on September 30, 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/36386
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36386.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36386#author-ralph

Reviews

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

Type	Reviewers
ACK	fametrano, hodlinator, maflcko, optout21
Stale ACK	jcastros

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36280 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36280.svg"> (validation: throw when a compressed script can't be decompressed by furszy)
#36074 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36074.svg"> (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)
#35511 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35511.svg"> (RFC: consensus: Make CAmount a class by hodlinator)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

l0rinc force-pushed on Sep 30, 2026

DrahtBot added the label CI failed on Sep 30, 2026

DrahtBot removed the label CI failed on Sep 30, 2026

maflcko commented at 7:58 AM on September 30, 2026: member

it could affect malformed stored records decoded into a reused destination.

Not sure, reusing a destination would be an unrelated bug? So 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.

lgtm either way.

l0rinc force-pushed on Sep 30, 2026

l0rinc commented at 6:12 PM on September 30, 2026: contributor

Not sure, reusing a destination would be an unrelated bug?

We do have some existing reuses, e.g. in https://github.com/bitcoin/bitcoin/blob/e7aef7e86da79000aa42f5ba6d13b0d123d9da7c/src/rpc/blockchain.cpp#L3435-L3462 the snapshot export reuses the same Coin in the loop. Regardless, added a comment clarifying that this tests the fallback directly, since normal UTXO insertion excludes oversized scripts.

jcastros commented at 3:27 PM on October 5, 2026: contributor

Tested ACK f313faff443e5cdbd858a54c6bb939799d7ce714

I built test_bitcoin (GCC 13.3, RelWithDebInfo) and ran compress_tests on this and on 11494efb01d0. Both are clean. Putting script << OP_RETURN back makes compress_oversized_script fail, so the new check covers the fix.

in src/test/compress_tests.cpp:176 in f313faff44

 168 | @@ -164,4 +169,12 @@ BOOST_AUTO_TEST_CASE(compress_p2pk_scripts_not_on_curve)
 169 |      }
 170 |  }
 171 |  
 172 | +BOOST_AUTO_TEST_CASE(compress_oversized_script)
 173 | +{
 174 | +    // Test the fallback directly, since normal UTXO insertion excludes oversized scripts
 175 | +    CScript restored{ParseScript("0x01")}; // Incomplete push: appending OP_RETURN would make it pushed data
 176 | +    (DataStream{} << Using<ScriptCompression>(CScript{CScript() << std::vector(MAX_SCRIPT_SIZE + 1, uint8_t{0})})) >> Using<ScriptCompression>(restored);

hodlinator commented at 9:36 AM on October 14, 2026:

This has a bit too much happening at once IMO (+ redundant CScript-ctor-call). Would rather have something closer to

    DataStream oversized_compressed{DataStream{} << Using<ScriptCompression>(CScript{} << std::vector(MAX_SCRIPT_SIZE + 1, uint8_t{0}))};
    oversized_compressed >> Using<ScriptCompression>(restored);

hodlinator commented at 9:39 AM on October 14, 2026:

nit: Could verify that restored starts out spendable before the compression round-trip makes it unspendable?

    CScript restored{ParseScript("0x01")}; // Incomplete push: appending OP_RETURN would make it pushed data
    BOOST_CHECK(!restored.IsUnspendable());
    (DataStream{} << Using<ScriptCompression>(CScript{CScript() << std::vector(MAX_SCRIPT_SIZE + 1, uint8_t{0})})) >> Using<ScriptCompression>(restored);

in src/test/compress_tests.cpp:177 in f313faff44 outdated

 168 | @@ -164,4 +169,12 @@ BOOST_AUTO_TEST_CASE(compress_p2pk_scripts_not_on_curve)
 169 |      }
 170 |  }
 171 |  
 172 | +BOOST_AUTO_TEST_CASE(compress_oversized_script)
 173 | +{
 174 | +    // Test the fallback directly, since normal UTXO insertion excludes oversized scripts
 175 | +    CScript restored{ParseScript("0x01")}; // Incomplete push: appending OP_RETURN would make it pushed data
 176 | +    (DataStream{} << Using<ScriptCompression>(CScript{CScript() << std::vector(MAX_SCRIPT_SIZE + 1, uint8_t{0})})) >> Using<ScriptCompression>(restored);
 177 | +    BOOST_CHECK(restored.IsUnspendable());

hodlinator commented at 9:43 AM on October 14, 2026:

Ralph suggested also pinning down the resulting script (https://git.fish.foo/bitcoin/bitcoin/pulls/36386#issuecomment-608275), which seems fair enough.

Guess it could be done through:

    BOOST_CHECK(restored.IsUnspendable());
    const CScript expected{OP_RETURN};
    BOOST_CHECK_EQUAL_COLLECTIONS(restored.begin(), restored.end(), expected.begin(), expected.end());

hodlinator commented at 9:43 AM on October 14, 2026: contributor

Concept ACK f313faff443e5cdbd858a54c6bb939799d7ce714

test: characterize oversized script decoding into a reused target

Decode an oversized script into a target containing an incomplete push.
Record that the resulting script fails IsUnspendable().
The following fix requires the decoded script to be unspendable.

1c8ba894ed

compressor: replace oversized scripts instead of appending

The oversized-script fallback appends OP_RETURN to the destination rather than replacing it as documented.
A reused target retains its previous contents, and an incomplete push can consume OP_RETURN as data.
Assign a new OP_RETURN script before skipping the oversized payload so the result is independent of the destination's prior contents.

426f29c3f1

l0rinc force-pushed on Oct 15, 2026

fametrano commented at 11:19 AM on October 25, 2026: none

tACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd

I decoded 1307 compressed scripts (raw sizes 0 to 39 and from just under MAX_SCRIPT_SIZE to 200000, truncated streams, every special type) into a fresh target and five reused ones, a P2PKH among them, and compared each result with what a library I maintain decodes from the same bytes. On the base, every complete oversized script decoded into a reused target keeps the old bytes and appends OP_RETURN (35 decodes). On this head every raw and oversized case matches for every target.

One path still depends on the target: an uncompressed P2PK (type 4 or 5) whose x is not on the curve leaves a reused target untouched, so a P2PKH stays a P2PKH. #36280 makes that path throw. Its compressor.h change merges cleanly with this one; only compress_tests.cpp conflicts. With both applied, no decode in the set depends on what the target held before. I can share the vectors and the test.

DrahtBot requested review from hodlinator on Oct 25, 2026

in src/test/compress_tests.cpp:175 in 426f29c3f1

 168 | @@ -164,4 +169,15 @@ BOOST_AUTO_TEST_CASE(compress_p2pk_scripts_not_on_curve)
 169 |      }
 170 |  }
 171 |  
 172 | +BOOST_AUTO_TEST_CASE(compress_oversized_script)
 173 | +{
 174 | +    // Test the fallback directly, since normal UTXO insertion excludes oversized scripts
 175 | +    CScript restored{ParseScript("0x01")}; // Incomplete push: appending OP_RETURN would make it pushed data

hodlinator commented at 10:35 AM on October 26, 2026:

nit: Could compute at compile time:

--- a/src/test/compress_tests.cpp
+++ b/src/test/compress_tests.cpp
@@ -28,6 +28,8 @@
 // amounts 50 .. 21000000
 #define NUM_MULTIPLES_50BTC 420000

+using namespace util::hex_literals;
+
 BOOST_FIXTURE_TEST_SUITE(compress_tests, BasicTestingSetup)

 bool static TestEncode(uint64_t in) {
@@ -172,7 +174,7 @@ BOOST_AUTO_TEST_CASE(compress_p2pk_scripts_not_on_curve)
 BOOST_AUTO_TEST_CASE(compress_oversized_script)
 {
     // Test the fallback directly, since normal UTXO insertion excludes oversized scripts
-    CScript restored{ParseScript("0x01")}; // Incomplete push: appending OP_RETURN would make it pushed data
+    auto restored{CScript{} << "01"_hex}; // Incomplete push: appending OP_RETURN would make it pushed data
     BOOST_CHECK(!restored.IsUnspendable());
     DataStream oversized_compressed{DataStream{} << Using<ScriptCompression>(CScript{} << std::vector(MAX_SCRIPT_SIZE + 1, uint8_t{0}))};
     oversized_compressed >> Using<ScriptCompression>(restored);

hodlinator approved

hodlinator commented at 10:45 AM on October 26, 2026: contributor

ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd

maflcko commented at 11:34 AM on October 27, 2026: member

review ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd 🏉

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd 🏉
ZMFrq8y3VWJkvdmEHZqxUEXnr07PVirQi5JeVZZ0cI9PWixMNhtsYF/RYb7eALrDfdaoDa4OPwLQP3GmubegDg==

</details>

in src/compressor.h:83 in 426f29c3f1

optout21 commented at 1:57 PM on October 27, 2026:

426f29c compressor: replace oversized scripts instead of appending:

The commit message states: "so the result is independent of the destination's prior contents". However, this is not always the case. In this branch, DecompressScript could fail in such a way that it does not modify script (in the 0x04/0x05 prefix + error case). Either adjust this codepath as well (reset it in case of error), or soften the claim.

optout21 commented at 2:20 PM on October 27, 2026:

I've just realized that #36280 addresses exactly this, so I think it's fine as it is in this PR.

in src/compressor.h:89 in 426f29c3f1

  85 | @@ -86,7 +86,7 @@ struct ScriptCompression
  86 |          nSize -= nSpecialScripts;
  87 |          if (nSize > MAX_SCRIPT_SIZE) {
  88 |              // Overly long script, replace with a short invalid one
  89 | -            script << OP_RETURN;
  90 | +            script = CScript{OP_RETURN};

optout21 commented at 2:15 PM on October 27, 2026:

426f29c compressor: replace oversized scripts instead of appending:

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).

optout21 commented at 2:16 PM on October 27, 2026: contributor

Reviewed, left some comments. Happy to re-check/ack once answered. (426f29c3f1639e4c97be63cdf4c2e76bb86b39dd)

optout21 commented at 2:40 PM on October 27, 2026: contributor

ACK 426f29c3f1639e4c97be63cdf4c2e76bb86b39dd Upgraded my previous review result, upon reconsideration. This change is a net improvement, independent of #36280 . I'd appreciate answers to my comments, nonetheless.