Skip to content

p2p: add flag to disable snap/1 serving#2152

Open
lucca30 wants to merge 2 commits intodevelopfrom
lmartins/p2p-no-snap-serving
Open

p2p: add flag to disable snap/1 serving#2152
lucca30 wants to merge 2 commits intodevelopfrom
lmartins/p2p-no-snap-serving

Conversation

@lucca30
Copy link
Copy Markdown
Contributor

@lucca30 lucca30 commented Mar 20, 2026

Changes

Adds a new p2p.nosnap config flag (nosnap in TOML under [p2p]) that decouples the snapshot tree from the snap/1 p2p sub-protocol.

The problem: before this change, --snapshot and snap/1 were hardcoded together — enabling the snapshot tree automatically registered snap/1. There was no way to have one without the other.

Why BPs want the snapshot tree: the flat snapshot accelerates state reads without trie traversal, benefiting both RPC queries and block building (every SLOAD, balance check, and storage read during EVM execution can hit the snapshot instead of walking the trie).

Why BPs don't want to serve snap/1: BPs are already fully synced and have no reason to be snap sync servers for other nodes. Serving snap/1 adds unnecessary network and CPU overhead.

Important clarification — snap/1 vs --snapshot:

  • snap/1 is the p2p sub-protocol used for snap sync state transfer between nodes. Data is served from the flat snapshot, Merkle proofs from the trie DB on disk
  • --snapshot / SnapshotCache is the in-memory flat snapshot tree used for fast local state reads

These are independent. nosnap=true disables snap/1 entirely (the node neither serves nor consumes snap sync), while the snapshot tree remains fully active for local performance.

Note: reducing TriesInMemory (dirty trie layers in memory) is a separate and independent optimization — it saves RAM by committing trie nodes to disk sooner. It does not affect snap/1 serving, which reads from the flat snapshot, not trie layers.

How it works

  • New NoSnapServing bool field in ethconfig.Config
  • Wired from P2PConfig.NoSnapServing (hcl:"nosnap" / toml:"nosnap") through buildEth
  • In Protocols(), snap/1 is only registered if SnapshotCache > 0 && !NoSnapServing — previously the condition was SnapshotCache > 0 alone, making snapshot and snap/1 inseparable

Testing

  • Unit test in eth/backend_protocols_test.go covering both the default (snap served) and NoSnapServing=true (snap not served) cases
  • Validated end-to-end in a local kurtosis devnet: nodes with nosnap = true started without advertising snap/1

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

@claude
Copy link
Copy Markdown

claude bot commented Mar 20, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.95%. Comparing base (2a0bfe7) to head (eb7c076).
⚠️ Report is 33 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2152      +/-   ##
===========================================
+ Coverage    51.51%   51.95%   +0.44%     
===========================================
  Files          882      884       +2     
  Lines       154140   155460    +1320     
===========================================
+ Hits         79403    80773    +1370     
+ Misses       69559    69480      -79     
- Partials      5178     5207      +29     
Files with missing lines Coverage Δ
eth/backend.go 53.26% <100.00%> (+1.05%) ⬆️
eth/ethconfig/config.go 78.94% <ø> (ø)
internal/cli/server/config.go 63.90% <100.00%> (-1.08%) ⬇️
internal/cli/server/flags.go 100.00% <100.00%> (ø)

... and 77 files with indirect coverage changes

Files with missing lines Coverage Δ
eth/backend.go 53.26% <100.00%> (+1.05%) ⬆️
eth/ethconfig/config.go 78.94% <ø> (ø)
internal/cli/server/config.go 63.90% <100.00%> (-1.08%) ⬇️
internal/cli/server/flags.go 100.00% <100.00%> (ø)

... and 77 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread eth/backend.go
Document that NoSnapServing=true disables snap/1 entirely (no serving
and no snap-syncing from peers), while the in-memory snapshot tree
remains active for local state reads.
@claude
Copy link
Copy Markdown

claude bot commented Apr 15, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants