Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 2m 17s —— View job Documentation Review CompleteI've completed a comprehensive review of this documentation rewrite PR. This is an excellent effort to restructure and expand the documentation, adding substantial content across concepts, guides, and technical specifications. However, there are several issues that need attention before merging. Critical Issues1. Complete File DuplicationLocation: These two files are 100% identical (759 lines each). This creates a maintenance burden where updates to one file won't be reflected in the other. Recommendation:
2. Incorrect Code ImplementationLocation: The // Current (INCORRECT):
RunE: func(cmd *cobra.Command, _ []string) error {
return server.Start(cmd, evabci_server.StartHandler())
},Should be (consistent with RunE: func(cmd *cobra.Command, _ []string) error {
return evabci_server.StartHandler(cmd, app.New)
},The current pattern wrapping High Priority Issues3. Technical Inconsistency: Backoff CapLocation:
These describe the same retry strategy but give different cap values. This needs to be consistent throughout the document. 4. Broken Internal LinkLocation: Links to - [Full Node](./full_node.md): Outlines... # BROKENShould be: - [Full Node](./full-node.md): Outlines... # CORRECTMedium Priority Issues5. Typo: Double SpaceLocation: Every node (both full and light) runs...Extra space between "Every" and "node". 6. Formatting: Stray CharacterLocation: The [Store] is initialized with `DefaultStore`, an implementation of the [store interface] which is used for storing and retrieving blocks, commits, and state. |Trailing 7. Formatting: Trailing SpacesLocation: The heading "### Data Sync Service" has trailing spaces. 8. Capitalization InconsistencyLocation: > **Note: The guide on how to run an evolve EVM full node...Should be "Evolve" (capitalized) for consistency with the rest of the documentation. Low Priority / Style Issues9. Outdated Description (Minor)Location: The text description mentions Code Quality Notesscripts/utils.mk ChangesThe changes to Positive ObservationsThis PR demonstrates excellent work in several areas:
Recommendations
SummaryThis is a significant and valuable documentation improvement. The issues identified are fixable and don't diminish the overall quality of the work. Once the critical issues are addressed, this will be a major enhancement to the project's documentation. Branch: marko/docs_rewrite |
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 56.05% 55.96% -0.10%
==========================================
Files 117 117
Lines 11844 11844
==========================================
- Hits 6639 6628 -11
- Misses 4475 4486 +11
Partials 730 730
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.
| RunE: func(cmd *cobra.Command, _ []string) error { | ||
| return server.Start(cmd, evabci_server.StartHandler()) | ||
| }, |
There was a problem hiding this comment.
The implementation of the start command's RunE function appears to be incorrect and is inconsistent with other guides like integration-guide.md and getting-started/cosmos/integrate-ev-abci.md. The use of server.Start wrapping the evabci_server.StartHandler is not the standard pattern and will likely cause an error, preventing the node from starting. Please make this consistent with the other guides.
| RunE: func(cmd *cobra.Command, _ []string) error { | |
| return server.Start(cmd, evabci_server.StartHandler()) | |
| }, | |
| RunE: func(cmd *cobra.Command, _ []string) error { | |
| return evabci_server.StartHandler(cmd, app.New) | |
| }, |
| - **Centralized State Management**: The `retryStrategy` struct manages attempt counts, backoff timing, and gas price adjustments | ||
| - **Multiple Backoff Types**: | ||
| - Exponential backoff for general failures (doubles each attempt, capped at `BlockTime`) | ||
| - Mempool-specific backoff (waits `MempoolTTL * BlockTime` for stuck transactions) | ||
| - Success-based backoff reset with gas price reduction |
There was a problem hiding this comment.
There's an inconsistency in the documentation regarding backoff caps. Line 296 states that exponential backoff is "capped at DABlockTime", while the "Retry Strategy Features" section on line 340 says it's "capped at BlockTime". Please ensure this is consistent across the document.
| @@ -0,0 +1,60 @@ | |||
| # P2P | |||
|
|
|||
| Every node (both full and light) runs a P2P client using [go-libp2p][go-libp2p] P2P networking stack for gossiping transactions in the chain's P2P network. The same P2P client is also used by the header and block sync services for gossiping headers and blocks. | |||
There was a problem hiding this comment.
There is a typo with a double space after "Every".
| Every node (both full and light) runs a P2P client using [go-libp2p][go-libp2p] P2P networking stack for gossiping transactions in the chain's P2P network. The same P2P client is also used by the header and block sync services for gossiping headers and blocks. | |
| Every node (both full and light) runs a P2P client using [go-libp2p][go-libp2p] P2P networking stack for gossiping transactions in the chain's P2P network. The same P2P client is also used by the header and block sync services for gossiping headers and blocks. |
| # Custom Precompiles | ||
|
|
||
| <!-- TODO: Create custom precompiles guide (ev-reth specific) | ||
| - What are precompiles | ||
| - ev-reth custom precompiles: | ||
| - Mint precompile | ||
| - Others as they exist | ||
| - Adding custom precompiles to ev-reth | ||
| - Chainspec activation configuration | ||
| - Reference: ev-reth repository | ||
| --> |
There was a problem hiding this comment.
This file is a placeholder with a TODO comment. Placeholder content should be completed or the file should be removed before merging a documentation rewrite PR. This comment applies to several other new files in this PR as well, such as docs/guides/operations/troubleshooting.md, docs/guides/operations/upgrades.md, docs/guides/running-nodes/aggregator.md, docs/guides/running-nodes/attester.md, docs/guides/running-nodes/light-node.md, and several files under docs/reference/.
docs/guides/da-layers/celestia.md
Outdated
| Now that we are posting to the Celestia DA instead of the local DA, the `evolve start` command requires three DA configuration flags: | ||
|
|
||
| - `--evnode.da.start_height` | ||
| - `--evnode.da.auth_token` | ||
| - `--evnode.da.namespace` |
There was a problem hiding this comment.
The description of required DA flags is outdated. It lists --evnode.da.namespace as a required flag, but this flag is deprecated. The text should be updated to reflect the use of --evnode.da.header_namespace and --evnode.da.data_namespace, as correctly shown in the command example later in the document.
| Now that we are posting to the Celestia DA instead of the local DA, the `evolve start` command requires three DA configuration flags: | |
| - `--evnode.da.start_height` | |
| - `--evnode.da.auth_token` | |
| - `--evnode.da.namespace` | |
| Now that we are posting to the Celestia DA instead of the local DA, the `evolve start` command requires several DA configuration flags: | |
| - `--evnode.da.start_height` | |
| - `--evnode.da.auth_token` | |
| - `--evnode.da.header_namespace` | |
| - `--evnode.da.data_namespace` |
|
|
||
| This guide covers how to set up a full node to run alongside a sequencer node in a Evolve-based blockchain network. A full node maintains a complete copy of the blockchain and helps validate transactions, improving the network's decentralization and security. | ||
|
|
||
| > **Note: The guide on how to run an evolve EVM full node can be found [in the evm section](./evm/single.md#setting-up-a-full-node).** |
There was a problem hiding this comment.
There's a typo in the note. It should be Evolve instead of evolve for consistency.
| > **Note: The guide on how to run an evolve EVM full node can be found [in the evm section](./evm/single.md#setting-up-a-full-node).** | |
| > **Note: The guide on how to run an Evolve EVM full node can be found [in the evm section](./evm/single.md#setting-up-a-full-node).** |
|
|
||
| ### Store | ||
|
|
||
| The [Store] is initialized with `DefaultStore`, an implementation of the [store interface] which is used for storing and retrieving blocks, commits, and state. | |
There was a problem hiding this comment.
There is a stray | character at the end of this line which should be removed.
| The [Store] is initialized with `DefaultStore`, an implementation of the [store interface] which is used for storing and retrieving blocks, commits, and state. | | |
| The [Store] is initialized with `DefaultStore`, an implementation of the [store interface] which is used for storing and retrieving blocks, commits, and state. |
| - Used by all node types (sequencer, full, and light) | ||
| - Essential for maintaining the canonical view of the chain | ||
|
|
||
| ### Data Sync Service |
There was a problem hiding this comment.
There are trailing spaces at the end of this markdown heading which should be removed.
| ### Data Sync Service | |
| ### Data Sync Service |
| - [Block Components](./block-manager.md): Explains the modular component architecture for block processing in Evolve. | ||
| - [Block Validity](./block-validity.md): Details the rules and checks for block validity within the protocol. | ||
| - [Data Availability (DA)](./da.md): Describes how Evolve ensures data availability and integrates with DA layers. | ||
| - [Full Node](./full_node.md): Outlines the architecture and operation of a full node in Evolve. |
There was a problem hiding this comment.
The link to the full node specification appears to be broken due to a filename mismatch. The target file is full-node.md, not full_node.md.
| - [Full Node](./full_node.md): Outlines the architecture and operation of a full node in Evolve. | |
| - [Full Node](./full-node.md): Outlines the architecture and operation of a full node in Evolve. |
- 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.
| - **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}
|
|
||
| #### 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.
| ### 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?
2 participants
Overview
This pr restructures the docs and writes new sections in order to be better suited for users