Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @julienrbrt, 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 enhances the node synchronization mechanism by introducing a "trusted height" feature. This allows nodes to bootstrap their state from a known, verified block height, significantly reducing the time required for initial synchronization, especially in long-running networks. The changes encompass new configuration options, robust validation to ensure secure usage, and integration into the core synchronization and state initialization processes. 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. Changelog
Activity
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
|
|
Claude finished @julienrbrt's task in 1m 54s —— View job Code Review: Trusted Height Sync FeatureI've completed a comprehensive code review of the trusted height synchronization feature. This is a valuable addition that allows nodes to bootstrap from a known verified block height instead of syncing from genesis. Here are my findings: Critical Issues1. State Initialization Logic May Cause Startup Failure
|
![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 valuable feature for faster node synchronization by starting from a trusted height. The implementation is spread across configuration, synchronization, and storage logic, and is mostly well-executed. I've identified a potential critical issue in the state initialization logic that could prevent nodes from starting correctly with a trusted height, along with some inconsistencies in the documentation and a minor code cleanup opportunity. Once these points are addressed, this will be a solid contribution.
| if s.config.P2P.TrustedHeight > 0 { | ||
| s.logger.Info().Uint64("trusted_height", s.config.P2P.TrustedHeight).Msg("initializing state from trusted height") | ||
|
|
||
| // Load and verify the trusted header | ||
| trustedHeader, err := s.getTrustedHeader(s.ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load trusted header: %w", err) | ||
| } | ||
|
|
||
| state = types.State{ | ||
| ChainID: s.genesis.ChainID, | ||
| InitialHeight: s.genesis.InitialHeight, | ||
| LastBlockHeight: s.genesis.InitialHeight - 1, | ||
| LastBlockTime: s.genesis.StartTime, | ||
| DAHeight: s.genesis.DAStartHeight, | ||
| AppHash: stateRoot, | ||
| // Initialize new chain state from the trusted header | ||
| stateRoot, initErr := s.exec.InitChain( | ||
| s.ctx, | ||
| trustedHeader.Time(), | ||
| trustedHeader.Height(), | ||
| trustedHeader.ChainID(), | ||
| ) | ||
| if initErr != nil { | ||
| return fmt.Errorf("failed to initialize execution client: %w", initErr) | ||
| } | ||
|
|
||
| state = types.State{ | ||
| Version: types.InitStateVersion, | ||
| ChainID: trustedHeader.ChainID(), | ||
| InitialHeight: trustedHeader.Height(), | ||
| LastBlockHeight: trustedHeader.Height(), | ||
| LastBlockTime: trustedHeader.Time(), | ||
| LastHeaderHash: trustedHeader.Hash(), // Hash of the trusted header | ||
| DAHeight: s.genesis.DAStartHeight, | ||
| AppHash: stateRoot, | ||
| } |
There was a problem hiding this comment.
This new logic for initializing from a trusted height looks mostly correct, but there's a potential critical issue with a subsequent call outside this block. After this block, execReplayer.SyncToHeight is called at line 421. When initializing from a trusted height, this can lead to a failure.
The replayer is initialized with the original genesis information and will likely try to replay blocks from genesis.InitialHeight. However, for a trusted sync, the store will not contain these intermediate blocks, causing GetBlockData to fail.
Since s.exec.InitChain is already called to set the executor's state to the trusted height, the subsequent replayer call seems unnecessary and problematic for this case. Consider restructuring the logic to skip the execReplayer.SyncToHeight call when the state is initialized from a trusted height.
| ```yaml | ||
| sync: | ||
| trusted_height: 100000 # Block height to trust for sync initialization | ||
| trusted_header_hash: "a1b2c3d4e5f6..." # Hex-encoded hash of the header at trusted_height | ||
| ``` | ||
|
|
||
| --- | ||
| **Command-line Flags:** | ||
|
|
||
| - `--evnode.sync.trusted_height <uint64>` - Block height to trust for sync initialization | ||
| - `--evnode.sync.trusted_header_hash <string>` - Hash of the trusted header for security verification (hex-encoded) | ||
|
|
||
| **Example:** | ||
|
|
||
| ```bash | ||
| testapp start \ | ||
| --evnode.sync.trusted_height 100000 \ | ||
| --evnode.sync.trusted_header_hash "abc123def456..." | ||
| ``` |
There was a problem hiding this comment.
The documentation for the new trusted sync feature appears to be inconsistent with the implementation in pkg/config/config.go.
- YAML Configuration: The documentation places
trusted_heightandtrusted_header_hashunder async:section. However, the code defines these fields withinP2PConfig, which is mapped to ap2p:section in YAML. - Command-line Flags: The documentation lists the flags as
--evnode.sync.trusted_heightand--evnode.sync.trusted_header_hash. The implementation defines them as--evnode.p2p.trusted_heightand--evnode.p2p.trusted_header_hash.
To avoid user confusion, the documentation should be updated to reflect the implementation. Here is a suggested correction:
**YAML:**
```yaml
p2p:
trusted_height: 100000 # Block height to trust for sync initialization
trusted_header_hash: "a1b2c3d4e5f6..." # Hex-encoded hash of the header at trusted_heightCommand-line Flags:
--evnode.p2p.trusted_height <uint64>- Block height to trust for sync initialization--evnode.p2p.trusted_header_hash <string>- Hash of the trusted header for security verification (hex-encoded)
Example:
testapp start \
--evnode.p2p.trusted_height 100000 \
--evnode.p2p.trusted_header_hash "abc123def456..."|
|
||
| // fetchAndVerifyTrustedHeader fetches the header at the trusted height from P2P | ||
| // and verifies it matches the trusted hash. If verification passes, it stores the header. | ||
| func (syncService *SyncService[H]) fetchAndVerifyTrustedHeader(ctx context.Context, peerIDs []peer.ID) error { |
There was a problem hiding this comment.
The peerIDs parameter is unused within this function and should be removed to simplify the signature. The call site at line 351 should be updated accordingly by removing the peerIDs argument.
| func (syncService *SyncService[H]) fetchAndVerifyTrustedHeader(ctx context.Context, peerIDs []peer.ID) error { | |
| func (syncService *SyncService[H]) fetchAndVerifyTrustedHeader(ctx context.Context) error { |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3050 +/- ##
==========================================
- Coverage 56.38% 56.19% -0.20%
==========================================
Files 118 118
Lines 12036 12092 +56
==========================================
+ Hits 6787 6795 +8
- Misses 4507 4554 +47
- Partials 742 743 +1
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:
|
1 participant
Overview
Re-implement sync from trusted height. With the recent changes, this should be more stable, and a great alternative to snapshot syncing.