docs(l1): general docs improvement #5771
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
January 7, 2026 16:23
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive documentation improvements for the ethrex L1 architecture, enhancing both high-level architectural documentation and code-level API documentation.
Key Changes
- Added new architecture documentation covering system overview, block execution pipeline, sync state machine, and crate organization
- Enhanced Rust API documentation with module-level docs, struct documentation, and usage examples across core crates
- Added README files for major crates (storage, RPC, P2P, blockchain) to improve discoverability
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
docs/l1/architecture/README.md |
New architecture section index |
docs/l1/architecture/overview.md |
Comprehensive system architecture overview with diagrams |
docs/l1/architecture/block_execution.md |
Detailed block execution pipeline documentation |
docs/l1/architecture/sync_state_machine.md |
Full sync and snap sync algorithm documentation |
docs/l1/architecture/crate_map.md |
Complete crate dependency map and responsibilities |
docs/SUMMARY.md |
Updated navigation to include new architecture docs |
crates/vm/levm/src/lib.rs |
Added comprehensive module-level documentation |
crates/vm/levm/src/vm.rs |
Enhanced VM and Substate struct documentation |
crates/storage/lib.rs |
Added storage crate overview documentation |
crates/storage/store.rs |
Documented Store struct and related types |
crates/storage/README.md |
New README with usage examples and architecture |
crates/networking/rpc/lib.rs |
Added RPC crate overview documentation |
crates/networking/rpc/rpc.rs |
Documented RPC handler trait and context types |
crates/networking/rpc/utils.rs |
Enhanced error type and request/response documentation |
crates/networking/rpc/eth/gas_price.rs |
Added gas price estimation documentation |
crates/networking/rpc/README.md |
New README with supported methods and examples |
crates/networking/p2p/p2p.rs |
Added P2P module overview documentation |
crates/networking/p2p/README.md |
New README covering P2P protocols and usage |
crates/blockchain/blockchain.rs |
Enhanced Blockchain struct documentation |
crates/blockchain/README.md |
New README with block execution flow and examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+58
to
+60
| /// The substate supports checkpointing via [`push_backup`] and restoration via | ||
| /// [`revert_backup`] or commitment via [`commit_backup`]. This is used to handle | ||
| /// nested calls where inner calls may fail and need to be reverted. |
There was a problem hiding this comment.
The documentation mentions "push_backup" and "revert_backup" or "commit_backup" but these method names should be enclosed in backticks or brackets for proper cross-referencing in Rust documentation.
Comment on lines
+62
to
+63
| /// Most fields are private by design. The backup mechanism only works correctly | ||
| /// if data modifications are append-only. |
There was a problem hiding this comment.
The documentation mentions the "backup mechanism" and states it "only works correctly if data modifications are append-only" but doesn't explain what "append-only" means in this context or provide examples. This could be confusing for developers unfamiliar with the implementation.
Suggested change
| /// Most fields are private by design. The backup mechanism only works correctly | |
| /// if data modifications are append-only. | |
| /// Most fields are private by design. The backup mechanism assumes that changes | |
| /// to the substate are *append-only*: code may add new entries (for example, | |
| /// pushing a new log, inserting into a set, or inserting a new key into a map), | |
| /// but must not remove existing entries or mutate them in place in a way that | |
| /// cannot be reconstructed from the parent checkpoint. For instance, adding a | |
| /// new address to `created_accounts` or pushing to `logs` is safe, while | |
| /// clearing `logs`, removing entries from `selfdestruct_set`, or overwriting a | |
| /// previously stored log would violate this assumption and can make | |
| /// [`revert_backup`] behave incorrectly. |
Comment on lines
+157
to
+165
| /// let store = Store::new("./data", EngineType::RocksDB)?; | ||
| /// | ||
| /// // Add a block | ||
| /// store.add_block(block).await?; | ||
| /// | ||
| /// // Query account balance | ||
| /// let info = store.get_account_info(block_number, address)?; | ||
| /// let balance = info.map(|a| a.balance).unwrap_or_default(); | ||
| /// ``` |
There was a problem hiding this comment.
The example code is marked with "ignore" but would be more helpful if it showed the actual expected types and error handling. Consider providing a more complete example or explaining why it's ignored.
Suggested change
| /// let store = Store::new("./data", EngineType::RocksDB)?; | |
| /// | |
| /// // Add a block | |
| /// store.add_block(block).await?; | |
| /// | |
| /// // Query account balance | |
| /// let info = store.get_account_info(block_number, address)?; | |
| /// let balance = info.map(|a| a.balance).unwrap_or_default(); | |
| /// ``` | |
| /// use crate::storage::Store; | |
| /// use crate::error::StoreError; | |
| /// | |
| /// // This example requires an async runtime (e.g. Tokio) and the `rocksdb` | |
| /// // feature to be enabled on this crate. | |
| /// async fn example() -> Result<(), StoreError> { | |
| /// let db_path = "./data"; | |
| /// let engine = EngineType::RocksDB; | |
| /// | |
| /// // Open the store | |
| /// let store = Store::new(db_path, engine)?; | |
| /// | |
| /// // Add a block | |
| /// store.add_block(block).await?; | |
| /// | |
| /// // Query account balance | |
| /// let info = store.get_account_info(block_number, address)?; | |
| /// let balance = info.map(|a| a.balance).unwrap_or_default(); | |
| /// | |
| /// println!("Balance: {balance}"); | |
| /// Ok(()) | |
| /// } | |
| /// ``` | |
| /// | |
| /// This example is marked `ignore` because it depends on the `rocksdb` feature | |
| /// and an async runtime, which are not available in the doctest environment. |
| **Purpose:** Zero-knowledge proof generation. | ||
|
|
||
| **Supported Provers:** | ||
| - SP1 (Succinct) |
There was a problem hiding this comment.
The word "Succinct" in the "Supported Provers" section is unclear. It should specify that SP1 is a Succinct-based prover, or clarify what "Succinct" refers to in this context.
Suggested change
| - SP1 (Succinct) | |
| - SP1 (Succinct-based prover) |
Comment on lines
+22
to
+26
| /// # Example Response | ||
| /// | ||
| /// ```json | ||
| /// "0x3b9aca00" // 1 Gwei in hexadecimal | ||
| /// ``` |
There was a problem hiding this comment.
The comment states "TODO: This does not need a struct, but I'm leaving it like this for consistency". This TODO comment should be removed from the documentation as it's not relevant to users and suggests incomplete implementation decisions.
Suggested change
| /// # Example Response | |
| /// | |
| /// ```json | |
| /// "0x3b9aca00" // 1 Gwei in hexadecimal | |
| /// ``` |
Comment on lines
+10
to
+11
| //! - **eth Protocol**: Block and transaction propagation | ||
| //! - **snap Protocol**: Fast state synchronization |
There was a problem hiding this comment.
The documentation refers to "eth Protocol" and "snap Protocol" without the / version specifier. For consistency with the rest of the documentation (e.g., "eth/68", "snap/1"), consider using "eth/68 Protocol" and "snap/1 Protocol".
Suggested change
| //! - **eth Protocol**: Block and transaction propagation | |
| //! - **snap Protocol**: Fast state synchronization | |
| //! - **eth/68 Protocol**: Block and transaction propagation | |
| //! - **snap/1 Protocol**: Fast state synchronization |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
iovoid
requested changes
Jan 8, 2026
Contributor
iovoid
left a comment
iovoid
left a comment
There was a problem hiding this comment.
I feel like duplicating the documentation between the rustdocs and README might eventually lead to them being out of sync.
crates/blockchain/README.md
Outdated
Comment on lines
136
to
140
| ## Fork Support | ||
|
|
||
| The blockchain automatically handles fork transitions based on chain config: | ||
|
|
||
| - **Homestead** through **London**: Legacy fee market |
Contributor
There was a problem hiding this comment.
Maybe a good place to mention we don't support pre-merge forks?
crates/networking/p2p/README.md
Outdated
| - Status messages and chain info | ||
|
|
||
| ### snap/1 | ||
| State snapshot protocol for fast sync: |
Contributor
There was a problem hiding this comment.
"fast sync" usually refers to a specific sync strategy, so it's probably better to say either "snap sync" or "faster syncs" (or similar rephrasings)
| use serde_json::Value; | ||
|
|
||
| // TODO: This does not need a struct, | ||
| // but I'm leaving it like this for consistency |
crates/storage/README.md
Outdated
|
|
||
| The store maintains several caches for performance: | ||
|
|
||
| - **Trie Layer Cache**: Recent trie nodes for fast state access without full trie traversal |
Contributor
There was a problem hiding this comment.
Maybe also mention difflayer management (which is the primary function of these)
fedacking
reviewed
Jan 8, 2026
jrchatruc
approved these changes
Jan 9, 2026
lferrigno
approved these changes
Jan 9, 2026
iovoid
approved these changes
Jan 9, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Description