perf(l1): replace synchronous disk I/O with async operations in snap sync

[pablodeymo]

Motivation

During snap sync, multiple file system operations block the tokio runtime:

• Directory creation (std::fs::create_dir_all)
• File reading (std::fs::read)
• Directory listing (std::fs::read_dir)
• Directory removal (std::fs::remove_dir_all)

These synchronous operations cause the async runtime to stall, preventing network operations from proceeding while disk I/O completes. This is particularly problematic during:

1. Snapshot file writing (account states, storage states)
2. Snapshot file reading during trie insertion
3. Cleanup after insertion completes

This PR addresses roadmap item 1.6 (Async Disk I/O) from the snap sync improvement plan.

Description

Approach: Hybrid Async Strategy

We use a hybrid approach for async file operations:

Operation	Method	Rationale
create_dir_all	tokio::fs	Simple, no iterator complexity
read	tokio::fs	Simple, direct async support
remove_dir_all	tokio::fs	Simple, direct async support
read_dir	spawn_blocking	Returns iterator with complex lifetimes; wrapping in spawn_blocking is cleaner than manual async iteration

New Module: snap/async_fs.rs

Created a new module with async wrappers that provide consistent error handling:

/// Ensures a directory exists, creating it if necessary.
pub async fn ensure_dir_exists(path: &Path) -> Result<(), SnapError>

/// Reads all file paths from a directory (sorted for deterministic ordering).
pub async fn read_dir_paths(dir: &Path) -> Result<Vec<PathBuf>, SnapError>

/// Reads the contents of a file asynchronously.
pub async fn read_file(path: &Path) -> Result<Vec<u8>, SnapError>

/// Removes a directory and all its contents asynchronously.
pub async fn remove_dir_all(path: &Path) -> Result<(), SnapError>

/// Writes data to a file asynchronously.
pub async fn write_file(path: &Path, contents: &[u8]) -> Result<(), SnapError>

/// Removes a directory if it exists, ignoring NotFound errors.
pub async fn remove_dir_all_if_exists(path: &Path) -> Result<(), SnapError>

All functions return SnapError::FileSystem with:

• Operation name (for debugging)
• Path (to identify which file/directory failed)
• Error kind (from std::io::ErrorKind)

Changes to snap/client.rs

Before (4 occurrences):

if !std::fs::exists(account_state_snapshots_dir).map_err(|_| {
    SnapError::SnapshotDir("State snapshots directory does not exist".to_string())
})? {
    std::fs::create_dir_all(account_state_snapshots_dir).map_err(|_| {
        SnapError::SnapshotDir("Failed to create state snapshots directory".to_string())
    })?;
}

After:

async_fs::ensure_dir_exists(account_state_snapshots_dir).await?;

Locations updated:

1. request_account_range() - account state snapshot directory (loop)
2. request_account_range() - account state snapshot directory (final flush)
3. request_storage_ranges() - storage snapshot directory (loop)
4. request_storage_ranges() - storage snapshot directory (final flush)

Changes to sync/snap_sync.rs

Code Hashes Processing

Before:

std::fs::create_dir_all(&code_hashes_snapshot_dir).map_err(|_| SyncError::CorruptPath)?;
// ...
for entry in std::fs::read_dir(&code_hashes_dir)
    .map_err(|_| SyncError::CodeHashesSnapshotsDirNotFound)?
{
    let entry = entry.map_err(|_| SyncError::CorruptPath)?;
    let snapshot_contents = std::fs::read(entry.path())
        .map_err(|err| SyncError::SnapshotReadError(entry.path(), err))?;
    // ...
}
std::fs::remove_dir_all(code_hashes_dir)
    .map_err(|_| SyncError::CodeHashesSnapshotsDirNotFound)?;

After:

async_fs::ensure_dir_exists(&code_hashes_snapshot_dir).await?;
// ...
let code_hash_files = async_fs::read_dir_paths(&code_hashes_dir).await?;
for file_path in code_hash_files {
    let snapshot_contents = async_fs::read_file(&file_path).await?;
    // ...
}
async_fs::remove_dir_all(&code_hashes_dir).await?;

Account Insertion (non-rocksdb)

Before:

for entry in std::fs::read_dir(account_state_snapshots_dir)
    .map_err(|_| SyncError::AccountStateSnapshotsDirNotFound)?
{
    let entry = entry.map_err(|err| SyncError::SnapshotReadError(...))?;
    let snapshot_path = entry.path();
    let snapshot_contents = std::fs::read(&snapshot_path)
        .map_err(|err| SyncError::SnapshotReadError(...))?;
    // ...
}
std::fs::remove_dir_all(account_state_snapshots_dir)
    .map_err(|_| SyncError::AccountStoragesSnapshotsDirNotFound)?;

After:

let snapshot_files = async_fs::read_dir_paths(account_state_snapshots_dir).await?;
for snapshot_path in snapshot_files {
    let snapshot_contents = async_fs::read_file(&snapshot_path).await?;
    // ...
}
async_fs::remove_dir_all(account_state_snapshots_dir).await?;

Storage Insertion (non-rocksdb)

Same pattern as account insertion - replaced read_dir + read + remove_dir_all with async equivalents.

