docs: rewrite and restructure docs #3026
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 2m 44s —— View job Claude finished @tac0turtle's task in 2m 30s —— View job Documentation Review CompleteI've conducted a comprehensive review of this documentation rewrite PR. This is an ambitious effort to restructure and expand the documentation, but there are several critical issues that need to be addressed before merging. Executive SummaryMajor Issues:
Recommendation: Substantial revision needed before merge. The issues below should be addressed systematically. Critical Issues1. Severe Content DuplicationBlock Lifecycle Documentation (CRITICAL)
Recommendation:
Getting Started vs. ev-abci Integration (HIGH)Multiple reviewers (pthmas, gemini-code-assist) identified duplication: Duplicated:
The ev-abci folder should be the single source of truth for ev-abci integration, with getting-started providing only minimal quickstart pointers. 2. Technical InaccuraciesBased Sequencing Description (MEDIUM - docs/guides/advanced/based-sequencing.md:63)Issue: Documentation states "No malicious actor concern - invalid blocks are automatically rejected by validation rules" Reality: The code shows based sequencing uses Location: docs/guides/advanced/based-sequencing.md:55-62 Retry Strategy Documentation (MEDIUM - docs/concepts/block-lifecycle.md:296,340)Inconsistency: Line 296 states exponential backoff is "capped at Code Check: I searched for Location: docs/concepts/block-lifecycle.md:296, 340 Fix backoff cap inconsistency → Deprecated Namespace Flag (MEDIUM - docs/guides/da-layers/celestia.md)Issue: Documentation describes Location: docs/guides/da-layers/celestia.md (description section vs. command example) Gas Price Detection Claims (LOW - docs/concepts/block-lifecycle.md:165)Issue: Reviewer pthmas notes: "Maybe a bit misleading? automatic gas price detection is set by default unless someone specifies with the Location: docs/concepts/block-lifecycle.md:165 (GasPrice configuration description) 3. Structural and Organizational IssuesUnnecessary Navigation Layers (MEDIUM)
Location: docs/getting-started/choose-your-path.md Reviewer comment (pthmas): "Not sure we need this page" Overview vs. Concepts Duplication (MEDIUM)
Reviewer comment (pthmas): "this feels also a bit like a duplicate of the docs/concepts folder. imo the overview folder should be one page" Placeholder Files (HIGH)Several files were flagged by gemini-code-assist review as containing only TODO comments:
Action Required: Either complete these files or remove them from this PR. 4. Documentation Quality IssuesInconsistent Migration Guidance (HIGH)docs/ev-abci/migration-from-cometbft.md:191 Gemini-code-assist review identified incorrect start command pattern: // Documented (INCORRECT):
RunE: func(cmd *cobra.Command, _ []string) error {
return server.Start(cmd, evabci_server.StartHandler(cmd, app.New))
},
// Should be:
RunE: func(cmd *cobra.Command, _ []string) error {
return evabci_server.StartHandler(cmd, app.New)
},This error could prevent successful migration for users. Attester Implementation Status (MEDIUM)docs/overview/architecture.md:89 - Reviewer alpe notes: "Should specify that this is not yet available" The architecture table lists "Attester" as a node type, but this may not be implemented yet. Location: docs/overview/architecture.md:89 Forced Inclusion / Based Sequencing Confusion (MEDIUM)docs/guides/advanced/forced-inclusion.md:101-115,128 Reviewer pthmas comments:
These sections conflate forced inclusion (censorship resistance mechanism) with based sequencing (fully decentralized mode). 5. Minor IssuesFormatting Issues
Unnecessary Summary Sections (LOW)Reviewer pthmas notes: "I think it's useless to have a summary at the end of the page"
Maintenance Concerns (MEDIUM)docs/reference/interfaces/da.md - Reviewer pthmas: "It's probably hard to maintain all the interfaces changes up to date and documented. Could we just point to the correct files in the repo?" Documenting full interfaces in markdown will quickly become stale. Consider linking to GoDoc or code instead. docs/guides/da-layers/celestia.md - Reviewer pthmas: "hard to maintain, should we just link the celestia docs?" External dependencies (like Celestia setup) should reference official docs rather than duplicating them. Log Messages Without Context (LOW)docs/guides/operations/troubleshooting.md - Reviewer pthmas notes the error examples provide messages without actionable context or troubleshooting steps beyond what users would already know. Positive AspectsDespite the issues, this PR has significant value:
RecommendationsImmediate Actions (Before Merge)
Medium-Term Improvements
Testing RecommendationsSince I cannot run markdown linters without permissions, the team should: # Run markdown linting
make lint
# Check for broken internal links
# (consider adding a link checker to CI)
# Verify code examples compile/run
# (test the integration examples)SummaryThis is valuable work that significantly expands the documentation, but it needs substantial revision before merge. The most critical issue is content duplication which will cause maintenance problems immediately. Technical inaccuracies could mislead users and cause failed deployments. Suggested Approach:
The documentation framework and structure are sound - the content just needs consolidation and accuracy review. |
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's documentation, providing a more organized and in-depth resource for developers and operators. It covers fundamental concepts, practical implementation steps for different execution environments, operational best practices, and detailed technical specifications, making it easier for users to understand, integrate, and manage Evolve-based chains. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3026 +/- ##
=======================================
Coverage 61.64% 61.64%
=======================================
Files 111 111
Lines 11120 11120
=======================================
Hits 6855 6855
Misses 3526 3526
Partials 739 739
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant rewrite and restructuring of the documentation, adding a large volume of new content across concepts, guides, and reference sections. The effort to improve the documentation is commendable. However, there are several areas that need attention before this can be merged.
Firstly, there's a notable amount of content duplication, particularly between docs/concepts/block-lifecycle.md and docs/reference/specs/block-manager.md. This could lead to maintenance issues and should be resolved, perhaps by using includes or linking between documents.
Secondly, a large number of placeholder files have been added, containing only TODO comments. These should be either completed or removed.
Finally, there are various inconsistencies in technical details, formatting issues (like the use of :::: instead of ::: for VitePress admonitions), minor typos, and some broken links. Addressing these points will greatly improve the quality and usability of the new documentation.
- Updated Celestia guide to clarify prerequisites, installation, and configuration for connecting Evolve chains to Celestia. - Enhanced Local DA documentation with installation instructions, configuration options, and use cases for development and testing. - Expanded troubleshooting guide with detailed diagnostic commands, common issues, and solutions for node operations. - Created comprehensive upgrades guide covering minor and major upgrades, version compatibility, and rollback procedures. - Added aggregator node documentation detailing configuration, block production settings, and monitoring options. - Introduced attester node overview with configuration and use cases for low-latency applications. - Removed outdated light node documentation. - Improved formatting and clarity in ev-reth chainspec reference for better readability.
tac0turtle
requested review from
alpe,
chatton,
julienrbrt,
pthmas and
randygrok
pthmas
left a comment
pthmas
left a comment
There was a problem hiding this comment.
A lot of duplicate that we need to remove but overall looks good. I can't guarantee that the evm part is correct i don't have enough knowledge about it.
docs/concepts/block-lifecycle.md
Outdated
| - **Gas Price Management**: | ||
| - Increases gas price by `GasMultiplier` on mempool failures | ||
| - Decreases gas price after successful submissions (bounded by initial price) | ||
| - Supports automatic gas price detection (`-1` value) |
There was a problem hiding this comment.
Maybe a bit misleading? automatic gas price detection is set by default unless someone specifies with the da.submit_options {gas_price: xx}
docs/concepts/block-lifecycle.md
Outdated
|
|
||
| #### Termination Condition | ||
|
|
||
| If the sequencer double-signs two blocks at the same height, evidence of the fault should be posted to DA. Evolve full nodes should process the longest valid chain up to the height of the fault evidence, and terminate. See diagram: |
There was a problem hiding this comment.
Is this actually implemented? I couldn't find anything in the codebase.
docs/concepts/block-lifecycle.md
Outdated
| ### Initialization and State Management | ||
|
|
||
| - Components load the initial state from the local store and use genesis if not found in the local store, when the node (re)starts | ||
| - During startup the Syncer invokes the execution Replayer to re-execute any blocks the local execution layer is missing; the replayer enforces strict app-hash matching so a mismatch aborts initialization instead of silently drifting out of sync | ||
| - The default mode for aggregator nodes is normal (not lazy) | ||
| - Components coordinate through channels and shared cache structures | ||
|
|
||
| ### Block Production (Executor Component) | ||
|
|
||
| - The Executor can produce empty blocks | ||
| - In lazy aggregation mode, the Executor maintains consistency with the DA layer by producing empty blocks at regular intervals, ensuring a 1:1 mapping between DA layer blocks and execution layer blocks | ||
| - The lazy aggregation mechanism uses a dual timer approach: | ||
| - A `blockTimer` that triggers block production when transactions are available | ||
| - A `lazyTimer` that ensures blocks are produced even during periods of inactivity | ||
| - Empty batches are handled differently in lazy mode - instead of discarding them, they are returned with the `ErrNoBatch` error, allowing the caller to create empty blocks with proper timestamps | ||
| - Transaction notifications from the `Reaper` to the `Executor` are handled via a non-blocking notification channel (`txNotifyCh`) to prevent backpressure | ||
|
|
||
| ### DA Submission (Submitter Component) | ||
|
|
||
| - The Submitter enforces `MaxPendingHeadersAndData` limit to prevent unbounded growth of pending queues during DA submission issues | ||
| - Headers and data are submitted separately to the DA layer using different namespaces, supporting the header/data separation architecture | ||
| - The Cache Manager uses persistent caches for headers and data to track seen items and DA inclusion status | ||
| - Namespace migration is handled transparently by the Syncer, with automatic detection and state persistence to optimize future operations | ||
| - The system supports backward compatibility with legacy single-namespace deployments while transitioning to separate namespaces | ||
| - Gas price management in the Submitter includes automatic adjustment with `GasMultiplier` on DA submission retries | ||
|
|
||
| ### Storage and Persistence | ||
|
|
||
| - Components use persistent storage (disk) when the `root_dir` and `db_path` configuration parameters are specified in `config.yaml` file under the app directory. If these configuration parameters are not specified, the in-memory storage is used, which will not be persistent if the node stops | ||
| - The Syncer does not re-apply blocks when they transition from soft confirmed to DA included status. The block is only marked DA included in the caches | ||
| - Header and data stores use separate prefixes for isolation in the underlying database | ||
| - The genesis `ChainID` is used to create separate `PubSubTopID`s for headers and data in go-header | ||
|
|
||
| ### P2P and Synchronization | ||
|
|
||
| - Block sync over the P2P network works only when a full node is connected to the P2P network by specifying the initial seeds to connect to via `P2PConfig.Seeds` configuration parameter when starting the full node | ||
| - Node's context is passed down to all components to support graceful shutdown and cancellation | ||
|
|
||
| ### Architecture Design Decisions | ||
|
|
||
| - The Executor supports custom signature payload providers for headers, enabling flexible signing schemes | ||
| - The component architecture supports the separation of header and data structures in Evolve. This allows for expanding the sequencing scheme beyond single sequencing and enables the use of a decentralized sequencer mode. For detailed information on this architecture, see the [Header and Data Separation ADR](../../adr/adr-014-header-and-data-separation.md) | ||
| - Components process blocks with a minimal header format, which is designed to eliminate dependency on CometBFT's header format and can be used to produce an execution layer tailored header if needed. For details on this header structure, see the [Evolve Minimal Header](../../adr/adr-015-rollkit-minimal-header.md) specification |
There was a problem hiding this comment.
This feels like a duplicate of the previous chapters of this same file.
| - The component architecture supports the separation of header and data structures in Evolve. This allows for expanding the sequencing scheme beyond single sequencing and enables the use of a decentralized sequencer mode. For detailed information on this architecture, see the [Header and Data Separation ADR](../../adr/adr-014-header-and-data-separation.md) | ||
| - Components process blocks with a minimal header format, which is designed to eliminate dependency on CometBFT's header format and can be used to produce an execution layer tailored header if needed. For details on this header structure, see the [Evolve Minimal Header](../../adr/adr-015-rollkit-minimal-header.md) specification | ||
|
|
||
| ## Metrics |
There was a problem hiding this comment.
Should the metric get a dedicated page?
| - **Guarantee**: Data availability sampling (DAS) | ||
| - **Latency**: ~12 seconds to finality | ||
|
|
||
| ### Custom DA |
|
|
||
| We are now ready to run our full node. If we are running the full node on the same machine as the sequencer, we need to make sure we update the ports to avoid conflicts. | ||
|
|
||
| Make sure to include these flags with your start command: |
There was a problem hiding this comment.
may we could also add that the config/ev-node file can be updated
| | **Aggregator** | Yes | Yes | Yes | Block producer (sequencer) | | ||
| | **Full Node** | No | Yes | No | RPC provider, validator | | ||
| | **Light Node** | No | Headers only | No | Mobile, embedded clients | | ||
| | **Attester** | No | Yes | No | Soft consensus participant | |
There was a problem hiding this comment.
Should specify that this is not yet available
| @@ -0,0 +1,185 @@ | |||
| # Architecture | |||
There was a problem hiding this comment.
this feels also a bit like a duplicate of the docs/concepts folder
| @@ -0,0 +1,95 @@ | |||
| # Introduction | |||
There was a problem hiding this comment.
imo the overview folder should be one page, for more information the reader should go to the concepts folder
| @@ -0,0 +1,193 @@ | |||
| # DA Interface | |||
There was a problem hiding this comment.
It's probably hard to maintain all the interfaces changes up to date and documented. Could we just point to the correct files in the repo?
3 participants
Overview
This pr restructures the docs and writes new sections in order to be better suited for users