fix(l1): charge snap serving budget on every lookup attempt

crates/networking/p2p/snap/server.rs

@@ -12,6 +12,16 @@ use super::constants::MAX_RESPONSE_BYTES;
 use super::error::SnapError;
 use super::proof_to_encodable;
 
+/// Minimum budget charged per lookup *attempt* in the snap serving handlers, independent of
+/// whether the lookup hit. The response-byte clamp only advances on a hit, so without this a
+/// miss/empty/overlong-path request costs nothing and the handler walks the entire
+/// peer-supplied list (`hashes` / `account_hashes` / `paths`) — O(N) DB/cache probes with a
+/// near-empty response, which the message-rate limiter (counting messages, not items) doesn't
+/// catch. Charging a per-attempt minimum bounds that walk against the same byte budget.
+/// Sized at a hash's wire length; with the 512 KiB response cap this bounds an all-miss walk
+/// to ~16K probes.
+const MIN_LOOKUP_COST: u64 = 32;
+
 // Request Processing
 
 pub async fn process_account_range_request(
@@ -95,7 +105,13 @@ pub async fn process_storage_ranges_request(
                 ));
             }
 
-            if !account_slots.is_empty() {
+            if account_slots.is_empty() {
+                // Absent account or empty range returns no slots; charge a per-probe minimum
+                // so an all-miss `account_hashes` list trips the budget instead of opening the
+                // storage trie once per entry. Hit accounts already paid 64 B/slot above, so
+                // they are not charged again (avoids over-debiting non-empty responses).
+                bytes_used += MIN_LOOKUP_COST;
+            } else {
                 slots.push(account_slots);
             }
 
@@ -125,9 +141,18 @@ pub async fn process_byte_codes_request(
         let mut codes = vec![];
         let mut bytes_used = 0;
         for code_hash in request.hashes {
-            if let Some(code) = store.get_account_code(code_hash)?.map(|c| c.code_bytes()) {
-                bytes_used += code.len() as u64;
-                codes.push(code);
+            match store.get_account_code(code_hash)? {
+                Some(code) => {
+                    let code = code.code_bytes();
+                    // Charge at least `MIN_LOOKUP_COST` even for a hit, so a long list of
+                    // duplicate hashes of a *tiny* code is bounded by probe count, not just by
+                    // response bytes.
+                    bytes_used += (code.len() as u64).max(MIN_LOOKUP_COST);
+                    codes.push(code);
+                }
+                // A missed lookup still costs a probe; charge it so an all-miss request trips
+                // the budget instead of walking the entire `hashes` list.
+                None => bytes_used += MIN_LOOKUP_COST,
             }
             if bytes_used >= byte_budget {
                 break;
@@ -162,9 +187,14 @@ pub async fn process_trie_nodes_request(
                 paths.into_iter().map(|bytes| bytes.to_vec()).collect(),
                 byte_budget,
             )?;
+            let returned_bytes = trie_nodes
+                .iter()
+                .fold(0u64, |acc, nodes| acc + nodes.len() as u64);
             nodes.extend(trie_nodes.iter().map(|nodes| Bytes::copy_from_slice(nodes)));
-            byte_budget = byte_budget
-                .saturating_sub(trie_nodes.iter().fold(0, |acc, nodes| acc + nodes.len()) as u64);
+            // Charge at least a per-pathset probe cost so overlong/empty-result pathsets
+            // (e.g. a path > 32 bytes, which resolves to an empty node) still consume the
+            // budget instead of letting the loop run over every decoded pathset.
+            byte_budget = byte_budget.saturating_sub(returned_bytes.max(MIN_LOOKUP_COST));
             if byte_budget == 0 {
                 break;
             }

crates/storage/store.rs

@@ -122,6 +122,12 @@ const CODE_CACHE_MAX_SIZE: u64 = 64 * 1024 * 1024;
 /// Key used to persist the `flushed_upto` block number in `MISC_VALUES`.
 const FLUSHED_UPTO_KEY: &[u8] = b"bodies_flushed_upto";
 
+/// Minimum budget charged per storage sub-path probe in `get_trie_nodes`. A missed or overlong
+/// path resolves to an empty node (0 bytes), so charging a per-probe minimum stops a single
+/// `GetTrieNodes` pathset packed with bogus paths from forcing an unbounded number of trie
+/// descents. Sized to match the snap serving handlers' `MIN_LOOKUP_COST`.
+const MIN_TRIE_NODE_LOOKUP_COST: u64 = 32;
+
 #[derive(Debug)]
 struct CodeCache {
     inner_cache: LruCache<H256, Code, FxBuildHasher>,
@@ -2993,7 +2999,11 @@ impl Store {
                 break;
             }
             let node = storage_trie.get_node(path)?;
-            bytes_used += node.len() as u64;
+            // Charge at least a per-probe minimum against the budget: a missed or overlong
+            // sub-path resolves to an empty node (0 bytes), so without this a single pathset
+            // packed with bogus storage paths would run one trie descent per entry while never
+            // advancing the budget (a request-amplification vector).
+            bytes_used += (node.len() as u64).max(MIN_TRIE_NODE_LOOKUP_COST);
             nodes.push(node);
         }
         Ok(nodes)

test/tests/p2p/snap_server_tests.rs

@@ -1010,8 +1010,99 @@ async fn byte_codes_clamps_response_bytes() -> Result<(), SnapError> {
     Ok(())
 }
 
-// NOTE: `process_trie_nodes_request` receives the same `request.bytes.min(MAX_RESPONSE_BYTES)`
-// clamp (see `snap/server.rs`), but is not given a dedicated regression test here: exercising
-// it requires a committed state trie readable through `get_trie_nodes`' `open_state_trie`
-// path, which the direct-write test fixtures don't populate. The clamp is mechanically
-// identical to the three handlers covered above.
+// NOTE: the `GetTrieNodes` path is charged in two places — per outer pathset in
+// `process_trie_nodes_request` (see `snap/server.rs`) and, crucially, per storage sub-path
+// inside `Store::get_trie_nodes` (which now adds `MIN_TRIE_NODE_LOOKUP_COST` per probe so a
+// single pathset packed with missing/overlong sub-paths can't force an unbounded trie walk).
+// Neither gets a dedicated miss/empty-path regression test here: exercising them requires a
+// committed state trie readable through `get_trie_nodes`' snapshot-gated `open_state_trie`
+// path, which the direct-write test fixtures don't populate (it goes through the block/layer
+// commit machinery). The per-probe charge mirrors the byte-codes/storage handlers above.
+
+// Request-side amplification guard (distinct from the response-byte clamp above): the byte
+// budget must advance on every lookup *attempt*, not only on hits. Otherwise an all-miss (or
+// miss-heavy) request never trips the budget and the handler walks the entire peer-supplied
+// list — O(N) DB/cache probes with a near-empty response, which the message-rate limiter
+// doesn't catch. Each test puts a real hit *after* a miss, with a 1-byte budget: charging the
+// miss must exhaust the budget and stop the walk before the trailing hit is ever served.
+
+/// `GetByteCodes`: a missed code lookup must consume the budget. Pre-fix, misses cost nothing,
+/// so the trailing hit is served (and an all-miss request walks the whole `hashes` vector).
+#[tokio::test]
+async fn byte_codes_charges_missed_lookups() -> Result<(), SnapError> {
+    let store = Store::new("null", EngineType::InMemory).unwrap();
+    let code = Code::from_bytecode(Bytes::from_static(b"hit"), &NativeCrypto);
+    let hit_hash = code.hash;
+    store.add_account_code(code).await.unwrap();
+
+    let miss_hash = H256::from_low_u64_be(0xdead);
+    let request = GetByteCodes {
+        id: 0,
+        hashes: vec![miss_hash, hit_hash],
+        bytes: 1,
+    };
+    let res = process_byte_codes_request(request, store).await.unwrap();
+
+    assert!(
+        res.codes.is_empty(),
+        "a missed lookup must consume the budget and stop the walk before the trailing hit; \
+         got {} code(s) — misses are not being charged",
+        res.codes.len()
+    );
+    Ok(())
+}
+
+/// `GetByteCodes`: duplicate hits of a *tiny* code must be bounded by probe count, not just by
+/// response bytes. Each hit is charged at least `MIN_LOOKUP_COST` (32 B), so a 3-byte code
+/// under a 64-byte budget serves at most 2 duplicates, not the whole list.
+#[tokio::test]
+async fn byte_codes_charges_min_cost_on_tiny_duplicate_hits() -> Result<(), SnapError> {
+    let store = Store::new("null", EngineType::InMemory).unwrap();
+    let code = Code::from_bytecode(Bytes::from_static(b"hit"), &NativeCrypto);
+    let hit_hash = code.hash;
+    store.add_account_code(code).await.unwrap();
+
+    let request = GetByteCodes {
+        id: 0,
+        hashes: vec![hit_hash; 10],
+        bytes: 64, // 64 / 32 = 2 lookups before the budget is exhausted
+    };
+    let res = process_byte_codes_request(request, store).await.unwrap();
+
+    assert!(
+        res.codes.len() < 10,
+        "duplicate tiny-code hits must be bounded by per-probe cost; got all {} back",
+        res.codes.len()
+    );
+    Ok(())
+}
+
+/// `GetStorageRanges`: a missed account (absent from state) must consume the budget. Pre-fix,
+/// a miss opens the trie, finds nothing, charges nothing, and continues — so an all-miss
+/// `account_hashes` list forces a trie open per entry unbounded.
+#[tokio::test]
+async fn storage_ranges_charges_missed_accounts() -> Result<(), SnapError> {
+    let account_hash = H256::from_low_u64_be(0xabcd);
+    let (store, root) = setup_large_storage_state(account_hash, 4)?;
+
+    let miss_account = H256::from_low_u64_be(0x1111);
+    let request = GetStorageRanges {
+        id: 0,
+        root_hash: root,
+        account_hashes: vec![miss_account, account_hash],
+        starting_hash: *HASH_MIN,
+        limit_hash: *HASH_MAX,
+        response_bytes: 1,
+    };
+    let res = process_storage_ranges_request(request, store)
+        .await
+        .unwrap();
+
+    assert!(
+        res.slots.is_empty(),
+        "a missed account must consume the budget and stop before the populated account is \
+         walked; got {} slot-set(s) — misses are not being charged",
+        res.slots.len()
+    );
+    Ok(())
+}