Skip to content

Conversation

@julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Dec 12, 2025

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 12, 2025

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedDec 19, 2025, 5:19 PM

@claude
Copy link
Contributor

claude bot commented Dec 12, 2025

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 Summary

Verdict: 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 SetDAHeight() not updating checkpoints, but those concerns were based on misunderstanding the implementation strategy. The actual design is more elegant and functionally correct.


1. Code Quality & Architecture ⭐⭐⭐⭐

Strengths:

  • Deferred Initialization Pattern: The design elegantly defers checkpoint synchronization until first use, avoiding unnecessary I/O during startup
  • Clean Separation: The submitter discovers genesis DA height and notifies the sequencer, which handles it at the appropriate time
  • Idiomatic Go: Code follows Go conventions with proper error handling and structured logging
  • Zero-dependency Core: Maintains the project's architecture principle of keeping core packages dependency-free

Implementation Details (Single Sequencer):

Genesis Flow (sequencers/single/sequencer.go):

  1. At Initialization (line 72): SetDAHeight(0) - atomic value set to 0
  2. Checkpoint Load (lines 83-95): Checkpoint initialized with DAHeight: 0
  3. First DA Inclusion (block/internal/submitting/submitter.go:374): Submitter calls SetDAHeight(actualGenesisHeight) after discovering actual DA height
  4. First GetNextBatch (lines 163-169): Detects daHeight > 0 && checkpoint.DAHeight == 0, overrides checkpoint, and reinitializes ForcedInclusionRetriever with correct daStartHeight
  5. Subsequent Calls: Normal operation with correct checkpoint

This pattern ensures:

  • ✅ No premature checkpoint saves during initialization
  • ✅ Automatic sync on first actual use
  • ✅ Works even if SetDAHeight() is called multiple times before first batch
  • ✅ The daHeight > 0 guard prevents fetching when DA height is unknown

Based Sequencer Approach:

Based sequencers require genesis.DAStartHeight > 0 (validated in pkg/cmd/run_node.go:139-141), so they don't rely on the deferred initialization pattern. Their checkpoint is initialized correctly from genesis immediately.


2. Error Handling & Resilience ⭐⭐⭐⭐⭐

Excellent Patterns:

Genesis DA Height Persistence (submitter.go:364-377):

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)
    }
}
  • ✅ Persists before notifying sequencer
  • ✅ Uses min(headerDAHeight, dataDAHeight) to handle out-of-order DA inclusions (line 358)
  • ✅ Safe nil check for sequencer (sync nodes don't have sequencers)

Forced Inclusion Error Handling (sequencer.go:333-344):

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)
}
  • ✅ Distinguishes between transient errors (ErrHeightFromFuture) and configuration issues
  • ✅ Proper error wrapping with context
  • ✅ Graceful degradation when forced inclusion not configured

Based Sequencer Validation (pkg/cmd/run_node.go:139-141):

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")
}
  • ✅ Prevents misconfiguration at startup
  • ✅ Clear error message explaining the requirement

Minor Suggestion:

Consider adding a debug log when the deferred initialization is triggered (sequencer.go:163-169) to aid debugging:

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 (sequencer.go:350-361):

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)
}
  • ✅ Validates transaction size limits for DA-sourced (untrusted) transactions
  • ✅ Logs violations without halting
  • ✅ Prevents DoS via oversized blobs

Genesis Configuration Security:

  • ✅ Based sequencer validation prevents nodes from starting with incompatible genesis configurations
  • ✅ Immutable genesis DA height after first inclusion (via genesisInclusion flag)
  • ✅ No secrets in logs or error messages

Input Validation:

  • ✅ Epoch boundary calculations use well-tested types.CalculateEpochBoundaries() function
  • ✅ Height comparisons check for underflow (daHeight < r.daStartHeight)
  • ✅ Nil checks for optional components (signer, sequencer)

4. Performance & Resource Efficiency ⭐⭐⭐⭐

