fix(l1): charge snap serving budget on every lookup attempt#6957
fix(l1): charge snap serving budget on every lookup attempt#6957ElFantasma wants to merge 5 commits into
Conversation
|
🤖 Codex Code Review
No other material correctness issues stood out in the diff. I could not run the Rust tests in this sandbox because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have enough to write the review. Review:
|
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR fixes a request-amplification vulnerability in the snap protocol server where missed lookups, absent accounts, and empty-result trie pathsets consumed no budget, allowing a peer to trigger O(N) DB probes with a near-empty response that the message-rate limiter could not catch. The fix introduces a
Confidence Score: 4/5Safe to merge; the fix correctly bounds all-miss walks across all three snap handlers and the regression tests verify the key behavior change. The core security fix is sound and well-tested for byte codes and storage ranges. The storage handler adds MIN_LOOKUP_COST on top of slot bytes for every account (not just misses), which slightly over-debits hit accounts compared to the max-based approach used in the trie-nodes handler — a minor inconsistency that reduces per-request throughput for hit-heavy storage queries but does not break correctness. The trie-nodes miss path has no dedicated regression test, leaving that leg of the fix untested at the unit level. crates/networking/p2p/snap/server.rs — specifically the storage-ranges accounting around MIN_LOOKUP_COST for hit accounts.
|
| Filename | Overview |
|---|---|
| crates/networking/p2p/snap/server.rs | Introduces MIN_LOOKUP_COST (32 B) charged per attempt in all three snap handlers. The fix is correct for misses in byte-codes and trie-nodes. The storage-ranges handler charges MIN_LOOKUP_COST additively for every account (hits AND misses), unlike the other two handlers which charge only the larger of actual vs minimum — a minor accounting inconsistency that slightly over-debits hit accounts. |
| test/tests/p2p/snap_server_tests.rs | Adds two focused regression tests (byte_codes_charges_missed_lookups, storage_ranges_charges_missed_accounts) that correctly verify miss charging using a 1-byte budget. No new test for the trie-nodes empty-pathset path, which is acknowledged in a comment. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming snap request] --> B[Clamp budget to MAX_RESPONSE_BYTES]
B --> C{Next hash / account / pathset}
C --> D[DB or trie lookup]
D --> E{Handler type}
E --> BC[byte codes]
BC --> BC1{Hit?}
BC1 -- Yes --> BC2[charge actual code length]
BC1 -- No --> BC3[charge MIN_LOOKUP_COST=32]
E --> SR[storage ranges]
SR --> SR1{Account found?}
SR1 -- Yes --> SR2[charge 64 x n_slots]
SR2 --> SR3[charge MIN_LOOKUP_COST=32 - NEW]
SR1 -- No --> SR3
E --> TN[trie nodes]
TN --> TN1[compute returned_bytes]
TN1 --> TN2[charge max of returned_bytes and MIN_LOOKUP_COST=32 - NEW]
BC2 --> CHK{Budget exhausted?}
BC3 --> CHK
SR3 --> CHK
TN2 --> CHK
CHK -- Yes --> STOP[Return partial response]
CHK -- No --> C
C -- Done --> DONE[Return full response]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Incoming snap request] --> B[Clamp budget to MAX_RESPONSE_BYTES]
B --> C{Next hash / account / pathset}
C --> D[DB or trie lookup]
D --> E{Handler type}
E --> BC[byte codes]
BC --> BC1{Hit?}
BC1 -- Yes --> BC2[charge actual code length]
BC1 -- No --> BC3[charge MIN_LOOKUP_COST=32]
E --> SR[storage ranges]
SR --> SR1{Account found?}
SR1 -- Yes --> SR2[charge 64 x n_slots]
SR2 --> SR3[charge MIN_LOOKUP_COST=32 - NEW]
SR1 -- No --> SR3
E --> TN[trie nodes]
TN --> TN1[compute returned_bytes]
TN1 --> TN2[charge max of returned_bytes and MIN_LOOKUP_COST=32 - NEW]
BC2 --> CHK{Budget exhausted?}
BC3 --> CHK
SR3 --> CHK
TN2 --> CHK
CHK -- Yes --> STOP[Return partial response]
CHK -- No --> C
C -- Done --> DONE[Return full response]
Comments Outside Diff (2)
-
crates/networking/p2p/snap/server.rs, line 108-116 (link)The storage-ranges handler charges
MIN_LOOKUP_COSTunconditionally for every account — including those that returned slots — making the effective cost64 × n_slots + 32. The other two handlers use a max-based approach: byte codes chargescode.len()for hits andMIN_LOOKUP_COSTonly for misses; trie nodes chargesmax(returned_bytes, MIN_LOOKUP_COST). Applying the samemaxpattern here avoids the unintentional penalty on non-empty accounts while still protecting against all-miss walks.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/networking/p2p/snap/server.rs Line: 108-116 Comment: The storage-ranges handler charges `MIN_LOOKUP_COST` unconditionally for every account — including those that returned slots — making the effective cost `64 × n_slots + 32`. The other two handlers use a max-based approach: byte codes charges `code.len()` for hits and `MIN_LOOKUP_COST` only for misses; trie nodes charges `max(returned_bytes, MIN_LOOKUP_COST)`. Applying the same `max` pattern here avoids the unintentional penalty on non-empty accounts while still protecting against all-miss walks. How can I resolve this? If you propose a fix, please make it concise.
-
test/tests/p2p/snap_server_tests.rs, line 1013-1017 (link)The comment acknowledges that
process_trie_nodes_requesthas no miss/empty-pathset regression test. Themax(returned_bytes, MIN_LOOKUP_COST)change is the most structurally different of the three fixes (it also refactors the byte accumulation fromusize-fold-then-cast to au64fold), so an untested path here is a gap worth tracking. Consider adding a test that sends a pathset whose path exceeds 32 bytes (resolved to an empty node) followed by a real pathset with a 1-byte budget, analogous to the two new tests above.Prompt To Fix With AI
This is a comment left during a code review. Path: test/tests/p2p/snap_server_tests.rs Line: 1013-1017 Comment: **Missing test for trie-nodes empty-pathset charging** The comment acknowledges that `process_trie_nodes_request` has no miss/empty-pathset regression test. The `max(returned_bytes, MIN_LOOKUP_COST)` change is the most structurally different of the three fixes (it also refactors the byte accumulation from `usize`-fold-then-cast to a `u64` fold), so an untested path here is a gap worth tracking. Consider adding a test that sends a pathset whose path exceeds 32 bytes (resolved to an empty node) followed by a real pathset with a 1-byte budget, analogous to the two new tests above. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/networking/p2p/snap/server.rs:108-116
The storage-ranges handler charges `MIN_LOOKUP_COST` unconditionally for every account — including those that returned slots — making the effective cost `64 × n_slots + 32`. The other two handlers use a max-based approach: byte codes charges `code.len()` for hits and `MIN_LOOKUP_COST` only for misses; trie nodes charges `max(returned_bytes, MIN_LOOKUP_COST)`. Applying the same `max` pattern here avoids the unintentional penalty on non-empty accounts while still protecting against all-miss walks.
```suggestion
if !account_slots.is_empty() {
slots.push(account_slots);
}
// Charge every account attempt (even a miss that returned no slots) so an
// all-miss `account_hashes` list trips the budget instead of opening the storage
// trie once per entry. Use max(actual, MIN_LOOKUP_COST) — consistent with the
// trie-nodes handler — so hit accounts are not over-penalised.
let account_bytes = (account_slots.len() as u64) * 64;
bytes_used += account_bytes.max(MIN_LOOKUP_COST);
if bytes_used >= byte_budget {
```
### Issue 2 of 2
test/tests/p2p/snap_server_tests.rs:1013-1017
**Missing test for trie-nodes empty-pathset charging**
The comment acknowledges that `process_trie_nodes_request` has no miss/empty-pathset regression test. The `max(returned_bytes, MIN_LOOKUP_COST)` change is the most structurally different of the three fixes (it also refactors the byte accumulation from `usize`-fold-then-cast to a `u64` fold), so an untested path here is a gap worth tracking. Consider adding a test that sends a pathset whose path exceeds 32 bytes (resolved to an empty node) followed by a real pathset with a 1-byte budget, analogous to the two new tests above.
Reviews (1): Last reviewed commit: "fix(l1): bound duplicate tiny-code snap ..." | Re-trigger Greptile
…iss cost on empty storage accounts
|
Thanks — the reviews caught a real gap I'd missed. Addressed in 368182a:
Storage-ranges over-charge (greptile P2 #1) — fixed. Now charges the per-probe minimum only when an account produced no slots (absent account / empty range); hit accounts keep paying 64 B/slot and aren't charged extra. Note the literal Trie-nodes miss-path test (greptile P2 #2 / Claude) — acknowledged, not added. Exercising Minor (greptile): the storage over-charge above was the substantive half of that note; the accounting is now consistent with the other two handlers. |
1 participant
Motivation
The snap serving handlers (
GetByteCodes,GetStorageRanges,GetTrieNodes) clamp the response to a byte budget, but that budget only advances when a lookup hits. A missed lookup, a duplicate, or an overlong path (which resolves to an empty node) costs nothing, so a request full of misses/empties never trips the budget and the handler walks the entire peer-supplied list (hashes/account_hashes/paths) — O(N) DB/cache probes returning a near-empty response. The per-peer rate limiter counts messages, not items, so it doesn't catch this.Description
Introduces
MIN_LOOKUP_COST(32 B, a hash's wire length) and charges it against the byte budget on every lookup attempt, regardless of hit/miss/empty, in all three handlers:process_byte_codes_request— charge misses.process_storage_ranges_request— charge every account attempt (covers absent accounts).process_trie_nodes_request— charge at leastMIN_LOOKUP_COSTper pathset (covers overlong/empty-result paths).With the 512 KiB response cap this bounds an all-miss walk to ~16K probes instead of the full list. This reuses the existing response-byte clamp mechanism; there is no reject/truncate behavior change for well-formed requests.
Adds regression tests (
byte_codes_charges_missed_lookups,storage_ranges_charges_missed_accounts): a real hit placed after a miss with a 1-byte budget must not be served, proving misses now consume the budget. The existing*_clamps_response_bytestests still pass.Checklist
cargo test -p ethrex-test --test ethrex_tests snap_server(17 tests) andcargo clippy -p ethrex-p2ppass.