validation: throw when a compressed script can't be decompressed #36280

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.

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.

<details> <summary>Require special records to re-encode unchanged</summary>

diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp
index 14ec0a10a3..40a74cb008 100644
--- a/src/test/fuzz/script.cpp
+++ b/src/test/fuzz/script.cpp
@@ -129,7 +129,7 @@ FUZZ_TARGET(script, .init = initialize_script)
             stream >> Using<ScriptCompression>(decompressed_script);
             assert(decodable);
             if (type < ScriptCompression::nSpecialScripts) {
-                assert(!decompressed_script.empty()); // future: check each type individually
+                assert(std::ranges::equal(DataStream{} << Using<ScriptCompression>(decompressed_script), DataStream{} << VARINT(type) << std::span{payload}));
             } else if (payload_size > MAX_SCRIPT_SIZE) {
                 assert(decompressed_script == CScript() << OP_RETURN);
             } else {

</details>

I guess this coverage wasn't that interesting but this is dropping the call of DecompressScript with different nSize.

nSize is type (the compression format) and is still provided by the fuzzer and tested against the code. Diff is that we are providing it inside the data stream we feed to ScriptCompression instead of a direct func arg.

Happy to know if you are seeing something else.

Sorry, my comment wasn't that clear, what has changed is that the fuzzer doesn't cover DecompressScript with nSize >5 anymore, but as I said, not that important coverage I guess.

ah, last one checked against is 6 (nSpecialScripts), which check the raw script storage path (any number over 5 takes the same path). This was on purpose because once we go over the last encoding format, we start consuming bytes from the script, which could make nSize to underflow and end up with a huge allocation. <-- not true, I need caffeine.

nit: sorting

nit: maybe use BOOST_REQUIRE instead of assert?

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.

Seems to me the off-curve and wrong-Y raw-storage assertions are the same, so could we extract them into a helper?

<details> <summary>Share P2PK raw-storage checks</summary>

diff --git a/src/test/compress_tests.cpp b/src/test/compress_tests.cpp
index eba72eb3be..533a2a49de 100644
--- a/src/test/compress_tests.cpp
+++ b/src/test/compress_tests.cpp
@@ -136,6 +136,16 @@ BOOST_AUTO_TEST_CASE(compress_script_to_uncompressed_pubkey_id)
     BOOST_CHECK_EQUAL(out[0], 0x04 | (script[65] & 0x01)); // least significant bit (lsb) of last char of pubkey is mapped into out[0]
 }
 
+// ScriptCompression must encode the script in serialized_size bytes and decode it back unchanged
+static void CheckScriptCompressionRoundTrip(const CScript& script, size_t serialized_size)
+{
+    DataStream s = DataStream{} << Using<ScriptCompression>(script);
+    BOOST_CHECK_EQUAL(s.size(), serialized_size);
+    CScript restored;
+    s >> Using<ScriptCompression>(restored);
+    BOOST_CHECK(restored == script);
+}
+
 BOOST_AUTO_TEST_CASE(compress_p2pk_scripts_not_on_curve)
 {
     XOnlyPubKey x_not_on_curve;
@@ -175,10 +185,7 @@ BOOST_AUTO_TEST_CASE(compress_p2pk_scripts_not_on_curve)
     }
 
     // Check off-curve script serialization round-trips properly
-    DataStream s = DataStream{} << Using<ScriptCompression>(script);
-    CScript restored;
-    s >> Using<ScriptCompression>(restored);
-    BOOST_CHECK(restored == script);
+    CheckScriptCompressionRoundTrip(script, 68);
 }
 
 BOOST_AUTO_TEST_CASE(compress_p2pk_wrong_y)
@@ -202,11 +209,7 @@ BOOST_AUTO_TEST_CASE(compress_p2pk_wrong_y)
 
         // Ensure serialize stores it raw (1-byte size prefix + 67, not 33 compressed)
         // and unserialize gives it back unchanged
-        DataStream s = DataStream{} << Using<ScriptCompression>(script);
-        BOOST_CHECK_EQUAL(s.size(), 68U);
-        CScript restored;
-        s >> Using<ScriptCompression>(restored);
-        BOOST_CHECK(restored == script);
+        CheckScriptCompressionRoundTrip(script, 68);
     }
 }
 

</details>

For completeness we could round-trip 2/3 as well

<details> <summary>Cover off-curve keys in compression classes 2 and 3</summary>

diff --git a/src/test/compress_tests.cpp b/src/test/compress_tests.cpp
index 533a2a49de..35594d2480 100644
--- a/src/test/compress_tests.cpp
+++ b/src/test/compress_tests.cpp
@@ -186,6 +186,11 @@ BOOST_AUTO_TEST_CASE(compress_p2pk_scripts_not_on_curve)
 
     // Check off-curve script serialization round-trips properly
     CheckScriptCompressionRoundTrip(script, 68);
+
+    // Classes 2/3 preserve off-curve keys verbatim and remain valid stored records
+    for (const CPubKey& compressed_pubkey : x_not_on_curve.GetCPubKeys()) {
+        CheckScriptCompressionRoundTrip(CScript() << compressed_pubkey << OP_CHECKSIG, 33);
+    }
 }
 
 BOOST_AUTO_TEST_CASE(compress_p2pk_wrong_y)

</details>

For diagnostics we could include the compression class and a hex dump of the payload as well.

<details> <summary>Include the compression class and payload in decode errors</summary>

diff --git a/src/compressor.h b/src/compressor.h
index d252c0b712..acd25aa2e8 100644
--- a/src/compressor.h
+++ b/src/compressor.h
@@ -6,11 +6,13 @@
 #ifndef BITCOIN_COMPRESSOR_H
 #define BITCOIN_COMPRESSOR_H
 
+#include <crypto/hex_base.h>
 #include <prevector.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
 #include <serialize.h>
 #include <span.h>
+#include <tinyformat.h>
 
 #include <ios>
 
@@ -83,7 +85,7 @@ struct ScriptCompression
             CompressedScript vch(GetSpecialScriptSize(nSize), 0x00);
             s >> std::span{vch};
             if (!DecompressScript(script, nSize, vch)) {
-                throw std::ios_base::failure("Non-decodable compressed script");
+                throw std::ios_base::failure(strprintf("Non-decodable compressed script (type %u, data %s)", nSize, HexStr(vch)));
             }
             return;
         }

</details>

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 that issue. Could we abort the copy on a failed read, either here or in a separate follow-up? Otherwise, dumptxoutset can succeed with a coin missing.

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? The existing corruption test can cover both RPCs before triggering the validation abort.