Efficient Design:

Atomic Operations (sequencer.go:313-320):

func (c *Sequencer) SetDAHeight(height uint64) {
    c.daHeight.Store(height)
}

func (c *Sequencer) GetDAHeight() uint64 {
    return c.daHeight.Load()
}
  • ✅ Lock-free atomic reads/writes for DA height
  • ✅ Minimal contention for high-frequency reads

Checkpoint Caching (sequencer.go:174, 197):

if daHeight > 0 && (len(c.cachedForcedInclusionTxs) == 0 || c.checkpoint.TxIndex >= ...) {
    // Fetch next epoch
}
  • ✅ Caches forced inclusion transactions per epoch, avoiding redundant DA fetches
  • ✅ Checkpoint tracks position within cached transactions for crash recovery

Lazy Retriever Initialization (sequencer.go:169):

c.fiRetriever = block.NewForcedInclusionRetriever(c.daClient, c.logger, getInitialDAStartHeight(ctx, c.db), c.genesis.DAEpochForcedInclusion)
  • ✅ Reinitializes retriever only when genesis DA height is discovered
  • ✅ Reuses retriever instance across calls

Minor Observation:

The getInitialDAStartHeight() function is called twice in the same code path (lines 107, 169). While this has minimal performance impact due to caching at the datastore level, it could be optimized to call once and store the result.


5. Testing & Reliability ⭐⭐⭐⭐

Test Coverage:

Submitter Tests (block/internal/submitting/submitter_test.go):

  • TestSubmitter_setSequencerHeightToDAHeight: Tests genesis DA height calculation with min(header, data)
  • TestSubmitter_processDAInclusionLoop_advances: Integration test verifying DA inclusion processing and metadata persistence
  • TestSubmitter_CacheClearedOnHeightInclusion: Verifies cache cleanup after DA inclusion

Forced Inclusion Retriever Tests (block/internal/da/forced_inclusion_retriever_test.go):

  • TestForcedInclusionRetriever_RetrieveForcedIncludedTxs_NotAtEpochStart: Validates empty response for non-epoch heights
  • TestForcedInclusionRetriever_RetrieveForcedIncludedTxs_EpochStartSuccess: Tests successful epoch retrieval
  • TestForcedInclusionRetriever_RetrieveForcedIncludedTxs_MultiHeightEpoch: Tests multi-height epoch transaction aggregation

Sequencer Tests (sequencers/single/sequencer_test.go):

  • ✅ Tests cover basic sequencer operations
  • ⚠️ Gap: No specific test for the deferred checkpoint initialization pattern (lines 163-169)

Recommendations:

  1. Add Integration Test for genesis DA height discovery flow:

    func TestSequencer_GenesisDAHeightDiscovery(t *testing.T) {
        // 1. Create sequencer with genesis.DAStartHeight = 0
        // 2. Verify checkpoint.DAHeight = 0
        // 3. Call SetDAHeight(150) to simulate submitter discovery
        // 4. Call GetNextBatch() and verify checkpoint is overridden to 150
        // 5. Verify subsequent GetNextBatch() calls use checkpoint correctly
    }
  2. Add Test for the guard condition preventing fetching when daHeight == 0:

    func TestSequencer_NoFetchWhenDAHeightZero(t *testing.T) {
        // Verify that GetNextBatch with daHeight=0 returns mempool txs only
    }

6. Concurrency Safety ⭐⭐⭐⭐

Thread-Safe Patterns:

Atomic DA Height (sequencer.go:44, 313-320):

daHeight atomic.Uint64

func (c *Sequencer) SetDAHeight(height uint64) {
    c.daHeight.Store(height)
}
  • ✅ Atomic operations prevent data races on daHeight
  • ✅ No mutex needed for simple reads/writes

Checkpoint Access:

  • ✅ Checkpoint is only accessed within GetNextBatch(), which is called sequentially by the executor
  • ✅ No concurrent modifications to checkpoint structure
  • ⚠️ Potential Race: If GetNextBatch() is called concurrently (not intended by design), the checkpoint initialization (lines 163-169) could race