Account Insertion (rocksdb)

Before:

let file_paths: Vec<PathBuf> = std::fs::read_dir(account_state_snapshots_dir)
    .map_err(|_| SyncError::AccountStateSnapshotsDirNotFound)?
    .collect::<Result<Vec<_>, _>>()
    .map_err(|_| SyncError::AccountStateSnapshotsDirNotFound)?
    .into_iter()
    .map(|res| res.path())
    .collect();
// ...
std::fs::remove_dir_all(account_state_snapshots_dir)
    .map_err(|_| SyncError::AccountStateSnapshotsDirNotFound)?;
std::fs::remove_dir_all(get_rocksdb_temp_accounts_dir(datadir))
    .map_err(|e| SyncError::AccountTempDBDirNotFound(e.to_string()))?;

After:

let file_paths: Vec<PathBuf> = async_fs::read_dir_paths(account_state_snapshots_dir).await?;
// ...
async_fs::remove_dir_all(account_state_snapshots_dir).await?;
async_fs::remove_dir_all(&get_rocksdb_temp_accounts_dir(datadir)).await?;

Storage Insertion (rocksdb)

Same pattern - replaced sync fs operations with async equivalents.

Summary of Changes

File	Before	After	Change
snap/async_fs.rs	-	189 lines	New module
snap/mod.rs	-	+1 line	Module export
snap/client.rs	42 lines	4 lines	-38 lines (4 patterns simplified)
sync/snap_sync.rs	82 lines	16 lines	-66 lines (16 calls simplified)
Cargo.toml	-	+1 line	tempfile dev-dependency
Cargo.lock	-	+1 line	tempfile entry

Net change: +223 lines, -93 lines = +130 lines (including 189 lines of new module with tests)

How to Test

Unit Tests

Run the async_fs module tests:

cargo test -p ethrex-p2p async_fs

Expected output:

running 6 tests
test snap::async_fs::tests::test_remove_dir_all_if_exists ... ok
test snap::async_fs::tests::test_ensure_dir_exists_idempotent ... ok
test snap::async_fs::tests::test_remove_dir_all ... ok
test snap::async_fs::tests::test_ensure_dir_exists_creates_new ... ok
test snap::async_fs::tests::test_read_write_file ... ok
test snap::async_fs::tests::test_read_dir_paths ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 38 filtered out

Compilation Tests

Verify both feature configurations compile:

# Without rocksdb (uses non-rocksdb insert_accounts/insert_storages)
cargo check -p ethrex-p2p

# With rocksdb (uses rocksdb-specific insert_accounts/insert_storages)
cargo check -p ethrex-p2p --features rocksdb

Integration Testing

Run a snap sync on testnet to verify:

1. Account state snapshots are written and read correctly
2. Storage snapshots are written and read correctly
3. Code hash snapshots are written and read correctly
4. All cleanup operations complete successfully
5. No tokio runtime blocking warnings appear in logs

# Example: Run snap sync on Holesky
make start-holesky-metrics-docker

Performance Verification

To verify async I/O is working:

1. Monitor CPU usage during snapshot write/read phases
2. Verify network operations continue during disk I/O
3. Check that tokio::time::sleep polling loops respond promptly

Files Changed

File	Description
crates/networking/p2p/snap/async_fs.rs	New async filesystem helpers module
crates/networking/p2p/snap/mod.rs	Export async_fs module
crates/networking/p2p/snap/client.rs	Use async_fs for directory creation
crates/networking/p2p/sync/snap_sync.rs	Use async_fs for all file operations
crates/networking/p2p/Cargo.toml	Add tempfile dev-dependency
Cargo.lock	Updated lockfile

Checklist

• Code compiles without rocksdb feature
• Code compiles with rocksdb feature
• All async_fs unit tests pass
• No remaining std::fs:: calls in snap sync code paths
• Error handling preserved (SnapError::FileSystem)
• Deterministic ordering maintained (read_dir_paths sorts results)

Related

• Snap Sync Roadmap: Item 1.6 (Async Disk I/O)
• Parent branch: refactor/snapsync-healing-unification

[github-actions]

Kimi AI Code Review

Code Correctness and Potential Bugs

• async_fs::ensure_dir_exists: The function checks if a directory exists and creates it if it doesn't. This is correct and aligns with the expected behavior. async_fs.rs#9-21

• async_fs::read_dir_paths: This function uses spawn_blocking to read directory contents, which is a good approach to avoid blocking the async runtime. However, it does not handle the case where the directory is empty. This should not be a problem unless the caller expects at least one file. async_fs.rs#24-39

• async_fs::read_file and async_fs::write_file: These functions correctly use tokio::fs for async file operations. The error handling is also appropriate. async_fs.rs#41-54

• async_fs::remove_dir_all_if_exists: This function correctly handles the case where the directory does not exist, avoiding unnecessary errors. async_fs.rs#63-72

Security Vulnerabilities

• The changes do not introduce any obvious security vulnerabilities. The use of spawn_blocking is safe here because it's used to perform read operations, which are not sensitive.

Performance Implications