<details> <summary>Report unreadable coins in rollback exports and UTXO scans</summary>

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index ae75072462..27223dda57 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2258,7 +2258,7 @@ bool FindScriptPubKey(std::atomic<int>& scan_progress, const std::atomic<bool>&
     while (cursor->Valid()) {
         COutPoint key;
         Coin coin;
-        if (!cursor->GetKey(key) || !cursor->GetValue(coin)) return false;
+        if (!cursor->GetKey(key) || !cursor->GetValue(coin)) throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");
         if (++count % 8192 == 0) {
             interruption_point();
             if (should_abort) {
@@ -3304,6 +3304,8 @@ UniValue CreateRolledBackUTXOSnapshot(
                 if (coins_count % 100'000 == 0) {
                     temp_cache.Flush();
                 }
+            } else {
+                throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");
             }
             cursor->Next();
         }
diff --git a/test/functional/feature_utxo_abort_on_error.py b/test/functional/feature_utxo_abort_on_error.py
index 5e69333c13..dbfcd0e262 100755
--- a/test/functional/feature_utxo_abort_on_error.py
+++ b/test/functional/feature_utxo_abort_on_error.py
@@ -22,7 +22,7 @@ except ImportError:
 
 from test_framework.blocktools import COINBASE_MATURITY
 from test_framework.test_framework import BitcoinTestFramework, SkipTest
-from test_framework.util import assert_equal
+from test_framework.util import assert_equal, assert_raises_rpc_error
 from test_framework.wallet import MiniWallet
 
 
@@ -87,6 +87,12 @@ class UTXOAbortOnErrorTest(BitcoinTestFramework):
         # touches them. The node starts cleanly regardless of the tampered entry.
         assert_equal(node0.getblockcount(), COINBASE_MATURITY + 1)
 
+        self.log.info("Rollback export must fail when it cannot read a coin")
+        assert_raises_rpc_error(-32603, "Unable to read UTXO set", node0.dumptxoutset, "rollback.dat", rollback=COINBASE_MATURITY)
+
+        self.log.info("UTXO set scan must fail when it cannot read a coin")
+        assert_raises_rpc_error(-32603, "Unable to read UTXO set", node0.scantxoutset, "start", ["raw(51)"])
+
         # Now spend the corrupted UTXO
         self.log.info("node1 broadcasts the spend and mines a block")
         node1.sendrawtransaction(spend_tx_hex)

</details>

nit: I think we can simplify this a bit

<details> <summary>Use XOnlyPubKey as the decodability oracle</summary>

diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp
index 3adf140b78..6cc391c9d6 100644
--- a/src/test/fuzz/script.cpp
+++ b/src/test/fuzz/script.cpp
@@ -117,13 +117,8 @@ FUZZ_TARGET(script, .init = initialize_script)
         std::vector<uint8_t> payload = ConsumeFixedLengthByteVector(fuzzed_data_provider, std::min(payload_size, 32U));
         payload.resize(payload_size);
         const std::vector<std::byte> trailing = ConsumeRandomLengthByteVector<std::byte>(fuzzed_data_provider);
-        // Decodable unless it is a pubkey record (type 4 or 5) whose X coordinate is not a valid compressed pubkey.
-        bool decodable{true};
-        if (type == 4 || type == 5) {
-            std::vector<unsigned char> pubkey{static_cast<unsigned char>(type - 2)};
-            pubkey.insert(pubkey.end(), payload.begin(), payload.end());
-            decodable = CPubKey{pubkey}.IsFullyValid();
-        }
+        // Decodable unless it is a pubkey record (type 4 or 5) whose X coordinate is not on the curve.
+        const bool decodable{(type != 4 && type != 5) || XOnlyPubKey{payload}.IsFullyValid()};
 
         DataStream stream = DataStream{} << VARINT(type) << std::span{payload} << std::span{trailing};
         try {

</details>

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.

<details> <summary>Correct where trailing data follows the script</summary>

diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp
index 2db84f6ef3..a6929d3a8f 100644
--- a/src/test/fuzz/script.cpp
+++ b/src/test/fuzz/script.cpp
@@ -95,8 +95,7 @@ FUZZ_TARGET(script, .init = initialize_script)
 
     {
         // Exercises ScriptCompression::Unser path.
-        // First, this always runs inside a Coin record, so more data follows the script
-        // in the stream and it must not be consumed.
+        // Undo records and snapshot files store more data after the script, which must not be consumed.
         //
         // A compressed script starts with its type: one of six special encodings (0 to 5) with
         // a fixed-size payload, or a raw script length plus nSpecialScripts. A special script

</details>

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 Coin and CTxUndo deserialization propagate this new failure?

<details> <summary>Check failure propagation through Coin and undo deserialization</summary>

diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index d2bd053d55..064e0bddc9 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -543,6 +543,10 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
     // scriptPubKey that ends beyond the end of the stream
     BOOST_CHECK_EXCEPTION(SpanReader{"000007"_hex} >> Coin{}, std::ios_base::failure, HasReason{"end of data"});

+    // Coin and undo deserialization must propagate the script decompression failure
+    BOOST_CHECK_EXCEPTION(SpanReader{"000004ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"_hex} >> Coin{}, std::ios_base::failure, HasReason{"Non-decodable compressed script"});
+    BOOST_CHECK_EXCEPTION(SpanReader{"01000004ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"_hex} >> CTxUndo{}, std::ios_base::failure, HasReason{"Non-decodable compressed script"});
+
     // Very large scriptPubKey (3*10^9 bytes) past the end of the stream
     DataStream tmp{};
     uint64_t x = 3000000000ULL;

</details>

nit: could we cap raw-payload consumption to leave more fuzz input for later checks? The fuzzer might already handle this, but I would expect it to have an easier time this way...

<details> <summary>Limit payload consumption while keeping varied bytes</summary>

diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp
index 40a74cb008..3adf140b78 100644
--- a/src/test/fuzz/script.cpp
+++ b/src/test/fuzz/script.cpp
@@ -113,7 +113,9 @@ FUZZ_TARGET(script, .init = initialize_script)
             type = payload_size + ScriptCompression::nSpecialScripts;
         }
 
-        const std::vector<uint8_t> payload = ConsumeFixedLengthByteVector(fuzzed_data_provider, payload_size);
+        // Keep varied payload bytes without letting long raw records drain the input
+        std::vector<uint8_t> payload = ConsumeFixedLengthByteVector(fuzzed_data_provider, std::min(payload_size, 32U));
+        payload.resize(payload_size);
         const std::vector<std::byte> trailing = ConsumeRandomLengthByteVector<std::byte>(fuzzed_data_provider);
         // Decodable unless it is a pubkey record (type 4 or 5) whose X coordinate is not a valid compressed pubkey.
         bool decodable{true};

</details>