Open
Conversation
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2152 +/- ##
===========================================
+ Coverage 51.51% 51.53% +0.02%
===========================================
Files 882 882
Lines 154140 154149 +9
===========================================
+ Hits 79403 79440 +37
+ Misses 69559 69533 -26
+ Partials 5178 5176 -2
... and 21 files with indirect coverage changes
🚀 New features to boost your workflow:
|
manav2401
reviewed
Mar 26, 2026
| func (s *Ethereum) Protocols() []p2p.Protocol { | ||
| protos := eth.MakeProtocols((*ethHandler)(s.handler), s.networkID, s.discmix) | ||
| if s.config.SnapshotCache > 0 { | ||
| if s.config.SnapshotCache > 0 && !s.config.NoSnapServing { |
Member
There was a problem hiding this comment.
Won't this disable snap/1 completely from being registered? In which case the node won't be able to sync from snap.
I think it's fine - but we can mention this fact in comment / description.
Basically, if we want to sync from snap, we can set nosnap to false which will allow snap/1 to be registered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Changes
Adds a new
p2p.nosnapconfig flag (nosnapin TOML under[p2p]) that decouples the snapshot tree from thesnap/1p2p sub-protocol.The problem: before this change,
--snapshotandsnap/1were 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/1vs--snapshot:snap/1is 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/SnapshotCacheis the in-memory flat snapshot tree used for fast local state readsThese are independent.
nosnap=truedisablessnap/1entirely (the node neither serves nor consumes snap sync), while the snapshot tree remains fully active for local performance.How it works
NoSnapServing boolfield inethconfig.ConfigP2PConfig.NoSnapServing(hcl:"nosnap"/toml:"nosnap") throughbuildEthProtocols(),snap/1is only registered ifSnapshotCache > 0 && !NoSnapServing— previously the condition wasSnapshotCache > 0alone, making snapshot andsnap/1inseparableTesting
eth/backend_protocols_test.gocovering both the default (snap served) andNoSnapServing=true(snap not served) casesnosnap = truestarted without advertisingsnap/1