Recommendation:

Add a comment documenting that GetNextBatch() must not be called concurrently:

// 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:

  • ✅ PR description clearly explains the problem and solution
  • ✅ Code comments explain non-obvious logic (e.g., "checkpoint init path, only hit when sequencer is bootstrapping")
  • ✅ Error messages are descriptive and actionable
  • ✅ Function names are self-documenting

Improvements:

  1. Document the Genesis Flow in sequencers/single/sequencer.go:

    // Genesis DA Height Discovery Flow:
    // 1. Sequencer starts with DAStartHeight from genesis (may be 0 for single sequencers)
    // 2. First block DA inclusion is discovered by submitter component
    // 3. Submitter persists actual genesis DA height and calls SetDAHeight()
    // 4. First GetNextBatch() call detects checkpoint.DAHeight == 0 and synchronizes
    // 5. Subsequent calls operate normally with correct checkpoint
    //
    // For based sequencers, genesis.DAStartHeight MUST be > 0 and agreed upon by all nodes.
  2. Add godoc to getInitialDAStartHeight():

    // getInitialDAStartHeight retrieves the persisted genesis DA height from the store.
    // This is the DA height at which the first chain block was included, discovered
    // by the submitter after genesis. Returns 0 if not yet persisted.

8. Specific Code Review Comments

sequencers/single/sequencer.go:163-169 - Checkpoint Initialization ✅

if daHeight > 0 && c.checkpoint.DAHeight == 0 {
    c.checkpoint = &seqcommon.Checkpoint{
        DAHeight: daHeight,
        TxIndex:  0,
    }
    c.fiRetriever = block.NewForcedInclusionRetriever(c.daClient, c.logger, getInitialDAStartHeight(ctx, c.db), c.genesis.DAEpochForcedInclusion)
}

Analysis: This is the core of the deferred initialization pattern. Previous reviews incorrectly flagged this as a bug, but it's intentionally designed this way.

Why it works:

  • The condition daHeight > 0 && c.checkpoint.DAHeight == 0 ensures this block runs exactly once, when:
    • The submitter has discovered the actual genesis DA height (daHeight > 0)
    • The checkpoint hasn't been initialized yet (c.checkpoint.DAHeight == 0)
  • This avoids premature checkpoint creation with the wrong DA height
  • The retriever is recreated with the correct daStartHeight from persistent storage

Recommendation: Add logging as suggested earlier for debugging visibility.


block/internal/submitting/submitter.go:364-376 - Genesis DA Height Persistence ✅

if genesisInclusion {
    genesisDAIncludedHeightBytes := make([]byte, 8)
    binary.LittleEndian.PutUint64(genesisDAIncludedHeightBytes, genesisDAIncludedHeight)

    if err := s.store.SetMetadata(ctx, store.GenesisDAHeightKey, genesisDAIncludedHeightBytes); err != nil {
        return err
    }

    // the sequencer will process DA epochs from this height.
    if s.sequencer != nil {
        s.sequencer.SetDAHeight(genesisDAIncludedHeight)
        s.logger.Debug().Uint64("genesis_da_height", genesisDAIncludedHeight).Msg("initialized sequencer DA height from persisted genesis DA height")
    }
}

Analysis: Excellent implementation with proper ordering.

Strengths:

  • ✅ Persists before notifying sequencer (maintains consistency on crash)
  • ✅ Uses min(headerDAHeight, dataDAHeight) to handle out-of-order submissions
  • ✅ Only runs once (genesisInclusion flag ensures this is height 1 only)
  • ✅ Nil-safe for sync nodes without sequencer

block/internal/da/forced_inclusion_retriever.go:35-46 - Retriever Constructor ✅

