From 1fe876d70700db92f2b3c6f2032abb5232cd4a14 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Tue, 3 Feb 2026 15:54:47 -0300 Subject: [PATCH 01/16] Snapsync roadmap --- docs/roadmaps/snap_sync_roadmap.md | 788 +++++++++++++++++++++++++++++ 1 file changed, 788 insertions(+) create mode 100644 docs/roadmaps/snap_sync_roadmap.md diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md new file mode 100644 index 00000000000..8961f92d28a --- /dev/null +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -0,0 +1,788 @@ +# Snap Sync Module Roadmap + +**Author:** Pablo Deymonnaz +**Date:** February 2026 +**Status:** Draft for Review + +--- + +## Executive Summary + +This roadmap outlines a strategic plan to improve the ethrex snap sync module in two phases: + +1. **Phase 1: Performance Optimization** - Make snap sync as fast as possible +2. **Phase 2: Code Quality & Maintainability** - Make the code clear, readable, and easier to understand + +The snap sync module currently comprises ~4,650 lines across 12 files. Our goal is to achieve sync times competitive with geth while maintaining code quality standards. + +--- + +## Table of Contents + +1. [Current State Analysis](#current-state-analysis) +2. [Phase 1: Performance Optimization](#phase-1-performance-optimization) +3. [Phase 2: Code Quality & Maintainability](#phase-2-code-quality--maintainability) +4. [Success Metrics](#success-metrics) +5. [Risk Assessment](#risk-assessment) +6. [Timeline](#timeline) +7. [Dependencies](#dependencies) + +--- + +## Current State Analysis + +### Module Structure + +| File | Lines | Purpose | +|------|-------|---------| +| `sync/snap_sync.rs` | 1,139 | Main snap sync orchestration | +| `snap/client.rs` | 1,416 | Client-side snap protocol requests | +| `sync/healing/storage.rs` | 728 | Storage trie healing | +| `sync/healing/state.rs` | 460 | State trie healing | +| `sync/full.rs` | 297 | Full sync implementation | +| `snap/server.rs` | 173 | Server-side snap protocol responses | +| `snap/error.rs` | 158 | Unified error types | +| `snap/constants.rs` | 118 | Protocol constants | +| `sync/code_collector.rs` | 100 | Bytecode collection | +| Other modules | ~61 | Supporting code | +| **Total** | **~4,650** | | + +### Snap Sync Phases + +The snap sync process consists of 6 sequential phases: + +``` +┌─────────────────────────────────────────────────────────────────────────┐ +│ SNAP SYNC PIPELINE │ +├─────────────────────────────────────────────────────────────────────────┤ +│ │ +│ 1. Header Download ──► 2. Pivot Selection ──► 3. Account Range Download │ +│ │ │ +│ ▼ │ +│ 6. Full Sync ◄── 5. Bytecode Download ◄── 4. Storage Range Download │ +│ │ │ +│ ▼ │ +│ [State Healing & Storage Healing run in parallel with phases 4-5] │ +│ │ +└─────────────────────────────────────────────────────────────────────────┘ +``` + +### Current Performance Bottlenecks + +Based on code analysis and profiling data: + +| Bottleneck | Location | Impact | Priority | +|------------|----------|--------|----------| +| Sequential header download | `sync_cycle_snap()` | Blocks state download start | Critical | +| Single-threaded account range processing | `request_account_range()` | Underutilizes peers | High | +| Inefficient trie node batching | `heal_state_trie()`, `heal_storage_trie()` | Excessive DB writes | High | +| Busy-wait loops | Multiple locations | CPU waste | Medium | +| Unbounded memory structures | `accounts_by_root_hash` | Memory pressure | Medium | +| Synchronous disk I/O | Snapshot dumping | Blocks network operations | Medium | + +### Existing Code Quality Issues + +| Issue | Location | Description | +|-------|----------|-------------| +| `#[allow(clippy::too_many_arguments)]` | `heal_state_trie()` | 8 parameters - needs context struct | +| Repeated code patterns | `snap/client.rs` | Snapshot dumping logic duplicated | +| Magic numbers | Various | Hardcoded values without constants | +| Missing documentation | Healing modules | Complex algorithms undocumented | +| Inconsistent error handling | Various | Mix of `?`, `.expect()`, silent drops | + +--- + +## Phase 1: Performance Optimization + +### Goal +Reduce snap sync time by 50% or more through parallelization, batching optimizations, and I/O improvements. + +--- + +### 1.1 Parallel Header Download (PR #6059 - In Progress) + +**Current State:** Headers are downloaded sequentially before state download begins. + +**Proposed Change:** Download headers in a background task while state download proceeds in parallel. + +**Implementation:** +- Add `header_receiver` channel to `SnapBlockSyncState` +- Spawn `download_headers_background()` task +- Process headers incrementally at strategic points +- Add early abort mechanism when switching to full sync + +**Expected Impact:** +- State download starts immediately instead of waiting for millions of headers +- Estimated time savings: 10-20% of total sync time + +**Status:** PR #6059 open, addressing review feedback + +--- + +### 1.2 Parallel Account Range Requests + +**Current State:** Account ranges are requested from peers sequentially within each chunk. + +**Proposed Change:** Increase parallelism by: +1. Using a work-stealing task pool for account range chunks +2. Implementing adaptive chunk sizing based on peer response times +3. Adding peer quality scoring to prefer faster peers + +**Implementation:** +```rust +// Current: Fixed 800 chunks, sequential processing +let chunk_count = 800; + +// Proposed: Adaptive chunking with parallel execution +struct AdaptiveChunker { + min_chunk_size: U256, + max_concurrent: usize, + peer_scores: HashMap, +} +``` + +**Expected Impact:** 2-3x faster account range download + +**Effort:** Medium (2-3 weeks) + +--- + +### 1.3 Optimize Trie Node Batching + +**Current State:** +- `NODE_BATCH_SIZE = 500` nodes per request +- `STORAGE_BATCH_SIZE = 300` accounts per batch +- Individual DB writes with `put_batch()` + +**Proposed Changes:** + +#### 1.3.1 Use `put_batch_no_alloc()` for Healing +```rust +// Current (healing/state.rs:304) +// PERF: use put_batch_no_alloc (note that it needs to remove nodes too) + +// Proposed: Pre-allocate buffers, reuse across batches +struct HealingBatchWriter { + node_buffer: Vec<(Nibbles, Node)>, + capacity: usize, +} +``` + +#### 1.3.2 Dynamic Batch Sizing +Adjust batch sizes based on: +- Available memory +- Peer response latency +- Current healing progress + +**Expected Impact:** 30-50% reduction in healing phase duration + +**Effort:** Medium (2 weeks) + +--- + +### 1.4 Reduce Busy-Wait Loops + +**Current State:** Multiple locations use `tokio::time::sleep()` in loops: +- `sync_cycle_snap()`: 100ms sleep waiting for headers +- `request_account_range()`: 10ms sleep waiting for peers +- Healing loops: Various polling intervals + +**Proposed Change:** Replace with proper async primitives: + +```rust +// Current (snap_sync.rs ~line 452) +loop { + if let Some(headers) = block_sync_state.header_receiver.try_recv() { ... } + tokio::time::sleep(Duration::from_millis(100)).await; +} + +// Proposed: Blocking receive with timeout +match tokio::time::timeout( + Duration::from_secs(30), + block_sync_state.header_receiver.recv() +).await { + Ok(Some(headers)) => { ... } + Ok(None) => break, // Channel closed + Err(_) => continue, // Timeout, check staleness +} +``` + +**Expected Impact:** Reduced CPU usage, faster response to events + +**Effort:** Low (1 week) + +--- + +### 1.5 Memory-Bounded Structures + +**Current State:** +- `accounts_by_root_hash` in `request_storage_ranges()` is unbounded +- Can grow to gigabytes on mainnet + +**Proposed Change:** +```rust +// Add memory limits with spill-to-disk +struct BoundedAccountMap { + in_memory: BTreeMap, + max_memory_bytes: usize, + spill_dir: PathBuf, +} + +impl BoundedAccountMap { + fn insert(&mut self, key: H256, value: AccountsWithStorage) { + if self.memory_usage() > self.max_memory_bytes { + self.spill_to_disk(); + } + self.in_memory.insert(key, value); + } +} +``` + +**Expected Impact:** Stable memory usage, prevents OOM on large states + +**Effort:** Medium (2 weeks) + +--- + +### 1.6 Async Disk I/O + +**Current State:** Snapshot dumping uses synchronous `std::fs` operations. + +**Proposed Change:** Use `tokio::fs` for non-blocking I/O: + +```rust +// Current +std::fs::create_dir_all(dir)?; +dump_accounts_to_file(&path, chunk)?; + +// Proposed +tokio::fs::create_dir_all(dir).await?; +tokio::task::spawn_blocking(move || { + dump_accounts_to_file(&path, chunk) +}).await??; +``` + +**Expected Impact:** Network operations not blocked by disk I/O + +**Effort:** Low (1 week) + +--- + +### 1.7 Peer Connection Optimization + +**Current State:** +- `PEER_REPLY_TIMEOUT = 15 seconds` +- `MAX_IN_FLIGHT_REQUESTS = 77` +- No adaptive timeout based on peer performance + +**Proposed Changes:** + +#### 1.7.1 Adaptive Timeouts +```rust +struct AdaptivePeerConfig { + base_timeout: Duration, + peer_latencies: HashMap, + + fn timeout_for_peer(&self, peer_id: &H256) -> Duration { + self.peer_latencies + .get(peer_id) + .map(|avg| avg.mean() * 3.0) // 3x average latency + .unwrap_or(self.base_timeout) + } +} +``` + +#### 1.7.2 Request Pipelining +Increase in-flight requests for high-quality peers: +```rust +fn max_requests_for_peer(&self, peer_id: &H256) -> u32 { + match self.peer_quality(peer_id) { + PeerQuality::Excellent => 100, + PeerQuality::Good => 77, + PeerQuality::Average => 50, + PeerQuality::Poor => 20, + } +} +``` + +**Expected Impact:** 20-30% improvement in peer utilization + +**Effort:** Medium (2 weeks) + +--- + +### 1.8 Parallel Storage Healing + +**Current State:** Storage healing processes accounts sequentially within batches. + +**Proposed Change:** Use rayon for parallel storage trie healing: + +```rust +// Current +for account in accounts_to_heal { + heal_storage_for_account(account).await?; +} + +// Proposed +accounts_to_heal + .par_iter() + .map(|account| heal_storage_for_account(account)) + .collect::, _>>()?; +``` + +**Caveat:** Need to handle DB write contention carefully. + +**Expected Impact:** 2-4x faster storage healing + +**Effort:** High (3 weeks) + +--- + +## Phase 2: Code Quality & Maintainability + +### Goal +Make the codebase clear, well-documented, and easy for new contributors to understand. + +--- + +### 2.1 Extract Context Structs + +**Current State:** Functions with many parameters: +```rust +#[allow(clippy::too_many_arguments)] +async fn heal_state_trie( + state_root: H256, + store: Store, + mut peers: PeerHandler, + staleness_timestamp: u64, + global_leafs_healed: &mut u64, + mut healing_queue: StateHealingQueue, + storage_accounts: &mut AccountStorageRoots, + code_hash_collector: &mut CodeHashCollector, +) -> Result +``` + +**Proposed Change:** +```rust +struct StateHealingContext { + state_root: H256, + store: Store, + staleness_timestamp: u64, +} + +struct StateHealingProgress { + global_leafs_healed: u64, + healing_queue: StateHealingQueue, + storage_accounts: AccountStorageRoots, + code_hash_collector: CodeHashCollector, +} + +async fn heal_state_trie( + ctx: &StateHealingContext, + peers: &mut PeerHandler, + progress: &mut StateHealingProgress, +) -> Result +``` + +**Files Affected:** +- `sync/healing/state.rs` +- `sync/healing/storage.rs` +- `sync/snap_sync.rs` + +**Effort:** Low (1 week) + +--- + +### 2.2 Comprehensive Documentation + +**Current State:** Module documentation is sparse; healing algorithms are complex and undocumented. + +**Proposed Documentation:** + +#### 2.2.1 Architecture Documentation +Create `docs/snap_sync_architecture.md`: +- High-level overview with diagrams +- Data flow through the system +- State machine for sync phases +- Interaction with storage layer + +#### 2.2.2 Algorithm Documentation +Document healing algorithms inline: +```rust +/// # State Trie Healing Algorithm +/// +/// The healing process fixes inconsistencies in the state trie that occur +/// when snap sync spans multiple pivot blocks. +/// +/// ## Algorithm +/// +/// 1. Start from the state root node +/// 2. For each node, check if all children exist in local storage +/// 3. For missing children: +/// a. Add to download queue +/// b. Request from peers in batches of NODE_BATCH_SIZE +/// 4. When a node's children are all present, flush to DB +/// 5. Repeat until no missing nodes remain +/// +/// ## Invariants +/// +/// - Parent nodes are only flushed after all children are present +/// - The healing queue tracks `missing_children_count` per node +/// - Staleness checks prevent infinite loops on changing state +/// +/// ## Complexity +/// +/// - Time: O(n) where n is the number of trie nodes +/// - Space: O(d * b) where d is max depth and b is branching factor +``` + +#### 2.2.3 Inline Code Comments +Add comments explaining non-obvious logic, especially: +- Hash boundary calculations +- Pivot staleness detection +- Proof verification + +**Effort:** Medium (2 weeks) + +--- + +### 2.3 Consolidate Error Handling + +**Current State:** Inconsistent error handling: +```rust +// Silent drop +if let Err(_) = sender.send(headers) { break; } + +// Expect with message +.expect("We shouldn't have a rocksdb error here") + +// Proper propagation +store.get_block_header(number)? +``` + +**Proposed Change:** Standardize on: +1. Use `?` for propagation +2. Use `tracing::warn!` for recoverable errors +3. Use `tracing::error!` before returning fatal errors +4. Never use `.expect()` in production paths + +```rust +// Before +if sender.send(headers).is_err() { break; } + +// After +if let Err(e) = sender.send(headers) { + warn!("Header channel closed unexpectedly: {}", e); + break; +} +``` + +**Files Affected:** All snap sync modules + +**Effort:** Low (1 week) + +--- + +### 2.4 Extract Helper Functions + +**Current State:** Duplicated patterns identified: +- Snapshot dumping (4 occurrences) - **Partially addressed** +- Peer selection and retry logic (6+ occurrences) +- Progress reporting (5+ occurrences) + +**Proposed Change:** Create shared utilities: + +```rust +// snap/utils.rs (new file) + +/// Retries an operation with exponential backoff +pub async fn with_retry( + max_attempts: u32, + operation: F, +) -> Result +where + F: Fn() -> Fut, + Fut: Future>, +{ ... } + +/// Reports progress at regular intervals +pub struct ProgressReporter { + interval: Duration, + last_report: Instant, + metrics_key: &'static str, +} + +impl ProgressReporter { + pub fn maybe_report(&mut self, current: u64, total: u64) { ... } +} +``` + +**Effort:** Low (1 week) + +--- + +### 2.5 State Machine Refactor + +**Current State:** Snap sync phases are implicit in control flow. + +**Proposed Change:** Make phases explicit: + +```rust +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum SnapSyncPhase { + Initializing, + DownloadingHeaders, + DownloadingAccounts, + DownloadingStorages, + DownloadingBytecodes, + HealingState, + HealingStorage, + FullSync, + Complete, +} + +pub struct SnapSyncStateMachine { + phase: SnapSyncPhase, + progress: SnapSyncProgress, + + pub fn transition(&mut self, event: SnapSyncEvent) -> Result<(), SyncError> { + match (self.phase, event) { + (DownloadingHeaders, HeadersComplete) => { + self.phase = DownloadingAccounts; + Ok(()) + } + // ... other transitions + _ => Err(SyncError::InvalidStateTransition), + } + } +} +``` + +**Benefits:** +- Clear phase boundaries +- Easier to add new phases +- Better logging and metrics +- Simpler testing + +**Effort:** High (3 weeks) + +--- + +### 2.6 Test Coverage Improvement + +**Current State:** +- 12 snap server tests +- Limited client-side testing +- No integration tests for full sync cycle + +**Proposed Testing Strategy:** + +#### 2.6.1 Unit Tests +- Test each healing algorithm with mock data +- Test pivot selection logic +- Test proof verification + +#### 2.6.2 Integration Tests +- Mock peer network +- Test full sync cycle with small state +- Test pivot updates mid-sync +- Test recovery from interrupted sync + +#### 2.6.3 Property-Based Tests +- Random account ranges +- Random trie structures +- Fuzz testing for proof verification + +**Target Coverage:** 80%+ for core modules + +**Effort:** High (4 weeks) + +--- + +### 2.7 Configuration Externalization + +**Current State:** Constants are hardcoded in `snap/constants.rs`. + +**Proposed Change:** Make tunable parameters configurable: + +```rust +// config/snap_sync.rs +#[derive(Debug, Clone, Deserialize)] +pub struct SnapSyncConfig { + /// Maximum response size in bytes (default: 512KB) + pub max_response_bytes: u64, + + /// Number of accounts per request (default: 128) + pub snap_limit: usize, + + /// In-memory buffer size before disk flush (default: 64MB) + pub range_file_chunk_size: usize, + + /// Maximum concurrent in-flight requests (default: 77) + pub max_in_flight_requests: u32, + + // ... other parameters +} + +impl Default for SnapSyncConfig { + fn default() -> Self { + Self { + max_response_bytes: 512 * 1024, + snap_limit: 128, + // ... defaults from current constants + } + } +} +``` + +**Benefits:** +- Tuning without recompilation +- Environment-specific configurations +- Easier benchmarking + +**Effort:** Medium (2 weeks) + +--- + +## Success Metrics + +### Phase 1: Performance + +| Metric | Current | Target | Measurement Method | +|--------|---------|--------|-------------------| +| Mainnet full sync time | TBD | -50% | End-to-end benchmark | +| Account download rate | TBD | 2x | Accounts/second metric | +| Storage healing time | TBD | -60% | Phase duration metric | +| Peak memory usage | TBD | -30% | Process monitoring | +| CPU utilization during sync | TBD | >80% | Process monitoring | + +### Phase 2: Code Quality + +| Metric | Current | Target | Measurement Method | +|--------|---------|--------|-------------------| +| Test coverage | ~20% | >80% | `cargo tarpaulin` | +| Clippy warnings | 0 | 0 | CI enforcement | +| Documentation coverage | ~30% | >90% | `cargo doc` coverage | +| Cyclomatic complexity | TBD | <15 per function | `cargo clippy` | +| Functions >100 lines | TBD | 0 | Custom lint | + +--- + +## Risk Assessment + +| Risk | Probability | Impact | Mitigation | +|------|-------------|--------|------------| +| Performance regression | Medium | High | Comprehensive benchmarking before/after each change | +| Data corruption during sync | Low | Critical | Extensive integration testing; checksums; recovery mechanisms | +| Breaking changes to peer protocol | Low | Medium | Hive test suite validation | +| Increased complexity | Medium | Medium | Code review; documentation requirements | +| Schedule overrun | Medium | Medium | Prioritize high-impact items; iterative delivery | + +--- + +## Timeline + +### Phase 1: Performance (12 weeks) + +``` +Week 1-2: 1.1 Parallel Header Download (complete PR #6059) +Week 2-3: 1.4 Reduce Busy-Wait Loops +Week 3-4: 1.6 Async Disk I/O +Week 4-6: 1.2 Parallel Account Range Requests +Week 6-8: 1.3 Optimize Trie Node Batching +Week 8-10: 1.5 Memory-Bounded Structures +Week 10-12: 1.7 Peer Connection Optimization +Week 12+: 1.8 Parallel Storage Healing (stretch goal) +``` + +### Phase 2: Code Quality (10 weeks) + +``` +Week 1: 2.1 Extract Context Structs +Week 1-2: 2.3 Consolidate Error Handling +Week 2-3: 2.4 Extract Helper Functions +Week 3-5: 2.2 Comprehensive Documentation +Week 5-7: 2.7 Configuration Externalization +Week 7-10: 2.5 State Machine Refactor +Week 8-12: 2.6 Test Coverage Improvement (parallel) +``` + +**Total Duration:** ~16 weeks (phases overlap) + +--- + +## Dependencies + +### External Dependencies + +| Dependency | Version | Purpose | +|------------|---------|---------| +| tokio | 1.x | Async runtime | +| rayon | 1.x | Parallel iterators | +| tracing | 0.1.x | Logging/metrics | + +### Internal Dependencies + +| Module | Dependency | Notes | +|--------|------------|-------| +| snap sync | ethrex-storage | Trie operations | +| snap sync | ethrex-trie | Merkle Patricia Trie | +| snap sync | peer_handler | Network layer | + +### Infrastructure Dependencies + +- Benchmarking environment with mainnet-like data +- CI pipeline for performance regression detection +- Test network (Sepolia/Holesky) access + +--- + +## Appendix A: Reference Implementation Comparison + +### geth Snap Sync +- Parallel header and state download +- Adaptive peer scoring +- In-memory trie caching + +### reth Snap Sync +- Staged sync architecture +- Parallel range downloads +- Memory-mapped storage + +### Key Takeaways +1. All major clients parallelize header and state download +2. Adaptive batching is common +3. Memory management is critical for mainnet scale + +--- + +## Appendix B: Existing TODOs/FIXMEs + +| Location | Issue | Priority | +|----------|-------|----------| +| `healing/storage.rs:156` | Better data receiver design | Medium | +| `healing/storage.rs:230` | Use `put_batch_no_alloc` | High | +| `healing/storage.rs:298` | Store error handling | High | +| `healing/state.rs:149` | Peer scoring for responses | Medium | +| `healing/state.rs:194` | Optimize trie leaf reaching | Low | +| `snap/client.rs:567` | Stable sort for binary search | Low | +| `snap/client.rs:599` | Replace with removable structure | Medium | +| `snap/client.rs:983` | Unnecessary unzip/memory | Low | + +--- + +## Appendix C: Glossary + +| Term | Definition | +|------|------------| +| **Pivot** | The block whose state we're syncing; updated when stale | +| **Healing** | Process of fixing trie inconsistencies after multi-pivot sync | +| **Staleness** | When the pivot block is too old relative to chain head | +| **Account Range** | A contiguous range of accounts in the state trie | +| **Storage Range** | A contiguous range of storage slots for an account | +| **In-flight Request** | A request sent to a peer awaiting response | + +--- + +*Document Version: 1.0* +*Last Updated: February 2026* From face77ceeec0e1e84c6d8892dadd511c3c0f0cdc Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 17:16:31 -0300 Subject: [PATCH 02/16] Update snap sync roadmap to reflect current status Mark sections 1.2, 1.5, and 1.8 as discarded. Mark 2.3 as merged (PR #5975). Link sections 1.4, 2.1, 2.4 to Issue #6140 (request_storage_ranges refactor). Add new section 2.8 for correctness bug fixes. Update timeline accordingly. --- docs/roadmaps/snap_sync_roadmap.md | 151 ++++++----------------------- 1 file changed, 30 insertions(+), 121 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 8961f92d28a..12b27f4a656 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -119,31 +119,9 @@ Reduce snap sync time by 50% or more through parallelization, batching optimizat --- -### 1.2 Parallel Account Range Requests +### 1.2 ~~Parallel Account Range Requests~~ (Discarded) -**Current State:** Account ranges are requested from peers sequentially within each chunk. - -**Proposed Change:** Increase parallelism by: -1. Using a work-stealing task pool for account range chunks -2. Implementing adaptive chunk sizing based on peer response times -3. Adding peer quality scoring to prefer faster peers - -**Implementation:** -```rust -// Current: Fixed 800 chunks, sequential processing -let chunk_count = 800; - -// Proposed: Adaptive chunking with parallel execution -struct AdaptiveChunker { - min_chunk_size: U256, - max_concurrent: usize, - peer_scores: HashMap, -} -``` - -**Expected Impact:** 2-3x faster account range download - -**Effort:** Medium (2-3 weeks) +> Discarded — not needed after profiling. --- @@ -180,7 +158,7 @@ Adjust batch sizes based on: --- -### 1.4 Reduce Busy-Wait Loops +### 1.4 Reduce Busy-Wait Loops (Issue #6140 — Step 9) **Current State:** Multiple locations use `tokio::time::sleep()` in loops: - `sync_cycle_snap()`: 100ms sleep waiting for headers @@ -213,34 +191,9 @@ match tokio::time::timeout( --- -### 1.5 Memory-Bounded Structures +### 1.5 ~~Memory-Bounded Structures~~ (Discarded) -**Current State:** -- `accounts_by_root_hash` in `request_storage_ranges()` is unbounded -- Can grow to gigabytes on mainnet - -**Proposed Change:** -```rust -// Add memory limits with spill-to-disk -struct BoundedAccountMap { - in_memory: BTreeMap, - max_memory_bytes: usize, - spill_dir: PathBuf, -} - -impl BoundedAccountMap { - fn insert(&mut self, key: H256, value: AccountsWithStorage) { - if self.memory_usage() > self.max_memory_bytes { - self.spill_to_disk(); - } - self.in_memory.insert(key, value); - } -} -``` - -**Expected Impact:** Stable memory usage, prevents OOM on large states - -**Effort:** Medium (2 weeks) +> Discarded — not a real bottleneck in practice. --- @@ -311,30 +264,9 @@ fn max_requests_for_peer(&self, peer_id: &H256) -> u32 { --- -### 1.8 Parallel Storage Healing +### 1.8 ~~Parallel Storage Healing~~ (Discarded) -**Current State:** Storage healing processes accounts sequentially within batches. - -**Proposed Change:** Use rayon for parallel storage trie healing: - -```rust -// Current -for account in accounts_to_heal { - heal_storage_for_account(account).await?; -} - -// Proposed -accounts_to_heal - .par_iter() - .map(|account| heal_storage_for_account(account)) - .collect::, _>>()?; -``` - -**Caveat:** Need to handle DB write contention carefully. - -**Expected Impact:** 2-4x faster storage healing - -**Effort:** High (3 weeks) +> Discarded — DB write contention makes this impractical without a larger storage refactor. --- @@ -345,7 +277,7 @@ Make the codebase clear, well-documented, and easy for new contributors to under --- -### 2.1 Extract Context Structs +### 2.1 Extract Context Structs (Issue #6140 — Steps 5, 6) **Current State:** Functions with many parameters: ```rust @@ -446,44 +378,11 @@ Add comments explaining non-obvious logic, especially: --- -### 2.3 Consolidate Error Handling - -**Current State:** Inconsistent error handling: -```rust -// Silent drop -if let Err(_) = sender.send(headers) { break; } - -// Expect with message -.expect("We shouldn't have a rocksdb error here") - -// Proper propagation -store.get_block_header(number)? -``` - -**Proposed Change:** Standardize on: -1. Use `?` for propagation -2. Use `tracing::warn!` for recoverable errors -3. Use `tracing::error!` before returning fatal errors -4. Never use `.expect()` in production paths - -```rust -// Before -if sender.send(headers).is_err() { break; } - -// After -if let Err(e) = sender.send(headers) { - warn!("Header channel closed unexpectedly: {}", e); - break; -} -``` - -**Files Affected:** All snap sync modules - -**Effort:** Low (1 week) +### 2.3 Consolidate Error Handling (Merged — PR #5975) --- -### 2.4 Extract Helper Functions +### 2.4 Extract Helper Functions (Issue #6140 — Steps 3, 4) **Current State:** Duplicated patterns identified: - Snapshot dumping (4 occurrences) - **Partially addressed** @@ -644,6 +543,18 @@ impl Default for SnapSyncConfig { --- +### 2.8 Fix Correctness Bugs in `request_storage_ranges` (Issue #6140 — Steps 1, 2) + +**Current State:** Two locations crash the node on recoverable errors: +- `panic!("Should have found the account hash")` (line 735) +- `.expect()` calls on store lookups (lines 554-558) + +**Proposed Change:** Replace with proper error propagation using `SnapError::InternalError` and `?` operator. + +**Effort:** Very low (< 1 day) + +--- + ## Success Metrics ### Phase 1: Performance @@ -686,28 +597,26 @@ impl Default for SnapSyncConfig { ``` Week 1-2: 1.1 Parallel Header Download (complete PR #6059) -Week 2-3: 1.4 Reduce Busy-Wait Loops +Week 2-3: 1.4 Reduce Busy-Wait Loops (Issue #6140) Week 3-4: 1.6 Async Disk I/O -Week 4-6: 1.2 Parallel Account Range Requests -Week 6-8: 1.3 Optimize Trie Node Batching -Week 8-10: 1.5 Memory-Bounded Structures -Week 10-12: 1.7 Peer Connection Optimization -Week 12+: 1.8 Parallel Storage Healing (stretch goal) +Week 4-6: 1.3 Optimize Trie Node Batching +Week 6-8: 1.7 Peer Connection Optimization ``` ### Phase 2: Code Quality (10 weeks) ``` -Week 1: 2.1 Extract Context Structs -Week 1-2: 2.3 Consolidate Error Handling -Week 2-3: 2.4 Extract Helper Functions +Week 1: 2.8 Fix Correctness Bugs (Issue #6140) +Week 1: 2.1 Extract Context Structs (Issue #6140) +Week 1-2: 2.4 Extract Helper Functions (Issue #6140) +Week 2-3: 2.3 Consolidate Error Handling (Merged — PR #5975) Week 3-5: 2.2 Comprehensive Documentation Week 5-7: 2.7 Configuration Externalization Week 7-10: 2.5 State Machine Refactor Week 8-12: 2.6 Test Coverage Improvement (parallel) ``` -**Total Duration:** ~16 weeks (phases overlap) +**Total Duration:** ~14 weeks (phases overlap) --- From c9d23280132b33308dfc1d07c70b1787773ba8d5 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 17:25:13 -0300 Subject: [PATCH 03/16] Remove "Unbounded memory structures" from bottlenecks table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not an issue on mainnet as confirmed by fedacking — accounts_by_root_hash doesn't grow to gigabytes due to limited accounts with storage. --- docs/roadmaps/snap_sync_roadmap.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 12b27f4a656..3b4a3a623b7 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -77,7 +77,6 @@ Based on code analysis and profiling data: | Single-threaded account range processing | `request_account_range()` | Underutilizes peers | High | | Inefficient trie node batching | `heal_state_trie()`, `heal_storage_trie()` | Excessive DB writes | High | | Busy-wait loops | Multiple locations | CPU waste | Medium | -| Unbounded memory structures | `accounts_by_root_hash` | Memory pressure | Medium | | Synchronous disk I/O | Snapshot dumping | Blocks network operations | Medium | ### Existing Code Quality Issues From d25c75ce8e8fd10f33bdc5947ea48ed1d703f4bf Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 17:30:08 -0300 Subject: [PATCH 04/16] Address fedacking's review comments on snap sync roadmap - Remove incorrect "Single-threaded account range processing" bottleneck (requests are already parallel between chunks) - Fix "Inefficient trie node batching" description (writes are already batched, note that impact needs measurement) - Clarify busy-wait loops only happen when no peers are available - Fix 1.6 Async Disk I/O description (already inside spawn_blocking, change is just for directory operations with tokio::fs) - Add complexity warning to 1.7 Adaptive Peer Timeouts --- docs/roadmaps/snap_sync_roadmap.md | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 3b4a3a623b7..12af72cfcbe 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -74,10 +74,9 @@ Based on code analysis and profiling data: | Bottleneck | Location | Impact | Priority | |------------|----------|--------|----------| | Sequential header download | `sync_cycle_snap()` | Blocks state download start | Critical | -| Single-threaded account range processing | `request_account_range()` | Underutilizes peers | High | -| Inefficient trie node batching | `heal_state_trie()`, `heal_storage_trie()` | Excessive DB writes | High | -| Busy-wait loops | Multiple locations | CPU waste | Medium | -| Synchronous disk I/O | Snapshot dumping | Blocks network operations | Medium | +| Trie node batching | `heal_state_trie()`, `heal_storage_trie()` | Writes are batched but could use `put_batch_no_alloc` | Medium | +| Busy-wait loops | Multiple locations | CPU waste (only when no peers available) | Medium | +| Disk I/O not using `tokio::fs` | Snapshot dumping | Already in `spawn_blocking`, but should use async fs | Low | ### Existing Code Quality Issues @@ -129,7 +128,7 @@ Reduce snap sync time by 50% or more through parallelization, batching optimizat **Current State:** - `NODE_BATCH_SIZE = 500` nodes per request - `STORAGE_BATCH_SIZE = 300` accounts per batch -- Individual DB writes with `put_batch()` +- DB writes use `put_batch()` (already batched) **Proposed Changes:** @@ -151,7 +150,9 @@ Adjust batch sizes based on: - Peer response latency - Current healing progress -**Expected Impact:** 30-50% reduction in healing phase duration +**Note:** Impact on healing duration needs empirical measurement — current batching may already be sufficient. + +**Expected Impact:** Needs measurement **Effort:** Medium (2 weeks) @@ -198,25 +199,21 @@ match tokio::time::timeout( ### 1.6 Async Disk I/O -**Current State:** Snapshot dumping uses synchronous `std::fs` operations. +**Current State:** Snapshot dumping is already inside `spawn_blocking`, but uses synchronous `std::fs` operations for directory creation and checks. -**Proposed Change:** Use `tokio::fs` for non-blocking I/O: +**Proposed Change:** Use `tokio::fs` for directory operations: ```rust // Current std::fs::create_dir_all(dir)?; -dump_accounts_to_file(&path, chunk)?; // Proposed tokio::fs::create_dir_all(dir).await?; -tokio::task::spawn_blocking(move || { - dump_accounts_to_file(&path, chunk) -}).await??; ``` -**Expected Impact:** Network operations not blocked by disk I/O +**Expected Impact:** Minor — main I/O is already non-blocking via `spawn_blocking`. -**Effort:** Low (1 week) +**Effort:** Low (< 1 week) --- @@ -259,6 +256,8 @@ fn max_requests_for_peer(&self, peer_id: &H256) -> u32 { **Expected Impact:** 20-30% improvement in peer utilization +**Note:** Could introduce excessive complexity. Should evaluate whether the gain justifies the added code. + **Effort:** Medium (2 weeks) --- From f04b6f3c8a529432410c59f5a5f26522b5873023 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 17:34:24 -0300 Subject: [PATCH 05/16] Add Issue #6140 steps summary to snap sync roadmap Adds a table with all 9 refactoring steps for request_storage_ranges, mapping each step to the roadmap section it addresses, with dependency graph and execution order. --- docs/roadmaps/snap_sync_roadmap.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 12af72cfcbe..44db38872d3 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -553,6 +553,35 @@ impl Default for SnapSyncConfig { --- +### Issue #6140 — Refactor `request_storage_ranges` (Steps Summary) + +9-step plan to refactor `request_storage_ranges` in `crates/networking/p2p/snap/client.rs`. Each step is one independently correct commit. Full details in [Issue #6140](https://github.com/lambdaclass/ethrex/issues/6140). + +| Step | Description | Sections | Risk | +|------|-------------|----------|------| +| 1 | Replace `panic!` with proper error return | 2.8 | Very low | +| 2 | Replace `.expect()` with `?` operator | 2.8 | Very low | +| 3 | Extract `ensure_snapshot_dir` helper (4 occurrences) | 2.4 | Very low | +| 4 | Extract big-account chunking helper — DRY (~70 dup lines) | 2.4 | Low | +| 5 | Introduce `TaskTracker` struct for task counting | 2.1 | Very low | +| 6 | Extract result processing into `StorageDownloadState.process_result()` | 2.1 | Medium | +| 7 | Track buffer size incrementally (O(1) instead of O(n)) | — | Low | +| 8 | Remove `accounts_done` HashMap (inline removal) | — | Low | +| 9 | Replace busy-poll (`try_recv` + `sleep`) with `tokio::select!` | 1.4 | Medium | + +**Dependencies:** +``` +Steps 1, 2, 3, 5 — independent +Step 4 — independent +Step 6 — depends on Steps 4, 5 +Steps 7, 8 — depend on Step 6 +Step 9 — depends on Step 6 +``` + +**Execution order:** 1 → 2 → 3 → 5 → 4 → 6 → 7 → 8 → 9 + +--- + ## Success Metrics ### Phase 1: Performance From 73c67402574a7ecca338dd4662f617ff61c29524 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 17:49:58 -0300 Subject: [PATCH 06/16] Add fedacking's review improvements to snap sync roadmap Add 9 new sections (2.9-2.17) from fedacking's PR #5975 review: - 2.9: Fix snap protocol capability bug (ETH vs SNAP) - 2.10: Add spawn_blocking to bytecodes handler - 2.11: Remove dead DumpError.contents field - 2.12: Use JoinSet instead of channels for workers - 2.13: Self-contained StorageTask with hashes instead of indices - 2.14: Move snap client methods off PeerHandler - 2.15: Guard write_set in account path - 2.16: Healing code unification (generic trie healing) - 2.17: Use existing constants for magic numbers Also extend 2.1 description to include AccountStorageRoots SoA simplification and named structs for tuple types. Update timeline with new items. --- docs/roadmaps/snap_sync_roadmap.md | 122 ++++++++++++++++++++++++++--- 1 file changed, 113 insertions(+), 9 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 44db38872d3..bf9e29f0c20 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -277,6 +277,8 @@ Make the codebase clear, well-documented, and easy for new contributors to under ### 2.1 Extract Context Structs (Issue #6140 — Steps 5, 6) +**Also includes:** `AccountStorageRoots` simplification — replace `BTreeMap, Vec<(H256, H256)>)>` with SoA approach and named struct instead of tuple. Named structs for channel types in `healing/state.rs:106` and worker return types in `client.rs:142`. + **Current State:** Functions with many parameters: ```rust #[allow(clippy::too_many_arguments)] @@ -553,6 +555,99 @@ impl Default for SnapSyncConfig { --- +### 2.9 Fix Snap Protocol Capability Bug + +**Current State:** All three `get_best_peer()` calls in `snap/client.rs` (lines 229, 423, 958) use `SUPPORTED_ETH_CAPABILITIES` instead of `SUPPORTED_SNAP_CAPABILITIES`. This selects peers that may not support the snap protocol. The healing modules already use the correct capabilities. + +**Proposed Change:** Replace `SUPPORTED_ETH_CAPABILITIES` with `SUPPORTED_SNAP_CAPABILITIES` in all three call sites. + +**Effort:** Very low (3 lines) + +--- + +### 2.10 Add `spawn_blocking` to Bytecodes Handler + +**Current State:** `process_byte_codes_request` in `snap/server.rs:107` calls `store.get_account_code()` (blocking I/O) without `spawn_blocking`. The other three handlers all use it correctly. Can block the tokio runtime. + +**Proposed Change:** Wrap the handler body in `tokio::task::spawn_blocking`, matching the pattern of the other handlers. + +**Effort:** Very low (small) + +--- + +### 2.11 Remove Dead `DumpError.contents` Field + +**Current State:** `DumpError` in `snap/error.rs:136` has a `contents: Vec` field that holds the full snapshot chunk (up to 64MB). This was for a retry mechanism that no longer exists. The custom `Debug` impl (line 140) was added to avoid printing the huge contents. + +**Proposed Change:** Remove `contents` field, replace custom `Debug` impl with `#[derive(Debug)]`. + +**Effort:** Very low (small, frees memory) + +--- + +### 2.12 Use `JoinSet` Instead of Channels for Workers + +**Current State:** Both `request_account_range` (line 141) and `request_storage_ranges` (line 589) use `mpsc::channel` for worker communication. If a worker panics, the message is lost silently and the main loop may hang waiting for results. + +**Proposed Change:** Migrate to `tokio::task::JoinSet` which propagates panics and handles task lifecycle. The bytecodes path already uses `JoinSet` as a reference. + +**Effort:** Medium (1-2 weeks) + +--- + +### 2.13 Self-Contained `StorageTask` with Hashes + +**Current State:** `StorageTask` in `snap/client.rs:77` references `accounts_by_root_hash` by index (`start_index`, `end_index`). Any mutation of the vector would silently corrupt in-flight tasks. The task is not self-contained. + +**Proposed Change:** Include actual account hashes and storage roots in `StorageTask` instead of indices. This makes tasks self-contained and eliminates the implicit coupling to the vector. + +**Effort:** Medium (1 week) + +--- + +### 2.14 Move Snap Client Methods Off `PeerHandler` + +**Current State:** `request_account_range`, `request_storage_ranges`, etc. are `impl PeerHandler` methods in `snap/client.rs`. These are complex orchestration functions (task queues, workers, file I/O), not peer operations. + +**Proposed Change:** Make them standalone functions that take `&mut PeerHandler` as a parameter. This clarifies that `PeerHandler` is a dependency, not the owner of this logic. + +**Effort:** Low (1 week) + +--- + +### 2.15 Guard `write_set` in Account Path + +**Current State:** `request_account_range` (line 176) spawns disk-write tasks without checking if one is already pending. The storage path (line 629) already does `!disk_joinset.is_empty()` check. Missing the guard can lead to multiple concurrent writes. + +**Proposed Change:** Add the same `!disk_joinset.is_empty()` guard to the account range disk write path, matching the storage path pattern. + +**Effort:** Very low (small) + +--- + +### 2.16 Healing Code Unification + +**Current State:** `healing/state.rs` (~420 lines) and `healing/storage.rs` (~530 lines) implement the same trie healing algorithm. Differences: path representation (single vs double nibbles) and leaf type (accounts vs U256). Lots of duplicated logic. + +**Proposed Change:** Extract a generic healing function parameterized by path and leaf type. Both modules would call into the shared implementation. + +**Effort:** High (3+ weeks) + +--- + +### 2.17 Use Existing Constants for Magic Numbers + +**Current State:** Several magic numbers in `snap/client.rs` already have named constants in `constants.rs` that aren't being used: +- Line 569: `300` → should be `STORAGE_BATCH_SIZE` +- Lines 827, 862: `H256::repeat_byte(0xff)` → should be `HASH_MAX` +- Line 111: `800` → should be a named constant + +**Proposed Change:** Replace magic numbers with existing constants, add new constant for `800`. + +**Effort:** Very low (trivial) + +--- + ### Issue #6140 — Refactor `request_storage_ranges` (Steps Summary) 9-step plan to refactor `request_storage_ranges` in `crates/networking/p2p/snap/client.rs`. Each step is one independently correct commit. Full details in [Issue #6140](https://github.com/lambdaclass/ethrex/issues/6140). @@ -633,17 +728,26 @@ Week 6-8: 1.7 Peer Connection Optimization ### Phase 2: Code Quality (10 weeks) ``` -Week 1: 2.8 Fix Correctness Bugs (Issue #6140) -Week 1: 2.1 Extract Context Structs (Issue #6140) -Week 1-2: 2.4 Extract Helper Functions (Issue #6140) -Week 2-3: 2.3 Consolidate Error Handling (Merged — PR #5975) -Week 3-5: 2.2 Comprehensive Documentation -Week 5-7: 2.7 Configuration Externalization -Week 7-10: 2.5 State Machine Refactor -Week 8-12: 2.6 Test Coverage Improvement (parallel) +Week 1: 2.9 Fix snap protocol capability bug (3 lines) +Week 1: 2.10 Add spawn_blocking to bytecodes handler +Week 1: 2.11 Remove DumpError.contents dead field +Week 1: 2.17 Use existing constants for magic numbers +Week 1: 2.15 Guard write_set in account path +Week 1: 2.8 Fix Correctness Bugs (Issue #6140) +Week 1-2: 2.1 Extract Context Structs (Issue #6140) +Week 1-2: 2.4 Extract Helper Functions (Issue #6140) +Week 2-3: 2.13 Self-contained StorageTask with hashes +Week 2-3: 2.3 Consolidate Error Handling (Merged — PR #5975) +Week 3-4: 2.12 Use JoinSet for snap workers +Week 3-5: 2.2 Comprehensive Documentation +Week 4-5: 2.14 Move snap client methods off PeerHandler +Week 5-7: 2.7 Configuration Externalization +Week 7-10: 2.5 State Machine Refactor +Week 8-12: 2.6 Test Coverage Improvement (parallel) +Week 10-14: 2.16 Healing Code Unification ``` -**Total Duration:** ~14 weeks (phases overlap) +**Total Duration:** ~16 weeks (phases overlap) --- From 0b54b5f10606fb0c3be790732f73063a08a1bbf5 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 18:40:36 -0300 Subject: [PATCH 07/16] Mark section 2.10 (spawn_blocking for bytecodes handler) as done in roadmap. Already fixed on the refactor/snapsync-healing-unification branch. --- docs/roadmaps/snap_sync_roadmap.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index bf9e29f0c20..383829dac33 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -565,13 +565,9 @@ impl Default for SnapSyncConfig { --- -### 2.10 Add `spawn_blocking` to Bytecodes Handler +### 2.10 Add `spawn_blocking` to Bytecodes Handler — ✅ DONE -**Current State:** `process_byte_codes_request` in `snap/server.rs:107` calls `store.get_account_code()` (blocking I/O) without `spawn_blocking`. The other three handlers all use it correctly. Can block the tokio runtime. - -**Proposed Change:** Wrap the handler body in `tokio::task::spawn_blocking`, matching the pattern of the other handlers. - -**Effort:** Very low (small) +**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. The function in `snap/server.rs:108-131` is already `async fn` with `spawn_blocking`, matching the pattern of all other handlers. --- @@ -729,7 +725,7 @@ Week 6-8: 1.7 Peer Connection Optimization ``` Week 1: 2.9 Fix snap protocol capability bug (3 lines) -Week 1: 2.10 Add spawn_blocking to bytecodes handler +Week 1: 2.10 Add spawn_blocking to bytecodes handler (✅ DONE) Week 1: 2.11 Remove DumpError.contents dead field Week 1: 2.17 Use existing constants for magic numbers Week 1: 2.15 Guard write_set in account path From 6965c17d4d0ba00eb4b10b6fbfcbd50b053afb29 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 18:43:14 -0300 Subject: [PATCH 08/16] Mark section 2.11 (remove dead DumpError.contents field) as done in roadmap. Already fixed on the refactor/snapsync-healing-unification branch. --- docs/roadmaps/snap_sync_roadmap.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 383829dac33..ac3329720bb 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -571,13 +571,9 @@ impl Default for SnapSyncConfig { --- -### 2.11 Remove Dead `DumpError.contents` Field +### 2.11 Remove Dead `DumpError.contents` Field — ✅ DONE -**Current State:** `DumpError` in `snap/error.rs:136` has a `contents: Vec` field that holds the full snapshot chunk (up to 64MB). This was for a retry mechanism that no longer exists. The custom `Debug` impl (line 140) was added to avoid printing the huge contents. - -**Proposed Change:** Remove `contents` field, replace custom `Debug` impl with `#[derive(Debug)]`. - -**Effort:** Very low (small, frees memory) +**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. `DumpError` in `snap/error.rs:132-137` no longer has the `contents` field and uses `#[derive(Debug, thiserror::Error)]` instead of a custom `Debug` impl. --- @@ -726,7 +722,7 @@ Week 6-8: 1.7 Peer Connection Optimization ``` Week 1: 2.9 Fix snap protocol capability bug (3 lines) Week 1: 2.10 Add spawn_blocking to bytecodes handler (✅ DONE) -Week 1: 2.11 Remove DumpError.contents dead field +Week 1: 2.11 Remove DumpError.contents dead field (✅ DONE) Week 1: 2.17 Use existing constants for magic numbers Week 1: 2.15 Guard write_set in account path Week 1: 2.8 Fix Correctness Bugs (Issue #6140) From a3df9755373e75a31124a290b9163187a7d3b804 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 18:50:42 -0300 Subject: [PATCH 09/16] Mark section 2.9 (snap protocol capability bug) as done in roadmap. Already fixed on the refactor/snapsync-healing-unification branch, PR #6154 for main. --- docs/roadmaps/snap_sync_roadmap.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index ac3329720bb..db4a9364302 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -555,13 +555,9 @@ impl Default for SnapSyncConfig { --- -### 2.9 Fix Snap Protocol Capability Bug +### 2.9 Fix Snap Protocol Capability Bug — ✅ DONE -**Current State:** All three `get_best_peer()` calls in `snap/client.rs` (lines 229, 423, 958) use `SUPPORTED_ETH_CAPABILITIES` instead of `SUPPORTED_SNAP_CAPABILITIES`. This selects peers that may not support the snap protocol. The healing modules already use the correct capabilities. - -**Proposed Change:** Replace `SUPPORTED_ETH_CAPABILITIES` with `SUPPORTED_SNAP_CAPABILITIES` in all three call sites. - -**Effort:** Very low (3 lines) +**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. All `get_best_peer()` calls in `snap/client.rs` now use `SUPPORTED_SNAP_CAPABILITIES`. PR #6154 provides the same fix for main. --- @@ -720,7 +716,7 @@ Week 6-8: 1.7 Peer Connection Optimization ### Phase 2: Code Quality (10 weeks) ``` -Week 1: 2.9 Fix snap protocol capability bug (3 lines) +Week 1: 2.9 Fix snap protocol capability bug (✅ DONE) Week 1: 2.10 Add spawn_blocking to bytecodes handler (✅ DONE) Week 1: 2.11 Remove DumpError.contents dead field (✅ DONE) Week 1: 2.17 Use existing constants for magic numbers From 0919b624626f35b3a6be969d1e543d46bd723efb Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 18:53:02 -0300 Subject: [PATCH 10/16] Mark section 2.14 (move snap client methods off PeerHandler) as done in roadmap. Already fixed on the refactor/snapsync-healing-unification branch. --- docs/roadmaps/snap_sync_roadmap.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index db4a9364302..1d1dc274be6 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -593,13 +593,9 @@ impl Default for SnapSyncConfig { --- -### 2.14 Move Snap Client Methods Off `PeerHandler` +### 2.14 Move Snap Client Methods Off `PeerHandler` — ✅ DONE -**Current State:** `request_account_range`, `request_storage_ranges`, etc. are `impl PeerHandler` methods in `snap/client.rs`. These are complex orchestration functions (task queues, workers, file I/O), not peer operations. - -**Proposed Change:** Make them standalone functions that take `&mut PeerHandler` as a parameter. This clarifies that `PeerHandler` is a dependency, not the owner of this logic. - -**Effort:** Low (1 week) +**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. Snap client methods were extracted from `peer_handler.rs` to `snap/client.rs` and converted from `PeerHandler` methods to standalone functions taking `peers: &mut PeerHandler` as a parameter. --- @@ -728,7 +724,7 @@ Week 2-3: 2.13 Self-contained StorageTask with hashes Week 2-3: 2.3 Consolidate Error Handling (Merged — PR #5975) Week 3-4: 2.12 Use JoinSet for snap workers Week 3-5: 2.2 Comprehensive Documentation -Week 4-5: 2.14 Move snap client methods off PeerHandler +Week 4-5: 2.14 Move snap client methods off PeerHandler (✅ DONE) Week 5-7: 2.7 Configuration Externalization Week 7-10: 2.5 State Machine Refactor Week 8-12: 2.6 Test Coverage Improvement (parallel) From ebf3b2234c1fdab40503f8dd057a80c03af58df4 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 18:54:10 -0300 Subject: [PATCH 11/16] Update roadmap sections 2.9, 2.10, 2.11, 2.14 to reference merged PR #5975 instead of the now-merged refactor/snapsync-healing-unification branch. --- docs/roadmaps/snap_sync_roadmap.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 1d1dc274be6..ee50fb521b7 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -557,19 +557,19 @@ impl Default for SnapSyncConfig { ### 2.9 Fix Snap Protocol Capability Bug — ✅ DONE -**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. All `get_best_peer()` calls in `snap/client.rs` now use `SUPPORTED_SNAP_CAPABILITIES`. PR #6154 provides the same fix for main. +**Status:** Merged in #5975. All `get_best_peer()` calls in `snap/client.rs` now use `SUPPORTED_SNAP_CAPABILITIES`. --- ### 2.10 Add `spawn_blocking` to Bytecodes Handler — ✅ DONE -**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. The function in `snap/server.rs:108-131` is already `async fn` with `spawn_blocking`, matching the pattern of all other handlers. +**Status:** Merged in #5975. The function in `snap/server.rs:108-131` is `async fn` with `spawn_blocking`, matching the pattern of all other handlers. --- ### 2.11 Remove Dead `DumpError.contents` Field — ✅ DONE -**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. `DumpError` in `snap/error.rs:132-137` no longer has the `contents` field and uses `#[derive(Debug, thiserror::Error)]` instead of a custom `Debug` impl. +**Status:** Merged in #5975. `DumpError` in `snap/error.rs:132-137` no longer has the `contents` field and uses `#[derive(Debug, thiserror::Error)]` instead of a custom `Debug` impl. --- @@ -595,7 +595,7 @@ impl Default for SnapSyncConfig { ### 2.14 Move Snap Client Methods Off `PeerHandler` — ✅ DONE -**Status:** Already fixed on `refactor/snapsync-healing-unification` branch. Snap client methods were extracted from `peer_handler.rs` to `snap/client.rs` and converted from `PeerHandler` methods to standalone functions taking `peers: &mut PeerHandler` as a parameter. +**Status:** Merged in #5975. Snap client methods were extracted from `peer_handler.rs` to `snap/client.rs` and converted from `PeerHandler` methods to standalone functions taking `peers: &mut PeerHandler` as a parameter. --- From f488de7f5f3bd8f16d700e0cc19cc57e7e2c4f12 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Feb 2026 19:56:33 -0300 Subject: [PATCH 12/16] Add sections 1.9, 1.10, 1.11 to roadmap and mark 1.11 as done (#6136) --- docs/roadmaps/snap_sync_roadmap.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index ee50fb521b7..463821ab4e3 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -266,6 +266,18 @@ fn max_requests_for_peer(&self, peer_id: &H256) -> u32 { > Discarded — DB write contention makes this impractical without a larger storage refactor. +### 1.9 Bytes for Trie Values — O(1) Clones (PR #6057 - In Progress) + +Use `Bytes` instead of `Vec` for trie values to enable O(1) reference-counted clones in cache lookups, avoiding expensive deep copies. + +### 1.10 Snap Sync Benchmark Tool (PR #6108 - In Progress) + +Python tool (`tooling/sync/sync_benchmark.py`) to analyze snap sync performance from container logs, identifying bottlenecks per phase. + +### 1.11 Per-Phase Timing Breakdown in Slack Notifications (✅ DONE — Merged in #6136) + +Surfaces per-phase completion timings (Block Headers, Account Ranges, Storage Ranges, Healing, etc.) directly in Slack notifications from the multisync monitoring script, so performance bottlenecks are visible at a glance. + --- ## Phase 2: Code Quality & Maintainability @@ -707,6 +719,9 @@ Week 2-3: 1.4 Reduce Busy-Wait Loops (Issue #6140) Week 3-4: 1.6 Async Disk I/O Week 4-6: 1.3 Optimize Trie Node Batching Week 6-8: 1.7 Peer Connection Optimization + 1.9 Bytes for trie values (PR #6057) + 1.10 Snap sync benchmark tool (PR #6108) + 1.11 Per-phase timing breakdown (✅ DONE) ``` ### Phase 2: Code Quality (10 weeks) From 97a8e7f8c948a36c0ee3413f2796b7b04d6cdf9b Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Thu, 12 Mar 2026 23:37:22 -0300 Subject: [PATCH 13/16] docs(l1): update snap sync roadmap with review fixes and Phase 3 --- docs/roadmaps/snap_sync_roadmap.md | 310 +++++++++++++++++++++++------ 1 file changed, 245 insertions(+), 65 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 463821ab4e3..ba79d17cbae 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -13,7 +13,7 @@ This roadmap outlines a strategic plan to improve the ethrex snap sync module in 1. **Phase 1: Performance Optimization** - Make snap sync as fast as possible 2. **Phase 2: Code Quality & Maintainability** - Make the code clear, readable, and easier to understand -The snap sync module currently comprises ~4,650 lines across 12 files. Our goal is to achieve sync times competitive with geth while maintaining code quality standards. +The snap sync module currently comprises ~4,900 lines across 11 files. Our goal is to achieve sync times competitive with geth while maintaining code quality standards. --- @@ -22,10 +22,11 @@ The snap sync module currently comprises ~4,650 lines across 12 files. Our goal 1. [Current State Analysis](#current-state-analysis) 2. [Phase 1: Performance Optimization](#phase-1-performance-optimization) 3. [Phase 2: Code Quality & Maintainability](#phase-2-code-quality--maintainability) -4. [Success Metrics](#success-metrics) -5. [Risk Assessment](#risk-assessment) -6. [Timeline](#timeline) -7. [Dependencies](#dependencies) +4. [Phase 3: Pipeline Architecture](#phase-3-pipeline-architecture) +5. [Success Metrics](#success-metrics) +6. [Risk Assessment](#risk-assessment) +7. [Timeline](#timeline) +8. [Dependencies](#dependencies) --- @@ -35,26 +36,26 @@ The snap sync module currently comprises ~4,650 lines across 12 files. Our goal | File | Lines | Purpose | |------|-------|---------| -| `sync/snap_sync.rs` | 1,139 | Main snap sync orchestration | -| `snap/client.rs` | 1,416 | Client-side snap protocol requests | -| `sync/healing/storage.rs` | 728 | Storage trie healing | -| `sync/healing/state.rs` | 460 | State trie healing | +| `snap/client.rs` | 1,401 | Client-side snap protocol requests | +| `sync/snap_sync.rs` | 1,181 | Main snap sync orchestration | +| `sync/healing/storage.rs` | 740 | Storage trie healing | +| `sync/healing/state.rs` | 463 | State trie healing | | `sync/full.rs` | 297 | Full sync implementation | -| `snap/server.rs` | 173 | Server-side snap protocol responses | -| `snap/error.rs` | 158 | Unified error types | -| `snap/constants.rs` | 118 | Protocol constants | +| `sync.rs` | 290 | Module root: `Syncer`, `AccountStorageRoots`, `SyncError` | +| `snap/server.rs` | 166 | Server-side snap protocol responses | +| `snap/error.rs` | 147 | Unified error types | +| `snap/constants.rs` | 121 | Protocol constants | | `sync/code_collector.rs` | 100 | Bytecode collection | -| Other modules | ~61 | Supporting code | -| **Total** | **~4,650** | | +| **Total** | **~4,906** | | ### Snap Sync Phases The snap sync process consists of 6 sequential phases: ``` -┌─────────────────────────────────────────────────────────────────────────┐ -│ SNAP SYNC PIPELINE │ -├─────────────────────────────────────────────────────────────────────────┤ +┌──────────────────────────────────────────────────────────────────────────┐ +│ SNAP SYNC PIPELINE │ +├──────────────────────────────────────────────────────────────────────────┤ │ │ │ 1. Header Download ──► 2. Pivot Selection ──► 3. Account Range Download │ │ │ │ @@ -62,9 +63,9 @@ The snap sync process consists of 6 sequential phases: │ 6. Full Sync ◄── 5. Bytecode Download ◄── 4. Storage Range Download │ │ │ │ │ ▼ │ -│ [State Healing & Storage Healing run in parallel with phases 4-5] │ +│ [State Healing & Storage Healing run in parallel with phases 4-5] │ │ │ -└─────────────────────────────────────────────────────────────────────────┘ +└──────────────────────────────────────────────────────────────────────────┘ ``` ### Current Performance Bottlenecks @@ -76,13 +77,13 @@ Based on code analysis and profiling data: | Sequential header download | `sync_cycle_snap()` | Blocks state download start | Critical | | Trie node batching | `heal_state_trie()`, `heal_storage_trie()` | Writes are batched but could use `put_batch_no_alloc` | Medium | | Busy-wait loops | Multiple locations | CPU waste (only when no peers available) | Medium | -| Disk I/O not using `tokio::fs` | Snapshot dumping | Already in `spawn_blocking`, but should use async fs | Low | +| Sync `std::fs` calls | Snapshot dumping | Already in `spawn_blocking`, but directory ops should use `tokio::fs` | Low | ### Existing Code Quality Issues | Issue | Location | Description | |-------|----------|-------------| -| `#[allow(clippy::too_many_arguments)]` | `heal_state_trie()` | 8 parameters - needs context struct | +| `#[allow(clippy::too_many_arguments)]` | `heal_state_trie()`, `process_node_responses()` | 8+ parameters - needs context struct | | Repeated code patterns | `snap/client.rs` | Snapshot dumping logic duplicated | | Magic numbers | Various | Hardcoded values without constants | | Missing documentation | Healing modules | Complex algorithms undocumented | @@ -134,7 +135,7 @@ Reduce snap sync time by 50% or more through parallelization, batching optimizat #### 1.3.1 Use `put_batch_no_alloc()` for Healing ```rust -// Current (healing/state.rs:304) +// Current (sync/healing/state.rs:302, sync/healing/storage.rs:231) // PERF: use put_batch_no_alloc (note that it needs to remove nodes too) // Proposed: Pre-allocate buffers, reuse across batches @@ -144,13 +145,13 @@ struct HealingBatchWriter { } ``` -#### 1.3.2 Dynamic Batch Sizing +#### 1.3.2 Dynamic Batch Sizing (Needs Measurement) Adjust batch sizes based on: - Available memory - Peer response latency - Current healing progress -**Note:** Impact on healing duration needs empirical measurement — current batching may already be sufficient. +**Note:** Impact on healing duration needs empirical measurement — current batching may already be sufficient. Should only pursue if benchmarks show batch sizing is a bottleneck. **Expected Impact:** Needs measurement @@ -160,10 +161,14 @@ Adjust batch sizes based on: ### 1.4 Reduce Busy-Wait Loops (Issue #6140 — Step 9) -**Current State:** Multiple locations use `tokio::time::sleep()` in loops: -- `sync_cycle_snap()`: 100ms sleep waiting for headers -- `request_account_range()`: 10ms sleep waiting for peers -- Healing loops: Various polling intervals +**Current State:** Multiple locations use `try_recv()` + `tokio::time::sleep()` in loops: +- `request_account_range()` (`snap/client.rs:193`): 10ms sleep waiting for peers +- `request_bytecodes()` (`snap/client.rs:383`): 10ms sleep waiting for peers +- `request_storage_ranges()` (`snap/client.rs:646`): 10ms sleep waiting for peers +- `heal_state_trie()` (`sync/healing/state.rs:151`): `try_recv` polling +- `heal_storage_trie()` (`sync/healing/storage.rs:261`): `try_recv` polling + +Note: busy-waits only trigger when no peers are available. **Proposed Change:** Replace with proper async primitives: @@ -197,11 +202,11 @@ match tokio::time::timeout( --- -### 1.6 Async Disk I/O +### 1.6 Use `tokio::fs` for Directory Operations -**Current State:** Snapshot dumping is already inside `spawn_blocking`, but uses synchronous `std::fs` operations for directory creation and checks. +**Current State:** Snapshot dumping is already inside `spawn_blocking`. However, directory creation and existence checks still use synchronous `std::fs` calls. -**Proposed Change:** Use `tokio::fs` for directory operations: +**Proposed Change:** Replace `std::fs` directory operations with `tokio::fs`: ```rust // Current @@ -254,9 +259,9 @@ fn max_requests_for_peer(&self, peer_id: &H256) -> u32 { } ``` -**Expected Impact:** 20-30% improvement in peer utilization +**Expected Impact:** 20-30% improvement in peer utilization (needs empirical measurement) -**Note:** Could introduce excessive complexity. Should evaluate whether the gain justifies the added code. +**Note:** Could introduce excessive complexity for marginal gain. Should only pursue if benchmarks show peer utilization is actually a bottleneck. **Effort:** Medium (2 weeks) @@ -264,7 +269,7 @@ fn max_requests_for_peer(&self, peer_id: &H256) -> u32 { ### 1.8 ~~Parallel Storage Healing~~ (Discarded) -> Discarded — DB write contention makes this impractical without a larger storage refactor. +> Discarded — storage healing is already parallelized via `JoinSet` with up to `MAX_IN_FLIGHT_REQUESTS` (77) concurrent requests. ### 1.9 Bytes for Trie Values — O(1) Clones (PR #6057 - In Progress) @@ -289,7 +294,7 @@ Make the codebase clear, well-documented, and easy for new contributors to under ### 2.1 Extract Context Structs (Issue #6140 — Steps 5, 6) -**Also includes:** `AccountStorageRoots` simplification — replace `BTreeMap, Vec<(H256, H256)>)>` with SoA approach and named struct instead of tuple. Named structs for channel types in `healing/state.rs:106` and worker return types in `client.rs:142`. +**Also includes:** `AccountStorageRoots` simplification (defined in `sync.rs:167`) — replace `BTreeMap, Vec<(H256, H256)>)>` with SoA approach and named struct instead of tuple. Named structs for channel types in `sync/healing/state.rs` and worker return types in `snap/client.rs`. **Current State:** Functions with many parameters: ```rust @@ -329,9 +334,10 @@ async fn heal_state_trie( ``` **Files Affected:** -- `sync/healing/state.rs` -- `sync/healing/storage.rs` +- `sync/healing/state.rs` (`#[allow(clippy::too_many_arguments)]` at line 76) +- `sync/healing/storage.rs` (`#[allow(clippy::too_many_arguments)]` at line 519) - `sync/snap_sync.rs` +- `sync.rs` (`AccountStorageRoots` at line 167) **Effort:** Low (1 week) @@ -557,9 +563,10 @@ impl Default for SnapSyncConfig { ### 2.8 Fix Correctness Bugs in `request_storage_ranges` (Issue #6140 — Steps 1, 2) -**Current State:** Two locations crash the node on recoverable errors: -- `panic!("Should have found the account hash")` (line 735) -- `.expect()` calls on store lookups (lines 554-558) +**Current State:** One location crashes the node on a recoverable error: +- `panic!("Should have found the account hash")` (`snap/client.rs:729`) + +Other `.expect()` calls (`snap/client.rs:630-631`) are on `JoinSet` results, not store lookups. **Proposed Change:** Replace with proper error propagation using `SnapError::InternalError` and `?` operator. @@ -569,25 +576,25 @@ impl Default for SnapSyncConfig { ### 2.9 Fix Snap Protocol Capability Bug — ✅ DONE -**Status:** Merged in #5975. All `get_best_peer()` calls in `snap/client.rs` now use `SUPPORTED_SNAP_CAPABILITIES`. +**Status:** Merged in #5975. All `get_best_peer()` calls now use `SUPPORTED_SNAP_CAPABILITIES`. --- ### 2.10 Add `spawn_blocking` to Bytecodes Handler — ✅ DONE -**Status:** Merged in #5975. The function in `snap/server.rs:108-131` is `async fn` with `spawn_blocking`, matching the pattern of all other handlers. +**Status:** Merged in #5975. Bytecodes handler in `snap/server.rs` uses `spawn_blocking`, matching the pattern of all other handlers. --- ### 2.11 Remove Dead `DumpError.contents` Field — ✅ DONE -**Status:** Merged in #5975. `DumpError` in `snap/error.rs:132-137` no longer has the `contents` field and uses `#[derive(Debug, thiserror::Error)]` instead of a custom `Debug` impl. +**Status:** Merged in #5975. `DumpError` in `snap/error.rs` no longer has the `contents` field and uses `#[derive(Debug, thiserror::Error)]` instead of a custom `Debug` impl. --- ### 2.12 Use `JoinSet` Instead of Channels for Workers -**Current State:** Both `request_account_range` (line 141) and `request_storage_ranges` (line 589) use `mpsc::channel` for worker communication. If a worker panics, the message is lost silently and the main loop may hang waiting for results. +**Current State:** Both `request_account_range` (`snap/client.rs:138`) and `request_storage_ranges` (`snap/client.rs:587`) use `mpsc::channel` for worker communication. If a worker panics, the message is lost silently and the main loop may hang waiting for results. **Proposed Change:** Migrate to `tokio::task::JoinSet` which propagates panics and handles task lifecycle. The bytecodes path already uses `JoinSet` as a reference. @@ -597,7 +604,7 @@ impl Default for SnapSyncConfig { ### 2.13 Self-Contained `StorageTask` with Hashes -**Current State:** `StorageTask` in `snap/client.rs:77` references `accounts_by_root_hash` by index (`start_index`, `end_index`). Any mutation of the vector would silently corrupt in-flight tasks. The task is not self-contained. +**Current State:** `StorageTask` in `snap/client.rs:75` references `accounts_by_root_hash` by index (`start_index`, `end_index`). Any mutation of the vector would silently corrupt in-flight tasks. The task is not self-contained. **Proposed Change:** Include actual account hashes and storage roots in `StorageTask` instead of indices. This makes tasks self-contained and eliminates the implicit coupling to the vector. @@ -607,13 +614,13 @@ impl Default for SnapSyncConfig { ### 2.14 Move Snap Client Methods Off `PeerHandler` — ✅ DONE -**Status:** Merged in #5975. Snap client methods were extracted from `peer_handler.rs` to `snap/client.rs` and converted from `PeerHandler` methods to standalone functions taking `peers: &mut PeerHandler` as a parameter. +**Status:** Merged in #5975. Snap client methods extracted from `peer_handler.rs` to `snap/client.rs` as standalone functions taking `peers: &mut PeerHandler`. --- ### 2.15 Guard `write_set` in Account Path -**Current State:** `request_account_range` (line 176) spawns disk-write tasks without checking if one is already pending. The storage path (line 629) already does `!disk_joinset.is_empty()` check. Missing the guard can lead to multiple concurrent writes. +**Current State:** `request_account_range` (`snap/client.rs:170`) spawns disk-write tasks without checking if one is already pending. The storage path (`snap/client.rs:625`) already does `!disk_joinset.is_empty()` check. Missing the guard can lead to multiple concurrent writes. **Proposed Change:** Add the same `!disk_joinset.is_empty()` guard to the account range disk write path, matching the storage path pattern. @@ -623,7 +630,7 @@ impl Default for SnapSyncConfig { ### 2.16 Healing Code Unification -**Current State:** `healing/state.rs` (~420 lines) and `healing/storage.rs` (~530 lines) implement the same trie healing algorithm. Differences: path representation (single vs double nibbles) and leaf type (accounts vs U256). Lots of duplicated logic. +**Current State:** `sync/healing/state.rs` (~463 lines) and `sync/healing/storage.rs` (~740 lines) implement the same trie healing algorithm. Differences: path representation (single vs double nibbles) and leaf type (accounts vs U256). Lots of duplicated logic. **Proposed Change:** Extract a generic healing function parameterized by path and leaf type. Both modules would call into the shared implementation. @@ -633,12 +640,12 @@ impl Default for SnapSyncConfig { ### 2.17 Use Existing Constants for Magic Numbers -**Current State:** Several magic numbers in `snap/client.rs` already have named constants in `constants.rs` that aren't being used: -- Line 569: `300` → should be `STORAGE_BATCH_SIZE` -- Lines 827, 862: `H256::repeat_byte(0xff)` → should be `HASH_MAX` -- Line 111: `800` → should be a named constant +**Current State:** Most magic numbers have been replaced with named constants: +- `STORAGE_BATCH_SIZE` (used at `snap/client.rs:567`) +- `HASH_MAX` (used at `snap/client.rs:821,856,1275`) +- `ACCOUNT_RANGE_CHUNK_COUNT` (used at `snap/client.rs:109`) -**Proposed Change:** Replace magic numbers with existing constants, add new constant for `800`. +Remaining: channel capacity `1000` appears at lines 138, 370, 587 — could be a named constant. **Effort:** Very low (trivial) @@ -646,7 +653,7 @@ impl Default for SnapSyncConfig { ### Issue #6140 — Refactor `request_storage_ranges` (Steps Summary) -9-step plan to refactor `request_storage_ranges` in `crates/networking/p2p/snap/client.rs`. Each step is one independently correct commit. Full details in [Issue #6140](https://github.com/lambdaclass/ethrex/issues/6140). +9-step plan to refactor `request_storage_ranges` in `snap/client.rs`. Each step is one independently correct commit. Full details in [Issue #6140](https://github.com/lambdaclass/ethrex/issues/6140). | Step | Description | Sections | Risk | |------|-------------|----------|------| @@ -673,6 +680,147 @@ Step 9 — depends on Step 6 --- +## Phase 3: Pipeline Architecture + +### Goal +Replace the sequential sync phase model with a pipelined actor architecture using Spawned, enabling concurrent execution of phases that are currently blocked on each other. + +### Context + +The rest of the p2p layer already uses `spawned_concurrency` actors (`ActorRef`, `#[protocol]`, `#[actor]`) for peer table, discovery, RLPx connections, and tx broadcasting. The snap sync module is the main holdout — it uses raw `tokio::spawn` + `mpsc::channel` for concurrency. + +Spawned is undergoing a major refactor (branch `feat/approach-b`, being tested on `worktree-spawned-migration`). The new API replaces `GenServer`/`GenServerHandle` with `Actor`/`ActorRef`, and uses `#[protocol]` trait macros to define message interfaces. Phase 3 should target the new Spawned API. + +### Motivation + +Phases 1 and 2 optimize individual sync stages but don't challenge the fundamental sequential pipeline: + +``` +Headers → Pivot → Accounts → Storage → Bytecodes → Healing → Full Sync +``` + +In practice, once an account batch is downloaded, its storage and bytecodes are immediately known — there's no reason to wait for ALL accounts before starting storage/bytecode downloads. Similarly, headers can be fetched in the background while state download proceeds. The current code can't express this easily because each phase is a monolithic function that must complete before the next starts. + +With actors, each phase becomes a message-driven pipeline stage: + +``` + ┌──────────────┐ + │ HeaderActor │ (background, feeds into FullSync later) + └──────────────┘ + +┌──────────────┐ ┌───────────────┐ ┌───────────────┐ +│ AccountActor │───►│ StorageActor │───►│ HealingActor │ +│ │───►│ BytecodeActor │ │ │ +└──────────────┘ └───────────────┘ └───────────────┘ +``` + +Each actor owns its own state, peer management, and timing — no shared mutable state or busy-wait coordination. + +--- + +### 3.1 Migrate Snap Sync to Spawned Actors + +**Current State:** `snap_sync.rs` orchestrates everything via sequential function calls. Workers are spawned with `tokio::spawn` and communicate via `mpsc::channel`. Peer acquisition uses `try_recv` + sleep busy-wait loops. + +**Proposed Change:** Define a `#[protocol]` for each sync stage and implement them as `Actor`s: + +```rust +#[protocol] +pub trait AccountDownloaderProtocol: Send + Sync { + fn download_range(&self, start: H256, end: H256) -> Result<(), ActorError>; + fn account_batch_ready(&self, accounts: Vec) -> Result<(), ActorError>; +} + +#[actor(protocol = AccountDownloaderProtocol)] +impl AccountDownloader { + #[started] + async fn started(&mut self, ctx: &Context) { ... } + + async fn handle_download_range(&mut self, start: H256, end: H256) { ... } +} +``` + +**Actors:** +- `HeaderActor` — downloads headers in background, notifies orchestrator on completion +- `AccountActor` — downloads account ranges, sends each batch downstream immediately +- `StorageActor` — receives account batches, starts storage downloads per-batch +- `BytecodeActor` — receives code hashes from account batches, downloads in parallel +- `HealingActor` — starts healing as soon as enough state is available +- `SyncOrchestrator` — coordinates state machine transitions and pivot updates + +**Subsumes:** 1.1 (parallel headers), 2.12 (JoinSet → actors), 2.5 (state machine refactor) + +**Effort:** High (6+ weeks) + +--- + +### 3.2 Peer Scoring + +**Current State:** `get_best_peer()` selects peers by capability match and in-flight request count. There's no tracking of peer throughput, latency, or reliability beyond simple success/failure scores. + +**Proposed Change:** Extend `PeerTable` with richer metrics per peer: +- Rolling average response latency +- Throughput (bytes/second) +- Reliability score (success rate over recent window) + +Actors request peers from `PeerTable` with requirements (e.g., "need a peer for storage range, prefer high-throughput"), and `PeerTable` returns the best match. + +**Subsumes:** 1.7 (adaptive timeouts, request pipelining) + +**Effort:** Medium (2-3 weeks) + +--- + +### 3.3 Pipelined Account → Storage Download + +**Current State:** Storage download starts only after ALL accounts are downloaded. In `snap_sync.rs`, `request_account_range()` must complete before `request_storage_ranges()` is called. + +**Proposed Change:** `AccountActor` sends each completed account batch to `StorageActor` immediately via actor messages. `StorageActor` starts downloading storage for those accounts while more accounts are still being fetched. + +This requires `StorageActor` to handle dynamically growing task queues (new account batches arrive while existing ones are being processed). + +**Expected Impact:** Significant — storage is one of the longest phases and can start much earlier. + +**Effort:** Medium (2-3 weeks, depends on 3.1) + +--- + +### 3.4 Pipelined Bytecode Download + +**Current State:** Bytecodes are downloaded in phase 5, after all storage ranges. Code hashes are known as soon as accounts are downloaded (they're part of the account state). + +**Proposed Change:** `AccountActor` sends code hashes to `BytecodeActor` immediately as accounts arrive. `BytecodeActor` downloads bytecodes in parallel with storage downloads. + +**Expected Impact:** Removes bytecode download as a sequential bottleneck — it becomes fully overlapped with storage. + +**Effort:** Low (1-2 weeks, depends on 3.1) + +--- + +### 3.5 Compute FKV on Insertion + +**Current State:** The flat key-value (FKV) store is populated by a background generator (`flatkeyvalue_generator` in `store.rs`) that runs after sync, iterating the trie to build denormalized lookup entries. This adds post-sync latency before the node is fully operational. + +**Proposed Change:** Compute and insert FKV entries as account/storage data arrives during snap sync, eliminating the post-sync generation step. Each actor writes FKV entries alongside trie nodes in the same batch. + +**Expected Impact:** Eliminates FKV generation as a post-sync step. Node becomes operational immediately after sync completes. + +**Effort:** Medium (2-3 weeks) + +--- + +### Phase 3 Dependency Graph + +``` +3.1 (Actor migration) + ├── 3.2 (Peer scoring) — independent, can start in parallel + ├── 3.3 (Pipelined storage) — depends on 3.1 + ├── 3.4 (Pipelined bytecodes) — depends on 3.1 + └── 3.5 (FKV on insertion) — independent of 3.1, can start earlier +``` + +--- + ## Success Metrics ### Phase 1: Performance @@ -695,6 +843,15 @@ Step 9 — depends on Step 6 | Cyclomatic complexity | TBD | <15 per function | `cargo clippy` | | Functions >100 lines | TBD | 0 | Custom lint | +### Phase 3: Pipeline Architecture + +| Metric | Current | Target | Measurement Method | +|--------|---------|--------|-------------------| +| Phase overlap | 0% (sequential) | >50% | Phase timing breakdown | +| Time from first account to first storage download | Full account phase | <30s | Per-phase logs | +| Post-sync FKV generation time | TBD | 0 (eliminated) | End-to-end benchmark | +| Total sync time improvement over Phase 1 | Baseline | -30% additional | End-to-end benchmark | + --- ## Risk Assessment @@ -706,6 +863,8 @@ Step 9 — depends on Step 6 | Breaking changes to peer protocol | Low | Medium | Hive test suite validation | | Increased complexity | Medium | Medium | Code review; documentation requirements | | Schedule overrun | Medium | Medium | Prioritize high-impact items; iterative delivery | +| Spawned refactor not ready | Medium | High | 3.5 and 3.2 are independent; Phase 1/2 items provide value meanwhile | +| Pipeline ordering bugs | Medium | High | Pipelining introduces data races if actors process out of order; needs careful invariant tracking | --- @@ -746,7 +905,19 @@ Week 8-12: 2.6 Test Coverage Improvement (parallel) Week 10-14: 2.16 Healing Code Unification ``` -**Total Duration:** ~16 weeks (phases overlap) +### Phase 3: Pipeline Architecture (10-14 weeks) + +``` +Week 1-2: 3.5 Compute FKV on insertion (independent, can start early) +Week 1-6: 3.1 Migrate snap sync to Spawned actors (blocked on Spawned refactor) +Week 3-5: 3.2 Peer scoring (can start in parallel with 3.1) +Week 7-9: 3.3 Pipelined account → storage download (depends on 3.1) +Week 7-8: 3.4 Pipelined bytecode download (depends on 3.1) +``` + +**Note:** Phase 3 depends on the Spawned `feat/approach-b` refactor landing. 3.5 and 3.2 can start independently. + +**Total Duration:** ~20+ weeks (all phases overlap) --- @@ -757,6 +928,7 @@ Week 10-14: 2.16 Healing Code Unification | Dependency | Version | Purpose | |------------|---------|---------| | tokio | 1.x | Async runtime | +| spawned-concurrency | `feat/approach-b` | Actor framework (Phase 3) | | rayon | 1.x | Parallel iterators | | tracing | 0.1.x | Logging/metrics | @@ -799,14 +971,22 @@ Week 10-14: 2.16 Healing Code Unification | Location | Issue | Priority | |----------|-------|----------| -| `healing/storage.rs:156` | Better data receiver design | Medium | -| `healing/storage.rs:230` | Use `put_batch_no_alloc` | High | -| `healing/storage.rs:298` | Store error handling | High | -| `healing/state.rs:149` | Peer scoring for responses | Medium | -| `healing/state.rs:194` | Optimize trie leaf reaching | Low | -| `snap/client.rs:567` | Stable sort for binary search | Low | -| `snap/client.rs:599` | Replace with removable structure | Medium | -| `snap/client.rs:983` | Unnecessary unzip/memory | Low | +| `sync/healing/storage.rs:157` | Better data receiver design | Medium | +| `sync/healing/storage.rs:231` | Use `put_batch_no_alloc` | High | +| `sync/healing/storage.rs:299` | Store error handling (`.expect()`) | High | +| `sync/healing/storage.rs:377` | Add error handling | Medium | +| `sync/healing/state.rs:150` | Peer scoring for responses | Medium | +| `sync/healing/state.rs:195` | Optimize trie leaf reaching | Low | +| `sync/healing/state.rs:246` | Check errors for stale block detection | Medium | +| `sync/healing/state.rs:283` | Reuse buffers | Low | +| `sync/healing/state.rs:302` | Use `put_batch_no_alloc` | High | +| `sync/healing/state.rs:346` | Change tuple to struct | Low | +| `snap/client.rs:175` | Check error type and handle properly | Medium | +| `snap/client.rs:281` | Repeated code, consider refactoring | Medium | +| `snap/client.rs:565` | Stable sort for binary search | Low | +| `snap/client.rs:595` | Replace with removable structure | Medium | +| `snap/client.rs:808` | DRY — duplicated big-account logic | Medium | +| `snap/client.rs:976` | Unnecessary unzip/memory | Low | --- @@ -823,5 +1003,5 @@ Week 10-14: 2.16 Healing Code Unification --- -*Document Version: 1.0* -*Last Updated: February 2026* +*Document Version: 1.1* +*Last Updated: March 2026* From 13fca73afc93a4ca7ac7dcc92a252a391f14dc83 Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Mon, 6 Apr 2026 16:04:47 -0300 Subject: [PATCH 14/16] docs(l1): update snap sync roadmap with April 2026 progress --- docs/roadmaps/snap_sync_roadmap.md | 228 +++++++++++++++++++++++------ 1 file changed, 182 insertions(+), 46 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index ba79d17cbae..616ec244b62 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -1,20 +1,23 @@ # Snap Sync Module Roadmap **Author:** Pablo Deymonnaz -**Date:** February 2026 +**Date:** February 2026 (updated April 2026) **Status:** Draft for Review --- ## Executive Summary -This roadmap outlines a strategic plan to improve the ethrex snap sync module in two phases: +This roadmap outlines a strategic plan to improve the ethrex snap sync module in three phases: 1. **Phase 1: Performance Optimization** - Make snap sync as fast as possible 2. **Phase 2: Code Quality & Maintainability** - Make the code clear, readable, and easier to understand +3. **Phase 3: Pipeline Architecture** - Migrate to spawned actors for pipelined concurrent execution The snap sync module currently comprises ~4,900 lines across 11 files. Our goal is to achieve sync times competitive with geth while maintaining code quality standards. +> **April 2026 update:** Spawned 0.5.0 has been [merged](#6295) — the actor framework blocker for Phase 3 is gone. Several new performance PRs (#6410, #6184, #6177, #6159, #6178) have been opened since the original roadmap. Phase 1 now includes trie building optimizations that represent the largest single improvement opportunity (-31% account insertion time). Phase 3 pipelining has been partially achieved without actors (#6184), validating the incremental approach. + --- ## Table of Contents @@ -70,13 +73,17 @@ The snap sync process consists of 6 sequential phases: ### Current Performance Bottlenecks -Based on code analysis and profiling data: +Based on code analysis and mainnet profiling data (PR #6410): | Bottleneck | Location | Impact | Priority | |------------|----------|--------|----------| +| **Trie building in insertion** | `insert_accounts`, `insert_storage` | **75-91% of insertion time** (883s/1184s for accounts, 2357s/2587s for storage on mainnet) | **Critical** | | Sequential header download | `sync_cycle_snap()` | Blocks state download start | Critical | +| Sequential phase pipeline | `snap_sync.rs` orchestration | Bytecodes/storage wait for all accounts to finish | High | +| Redundant code hash pass | `insert_accounts` | Extra full iteration over temp DB | Medium | | Trie node batching | `heal_state_trie()`, `heal_storage_trie()` | Writes are batched but could use `put_batch_no_alloc` | Medium | | Busy-wait loops | Multiple locations | CPU waste (only when no peers available) | Medium | +| SST file intermediate step | Account/storage download | Overlapping key ranges force RocksDB merge during ingestion | Medium | | Sync `std::fs` calls | Snapshot dumping | Already in `spawn_blocking`, but directory ops should use `tokio::fs` | Low | ### Existing Code Quality Issues @@ -271,9 +278,9 @@ fn max_requests_for_peer(&self, peer_id: &H256) -> u32 { > Discarded — storage healing is already parallelized via `JoinSet` with up to `MAX_IN_FLIGHT_REQUESTS` (77) concurrent requests. -### 1.9 Bytes for Trie Values — O(1) Clones (PR #6057 - In Progress) +### 1.9 ~~Bytes for Trie Values — O(1) Clones~~ (Discarded) -Use `Bytes` instead of `Vec` for trie values to enable O(1) reference-counted clones in cache lookups, avoiding expensive deep copies. +> Discarded — PR #6057 closed without merging. ### 1.10 Snap Sync Benchmark Tool (PR #6108 - In Progress) @@ -285,6 +292,86 @@ Surfaces per-phase completion timings (Block Headers, Account Ranges, Storage Ra --- +### 1.12 Optimize Trie Building in Snap Sync Insertion (PR #6410 — In Progress) + +**Current State:** Trie building is 75-91% of insertion time (profiled on mainnet). Account insertion took ~20 minutes and storage insertion ~43 minutes — together 61% of total snap sync time. The trie build is entirely CPU-bound (0% I/O wait). + +**Changes:** +1. **Eliminate redundant code hash iteration pass** — original code iterated all accounts twice (once for code hashes, once for trie). Merged into single pass. +2. **Reuse `nodehash_buffer` across calls** — avoids ~700M allocations on mainnet. +3. **Parallel state trie building across 16 nibble ranges** — splits state trie into 16 independent sub-tries built concurrently. + +**Benchmarks (mainnet, release profile, no validation):** + +| Phase | Before | After | Delta | +|-------|--------|-------|-------| +| Account Insertion | 1184s (19m 44s) | 818s (13m 40s) | **-31%** | +| Storage Insertion | 2587s (43m 7s) | 2433s (40m 30s) | **-6%** | + +| Network | Before | After | Saved | +|---------|--------|-------|-------| +| Hoodi | ~25m | ~13m | ~12m | +| Sepolia | ~90m | ~43m | ~47m | +| Mainnet | ~1h 42m | ~1h 35m | ~7m | + +**Note:** This optimization is orthogonal to the concurrency model — it operates at the trie-building level, below the orchestration layer. No dependency on actor migration. + +**Status:** PR #6410 open, benchmarked on mainnet + +--- + +### 1.13 Pipeline Bytecode Downloads & Background Storage Healing (PR #6184 — In Progress) + +**Current State:** Bytecodes wait for all healing; storage healing must reach 100% before finalization. + +**Changes:** +1. **Concurrent bytecode downloads** — stream code hashes via `mpsc` channel to a concurrent download task running alongside healing. Content-addressed (hash = key), safe regardless of pivot changes. +2. **Background storage healing with 99% threshold** — state healing runs to 100%, but storage healing finalizes at 99% and completes the remaining <1% in a background task after finalization. + +**Benchmarks (Hoodi):** -13% total sync time (489s vs 564s baseline). + +**Note:** Implements Phase 3 pipelining goals (3.3, 3.4) incrementally using `mpsc` channels, without requiring a full actor migration. Validates that the orchestration can be improved without restructuring to actors first. + +**Status:** PR #6184 open + +--- + +### 1.14 Eliminate SST File Intermediate Step (PR #6177 — In Progress) + +Replace SST file writer + ingest flow with direct `WriteBatch` writes to temp RocksDB during download phase. Removes overlapping key range merge overhead. Also merges the two iterator passes in `insert_accounts` into a single pass. + +**Status:** PR #6177 open, needs mainnet testing + +--- + +### 1.15 Optimize Insertion and Healing Write Paths (PR #6159 — In Progress) + +Addresses multiple hot-path bottlenecks found via profiling: +- Single-element `Vec` alloc per key in `put_batch` (~20k allocs per flush) +- `put_batch_no_alloc` actually allocates (encodes all nodes, collects into Vec) +- BTreeMap overwrites in healing batch writes +- Hardcoded 12 threads for trie building + +**Status:** PR #6159 open + +--- + +### 1.16 Disable WAL and Improve Concurrency (PR #6178 — In Progress) + +Disable write-ahead log for snap sync temp DBs (crash recovery not needed) and tune RocksDB concurrency settings. + +**Status:** PR #6178 open + +--- + +### 1.17 Fill All Peer Slots per Tick in Healing Dispatch (PR #6175 — In Progress) + +Current healing dispatch only fills one peer slot per loop iteration. Fill all available slots per tick to maximize network utilization. + +**Status:** PR #6175 open + +--- + ## Phase 2: Code Quality & Maintainability ### Goal @@ -438,7 +525,9 @@ impl ProgressReporter { --- -### 2.5 State Machine Refactor +### 2.5 State Machine Refactor (Consider deferring — subsumed by 3.1) + +> **Note:** This would be subsumed by the actor migration (3.1), where each actor naturally represents a phase with explicit state. Consider skipping this as a standalone item if 3.1 is planned soon. **Current State:** Snap sync phases are implicit in control flow. @@ -592,7 +681,9 @@ Other `.expect()` calls (`snap/client.rs:630-631`) are on `JoinSet` results, not --- -### 2.12 Use `JoinSet` Instead of Channels for Workers +### 2.12 Use `JoinSet` Instead of Channels for Workers (Consider deferring — subsumed by 3.1) + +> **Note:** The actor migration (3.1) would replace both channels and JoinSets with actor messages. Consider skipping this intermediate step if 3.1 is planned soon. **Current State:** Both `request_account_range` (`snap/client.rs:138`) and `request_storage_ranges` (`snap/client.rs:587`) use `mpsc::channel` for worker communication. If a worker panics, the message is lost silently and the main loop may hang waiting for results. @@ -689,7 +780,9 @@ Replace the sequential sync phase model with a pipelined actor architecture usin The rest of the p2p layer already uses `spawned_concurrency` actors (`ActorRef`, `#[protocol]`, `#[actor]`) for peer table, discovery, RLPx connections, and tx broadcasting. The snap sync module is the main holdout — it uses raw `tokio::spawn` + `mpsc::channel` for concurrency. -Spawned is undergoing a major refactor (branch `feat/approach-b`, being tested on `worktree-spawned-migration`). The new API replaces `GenServer`/`GenServerHandle` with `Actor`/`ActorRef`, and uses `#[protocol]` trait macros to define message interfaces. Phase 3 should target the new Spawned API. +**Update (April 2026):** The spawned 0.5.0 migration has been merged (PR #6295, March 31). All existing actors across ethrex now use the new `#[protocol]` + `#[actor]` macro API. The spawned framework blocker is resolved — Phase 3 can proceed whenever the team is ready. + +Additionally, PR #6184 has demonstrated that key pipelining goals (concurrent bytecodes, background storage healing) can be achieved incrementally with `mpsc` channels, without a full actor rewrite. This validates an incremental approach: land targeted optimizations first, then migrate to actors for cleaner architecture. ### Motivation @@ -781,6 +874,8 @@ This requires `StorageActor` to handle dynamically growing task queues (new acco **Expected Impact:** Significant — storage is one of the longest phases and can start much earlier. +**Note:** PR #6184 partially addresses this with background storage healing (99% threshold + background task), but doesn't pipeline the download phase itself. The actor-based approach would go further. + **Effort:** Medium (2-3 weeks, depends on 3.1) --- @@ -793,6 +888,8 @@ This requires `StorageActor` to handle dynamically growing task queues (new acco **Expected Impact:** Removes bytecode download as a sequential bottleneck — it becomes fully overlapped with storage. +**Note:** PR #6184 already implements concurrent bytecode downloads via `mpsc` channel (without actors). If #6184 merges first, the actor migration would formalize the existing pattern. + **Effort:** Low (1-2 weeks, depends on 3.1) --- @@ -863,59 +960,98 @@ This requires `StorageActor` to handle dynamically growing task queues (new acco | Breaking changes to peer protocol | Low | Medium | Hive test suite validation | | Increased complexity | Medium | Medium | Code review; documentation requirements | | Schedule overrun | Medium | Medium | Prioritize high-impact items; iterative delivery | -| Spawned refactor not ready | Medium | High | 3.5 and 3.2 are independent; Phase 1/2 items provide value meanwhile | +| ~~Spawned refactor not ready~~ | ~~Medium~~ | ~~High~~ | ✅ Resolved — spawned 0.5.0 merged in #6295 (March 31, 2026) | | Pipeline ordering bugs | Medium | High | Pipelining introduces data races if actors process out of order; needs careful invariant tracking | --- ## Timeline -### Phase 1: Performance (12 weeks) +### Recommended execution order (updated April 2026) + +The key insight from profiling is that **trie building dominates insertion time** (75-91%). The recommended priority is: + +1. **Land trie building optimizations first** (1.12, #6410) — largest single improvement, orthogonal to concurrency model +2. **Land write path optimizations** (1.14, 1.15, 1.16) — compound on trie improvements +3. **Land pipelining** (1.13, #6184) — concurrent bytecodes + background healing +4. **Phase 2 quick wins** (2.8, 2.17, 2.15, 2.1, 2.4) — low-effort correctness/quality +5. **Actor migration** (3.1) — clean architecture, now unblocked, subsumes 2.5/2.12 +6. **Remaining Phase 2/3 items** — documentation, testing, peer scoring + +### Phase 1: Performance ``` -Week 1-2: 1.1 Parallel Header Download (complete PR #6059) -Week 2-3: 1.4 Reduce Busy-Wait Loops (Issue #6140) -Week 3-4: 1.6 Async Disk I/O -Week 4-6: 1.3 Optimize Trie Node Batching -Week 6-8: 1.7 Peer Connection Optimization - 1.9 Bytes for trie values (PR #6057) - 1.10 Snap sync benchmark tool (PR #6108) - 1.11 Per-phase timing breakdown (✅ DONE) +Priority 1 (high impact, ready now): + 1.12 Optimize trie building (PR #6410) — -31% account insertion + 1.15 Optimize insertion/healing write paths (PR #6159) + 1.14 Eliminate SST file intermediate step (PR #6177) + 1.13 Pipeline bytecodes + background healing (PR #6184) — -13% total + +Priority 2 (medium impact): + 1.16 Disable WAL and improve concurrency (PR #6178) + 1.17 Fill all peer slots per tick in healing (PR #6175) + 1.1 Parallel Header Download (PR #6059) + 1.6 Async Disk I/O (PR #6113) + 1.4 Reduce Busy-Wait Loops (Issue #6140) + +Lower priority / needs measurement: + 1.3 Optimize Trie Node Batching + 1.7 Peer Connection Optimization (PR #6117) + 1.10 Snap sync benchmark tool (PR #6108) + +Done: + 1.11 Per-phase timing breakdown (✅ Merged #6136) + +Discarded: + 1.2 Parallel Account Range Requests + 1.5 Memory-Bounded Structures + 1.8 Parallel Storage Healing + 1.9 Bytes for Trie Values (PR #6057 closed) ``` -### Phase 2: Code Quality (10 weeks) +### Phase 2: Code Quality ``` -Week 1: 2.9 Fix snap protocol capability bug (✅ DONE) -Week 1: 2.10 Add spawn_blocking to bytecodes handler (✅ DONE) -Week 1: 2.11 Remove DumpError.contents dead field (✅ DONE) -Week 1: 2.17 Use existing constants for magic numbers -Week 1: 2.15 Guard write_set in account path -Week 1: 2.8 Fix Correctness Bugs (Issue #6140) -Week 1-2: 2.1 Extract Context Structs (Issue #6140) -Week 1-2: 2.4 Extract Helper Functions (Issue #6140) -Week 2-3: 2.13 Self-contained StorageTask with hashes -Week 2-3: 2.3 Consolidate Error Handling (Merged — PR #5975) -Week 3-4: 2.12 Use JoinSet for snap workers -Week 3-5: 2.2 Comprehensive Documentation -Week 4-5: 2.14 Move snap client methods off PeerHandler (✅ DONE) -Week 5-7: 2.7 Configuration Externalization -Week 7-10: 2.5 State Machine Refactor -Week 8-12: 2.6 Test Coverage Improvement (parallel) -Week 10-14: 2.16 Healing Code Unification +Quick wins (do alongside Phase 1): + 2.8 Fix Correctness Bugs (Issue #6140) + 2.17 Use existing constants for magic numbers + 2.15 Guard write_set in account path + 2.1 Extract Context Structs (Issue #6140) + 2.4 Extract Helper Functions (Issue #6140) + 2.13 Self-contained StorageTask with hashes + +Defer until after actor migration (subsumed by 3.1): + 2.5 State Machine Refactor + 2.12 Use JoinSet for snap workers + +Independent (do when bandwidth available): + 2.2 Comprehensive Documentation + 2.6 Test Coverage Improvement + 2.7 Configuration Externalization + 2.16 Healing Code Unification + +Done: + 2.3 Consolidate Error Handling (✅ Merged #5975) + 2.9 Fix snap protocol capability bug (✅ Merged #5975) + 2.10 Add spawn_blocking to bytecodes handler (✅ Merged #5975) + 2.11 Remove DumpError.contents dead field (✅ Merged #5975) + 2.14 Move snap client methods off PeerHandler (✅ Merged #5975) ``` -### Phase 3: Pipeline Architecture (10-14 weeks) +### Phase 3: Pipeline Architecture ``` -Week 1-2: 3.5 Compute FKV on insertion (independent, can start early) -Week 1-6: 3.1 Migrate snap sync to Spawned actors (blocked on Spawned refactor) -Week 3-5: 3.2 Peer scoring (can start in parallel with 3.1) -Week 7-9: 3.3 Pipelined account → storage download (depends on 3.1) -Week 7-8: 3.4 Pipelined bytecode download (depends on 3.1) +Unblocked (spawned 0.5.0 merged in #6295): + 3.1 Migrate snap sync to Spawned actors + 3.2 Peer scoring (independent of 3.1) + 3.5 Compute FKV on insertion (independent of 3.1) + +After 3.1: + 3.3 Pipelined account → storage download + 3.4 Pipelined bytecode download (partially done by #6184) ``` -**Note:** Phase 3 depends on the Spawned `feat/approach-b` refactor landing. 3.5 and 3.2 can start independently. +**Note:** Spawned 0.5.0 has landed (#6295, March 31). Phase 3 is no longer blocked. However, Phase 1 performance wins should land first since they're orthogonal to the concurrency model and provide immediate measurable impact. **Total Duration:** ~20+ weeks (all phases overlap) @@ -928,7 +1064,7 @@ Week 7-8: 3.4 Pipelined bytecode download (depends on 3.1) | Dependency | Version | Purpose | |------------|---------|---------| | tokio | 1.x | Async runtime | -| spawned-concurrency | `feat/approach-b` | Actor framework (Phase 3) | +| spawned-concurrency | 0.5.0 (✅ merged in #6295) | Actor framework (Phase 3) | | rayon | 1.x | Parallel iterators | | tracing | 0.1.x | Logging/metrics | @@ -1003,5 +1139,5 @@ Week 7-8: 3.4 Pipelined bytecode download (depends on 3.1) --- -*Document Version: 1.1* -*Last Updated: March 2026* +*Document Version: 1.2* +*Last Updated: April 2026* From 88a945ca1894783fab6a201b46c18e0a13ec4504 Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Wed, 15 Apr 2026 12:46:33 -0300 Subject: [PATCH 15/16] docs(l1): update snap sync roadmap with 2026-04-15 progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add §1.18 observability tooling (PR #6470) - Add §1.19 pivot update reliability (PR #6475, issue #6474) - Add §1.20 big-account within-trie parallelization (issue #6477) - Add §1.21 small-account batching (issue #6476) - Add §1.22 decoded TrieLayerCache (PR #6348) - Add §1.23 bloom filter for non-existent storage (PR #6288) - Add §1.24 adaptive request sizing + bisection (PR #6181) - Add §1.25 concurrent bytecode + storage (PR #6205) - Add §1.26 phase completion markers (PR #6189) - Add §2.18 StorageTrieTracker refactor (PR #6171) - Update current-state bottleneck table with small-account and pivot-update findings - Reprioritize timeline: pivot-update crash fix is now priority 0 - Add two risks (pivot crash masks perf work, DB corruption on every crash) - Bump doc version to 1.3 --- docs/roadmaps/snap_sync_roadmap.md | 220 +++++++++++++++++++++++++++-- 1 file changed, 205 insertions(+), 15 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 616ec244b62..84de643b9d2 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -1,7 +1,7 @@ # Snap Sync Module Roadmap -**Author:** Pablo Deymonnaz -**Date:** February 2026 (updated April 2026) +**Author:** Pablo Deymonnaz (original), ElFantasma (ongoing updates) +**Date:** February 2026 (last updated 2026-04-15) **Status:** Draft for Review --- @@ -16,7 +16,17 @@ This roadmap outlines a strategic plan to improve the ethrex snap sync module in The snap sync module currently comprises ~4,900 lines across 11 files. Our goal is to achieve sync times competitive with geth while maintaining code quality standards. -> **April 2026 update:** Spawned 0.5.0 has been [merged](#6295) — the actor framework blocker for Phase 3 is gone. Several new performance PRs (#6410, #6184, #6177, #6159, #6178) have been opened since the original roadmap. Phase 1 now includes trie building optimizations that represent the largest single improvement opportunity (-31% account insertion time). Phase 3 pipelining has been partially achieved without actors (#6184), validating the incremental approach. +> **April 2026 update (initial):** Spawned 0.5.0 has been [merged](#6295) — the actor framework blocker for Phase 3 is gone. Several new performance PRs (#6410, #6184, #6177, #6159, #6178) have been opened since the original roadmap. Phase 1 now includes trie building optimizations that represent the largest single improvement opportunity (-31% account insertion time). Phase 3 pipelining has been partially achieved without actors (#6184), validating the incremental approach. + +> **2026-04-15 update:** Multiple workstreams advanced significantly: +> +> - **Reliability:** Discovered and diagnosed three compounding bugs causing ~20% of mainnet snap syncs to crash on pivot update (peer rotation, weight function, BlockRangeUpdate filtering — see Issue #6474). Quick fix shipped as PR #6475; proper multi-bug fix tracked as #6474. +> - **Observability:** PR #6470 opened adding `admin_syncStatus` / `admin_peerScores` RPC endpoints, live `peer_top.py` TUI, Grafana dashboards, and monitor improvements. Tooling was used to produce the forensic analysis for the reliability work. +> - **Profiling-driven perf candidates:** Mainnet profiling of `insert_storages` revealed that **~80% of idle thread-seconds** come from small-account dispatcher overhead, not from the monster account. Two new issues opened: +> - **#6476** small-account batching — amortize dispatcher overhead for the 26M <1ms trie builds +> - **#6477** big-account parallelization in snap sync — analogous to PR #6410 but for large storage tries (realistic gain ~5-6%, smaller than initially thought) +> - **Pipelining context added** to Issue #4240 (Phase 3 spawned rewrite) with a concrete proposal: ~13% gain from phase overlap (Accounts → Storage + Bytecodes → Healing pipelined via actors). +> - **New roadmap items for reliability** (see §1.18 and §1.19 below). --- @@ -73,14 +83,17 @@ The snap sync process consists of 6 sequential phases: ### Current Performance Bottlenecks -Based on code analysis and mainnet profiling data (PR #6410): +Based on code analysis and mainnet profiling data (PR #6410; April 2026 profiling run `20260412_172457`): | Bottleneck | Location | Impact | Priority | |------------|----------|--------|----------| +| **Pivot update crashes** | `update_pivot` in `snap_sync.rs` | **~20% of mainnet runs crash** with `process::exit(2)` + DB corruption — requires full resync | **Critical (reliability)** | | **Trie building in insertion** | `insert_accounts`, `insert_storage` | **75-91% of insertion time** (883s/1184s for accounts, 2357s/2587s for storage on mainnet) | **Critical** | +| **Small-account dispatcher overhead** | `insert_storages` dispatcher | **~80% of the 49% idle thread-seconds** (≈14,915 thread-seconds on a 2347s mainnet storage phase). 26.3M accounts × avg <1ms, dispatcher blocked 69% of wall on slot turnover | **Critical** (newly quantified) | +| **Large storage tries run single-threaded** | `insert_storages` per-account task | Monster (Uniswap-class, 159.6M leaves) runs 244.9s on 1 thread; ~20% of idle thread-seconds | **High** | | Sequential header download | `sync_cycle_snap()` | Blocks state download start | Critical | | Sequential phase pipeline | `snap_sync.rs` orchestration | Bytecodes/storage wait for all accounts to finish | High | -| Redundant code hash pass | `insert_accounts` | Extra full iteration over temp DB | Medium | +| Redundant code hash pass | `insert_accounts` | Extra full iteration over temp DB (addressed in #6410) | Medium | | Trie node batching | `heal_state_trie()`, `heal_storage_trie()` | Writes are batched but could use `put_batch_no_alloc` | Medium | | Busy-wait loops | Multiple locations | CPU waste (only when no peers available) | Medium | | SST file intermediate step | Account/storage download | Overlapping key ranges force RocksDB merge during ingestion | Medium | @@ -372,6 +385,145 @@ Current healing dispatch only fills one peer slot per loop iteration. Fill all a --- +### 1.18 Snap Sync Observability Tooling (PR #6470 — In Progress, added 2026-04-15) + +**Current State:** Diagnosing snap sync failures required manual log grep and docker inspection. No runtime visibility into peer scores, sync phase, or request distribution. + +**Changes:** +- `admin_syncStatus` RPC endpoint — live sync phase, pivot block, progress metrics, error history +- `admin_peerScores` RPC endpoint — per-peer scores, inflight request counts, capabilities, last BlockRangeUpdate, eligibility +- `admin_setLogLevel` RPC — dynamically raise to TRACE during incidents +- `tooling/sync/peer_top.py` — live TUI showing peer scores, request distribution, and selection patterns in real time +- Grafana dashboard panels: sync progress, peer scoring distribution, request rates, per-phase rate overview +- Docker monitor improvements: rolling snapshots, degradation detection, 5s polling, force-dump on failure +- Header-download diagnostics logging in `snap_sync.rs` + +**Impact:** No direct sync-time impact. Enables diagnosis of every other reliability/perf issue. The forensic analysis for §1.19 (pivot-update crashes) was only possible because of this tooling. + +**Status:** PR #6470 open + +--- + +### 1.19 Pivot Update Reliability (PRs/Issues #6475, #6474 — added 2026-04-15) + +**Current State:** `update_pivot` crashes the node (`process::exit(2)`) on ~20% of mainnet sync runs. The exit leaves the DB in an inconsistent state (`Unknown state found in DB`), requiring a full `removedb` and resync from scratch. + +**Root causes** (full forensics in Issue #6474): + +- **Bug A — `update_pivot` classified as irrecoverable.** Commit `583795955` changed the retry loop from infinite to `MAX_TOTAL_FAILURES=15` with exponential backoff. 15 failures exhaust in ~2-3 min, then `PeerHandlerError::BlockHeaders` → classified irrecoverable → `process::exit(2)`. +- **Bug B — Deterministic peer selection never rotates.** `get_best_peer` + `.max_by_key(weight_peer)` always returns the same top-scored peer. The weight function `score - inflight_requests` systematically prefers idle/incapable peers (e.g., eth/70-only erigon with 0 snap requests, score 47) over busy healthy peers (Geth with 47 snap requests, weight 3). +- **Bug C — `BlockRangeUpdate.range_to` unused in selection.** Peers advertise their chain tip; we store it but don't filter by it. On hoodi we kept asking a peer for pivot block 2593928 while its last BlockRangeUpdate said range_to=2593886 (42 blocks behind). + +**Evidence:** Hoodi failure `run_20260411_033943` — only 2 distinct peers tried across 9 attempts; at least 6 other peers had the block. Mainnet failure `run_20260414_011127` — stuck on erigon/v3.5.0-dev with weight 47 while 11 healthy peers had weight 3. + +**Quick fix (PR #6475 — shipped):** +- Reclassify `PeerHandler`/`NoBlockHeaders` errors as recoverable (narrow per-variant via `PeerHandlerError::is_recoverable()`) +- Add `get_best_peer_excluding(caps, excluded)` — rotation-aware peer selection +- Replace `MAX_TOTAL_FAILURES=15` with `MAX_ROTATIONS=5` (scales with peer count) +- Catch recoverable errors inside retry loop so protocol errors advance rotation + +**Proper fix (Issue #6474 — deferred):** tackle the deeper peer-selection bugs: +1. Fix `weight_peer` for control-plane requests (pivot update, header resolution) — don't penalize data-plane inflight +2. Filter peers by `BlockRangeUpdate.range_to` before selection +3. Broaden rotation across all eligible peers +4. Don't count passive waits as failures +5. Revert irrecoverable classification of post-pivot-header fetch failures +6. Raise/remove `MAX_TOTAL_FAILURES` +7. Cherry-pick fixes from `fullsync-acceleration` branches (880244afe, efaa344d4) +8. DB cleanup / graceful shutdown on sync failure + +**Impact:** Removes a reliability failure that was ~20% of mainnet runs. Currently masks optimization progress (slower runs → more pivot updates → more exposure to the bug). + +**Status:** PR #6475 open (AI agent feedback addressed 2026-04-15); Issue #6474 open as follow-up. + +--- + +### 1.20 Within-Trie Parallelization for Large Storage Tries (Issue #6477 — added 2026-04-15) + +**Current State:** `insert_storages` is parallel across accounts (16 worker threads, one account per task), but each individual account's trie is built single-threaded. For large storage tries (Uniswap-class, 159M+ leaves), this means a single thread runs for ~245s while the monster is processed. + +**Proposed Change:** Apply `trie_from_sorted_parallel` (from PR #6410) *inside* each large storage task, splitting the trie build across 16 storage-slot-nibble ranges. + +**Distinction from #5482:** Issue #5482 (existing, open) addresses the same idea but for *block execution* — parallelizing per-tx storage updates during state-root computation. This issue (#6477) is the analogous change for snap sync's initial trie construction. + +**Expected Impact:** ~100-150s saved (~5-6% of storage phase). Smaller than originally projected because the other 15 threads aren't actually idle during the monster's solo run (they're working on the long tail). + +**Status:** Issue #6477 open + +--- + +### 1.21 Small-Account Batching in insert_storages (Issue #6476 — added 2026-04-15) + +**Current State:** `insert_storages` profiling shows only 8.1 of 16 threads used on average (**49% of thread-seconds idle**, 18,589 / 37,554). Decomposition: +- Monster account serialization: ~20% of idle time +- **Small-account dispatcher overhead: ~80% of idle time** — 26.3M accounts with avg <1ms trie build each, dispatcher blocked 69% of wall on slot turnover + +**Proposed Change:** Bundle N small accounts per worker job to amortize send/reap/slot-free overhead. Large accounts still run as single tasks. + +**Expected Impact:** Potentially 5-15 min off the 39-min storage phase (30-40%). This is the dominant parallelism killer and **higher-value than #6477** (big-account). + +**Architectural dependencies:** None. Change is inside the existing dispatcher loop — independent of the spawned migration (Phase 3) and of #6477 (complementary, additive). + +**Status:** Issue #6476 open + +--- + +### 1.22 Decoded `TrieLayerCache` (PR #6348 — In Progress, added to roadmap 2026-04-15) + +**Current State:** `TrieLayerCache` hits still go through `Node::decode()` even when the node was just cached — decoded representation is discarded and re-derived on every access. + +**Proposed Change:** Cache the decoded `Node` value alongside the encoded bytes. Skip `Node::decode()` on cache hits. + +**Expected Impact:** TBD — needs benchmark. Hot path in trie traversal; decode is not free. + +**Status:** PR #6348 open (author: Arkenan) + +--- + +### 1.23 Bloom Filter for Non-Existent Storage Slots (PR #6288 — In Progress, added to roadmap 2026-04-15) + +**Current State:** Storage trie seeks are issued for every slot read, including for accounts that have no storage or slots that don't exist. On large contracts, many lookups miss. + +**Proposed Change:** Add a bloom filter to skip trie seeks for slots known not to exist. + +**Expected Impact:** TBD — needs benchmark. Could help both sync-time trie seeks and runtime reads. + +**Status:** PR #6288 open (author: ilitteri) + +--- + +### 1.24 Adaptive Request Sizing & Storage Bisection (PR #6181 — In Progress, added to roadmap 2026-04-15) + +**Current State:** Storage range requests use fixed size per request. Adaptive sizing based on peer response history and bisection on oversized responses could improve throughput. + +**Proposed Change:** Adaptive request sizing + storage bisection on oversized responses + parallel trie construction for storage. + +**Status:** PR #6181 open (author: ilitteri) + +--- + +### 1.25 Concurrent Bytecode + Storage Download (PR #6205 — In Progress, added to roadmap 2026-04-15) + +**Current State:** Bytecodes download as a distinct phase after storage. Pipeline opportunity similar to what PR #6184 did for bytecodes + healing. + +**Proposed Change:** Run bytecode downloads concurrently with storage downloads. + +**Note:** May overlap partially with PR #6184 (which concurrently runs bytecodes with *healing*). Needs review to reconcile. + +**Status:** PR #6205 open (author: ilitteri) + +--- + +### 1.26 Phase Completion Markers for Validation (PR #6189 — In Progress, added to roadmap 2026-04-15) + +**Current State:** No persisted markers for phase completion; recovery and validation tooling has to infer progress. + +**Proposed Change:** Add phase completion markers to the snap sync validation flow. + +**Status:** PR #6189 open (author: ilitteri) + +--- + ## Phase 2: Code Quality & Maintainability ### Goal @@ -742,6 +894,18 @@ Remaining: channel capacity `1000` appears at lines 138, 370, 587 — could be a --- +### 2.18 Storage Download Refactor via `StorageTrieTracker` (PR #6171 — In Progress, added 2026-04-15) + +**Current State:** `AccountStorageRoots` tracks storage downloads per-account with complex index-based referencing into `accounts_by_root_hash`. Tasks reference accounts by index, results carry index ranges, big-account promotion mutates intervals — hard to follow and brittle. + +**Proposed Change:** New `StorageTrieTracker` groups storage tries by root hash from the start, separating small (single-request) from big (multi-request) tries. Moves trie data into tasks and back in results, eliminating index-based coupling. + +**Relation:** Complementary to #6140 (same file, orthogonal concerns). This is the data-ownership refactor; #6140 is the readability/correctness cleanup. + +**Status:** PR #6171 open (author: fedacking) + +--- + ### Issue #6140 — Refactor `request_storage_ranges` (Steps Summary) 9-step plan to refactor `request_storage_ranges` in `snap/client.rs`. Each step is one independently correct commit. Full details in [Issue #6140](https://github.com/lambdaclass/ethrex/issues/6140). @@ -962,43 +1126,68 @@ This requires `StorageActor` to handle dynamically growing task queues (new acco | Schedule overrun | Medium | Medium | Prioritize high-impact items; iterative delivery | | ~~Spawned refactor not ready~~ | ~~Medium~~ | ~~High~~ | ✅ Resolved — spawned 0.5.0 merged in #6295 (March 31, 2026) | | Pipeline ordering bugs | Medium | High | Pipelining introduces data races if actors process out of order; needs careful invariant tracking | +| **Pivot-update crash masks optimization progress** | **High** | **High** | Ship §1.19 quick fix (#6475) before investing in further perf work — otherwise measurements are noisy and users hit the crash before seeing gains. Identified 2026-04-15. | +| **DB corruption requires full resync on any crash** | **High** | **High** | `process::exit(2)` paths leave inconsistent state. Addressed in §1.19 proper fix (#6474 item 8). Until fixed, every sync crash costs a full resync. | --- ## Timeline -### Recommended execution order (updated April 2026) +### Recommended execution order (updated 2026-04-15) + +Two key insights drive priorities: +- **Trie building dominates insertion time** (75-91% from 1.12 profiling) +- **Small-account dispatcher overhead accounts for ~80% of idle thread-seconds** in `insert_storages` (new finding from #6476 profiling) -The key insight from profiling is that **trie building dominates insertion time** (75-91%). The recommended priority is: +Recommended priority: -1. **Land trie building optimizations first** (1.12, #6410) — largest single improvement, orthogonal to concurrency model -2. **Land write path optimizations** (1.14, 1.15, 1.16) — compound on trie improvements -3. **Land pipelining** (1.13, #6184) — concurrent bytecodes + background healing -4. **Phase 2 quick wins** (2.8, 2.17, 2.15, 2.1, 2.4) — low-effort correctness/quality -5. **Actor migration** (3.1) — clean architecture, now unblocked, subsumes 2.5/2.12 -6. **Remaining Phase 2/3 items** — documentation, testing, peer scoring +1. **Unblock users first** — ship pivot-update crash fix (#6475) so ~20% of mainnet runs stop crashing. **This is the highest-priority item right now** — every other perf win is masked by the crash. +2. **Land observability tooling** (1.18, #6470) — enables measuring all other changes +3. **Land trie building optimizations** (1.12, #6410) — largest single improvement, orthogonal to concurrency model +4. **Land write path optimizations** (1.14, 1.15, 1.16) — compound on trie improvements +5. **Explore small-account batching** (1.21, #6476) — potentially 30-40% of storage phase (largest unexploited opportunity) +6. **Land pipelining** (1.13, #6184) — concurrent bytecodes + background healing +7. **Big-account within-trie parallelization** (1.20, #6477) — ~5-6%, complementary to #6476 +8. **Phase 2 quick wins** (2.8, 2.17, 2.15, 2.1, 2.4) — low-effort correctness/quality +9. **Proper pivot-update fix** (#6474) — deeper peer selection bugs, tackle after quick fix stabilizes +10. **Actor migration** (3.1) — clean architecture, now unblocked, subsumes 2.5/2.12 +11. **Remaining Phase 2/3 items** — documentation, testing, peer scoring ### Phase 1: Performance ``` +Priority 0 (CRITICAL — user-facing reliability): + 1.19 Pivot update reliability quick fix (PR #6475) — stops ~20% crash rate + Priority 1 (high impact, ready now): + 1.18 Snap sync observability tooling (PR #6470) — prereq for measuring others 1.12 Optimize trie building (PR #6410) — -31% account insertion 1.15 Optimize insertion/healing write paths (PR #6159) 1.14 Eliminate SST file intermediate step (PR #6177) 1.13 Pipeline bytecodes + background healing (PR #6184) — -13% total + 1.21 Small-account batching (Issue #6476) — potentially 30-40% of storage phase Priority 2 (medium impact): 1.16 Disable WAL and improve concurrency (PR #6178) 1.17 Fill all peer slots per tick in healing (PR #6175) + 1.20 Big-account within-trie parallelization (Issue #6477) — ~5-6% + 1.24 Adaptive request sizing + storage bisection (PR #6181) 1.1 Parallel Header Download (PR #6059) 1.6 Async Disk I/O (PR #6113) 1.4 Reduce Busy-Wait Loops (Issue #6140) Lower priority / needs measurement: + 1.22 Decoded TrieLayerCache (PR #6348) + 1.23 Bloom filter for non-existent storage slots (PR #6288) + 1.25 Concurrent bytecode + storage (PR #6205) — may overlap with 1.13 + 1.26 Phase completion markers (PR #6189) 1.3 Optimize Trie Node Batching 1.7 Peer Connection Optimization (PR #6117) 1.10 Snap sync benchmark tool (PR #6108) +Follow-up / deferred: + 1.19b Pivot update proper fix (Issue #6474) — deeper peer selection bugs + Done: 1.11 Per-phase timing breakdown (✅ Merged #6136) @@ -1029,6 +1218,7 @@ Independent (do when bandwidth available): 2.6 Test Coverage Improvement 2.7 Configuration Externalization 2.16 Healing Code Unification + 2.18 StorageTrieTracker refactor (PR #6171) Done: 2.3 Consolidate Error Handling (✅ Merged #5975) @@ -1139,5 +1329,5 @@ After 3.1: --- -*Document Version: 1.2* -*Last Updated: April 2026* +*Document Version: 1.3* +*Last Updated: 2026-04-15* From c7db02bad5e55a9e56ed559895326cad5d3fbd6e Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Tue, 5 May 2026 12:44:42 -0300 Subject: [PATCH 16/16] docs(l1): update PR #6410 benchmarks with mainnet-10 main vs PR comparison --- docs/roadmaps/snap_sync_roadmap.md | 34 ++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/docs/roadmaps/snap_sync_roadmap.md b/docs/roadmaps/snap_sync_roadmap.md index 84de643b9d2..5de685b40d7 100644 --- a/docs/roadmaps/snap_sync_roadmap.md +++ b/docs/roadmaps/snap_sync_roadmap.md @@ -1,7 +1,7 @@ # Snap Sync Module Roadmap **Author:** Pablo Deymonnaz (original), ElFantasma (ongoing updates) -**Date:** February 2026 (last updated 2026-04-15) +**Date:** February 2026 (last updated 2026-05-05) **Status:** Draft for Review --- @@ -314,22 +314,34 @@ Surfaces per-phase completion timings (Block Headers, Account Ranges, Storage Ra 2. **Reuse `nodehash_buffer` across calls** — avoids ~700M allocations on mainnet. 3. **Parallel state trie building across 16 nibble ranges** — splits state trie into 16 independent sub-tries built concurrently. -**Benchmarks (mainnet, release profile, no validation):** +**Original benchmarks (mainnet-8, release profile, no validation, March 2026):** | Phase | Before | After | Delta | |-------|--------|-------|-------| | Account Insertion | 1184s (19m 44s) | 818s (13m 40s) | **-31%** | -| Storage Insertion | 2587s (43m 7s) | 2433s (40m 30s) | **-6%** | +| Storage Insertion | 2587s (43m 7s) | 2433s (40m 30s) | -6% | -| Network | Before | After | Saved | -|---------|--------|-------|-------| -| Hoodi | ~25m | ~13m | ~12m | -| Sepolia | ~90m | ~43m | ~47m | -| Mainnet | ~1h 42m | ~1h 35m | ~7m | +**Updated benchmarks (mainnet-10, release-with-debug-assertions, May 2026 — 2026-05-05):** + +After several main-branch perf merges, re-benchmarked with **4 main runs vs 4 PR runs**, full validation enabled. + +Account Insertion (the optimization's main target): + +| Network | Main avg | Range | PR avg | Range | Δ | +|---------|----------|-------|--------|-------|---| +| Hoodi | 1m 22s | 1:20–1:30 | **0m 57s** | 0:50–1:00 | **−30%** ✅ | +| Sepolia | 6m 52s | 5:30–8:20 | **4m 02s** | 3:30–4:30 | **−41%** ✅ | +| Mainnet | 17m 33s | 13:40–21:20 | **16m 48s** | 16:20–17:40 | **−4%** (within main's variance) | + +On mainnet, single-run main numbers vary widely (13:40, 14:10, 21:00, 21:20). PR runs are tight (16:20–17:40, ±50s). The parallelization both improves the mean and stabilizes runtime. + +Storage Insertion shows a small consistent regression (~2–3 min absolute) across all networks: hoodi +39%, sepolia +13%, mainnet +5%. Worth investigating as a follow-up but not large enough to block. + +Storage healing is unchanged within ±3% — it's dominated by `release-with-debug-assertions` validation, not the optimization. **Note:** This optimization is orthogonal to the concurrency model — it operates at the trie-building level, below the orchestration layer. No dependency on actor migration. -**Status:** PR #6410 open, benchmarked on mainnet +**Status:** PR #6410 open, re-benchmarked on mainnet-10, validation passed on all 8 runs (4 main + 4 PR). --- @@ -1329,5 +1341,5 @@ After 3.1: --- -*Document Version: 1.3* -*Last Updated: 2026-04-15* +*Document Version: 1.4* +*Last Updated: 2026-05-05*