fix(utxo): a corrupt record could panic the amount decoder

`decompress_amount` ended in a `while` loop multiplying by ten up to nine
times, with no bound on its input. `read_varint` hands it whatever a record
contains, and `validate_encoded` runs it over every output of every record
loaded from a snapshot, so a file on disk could reach it with an arbitrary
`u64`. `decompress_amount(u64::MAX)` is 2.05e22 — a panic in a debug build, a
silent wrap in a release one.

Reproduced before fixing:
`an_absurd_compressed_amount_is_rejected_rather_than_overflowing` failed with
`attempt to multiply with overflow` from `compress.rs:193`.

The function now returns `Option` and requires the decompressed value back
inside the compressible domain. That also closes a canonicality hole that was
open until now: the compact form may encode only amounts the escape refuses,
and the escape refuses exactly the amounts the compact form covers, so every
amount has one spelling and no other. `decompress_accepts_exactly_the_encoder_image`
states it as a property over every `u64` — whatever the decoder accepts must
round-trip back to the same compressed value through the encoder, which pins
the accepted set to the encoder's image rather than merely checking that
nothing panics.

Found while investigating the fixed ~12 ns `find_output` cost, which is what
the same loop was suspected of. Replacing it with a power-of-ten lookup does
help, but only 12.5 ns -> 11.3 ns (1.1x): the remaining fixed cost is the
directory read and building the returned view, not the arithmetic. Quoted that
way in the doc rather than as a speed win, and the crossover table is
re-measured.

The `miss` arms are unchanged at every record size, which is the control this
wants: that path never decompresses an amount, so a change to the amount
transform must not move it.

Author: rabbitson87

crates/utxo/proptest-regressions/compress.txt

@@ -0,0 +1,8 @@
+# Seeds for failure cases proptest has generated in the past. It is
+# automatically read and these particular cases re-run before any
+# novel cases are generated.
+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc 4e2ac50ef64e8005fb57c4087442a76f3afce04d99bd88960085967f87b78607 # shrinks to value = 2049638230412201671
+cc b5d65560bbc086aba36aa4e4932cc62072e7c017b83c25612b4ade02a28ad9a7 # shrinks to a = 0, b = 2049672486957746581

crates/utxo/src/compress.rs

@@ -164,36 +164,59 @@ thread_local! {
     pub(crate) static DECOMPRESS_CALLS: core::cell::Cell<usize> = const { core::cell::Cell::new(0) };
 }
 
-/// Inverse of [`compress_amount`].
+/// The powers of ten the transform can factor out, indexed by exponent.
+///
+/// Replaces a `while` loop of up to nine dependent multiplies. Decoding an
+/// amount is on the record read path, and that loop was most of its fixed cost.
+const POW10: [u64; 10] = [
+    1,
+    10,
+    100,
+    1_000,
+    10_000,
+    100_000,
+    1_000_000,
+    10_000_000,
+    100_000_000,
+    1_000_000_000,
+];
+
+/// Inverse of [`compress_amount`], or `None` when `compressed` is not something
+/// [`compress_amount`] could have produced.
+///
+/// The rejection is not defensive tidiness. `read_varint` will hand this any
+/// `u64` a corrupt or hostile record contains, and the transform multiplies by
+/// up to 10^9: `decompress_amount(u64::MAX)` is 2.05e22, which **panics in a
+/// debug build** and wraps silently in a release one. `validate_encoded`
+/// decodes every output of every record loaded from a snapshot, so that path is
+/// reachable from a file on disk.
+///
+/// Requiring the result back inside the compressible domain also completes the
+/// canonicality rule: the compact form may encode only amounts the escape
+/// refuses, and the escape refuses exactly the amounts the compact form
+/// covers. Together they leave each amount exactly one spelling.
 #[inline]