func NewForcedInclusionRetriever(
    client Client,
    logger zerolog.Logger,
    daStartHeight, daEpochSize uint64,
) *ForcedInclusionRetriever {
    return &ForcedInclusionRetriever{
        client:        client,
        logger:        logger.With().Str("component", "forced_inclusion_retriever").Logger(),
        daStartHeight: daStartHeight,
        daEpochSize:   daEpochSize,
    }
}

Analysis: Clean refactoring that decouples from genesis.

Why this is better:

  • ✅ Retriever no longer depends on genesis.Genesis type
  • ✅ Allows passing the actual discovered DA start height instead of genesis value
  • ✅ More testable (can create retriever with arbitrary parameters)

Note: The gemini-code-assist review raised concerns about daStartHeight never being updated, but this is by design - the retriever is recreated when the DA start height changes (see sequencer.go:169).


pkg/cmd/run_node.go:139-141 - 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")
}

Analysis: Critical validation preventing misconfiguration.

Strengths:

  • ✅ Fails fast at startup rather than during operation
  • ✅ Clear error message explaining the requirement and implications
  • ✅ Prevents based sequencer nodes from diverging due to different DA start heights

9. Architecture Review ⭐⭐⭐⭐⭐

Design Patterns:

Separation of Concerns:

  • Submitter: Discovers genesis DA height via DA inclusion monitoring
  • Sequencer: Manages checkpoint and forced inclusion transaction fetching
  • Executor: Coordinates block production using sequencer batches

Data Flow:

1. Submitter monitors DA inclusions
2. Submitter detects first block inclusion (height 1)
3. Submitter calculates genesis DA height = min(header_da_height, data_da_height)
4. Submitter persists to store.GenesisDAHeightKey
5. Submitter calls sequencer.SetDAHeight(genesisDAHeight)
6. Sequencer's atomic daHeight is updated
7. On next GetNextBatch(), sequencer detects checkpoint.DAHeight == 0
8. Sequencer overrides checkpoint with correct DA height
9. Sequencer recreates ForcedInclusionRetriever with correct daStartHeight
10. Normal operation continues

Why This Design is Superior:

  • ✅ No manual genesis editing required
  • ✅ Works automatically for single sequencers
  • ✅ Explicit validation for based sequencers (where community consensus is required)
  • ✅ Crash-safe through persistent storage before notification
  • ✅ Minimal state synchronization (only atomic daHeight updated cross-component)

10. Final Recommendations

Critical (Must Fix):

None - the implementation is functionally correct.

High Priority (Should Fix):

  1. Add integration test for genesis DA height discovery flow (as outlined in section 5)
  2. Add concurrency safety documentation to GetNextBatch() method signature

Medium Priority (Nice to Have):

  1. Add debug logging when deferred initialization triggers (line 163)
  2. Optimize duplicate getInitialDAStartHeight() calls
  3. Enhance godoc for the genesis flow in sequencer package

Low Priority (Consider):

  1. Add metrics for genesis DA height discovery timing
  2. Consider adding a CLI command to view the persisted genesis DA height

Summary

This PR successfully implements a critical feature for proper DA epoch handling at genesis. The deferred initialization pattern is well-designed and addresses the core problem elegantly. Previous bot reviews raised concerns that were based on misunderstanding the implementation strategy - the actual code is correct.

Key Achievements:

  • ✅ Eliminates manual genesis editing for single sequencers
  • ✅ Maintains safety through persistent storage and validation
  • ✅ Clean architectural separation between components
  • ✅ Comprehensive error handling and graceful degradation
  • ✅ Strong security posture with input validation
  • ✅ Good test coverage with minor gaps

Recommendation: APPROVE with the suggestions above addressed in follow-up PRs if needed.


@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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 Submitter to inform the Sequencer about the actual DA inclusion height at genesis and deferring DA height updates in the Executor until the chain has progressed, the system ensures accurate DA epoch fetching from the start, eliminating the need for manual genesis configuration.

