-
Notifications
You must be signed in to change notification settings - Fork 246
fix(block): fix init logic sequencer for da epoch fetching #2926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 4m 29s —— View job Comprehensive Code Review: DA Epoch Fetching Initialization ✅I've completed a thorough review of PR #2926, which addresses the critical issue of sequencer genesis DA height initialization. The implementation correctly solves the problem through a deferred initialization pattern. Executive SummaryVerdict: APPROVED with Minor Recommendations ✅ The PR successfully implements genesis DA height discovery for single sequencers through a well-designed deferred initialization pattern. The previous bot reviews raised concerns about 1. Code Quality & Architecture ⭐⭐⭐⭐Strengths:
Implementation Details (Single Sequencer):Genesis Flow (
This pattern ensures:
Based Sequencer Approach:Based sequencers require 2. Error Handling & Resilience ⭐⭐⭐⭐⭐Excellent Patterns:Genesis DA Height Persistence ( if genesisInclusion {
// Persist genesis DA height
if err := s.store.SetMetadata(ctx, store.GenesisDAHeightKey, genesisDAIncludedHeightBytes); err != nil {
return err
}
// Notify sequencer
if s.sequencer != nil {
s.sequencer.SetDAHeight(genesisDAIncludedHeight)
}
}
Forced Inclusion Error Handling ( forcedTxsEvent, err := c.fiRetriever.RetrieveForcedIncludedTxs(ctx, currentDAHeight)
if err != nil {
if errors.Is(err, datypes.ErrHeightFromFuture) {
// Graceful backoff - log and return zero
return 0, nil
} else if errors.Is(err, block.ErrForceInclusionNotConfigured) {
// Return empty txs, continue without forced inclusion
c.cachedForcedInclusionTxs = [][]byte{}
return 0, nil
}
return 0, fmt.Errorf("failed to retrieve forced inclusion transactions: %w", err)
}
Based Sequencer Validation ( if nodeConfig.Node.BasedSequencer && genesis.DAStartHeight == 0 {
return fmt.Errorf("based sequencing requires DAStartHeight to be set in genesis. This value should be identical for all nodes of the chain")
}
Minor Suggestion:Consider adding a debug log when the deferred initialization is triggered ( if daHeight > 0 && c.checkpoint.DAHeight == 0 {
c.logger.Debug().
Uint64("da_height", daHeight).
Msg("initializing checkpoint with discovered genesis DA height")
// ... existing code
}3. Security ⭐⭐⭐⭐⭐Strong Security Posture:Forced Inclusion Transaction Validation ( validTxs := make([][]byte, 0, len(forcedTxsEvent.Txs))
skippedTxs := 0
for _, tx := range forcedTxsEvent.Txs {
if uint64(len(tx)) > maxBytes {
c.logger.Warn().
Uint64("da_height", forcedTxsEvent.StartDaHeight).
Int("blob_size", len(tx)).
Uint64("max_bytes", maxBytes).
Msg("forced inclusion blob exceeds maximum size - skipping")
skippedTxs++
continue
}
validTxs = append(validTxs, tx)
}
Genesis Configuration Security:
Input Validation:
4. Performance & Resource Efficiency ⭐⭐⭐⭐Efficient Design:Atomic Operations ( func (c *Sequencer) SetDAHeight(height uint64) {
c.daHeight.Store(height)
}
func (c *Sequencer) GetDAHeight() uint64 {
return c.daHeight.Load()
}
Checkpoint Caching ( if daHeight > 0 && (len(c.cachedForcedInclusionTxs) == 0 || c.checkpoint.TxIndex >= ...) {
// Fetch next epoch
}
Lazy Retriever Initialization ( c.fiRetriever = block.NewForcedInclusionRetriever(c.daClient, c.logger, getInitialDAStartHeight(ctx, c.db), c.genesis.DAEpochForcedInclusion)
Minor Observation:The 5. Testing & Reliability ⭐⭐⭐⭐Test Coverage:Submitter Tests (
Forced Inclusion Retriever Tests (
Sequencer Tests (
Recommendations:
6. Concurrency Safety ⭐⭐⭐⭐Thread-Safe Patterns:Atomic DA Height ( daHeight atomic.Uint64
func (c *Sequencer) SetDAHeight(height uint64) {
c.daHeight.Store(height)
}
Checkpoint Access:
Recommendation:Add a comment documenting that // GetNextBatch implements sequencing.Sequencer.
// IMPORTANT: This method is not thread-safe and must not be called concurrently.
// It is designed to be called sequentially by a single executor goroutine.
func (c *Sequencer) GetNextBatch(ctx context.Context, req coresequencer.GetNextBatchRequest) (*coresequencer.GetNextBatchResponse, error) {7. Documentation & Developer Experience ⭐⭐⭐⭐Strengths:
Improvements:
8. Specific Code Review Comments
|
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 addresses a critical issue where the sequencer's initial Data Availability (DA) epoch fetching logic was flawed at genesis, requiring manual intervention. The changes introduce a more robust mechanism for the sequencer to correctly determine and initialize its DA height. By enabling the Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to correct the sequencer's initialization logic for Data Availability (DA) epoch fetching. The approach is to delay setting the sequencer's DA height until the first block's inclusion in the DA layer is confirmed, at which point its actual DA height is known. The changes in the executor and submitter components correctly implement the mechanism to discover and propagate this height.
However, my review has identified two critical issues that prevent this from functioning as intended:
- The
SetDAHeightmethod on bothsingleandbasedsequencers does not update the internalcheckpointthat drives the fetching logic. As a result, the correct DA height provided by thesubmitteris ignored. - The
ForcedInclusionRetrievercontinues to use adaStartHeightfrom the genesis file for epoch calculations, instead of the actual genesis DA height discovered post-genesis. This will lead to incorrect epoch boundaries.
I have included detailed comments with suggestions to resolve these critical issues, which are necessary for the pull request to achieve its goal.
| logger: logger.With().Str("component", "forced_inclusion_retriever").Logger(), | ||
| daEpochSize: genesis.DAEpochForcedInclusion, | ||
| client: client, | ||
| daStartHeight: genesis.DAStartHeight, // TODO: this should be genesis da start height (for full nodes) or store metadata da start height (for sequencers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the TODO comment highlights, daStartHeight is initialized from genesis.DAStartHeight and is never updated. For sequencers, this value should come from the store after it's determined and persisted by the submitter. Using a stale or incorrect daStartHeight will lead to incorrect epoch calculations in RetrieveForcedIncludedTxs, which can break the forced transaction inclusion mechanism.
To fix this, ForcedInclusionRetriever needs a way to get the correct genesis DA height. One approach is to give it access to the store so it can read the GenesisDAHeightKey metadata value.
For example, you could pass a store.Store to NewForcedInclusionRetriever and then in RetrieveForcedIncludedTxs you could do something like this:
startHeight := r.daStartHeight // Fallback to genesis value
genesisDAHeightBytes, err := r.store.GetMetadata(ctx, store.GenesisDAHeightKey)
if err == nil {
startHeight = binary.LittleEndian.Uint64(genesisDAHeightBytes)
} else if !errors.Is(err, datastore.ErrNotFound) {
r.logger.Warn().Err(err).Msg("failed to get genesis DA height from store, using default from genesis")
}
epochStart, epochEnd, currentEpochNumber := types.CalculateEpochBoundaries(daHeight, startHeight, r.daEpochSize)This would ensure the correct daStartHeight is used for epoch calculations once it becomes available.
sequencers/single/sequencer.go
Outdated
| // will be overridden by the executor or submitter (at genesis) | ||
| // during genesis time, the sequencer will fetch unnecessary heights from DA genesis | ||
| // this is kept on purpose as some DAs (like local-da), do start at genesis. | ||
| s.SetDAHeight(genesis.DAStartHeight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on this line states that s.SetDAHeight will be overridden. While this is true, the current implementation of SetDAHeight is not sufficient to achieve the goal of this PR.
The SetDAHeight method only updates an atomic c.daHeight value. However, the sequencer's fetching logic in GetNextBatch is driven by c.checkpoint.DAHeight. The checkpoint is not updated by SetDAHeight, so when the submitter calls SetDAHeight with the correct genesis DA height, the sequencer will not start fetching from that height. It will continue using the stale height from the checkpoint.
To fix this, SetDAHeight must also update c.checkpoint.DAHeight and persist it. Here is a suggested implementation:
func (c *Sequencer) SetDAHeight(height uint64) {
c.daHeight.Store(height)
// If checkpoint is not initialized yet, we can't update it.
// It will be initialized with the stored daHeight later.
if c.checkpoint == nil {
return
}
// Also update the checkpoint to ensure the sequencer fetches from the new height.
if height > c.checkpoint.DAHeight {
c.logger.Info().
Uint64("old_da_height", c.checkpoint.DAHeight).
Uint64("new_da_height", height).
Msg("updating sequencer DA height checkpoint")
c.checkpoint.DAHeight = height
c.checkpoint.TxIndex = 0
// Persisting the checkpoint here ensures that even after a restart,
// the sequencer will start from the correct DA height.
// A background context is acceptable here as this is a critical state update.
if err := c.checkpointStore.Save(context.Background(), c.checkpoint); err != nil {
c.logger.Error().Err(err).Msg("failed to save updated checkpoint in SetDAHeight")
}
}
}| daEpochSize: genesis.DAEpochForcedInclusion, | ||
| } | ||
|
|
||
| // check for inclusion da height on store (sequencer) or genesis da height (full nodes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a test for that.
|
@claude comment is true. Need to update checkpoint as well |
|
Claude finished @julienrbrt's task in 2m 22s —— View job Fixed SetDAHeight to Update Checkpoint ✅I've successfully implemented the fix for the critical issue where Summary of ChangesFixed Files:
What the fix does:
Why this matters: Commit CreatedThe changes have been committed locally:
To push the changes: git push origin julien/genesis-fi--- | |
Co-authored-by: Marko <[email protected]>
|
@claude the current code fixes the issue. The checkpoint is overridden if daheight is 0. Nothing is fetched when da height is 0 |
|
Claude finished @julienrbrt's task in 1m 20s —— View job Analysis: Current Implementation Actually Works Correctly ✅After carefully tracing through the code, I can confirm that @julienrbrt is correct - the current implementation does solve the checkpoint issue through a different mechanism than what the previous reviews expected. The Key Mechanism: Checkpoint Override on First FetchThe critical code is in // checkpoint init path, only hit when sequencer is bootstrapping
if c.checkpoint.DAHeight == 0 {
c.checkpoint = &seqcommon.Checkpoint{
DAHeight: daHeight,
TxIndex: 0,
}
}How It Works (Genesis Flow)For Single Sequencer:
For Based Sequencer: The same mechanism exists at Why Previous Reviews Were ConcernedThe previous Claude and gemini-code-assist reviews expected
This approach:
Verification: Nothing Is Fetched When DA Height Is 0@julienrbrt's statement "Nothing is fetched when da height is 0" is also correct: Line 162 has the guard: if daHeight > 0 && (len(c.cachedForcedInclusionTxs) == 0 || c.checkpoint.TxIndex >= uint64(len(c.cachedForcedInclusionTxs))) {If
This is exactly the desired behavior at genesis before DA height is discovered. ConclusionThe current implementation is correct and complete. The mechanism works as follows:
The previous review's concern about checkpoint not being updated in Status: No issues found. The PR correctly implements genesis DA height discovery and initialization. ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2926 +/- ##
==========================================
- Coverage 59.31% 59.14% -0.17%
==========================================
Files 90 90
Lines 8575 8596 +21
==========================================
- Hits 5086 5084 -2
- Misses 2913 2930 +17
- Partials 576 582 +6
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:
|
tac0turtle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, nice cleanup
Overview
The sequencer doesn't know at genesis what will be the first da block it publishes to.
So it is a manual operation to edit the genesis and share it with other nodes afterwards.
We need the sequencer to start with a pre-init da height, as we use the daHeight from the genesis as well.
Basically it should be the current da height at the time the first block of the chain was created.
For epochs to work, this would need to be the same across all nodes (so you CANNOT change it).
This PR let the first da height be set based on the first submitted height for a single sequencer
For a based sequencer, the chain developers basically need to have a da height set in the genesis.
Added a check to avoid issues.
ref: #2906