-pub(crate) fn decompress_amount(compressed: u64) -> u64 {
+pub(crate) fn decompress_amount(compressed: u64) -> Option<u64> {
     #[cfg(test)]
     DECOMPRESS_CALLS.with(|calls| calls.set(calls.get() + 1));
-    decompress_amount_inner(compressed)
-}
 
-#[inline]
-const fn decompress_amount_inner(compressed: u64) -> u64 {
     if compressed == 0 {
-        return 0;
+        return Some(0);
     }
     let x = compressed - 1;
     let exponent = x % 10;
     let mut n = x / 10;
     if exponent < 9 {
         let last_digit = n % 9;
         n /= 9;
-        n = n * 10 + last_digit + 1;
+        n = n.checked_mul(10)?.checked_add(last_digit + 1)?;
     } else {
-        n += 1;
+        n = n.checked_add(1)?;
     }
-    let mut result = n;
-    let mut remaining = exponent;
-    while remaining > 0 {
-        result *= 10;
-        remaining -= 1;
-    }
-    result
+    // `exponent` is `x % 10`, so the lookup is in range by construction.
+    let scale = POW10.get(usize::try_from(exponent).ok()?)?;
+    let value = n.checked_mul(*scale)?;
+    (value <= MAX_COMPRESSIBLE_AMOUNT).then_some(value)
 }
 
 #[cfg(test)]
@@ -324,7 +347,7 @@ mod tests {
             let compressed = compress_amount(value).expect("within the money supply");
             assert_eq!(
                 decompress_amount(compressed),
-                value,
+                Some(value),
                 "amount round trip failed for {value}"
             );
         }
@@ -362,7 +385,23 @@ mod tests {
         #[test]
         fn compressed_amounts_round_trip(value in 0..=MAX_COMPRESSIBLE_AMOUNT) {
             let compressed = compress_amount(value).expect("in range");
-            prop_assert_eq!(decompress_amount(compressed), value);
+            prop_assert_eq!(decompress_amount(compressed), Some(value));
+        }
+
+        /// No `u64` may panic the decoder, and every value it accepts must be
+        /// one the encoder could have produced.
+        ///
+        /// `read_varint` hands this whatever a corrupt or hostile record
+        /// contains, and `validate_encoded` runs it over every output of every
+        /// record loaded from a snapshot. The second half is the canonicality
+        /// rule: if some compressed value outside the encoder's image were
+        /// accepted, one amount would have two spellings.
+        #[test]
+        fn decompress_accepts_exactly_the_encoder_image(compressed in any::<u64>()) {
+            if let Some(value) = decompress_amount(compressed) {
+                prop_assert!(value <= MAX_COMPRESSIBLE_AMOUNT);
+                prop_assert_eq!(compress_amount(value).ok(), Some(compressed));
+            }
         }
 
         /// The compression must be injective over the range it will ever see,

crates/utxo/src/record.rs

@@ -1472,7 +1472,10 @@ fn decode_output_at<'a>(
         }
         (value, end)
     } else {
-        (crate::compress::decompress_amount(amount), cursor)
+        (
+            crate::compress::decompress_amount(amount).ok_or(UtxoError::CorruptRecord)?,
+            cursor,
+        )
     };
 
     let (packed, cursor) = crate::compress::read_varint(body, cursor)?;
@@ -2013,6 +2016,31 @@ mod tests {
         Ok(())
     }
 
+    /// A corrupt record must not be able to panic the decoder.
+    #[test]
+    fn an_absurd_compressed_amount_is_rejected_rather_than_overflowing() -> Result<(), UtxoError> {
+        // `varint(u64::MAX - 1)`: ten bytes, and not the escape sentinel, so it
+        // reaches the amount transform.
+        let mut payload = vec![0xFE_u8];
+        payload.extend_from_slice(&[0xFF; 8]);
+        payload.push(0x01);
+        payload.extend_from_slice(&[0x02, 0x51, 0xAC]);
+
+        let mut bytes = Hash256::default().to_le_bytes().to_vec();
+        bytes.extend_from_slice(&1_u32.to_le_bytes());
+        bytes.push(1);
+        bytes.push(0x11);
+        bytes.push(0x00);
+        bytes.push(u8::try_from(payload.len()).unwrap_or(0));
+        bytes.extend_from_slice(&payload);
+
+        assert!(matches!(
+            UtxoRecord::from_encoded(ThinRecordBuf::from_slice(&bytes)?),
+            Err(UtxoError::CorruptRecord)
+        ));
+        Ok(())
+    }
+
     /// v5 has no invalid bool byte — `coinbase` is one bit of a varint, so
     /// every value is meaningful. What it has instead is several ways to spell
     /// one output, and all of them must be refused: `UtxoRecord` compares by

docs/benchmarks/utxo-memory.md

@@ -151,18 +151,24 @@ of about 3 ns on a typical record and a lookup *win* on a fat one.**
 | outputs | `miss` | | | `hit_last` | | |
 |---|---:|---:|---:|---:|---:|---:|
 | | v4 | v5 | ratio | v4 | v5 | ratio |
-| 1 | 1.7 ns | 3.6 ns | 0.48x | 2.1 ns | 12.5 ns | 0.16x |
-| **3.626 (measured mainnet average)** | ~4 ns | ~7 ns | **~0.57x** | 3.7 ns | 18.4 ns | 0.20x |
-| 16 | 16.6 ns | 20.6 ns | 0.80x | 16.8 ns | 40.6 ns | 0.41x |
-| 64 | 109.3 ns | 79.5 ns | **1.37x** | 126.3 ns | 136.6 ns | 0.92x |
-| 256 | 471.8 ns | 298.7 ns | **1.58x** | 481.6 ns | 499.7 ns | 0.96x |
+| 1 | 2.6 ns | 7.6 ns | 0.35x | 2.5 ns | 15.6 ns | 0.16x |
+| **3.626 (measured mainnet average)** | 3.9 ns | 6.9 ns | 0.57x | 3.6 ns | 17.1 ns | 0.21x |
+| 16 | 16.6 ns | 20.7 ns | 0.80x | 17.1 ns | 39.7 ns | 0.43x |
+| 64 | 108.4 ns | 78.7 ns | 1.38x | 126.8 ns | 135.4 ns | 0.94x |
+| 256 | 462.1 ns | 294.1 ns | 1.57x | 476.7 ns | 492.5 ns | 0.97x |
 
-v5 pays a fixed ~10 ns to read the directory header and decode the matched
+v5 pays a fixed ~11 ns to read the directory header and decode the matched
 payload, and then scans at a fraction of v4's per-output cost. Below roughly 64
 outputs the fixed cost dominates; above it the scan does. `hit_first` never
 crosses, because v4's best case is a single constant-offset read the optimizer
 reduces to almost nothing.
 
+Replacing the amount transform's `while` loop of up to nine dependent multiplies
+with a power-of-ten lookup took that fixed cost from 12.5 ns to 11.3 ns — 1.1x,
+real but small. It was worth doing for a different reason (see below); the
+remaining fixed cost is the directory read and building the returned view, not
+the arithmetic.
+
 **In absolute terms the loss is 3 ns per lookup at the mainnet average.** At
 ~4,000 spent inputs per block that is 12 µs against a 50 ms commit budget —
 0.02% — bought with 21.7% of the record payload. The win concentrates on batch
@@ -233,6 +239,31 @@ The directories fix exactly that: a lookup scans one dense fixed-width array and
 sums a second, touching ~2 bytes per output instead of ~35. It is why `get_miss`
 ends up **2.3x faster than v4**, not merely level with it.
 
+### A corrupt record could panic the decoder
+
+Found while looking at why `find_output` has a fixed cost, and worth more than
+the answer to that question.
+
+`decompress_amount` finished with a `while` loop multiplying by ten up to nine
+times. `read_varint` hands it whatever a record contains, and
+`validate_encoded` runs it over every output of every record loaded from a
+snapshot — so a file on disk could reach it with an arbitrary `u64`.
+`decompress_amount(u64::MAX)` is 2.05e22: **a panic in a debug build, a silent
+wrap in a release one.** Reproduced before fixing, as
+`an_absurd_compressed_amount_is_rejected_rather_than_overflowing`, which failed
+with `attempt to multiply with overflow`.
+
+The fix returns `Option` and requires the decompressed value back inside the
+compressible domain, which also closes a canonicality hole that was open until
+now: the compact form may encode only amounts the escape refuses, and the escape
+refuses exactly the amounts the compact form covers, so each amount has one
+spelling and no other.
+
+`decompress_accepts_exactly_the_encoder_image` states that as a property over
+every `u64`: whatever the decoder accepts must round-trip back to the same
+compressed value through the encoder. It does not merely check for absence of
+panics — it pins the accepted set to the encoder's image exactly.
+
 ### What it costs
 
 Encoding is 1.6-2.4x slower, because the directory widths are a property of the