Highlights

  • Sequencer DA Height Initialization: The logic for initializing the sequencer's Data Availability (DA) height has been refined to ensure it's set correctly based on actual DA inclusion events, rather than a potentially premature genesis value.
  • Submitter-Sequencer Communication: The Submitter component can now directly update the Sequencer's DA height, specifically when the initial DA inclusion at genesis is observed and persisted.
  • Deferred DA Height Setting: The Executor now defers setting the Sequencer's DA height until the chain has processed at least one block beyond the initial genesis height, preventing incorrect DA height updates during chain initialization.
  • Forced Inclusion Retriever Refactor: The ForcedInclusionRetriever has been updated to manage its own daStartHeight, decoupling it from direct genesis object access and preparing for more flexible DA height sourcing.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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:

  1. The SetDAHeight method on both single and based sequencers does not update the internal checkpoint that drives the fetching logic. As a result, the correct DA height provided by the submitter is ignored.
  2. The ForcedInclusionRetriever continues to use a daStartHeight from 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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")
		}
	}
}

@julienrbrt julienrbrt mentioned this pull request Dec 17, 2025
13 tasks
@julienrbrt julienrbrt marked this pull request as ready for review December 17, 2025 16:10
daEpochSize: genesis.DAEpochForcedInclusion,
}

// check for inclusion da height on store (sequencer) or genesis da height (full nodes)
Copy link
Member Author

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.

@julienrbrt
Copy link
Member Author

@claude comment is true. Need to update checkpoint as well

@claude
Copy link
Contributor

claude bot commented Dec 17, 2025

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 SetDAHeight() wasn't updating the checkpoint structure.

Summary of Changes

Fixed Files:

  1. sequencers/single/sequencer.go:296-320 - Updated SetDAHeight() to update and persist checkpoint
  2. sequencers/based/sequencer.go:248-272 - Updated SetDAHeight() to update and persist checkpoint
  3. sequencers/based/sequencer_test.go:366-385 - Enhanced test to verify checkpoint update

