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 7 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. |
| // 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)); |
There was a problem hiding this comment.
MAX_RESPONSE_BYTES/MIN_LOOKUP_COST is currently 16k, plus each trie lookup requiring several lookups (since it has to traverse the trie), makes the worst case very resource intensive
| // 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)); |
There was a problem hiding this comment.
The cost should be 32 per path, not total. Otherwise the user can request as many paths as the protocol supports and if they all miss, only pay once.
3 participants
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.