• Replacing synchronous disk I/O with async operations is a significant improvement for performance, especially in a network-heavy application like a blockchain client. It prevents blocking the async runtime and allows for better concurrency.

Rust Best Practices and Idiomatic Patterns

• The use of tokio::fs for async file operations is idiomatic in Rust for async code.
• The error handling using SnapError is consistent with the existing codebase and follows Rust's error handling patterns.

Memory Safety and Proper Error Handling

• The code appears to be memory safe. It properly handles errors and converts them into the SnapError type, which is then propagated up the call stack.
• The use of map_err for error conversion is correct and follows Rust's best practices.

Code Readability and Maintainability

• The code is well-structured and readable. The comments and function names are descriptive, which helps maintainability.
• The tests in async_fs are a good addition, ensuring that the async file operations work as expected.

Ethereum-specific Considerations

• EVM Opcode Correctness and Gas Accounting: Not applicable in this PR as it does not deal with EVM execution.

• Consensus Rules and EIP Compliance: The changes do not affect consensus rules or EIP compliance directly.

• State Trie and Storage Operations: The async file operations are used for state snapshot management, which is crucial for the state trie. The changes appear to handle this correctly.

• RLP Encoding/Decoding Correctness: The PR does not introduce any changes to RLP encoding/decoding.

• Transaction and Block Validation Logic: The changes do not affect transaction or block validation logic directly.

Conclusion

The PR looks good overall. It correctly replaces synchronous disk I/O with async operations, improving the performance of the Ethereum client. The code is well-written, follows Rust best practices, and maintains a high level of security and correctness.

One minor suggestion is to consider adding more comments or documentation for the async_fs module, explaining the rationale behind using spawn_blocking for read_dir_paths and the potential implications for callers.

---

Automated review by Kimi (Moonshot AI)

[github-actions]

Lines of code report

Total lines added: 104
Total lines removed: 112
Total lines changed: 216

Detailed view

+------------------------------------------------+-------+------+
| File                                           | Lines | Diff |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/snap/async_fs.rs  | 103   | +103 |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/snap/client.rs    | 1157  | -9   |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/snap/mod.rs       | 23    | +1   |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/sync/snap_sync.rs | 916   | -103 |
+------------------------------------------------+-------+------+

[greptile-apps]

Greptile Overview

Greptile Summary

This PR converts synchronous disk I/O operations to async operations in snap sync, preventing the tokio runtime from blocking during file system operations. The changes introduce a new async_fs module with wrapper functions and systematically replace all std::fs calls across the snap sync codebase.

Key Changes

• New async_fs module: Provides async wrappers for common file operations using tokio::fs for simple operations and spawn_blocking for complex operations like directory iteration
• Client changes: Replaced 4 instances of sync directory creation with async_fs::ensure_dir_exists
• Snap sync changes: Converted all file reading, directory listing, and cleanup operations to async across both rocksdb and non-rocksdb code paths
• Proper error handling: All async_fs functions return SnapError::FileSystem with operation name, path, and error kind

Issues Found

• Critical: The read_dir_paths function silently ignores directory entry errors using filter_map(|entry| entry.ok()), which is a regression from the original code that propagated these errors as SyncError::CorruptPath. This could hide file system corruption or permission issues during snapshot processing.

Recommendations

• Fix the error handling regression in async_fs::read_dir_paths to properly propagate directory entry errors
• Consider adding integration tests to verify the async operations behave identically to the original sync operations, especially error cases

Confidence Score: 3/5

• Not safe to merge without fixing the error handling regression in read_dir_paths
• The PR successfully converts sync I/O to async, but introduces a critical error handling regression where directory entry errors are silently ignored instead of being propagated. This could mask file system corruption or permission issues during production snap sync operations.
• crates/networking/p2p/snap/async_fs.rs needs the error handling fix in read_dir_paths (line 52)

Important Files Changed

Filename	Overview
crates/networking/p2p/snap/async_fs.rs	New async filesystem wrapper module with comprehensive tests, but read_dir_paths silently ignores entry errors (regression from original code)
crates/networking/p2p/snap/client.rs	Cleanly replaced 4 instances of sync directory creation with async_fs::ensure_dir_exists, properly awaited
crates/networking/p2p/sync/snap_sync.rs	Systematically replaced all sync fs operations with async equivalents across both rocksdb and non-rocksdb code paths

Sequence Diagram

sequenceDiagram
    participant Runtime as Tokio Runtime
    participant SnapSync as snap_sync.rs
    participant Client as snap/client.rs
    participant AsyncFS as async_fs.rs
    participant Disk as File System

    Note over SnapSync,Disk: Account Range Processing
    Client->>AsyncFS: ensure_dir_exists(account_state_dir)
    AsyncFS->>AsyncFS: tokio::fs::try_exists()
    AsyncFS->>Disk: Check if directory exists
    Disk-->>AsyncFS: Result
    alt Directory doesn't exist
        AsyncFS->>Disk: tokio::fs::create_dir_all()
        Disk-->>AsyncFS: Created
    end
    AsyncFS-->>Client: Ok(())
    
    Note over SnapSync,Disk: Account Insertion Phase
    SnapSync->>AsyncFS: read_dir_paths(account_state_dir)
    AsyncFS->>Runtime: spawn_blocking
    Runtime->>Disk: std::fs::read_dir()
    Disk-->>Runtime: Iterator<DirEntry>
    Runtime->>Runtime: Collect and sort paths
    Runtime-->>AsyncFS: Vec<PathBuf>
    AsyncFS-->>SnapSync: Sorted file paths
    
    loop For each snapshot file
        SnapSync->>AsyncFS: read_file(path)
        AsyncFS->>Disk: tokio::fs::read()
        Disk-->>AsyncFS: File contents
        AsyncFS-->>SnapSync: Vec<u8>
        SnapSync->>SnapSync: Process and insert into trie
    end
    
    Note over SnapSync,Disk: Cleanup Phase
    SnapSync->>AsyncFS: remove_dir_all(snapshots_dir)
    AsyncFS->>Disk: tokio::fs::remove_dir_all()
    Disk-->>AsyncFS: Removed
    AsyncFS-->>SnapSync: Ok(())
    
    Note over Runtime: Async I/O allows runtime to handle<br/>network operations concurrently

Loading

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[github-actions]

Kimi AI Code Review

This PR introduces asynchronous file system operations to improve the performance of the snap sync process by avoiding blocking the tokio runtime during disk I/O. Here's a detailed review focusing on the specified areas:

Code Correctness and Potential Bugs

• The new async_fs module provides async wrappers for file system operations, which is a correct approach to avoid blocking the tokio runtime. The implementation seems correct, and the tests cover the main functionality.

• The usage of tokio::fs for simple operations like create_dir_all, read, remove_dir_all, and write is appropriate, as these are single system calls that can be efficiently handled by tokio's async I/O thread pool.

• The use of spawn_blocking for iterator-based operations like read_dir is a good choice, as it simplifies the code and avoids the complexity of handling ownership and lifetimes with tokio::fs::read_dir.

Security Vulnerabilities

• The PR does not introduce any new security vulnerabilities. It correctly handles errors and propagates them using the SnapError enum, which is essential for maintaining a secure and reliable system.

Performance Implications

• The PR significantly improves the performance of the snap sync process by replacing synchronous disk I/O with asynchronous operations. This allows the tokio runtime to continue processing network operations while waiting for disk I/O, leading to better utilization of available bandwidth and reduced peer timeouts.

Rust Best Practices and Idiomatic Patterns

• The PR follows Rust best practices by using Result and Error types for error handling, which is idiomatic in Rust.

• The use of async/await for asynchronous operations is consistent with Rust's asynchronous programming patterns.

Memory Safety and Proper Error Handling

• The PR correctly handles potential errors from file system operations, propagating them using the SnapError enum. This ensures that errors are not silently ignored and can be handled appropriately by the caller.

Code Readability and Maintainability

• The code is well-structured and follows a clear pattern for each file system operation. The use of comments and documentation helps in understanding the purpose and behavior of each function.

• The tests in the async_fs module are comprehensive and cover the main functionality, which aids in maintaining the code in the long run.

Ethereum-Specific Considerations

• The PR does not directly affect EVM opcode correctness, gas accounting, consensus rules, EIP compliance, state trie operations, RLP encoding/decoding, or transaction and block validation logic. However, by improving the performance of the snap sync process, it indirectly contributes to the overall reliability and efficiency of the Ethereum execution client.

Conclusion

The PR looks good overall. It introduces significant performance improvements by replacing synchronous disk I/O with asynchronous operations in the snap sync process. The code is well-structured, follows Rust best practices, and includes comprehensive tests. The changes are consistent with Ethereum-specific considerations.

One minor suggestion for improvement:

• In the async_fs::ensure_dir_exists function, consider adding a comment explaining why the try_exists method is used instead of exists. This would provide additional clarity on the design choice.

Overall, the PR is well-implemented and ready to be merged. Great work on improving the performance of the snap sync process!

---

Automated review by Kimi (Moonshot AI)

[pablodeymo]

Kimi AI Code Review

Code Correctness and Potential Bugs

* **async_fs::ensure_dir_exists**: The function checks if a directory exists and creates it if it doesn't. This is correct and aligns with the expected behavior. [async_fs.rs#9-21](https://github.com/your-repo/your-repo/pull/1234/files#diff-351fab519f)

* **async_fs::read_dir_paths**: This function uses `spawn_blocking` to read directory contents, which is a good approach to avoid blocking the async runtime. However, it does not handle the case where the directory is empty. This should not be a problem unless the caller expects at least one file. [async_fs.rs#24-39](https://github.com/your-repo/your-repo/pull/1234/files#diff-351fab519f)

* **async_fs::read_file and async_fs::write_file**: These functions correctly use `tokio::fs` for async file operations. The error handling is also appropriate. [async_fs.rs#41-54](https://github.com/your-repo/your-repo/pull/1234/files#diff-351fab519f)

* **async_fs::remove_dir_all_if_exists**: This function correctly handles the case where the directory does not exist, avoiding unnecessary errors. [async_fs.rs#63-72](https://github.com/your-repo/your-repo/pull/1234/files#diff-351fab519f)

Security Vulnerabilities

* The changes do not introduce any obvious security vulnerabilities. The use of `spawn_blocking` is safe here because it's used to perform read operations, which are not sensitive.

Performance Implications

* Replacing synchronous disk I/O with async operations is a significant improvement for performance, especially in a network-heavy application like a blockchain client. It prevents blocking the async runtime and allows for better concurrency.

Rust Best Practices and Idiomatic Patterns

* The use of `tokio::fs` for async file operations is idiomatic in Rust for async code.

* The error handling using `SnapError` is consistent with the existing codebase and follows Rust's error handling patterns.

Memory Safety and Proper Error Handling

* The code appears to be memory safe. It properly handles errors and converts them into the `SnapError` type, which is then propagated up the call stack.

* The use of `map_err` for error conversion is correct and follows Rust's best practices.

Code Readability and Maintainability

* The code is well-structured and readable. The comments and function names are descriptive, which helps maintainability.

* The tests in `async_fs` are a good addition, ensuring that the async file operations work as expected.

Ethereum-specific Considerations

* **EVM Opcode Correctness and Gas Accounting**: Not applicable in this PR as it does not deal with EVM execution.

* **Consensus Rules and EIP Compliance**: The changes do not affect consensus rules or EIP compliance directly.

* **State Trie and Storage Operations**: The async file operations are used for state snapshot management, which is crucial for the state trie. The changes appear to handle this correctly.

* **RLP Encoding/Decoding Correctness**: The PR does not introduce any changes to RLP encoding/decoding.

* **Transaction and Block Validation Logic**: The changes do not affect transaction or block validation logic directly.

Conclusion

The PR looks good overall. It correctly replaces synchronous disk I/O with async operations, improving the performance of the Ethereum client. The code is well-written, follows Rust best practices, and maintains a high level of security and correctness.

One minor suggestion is to consider adding more comments or documentation for the async_fs module, explaining the rationale behind using spawn_blocking for read_dir_paths and the potential implications for callers.

Automated review by Kimi (Moonshot AI)

address documentation comments

[github-actions]

Kimi AI Code Review

Code Correctness and Potential Bugs

The PR introduces asynchronous file system operations to replace synchronous disk I/O in the snap sync process. This change is crucial for improving the performance of the Ethereum client by avoiding blocking operations on the Tokio runtime.

• async_fs::ensure_dir_exists: The function correctly checks if a directory exists and creates it if necessary, using tokio::fs::try_exists and tokio::fs::create_dir_all. This approach is correct and avoids potential race conditions compared to the synchronous version.

• async_fs::read_dir_paths: This function uses spawn_blocking to read the directory contents synchronously but within a blocking task, which is a good approach for simplicity and efficiency in this context. The returned paths are sorted alphabetically, which is important for deterministic processing order.

• async_fs::read_file and async_fs::write_file: These functions use tokio::fs::read and tokio::fs::write respectively, which are appropriate for simple file read and write operations.

• Error Handling: The error handling in async_fs is consistent, wrapping std::io::Error in SnapError::FileSystem. This is a good practice for maintaining clean error propagation.

Security Vulnerabilities

The changes do not introduce any new security vulnerabilities. The use of asynchronous I/O does not expose any additional attack surfaces, and the error handling is consistent with the existing codebase.

Performance Implications

The replacement of synchronous I/O with asynchronous operations is expected to have a positive impact on performance. By not blocking the Tokio runtime, the client can continue processing network operations while waiting for disk I/O, leading to better utilization of available resources and reduced latency.

Rust Best Practices and Idiomatic Patterns

The PR follows Rust best practices and idiomatic patterns:

• Error Handling: Using map_err to convert std::io::Error to SnapError is idiomatic Rust.

• Async/Await: The use of async functions and await for asynchronous operations is correct and follows Rust's async/await pattern.

Memory Safety and Proper Error Handling

The code is memory safe, and error handling is properly implemented:

• Memory Safety: The use of Rust's ownership and borrowing rules ensures memory safety.

• Error Handling: Errors are propagated using Result and SnapError, which is consistent with Rust's error handling conventions.

Code Readability and Maintainability

The code is well-structured and readable:

• Modular Design: The async_fs module is a good example of modular design, encapsulating asynchronous file system operations.

• Documentation: The code is well-documented, explaining the rationale behind the design choices and the implications of using asynchronous I/O.

Ethereum-Specific Considerations

The changes do not affect Ethereum-specific logic such as EVM opcode correctness, gas accounting, consensus rules, or RLP encoding/decoding. The focus is on improving the performance of the snap sync process, which is orthogonal to these aspects.

Conclusion

The PR is well-written, follows best practices, and is expected to improve the performance of the Ethereum client by replacing synchronous disk I/O with asynchronous operations. The changes are correct, secure, and maintain the idiomatic Rust style. The tests included in the PR cover the new functionality, ensuring that the changes are reliable.

Suggested Changes:

• N/A

Overall Impression:

The PR looks good and is ready to be merged. The changes are well-justified and follow best practices for asynchronous programming in Rust. The tests included in the PR provide confidence in the correctness of the new functionality.

---

Automated review by Kimi (Moonshot AI)

[pablodeymo]

Thanks for the thorough review! I've addressed the critical issue:

Fixed in commit e1a25e9: The read_dir_paths function now properly propagates directory entry errors instead of silently ignoring them with filter_map.

The fix changes from:

.filter_map(|entry| entry.ok().map(|e| e.path()))

To:

.map(|entry| {
    entry
        .map(|e| e.path())
        .map_err(|e| SnapError::FileSystem {
            operation: "read directory entry",
            path: dir.clone(),
            kind: e.kind(),
        })
})
.collect::<Result<Vec<_>, _>>()?;

This ensures that any filesystem issues (permission denied, corruption, etc.) are properly surfaced rather than silently skipped.

Regarding the documentation suggestion: the module already includes extensive docstrings explaining the hybrid approach (spawn_blocking vs tokio::fs), thread pool implications, and error handling rationale.

[github-actions]

Kimi AI Code Review

This PR introduces asynchronous file system operations to replace synchronous disk I/O in the snap sync process. This is a significant improvement for performance as it prevents blocking the Tokio runtime, which can lead to timeouts and reduced throughput. Here are my detailed comments:

Code Correctness and Potential Bugs

1. Async File System Operations: The introduction of async_fs module is correct and necessary for non-blocking I/O operations. The usage of tokio::fs for simple operations like create_dir_all, read, remove_dir_all, and write is appropriate.

   • Line References: async_fs.rs

2. Error Handling: The error handling in async_fs is comprehensive, mapping I/O errors to SnapError::FileSystem with detailed context. This is crucial for debugging and ensures that all possible error paths are covered.

   • Line References: async_fs.rs

3. Usage of spawn_blocking: For read_dir, using spawn_blocking is a good choice because it simplifies the code and avoids the complexity of handling an async stream. The explanation in the doc comment is clear.

   • Line References: async_fs.rs

Security Vulnerabilities

1. File Path Handling: The code correctly handles file paths and avoids any potential security issues like path traversal. All paths are constructed using Path and PathBuf, which are safe.
   • Line References: async_fs.rs

Performance Implications

1. Non-Blocking I/O: The main benefit of this PR is the improvement in performance due to non-blocking I/O. This allows the Tokio runtime to handle network operations concurrently with disk I/O, which is crucial for the snap sync process.
   • Line References: async_fs.rs

Rust Best Practices and Idiomatic Patterns

1. Async/Await: The use of async/await is idiomatic and follows Rust's best practices for asynchronous programming.

   • Line References: async_fs.rs

2. Error Handling: The use of Result and map_err for error handling is idiomatic and follows Rust's best practices.

   • Line References: async_fs.rs

Memory Safety and Proper Error Handling

1. Memory Safety: The code is safe in terms of memory usage. There are no potential issues like memory leaks or use-after-free.

   • Line References: async_fs.rs

2. Error Handling: The error handling is comprehensive, covering all possible failure points and providing detailed error context.

   • Line References: async_fs.rs

Code Readability and Maintainability

1. Documentation: The code is well-documented, with clear explanations of the design choices and the implications of using async I/O.

   • Line References: async_fs.rs

2. Modularity: The async_fs module is a good example of modular design, encapsulating async file system operations and making the code more maintainable.

   • Line References: async_fs.rs

Ethereum-Specific Considerations

1. Consensus Rules and EIP Compliance: The changes do not affect consensus rules or EIP compliance as they are purely about improving performance.

   • Line References: N/A

2. State Trie and Storage Operations: The changes do not affect state trie or storage operations as they are focused on improving I/O performance.

   • Line References: N/A

3. RLP Encoding/Decoding Correctness: The changes do not affect RLP encoding/decoding as they are focused on improving I/O performance.

   • Line References: N/A

4. Transaction and Block Validation Logic: The changes do not affect transaction and block validation logic as they are focused on improving I/O performance.

   • Line References: N/A

Conclusion

The PR is well-written, follows Rust best practices, and introduces significant performance improvements by replacing synchronous disk I/O with asynchronous operations. The code is correct, secure, and well-documented. It adheres to Ethereum-specific considerations and maintains consensus rules and EIP compliance.

Overall, I approve this PR. It is a valuable contribution to improving the performance of the snap sync process in EthRex.

---

Automated review by Kimi (Moonshot AI)

[pablodeymo]

what do you think @greptile-apps?

[greptile-apps]

This is a well-architected solution to a real performance problem! You've identified the core issue - synchronous filesystem operations blocking the tokio runtime during snap sync - and addressed it systematically.

Strong points:

1. Clear problem identification: Blocking I/O during snap sync is a legitimate performance bottleneck that prevents network/disk operation overlap

2. Pragmatic hybrid approach: Your decision matrix for tokio::fs vs spawn_blocking makes sense:

   • Simple operations → tokio::fs
   • Iterator-based read_dir → spawn_blocking (avoiding async stream complexity)

3. Excellent documentation: The module docs clearly explain the "why" behind each choice, thread pool implications, and error handling strategy

4. Consistent error handling: Unified SnapError::FileSystem with operation/path/kind context makes debugging much easier

5. Deterministic ordering: Sorting directory entries ensures reproducible behavior - critical for consensus systems

6. Comprehensive testing: Good coverage of all async_fs functions with realistic test scenarios

Code quality observations:

• The error propagation with ? throughout the codebase is clean
• Function signatures are well-designed (taking &Path consistently)
• Good use of to_path_buf() to avoid ownership issues in spawn_blocking
• The remove_dir_all_if_exists helper is a nice touch for idempotent cleanup

Minor considerations:

• Thread pool pressure: While you mention <100 concurrent reads should be fine, consider adding metrics to monitor spawn_blocking queue depth if this becomes a bottleneck
• Error context: The current error handling is good, but you might want to preserve the original error chain for deeper debugging

Performance impact assessment:

This should meaningfully improve snap sync performance by allowing network requests to proceed while disk I/O happens in parallel. The transformation from blocking patterns like:

// Before: blocks entire runtime
for entry in std::fs::read_dir(dir)? { ... }

to:

// After: yields to other tasks
let files = async_fs::read_dir_paths(dir).await?;
for file_path in files { ... }

...is exactly the right fix.

The code simplification is also a nice bonus - reducing 42 lines to 4 lines while adding async behavior is impressive.

Overall, this is a solid performance optimization with clean implementation. The approach is sound and the code quality is high.

[github-actions]

Kimi AI Code Review

This PR introduces asynchronous file system operations to replace synchronous disk I/O in the snap sync process, which is a significant improvement for performance and responsiveness in a networked blockchain client. Here's a detailed review:

Code Correctness and Potential Bugs

1. Async File System Wrappers (async_fs.rs):

   • The new module async_fs correctly implements async wrappers around file system operations. The use of tokio::fs for simple operations and spawn_blocking for iterator-based operations like read_dir is appropriate and well-justified.
   • Error handling is consistent, wrapping std::io::Error into SnapError::FileSystem, which is good for maintaining a unified error type across the snap sync process.

2. Client Implementation (client.rs):

   • The replacement of synchronous file operations with async counterparts is correctly done. The calls to async_fs methods are properly awaited, ensuring that the file operations do not block the tokio runtime.

Security Vulnerabilities

• The changes do not introduce any new security vulnerabilities. They simply shift from synchronous to asynchronous I/O, which is a safe operation in terms of security.

Performance Implications

• The shift to asynchronous I/O will significantly improve the performance of the snap sync process by not blocking the tokio runtime on disk operations. This allows network operations to proceed concurrently, improving throughput and responsiveness.

Rust Best Practices and Idiomatic Patterns

• The use of tokio::fs and spawn_blocking follows Rust's best practices for asynchronous programming. The code is idiomatic and leverages Rust's async/await syntax effectively.

Memory Safety and Proper Error Handling

• The code is memory safe, as it properly handles ownership and lifetimes, especially in the context of async operations.
• Error handling is robust, with all potential I/O errors being captured and converted into a SnapError, which simplifies error propagation and handling in the calling code.

Code Readability and Maintainability

• The code is well-structured and readable. The comments in async_fs.rs provide a clear explanation of the design choices and implications, which is helpful for maintainability.
• The tests in async_fs are comprehensive and cover the new async file operations, ensuring that future changes can be validated against these tests.

Ethereum-Specific Considerations

• EVM Opcode Correctness and Gas Accounting: Not applicable, as this PR does not affect the EVM execution.
• Consensus Rules and EIP Compliance: The changes do not affect consensus rules or EIP compliance.
• State Trie and Storage Operations: The async I/O operations are correctly implemented and should not affect the correctness of state trie and storage operations.
• RLP Encoding/Decoding Correctness: Not directly applicable, but the changes maintain the integrity of RLP operations by not altering the data handling logic.
• Transaction and Block Validation Logic: The changes do not impact transaction or block validation logic.

Conclusion

The PR is well-implemented, follows best practices, and is likely to improve the performance of the Ethereum execution client. The changes are correctly scoped, and the new async file operations are implemented with proper error handling and testing. I approve this PR with no specific changes requested.

---

Automated review by Kimi (Moonshot AI)

[github-actions]

Benchmark Block Execution Results Comparison Against Main

Command	Mean [s]	Min [s]	Max [s]	Relative
base	60.390 ± 0.238	60.022	60.826	1.00
head	60.403 ± 0.181	60.094	60.674	1.00 ± 0.00

[pablodeymo]

Thanks @ElFantasma! Fixed the documentation to clarify that tokio::fs internally uses spawn_blocking for most operations. The benefit is the clean async API rather than a fundamentally different execution model.

[github-actions]

Kimi AI Code Review

This PR aims to replace synchronous disk I/O operations with asynchronous ones in the snap sync module to prevent blocking the Tokio runtime. Here's a review based on the provided criteria:

Code Correctness and Potential Bugs

The changes appear to correctly replace synchronous file operations with their asynchronous counterparts using tokio::fs and spawn_blocking. The logic seems sound, and the error handling is consistent with the existing patterns.

Suggestion:

• In async_fs::ensure_dir_exists, consider adding a check to avoid unnecessary directory creation if the directory already exists to prevent potential race conditions (line 40-42). However, this might be a minor optimization and not a correctness issue.

Security Vulnerabilities

The changes do not introduce any new security vulnerabilities. They adhere to the principle of least privilege by not introducing any new file permissions or access patterns.

Performance Implications

The performance implications are positive. By offloading disk I/O to non-blocking operations, the Tokio runtime can continue to process other tasks, such as network operations, which can lead to better overall performance and responsiveness.

Rust Best Practices and Idiomatic Patterns

The use of tokio::fs and spawn_blocking is idiomatic in Rust for asynchronous file operations. The code follows Rust's best practices by handling errors using Result and map_err.

Memory Safety and Proper Error Handling

The code is memory-safe and correctly handles errors. It uses Result for error propagation and provides detailed error messages, which is crucial for diagnosing issues in production.

Code Readability and Maintainability

The code is well-structured and readable. The use of comments and clear function names makes it easy to understand the purpose of each section. The tests in async_fs module also help in maintaining the code by providing clear examples of usage.

Ethereum-Specific Considerations

The changes do not affect EVM opcode correctness, gas accounting, consensus rules, EIP compliance, state trie operations, RLP encoding/decoding, or transaction and block validation logic. They are focused on improving the performance of the snap sync module.

Overall Assessment:

The PR is well-written and addresses the issue of synchronous disk I/O blocking the Tokio runtime effectively. It follows Rust best practices and maintains a high level of code quality. The changes are likely to improve the performance of the snap sync module without introducing new bugs or security vulnerabilities.

Final Verdict: Approved with a suggestion for a minor optimization.

---

Automated review by Kimi (Moonshot AI)

[pablodeymo]

@greptile-apps ?

[greptile-apps]

I'm not sure what specific question you have - just seeing "?" Could you clarify what you'd like me to look at or comment on?

If you're asking about the documentation update you just made (clarifying that tokio::fs uses spawn_blocking internally), that's a good clarification! The updated docs accurately explain that the benefit is the cleaner async API rather than a fundamentally different execution model.

[Copilot]

Pull request overview

This PR aims to prevent Tokio runtime stalls during snap sync by replacing synchronous filesystem calls with async equivalents, centralizing common filesystem operations under a new snap::async_fs module.

Changes:

• Added crates/networking/p2p/snap/async_fs.rs with async helpers for directory and file operations (including deterministic read_dir_paths).
• Updated snap sync and snap client code paths to use async filesystem helpers instead of std::fs::*.
• Added a new snap protocol server integration test suite and updated dependencies/changelog.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/networking/p2p/snap/async_fs.rs	Introduces async FS wrappers (tokio::fs + spawn_blocking for read_dir) and module-level tests.
crates/networking/p2p/snap/mod.rs	Exports the new async_fs module.
crates/networking/p2p/snap/client.rs	Replaces snapshot directory creation logic with async_fs::ensure_dir_exists.
crates/networking/p2p/sync/snap_sync.rs	Switches snapshot read/list/remove operations to async equivalents via async_fs.
crates/networking/p2p/Cargo.toml	Adds tempfile dev-dependency for async_fs tests.
crates/networking/p2p/tests/snap_server_tests.rs	Adds a large integration test suite for snap server AccountRange behavior.
CHANGELOG.md	Adds a perf changelog entry for async disk I/O in snap sync.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

BigEndianHash is imported but never used in this test module. This will at least generate an unused import warning; please remove it or use it if intended.

Suggested change

	use ethrex_common::{BigEndianHash, H256, types::AccountStateSlimCodec};
	use ethrex_common::{H256, types::AccountStateSlimCodec};

[ElFantasma]

N/A — the test file was already merged to main via #5975 and is no longer part of this PR's diff.

[Copilot]

In this loop, file_path.clone() is only used to build the decode error, but file_path itself isn’t used afterwards. You can avoid the extra allocation by moving file_path into the error (and only borrowing it for the read).

Suggested change

	.map_err(|_| SyncError::CodeHashesSnapshotDecodeError(file_path.clone()))?;
	.map_err(|_| SyncError::CodeHashesSnapshotDecodeError(file_path))?;

[ElFantasma]

Fixed in fe5f704 — file_path is moved into the error directly.

[Copilot]

This PR description focuses on async disk I/O changes, but it also adds a large new integration test suite (snap_server_tests.rs). Please update the PR description (or split into a separate PR) so the scope and motivation of these tests are clear.

[ElFantasma]

N/A — the test file was already merged to main via #5975 and is no longer part of this PR's diff.

[iovoid]

Never called

[ElFantasma]

Removed in fe5f704.

[iovoid]

Never called

[ElFantasma]

Removed in fe5f704.

[iovoid]

IMO the complexity is overstated. The async version would look almost identical, and doesn't have any ownership/lifetime related annoyance.

I agree with the choice, but the sections describing why should be trimmed.

[ElFantasma]

Agreed. Trimmed the module docs from ~45 lines to 6 in fe5f704.

[iovoid]

It should be explained why try_exists is called when create_dir_all is idempotent.

[ElFantasma]

Good catch — create_dir_all is idempotent and handles the exists case. Simplified to just tokio::fs::create_dir_all in fe5f704.

All comments addressed in fe5f704: removed unused functions, trimmed docs, simplified ensure_dir_exists.