What the fix does:

  • ✅ Updates the checkpoint's DAHeight when a higher value is set via SetDAHeight()
  • ✅ Resets TxIndex to 0 when moving to a new DA height (since we're starting from a new epoch)
  • ✅ Persists the checkpoint to disk using checkpointStore.Save() to ensure the change survives restarts
  • ✅ Adds structured logging to track DA height updates for operational visibility
  • ✅ Handles the case where checkpoint might not be initialized yet (early in startup)

Why this matters:
This was a critical bug preventing the PR's goal from being achieved. When the submitter discovers the actual genesis DA height and calls SetDAHeight(), the sequencer now properly updates its checkpoint (which drives the fetchNextDAEpoch logic in GetNextBatch). Previously, only the atomic value was updated, so the sequencer ignored the correct genesis DA height.

Commit Created

The changes have been committed locally:

  • Commit: 0689070 - "fix(sequencers): update checkpoint in SetDAHeight"
  • Files changed: 3 files, 47 insertions(+)

⚠️ Note: I encountered a permission issue when trying to push to the remote repository (GitHub Actions bot doesn't have write permissions). You'll need to push the commit manually or configure the appropriate permissions for the GitHub Actions workflow.

To push the changes:

git push origin julien/genesis-fi

--- |

tac0turtle
tac0turtle previously approved these changes Dec 19, 2025
Co-authored-by: Marko <marko@baricevic.me>
@julienrbrt
Copy link
Member Author

@claude the current code fixes the issue. The checkpoint is overridden if daheight is 0. Nothing is fetched when da height is 0

@claude
Copy link
Contributor

claude bot commented Dec 19, 2025

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 Fetch

The critical code is in sequencers/single/sequencer.go:153-158:

// 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:

  1. At genesis (NewSequencer):

    • SetDAHeight(0) sets atomic daHeight to 0 (line 77)
    • Checkpoint initialized from DB or created with DAHeight: 0 (lines 92-95)
  2. First DA inclusion (submitter.go:364-376):

    • When first block is DA-included, submitter discovers actual genesis DA height (e.g., 150)
    • Calls sequencer.SetDAHeight(150) - updates atomic value only (line 374)
    • Checkpoint still has DAHeight: 0
  3. First GetNextBatch call (sequencer.go:153-158):

    • Gets daHeight = 150 from atomic value (line 150)
    • Detects checkpoint.DAHeight == 0 (line 153)
    • Overrides entire checkpoint with DAHeight: 150 (lines 154-157)
    • ✅ Now checkpoint is synchronized with the correct genesis DA height
  4. Subsequent calls (sequencer.go:162-169):

    • Checkpoint condition daHeight > 0 is true (line 162)
    • fetchNextDAEpoch uses c.checkpoint.DAHeight (line 316)
    • ✅ Fetches from correct DA height

For Based Sequencer:

The same mechanism exists at sequencers/based/sequencer.go:103 with identical logic. However, based sequencers require genesis.DAStartHeight > 0 (validated in pkg/cmd/run_node.go:139-141), so they don't rely on this mechanism - their checkpoint is initialized correctly from genesis.


Why Previous Reviews Were Concerned

The previous Claude and gemini-code-assist reviews expected SetDAHeight() to directly update the checkpoint. However, the actual design is more elegant:

  • SetDAHeight() updates only the atomic value (lightweight, no I/O)
  • GetNextBatch() detects the daHeight == 0 condition and performs the one-time checkpoint sync

This approach:

  • ✅ Avoids unnecessary checkpoint saves during initialization
  • ✅ Defers checkpoint update until first actual use
  • ✅ Works correctly even if SetDAHeight is called multiple times before first batch
  • ✅ The condition if c.checkpoint.DAHeight == 0 ensures this override only happens once

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 daHeight == 0, the entire fetch block is skipped, so:

  • No DA epoch fetching happens
  • No forced inclusion transactions are processed
  • Sequencer continues with normal mempool transactions

This is exactly the desired behavior at genesis before DA height is discovered.


Conclusion

The current implementation is correct and complete. The mechanism works as follows:

  1. Single sequencers start with daHeight = 0
  2. Submitter discovers actual genesis DA height on first inclusion
  3. Submitter calls SetDAHeight() with actual height
  4. First GetNextBatch() call detects checkpoint.DAHeight == 0 and overrides it
  5. All subsequent operations use the correct checkpoint

The previous review's concern about checkpoint not being updated in SetDAHeight() was based on not understanding this deferred initialization pattern. No changes are needed.


Status: No issues found. The PR correctly implements genesis DA height discovery and initialization. ✅

@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 56.66667% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.14%. Comparing base (a406515) to head (c2e2cb8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
sequencers/single/sequencer.go 57.14% 8 Missing and 4 partials ⚠️
pkg/store/kv.go 0.00% 5 Missing ⚠️
block/internal/submitting/submitter.go 25.00% 2 Missing and 1 partial ⚠️
block/internal/da/forced_inclusion_retriever.go 71.42% 1 Missing and 1 partial ⚠️
pkg/cmd/run_node.go 0.00% 2 Missing ⚠️
block/internal/syncing/syncer.go 0.00% 1 Missing ⚠️
block/public.go 0.00% 1 Missing ⚠️
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     
Flag Coverage Δ
combined 59.14% <56.66%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, nice cleanup

@julienrbrt julienrbrt added this pull request to the merge queue Dec 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 19, 2025
@julienrbrt julienrbrt added this pull request to the merge queue Dec 19, 2025
Merged via the queue into main with commit 742b8bb Dec 19, 2025
43 of 46 checks passed
@julienrbrt julienrbrt deleted the julien/genesis-fi branch December 19, 2025 20:08
alpe added a commit that referenced this pull request Dec 22, 2025
* main:
  fix: make evm_execution more robust (#2942)
  fix(sequencers/single): deterministic queue (#2938)
  fix(block): fix init logic sequencer for da epoch fetching (#2926)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants