Skip to content

Conversation

@tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Dec 12, 2025

Overview

upstreams #2925 to main

@tac0turtle tac0turtle marked this pull request as ready for review December 12, 2025 21:02
@github-actions
Copy link
Contributor

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedDec 12, 2025, 9:02 PM

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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 adjusts the initial state validation within the syncer component, specifically modifying how the Data Availability (DA) height is checked during initialization. The change ensures that a zero DA height does not trigger an error, which is essential for proper handling of genesis states or initial synchronization without existing DA height information.

Highlights

  • Syncer State Initialization: Modified the initializeState function in syncer.go to relax the Data Availability (DA) height validation. The check state.DAHeight < s.genesis.DAStartHeight now includes an additional condition state.DAHeight != 0, preventing an error when DAHeight is zero.
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.

@claude
Copy link
Contributor

claude bot commented Dec 12, 2025

Claude finished @tac0turtle's task in 1m 39s —— View job


Code Review Complete

Summary

This PR loosens the DA height validation check in the syncer's initializeState function to allow a zero DA height, which is essential for properly handling genesis states where DA height has not yet been set.

Change Analysis

Modified: block/internal/syncing/syncer.go:270

- if state.DAHeight < s.genesis.DAStartHeight {
+ if state.DAHeight != 0 && state.DAHeight < s.genesis.DAStartHeight {

The change adds a condition state.DAHeight != 0 before checking if state.DAHeight < s.genesis.DAStartHeight, preventing an error when DAHeight is zero.

Review Findings

✅ Correctness & Logic

Approved - The change is logically sound and addresses a valid edge case:

  1. Genesis State Handling: When a fresh node initializes (lines 247-268), the state is created with DAHeight: s.genesis.DAStartHeight. However, if DAStartHeight is 0 (as seen in multiple test cases), the validation would previously fail with "DA height (0) is lower than DA start height (0)".

  2. Fallback Recovery: Line 277 shows that after validation, the DA retriever height is set to max(s.genesis.DAStartHeight, s.cache.DaHeight(), state.DAHeight, s.getHighestStoredDAHeight()), which properly handles the zero case.

  3. Test Coverage: Multiple test files use DAStartHeight: 0, confirming this is a valid configuration:

    • syncer_test.go:592
    • syncer_forced_inclusion_test.go:357, 442, 556, 675

⚠️ Potential Issues

1. Edge Case: state.DAHeight = 0 with genesis.DAStartHeight > 0

The new condition allows state.DAHeight = 0 to pass validation even when s.genesis.DAStartHeight > 0. Consider this scenario:

  • Genesis has DAStartHeight = 100 (indicating DA started at height 100)
  • Node state has DAHeight = 0 (could happen if state was corrupted or manually modified)
  • The validation would now pass but this represents an invalid state

Recommendation: Consider making the check more explicit:

// Allow zero DAHeight (uninitialized state) or valid DA height >= DAStartHeight
if state.DAHeight != 0 && state.DAHeight < s.genesis.DAStartHeight {
    return fmt.Errorf("DA height (%d) is lower than DA start height (%d)", state.DAHeight, s.genesis.DAStartHeight)
}

Or add a comment explaining why zero is allowed:

// Allow DAHeight = 0 for fresh nodes or when DA syncing hasn't started yet
if state.DAHeight != 0 && state.DAHeight < s.genesis.DAStartHeight {
    return fmt.Errorf("DA height (%d) is lower than DA start height (%d)", state.DAHeight, s.genesis.DAStartHeight)
}

2. Missing Test Coverage for This Specific Fix

The change addresses a bug where state.DAHeight = 0 and genesis.DAStartHeight = 0 would cause a false validation error. However, I don't see a specific test that:

  • Loads existing state with DAHeight = 0
  • Has genesis.DAStartHeight = 0
  • Verifies that initialization succeeds

Recommendation: Add a regression test to syncer_test.go:

func TestInitializeState_ZeroDAHeight(t *testing.T) {
    // Test that state with DAHeight=0 and genesis with DAStartHeight=0 
    // doesn't trigger validation error
    ctx := context.Background()
    
    gen := genesis.Genesis{
        ChainID:       "test-chain",
        InitialHeight: 1,
        StartTime:     time.Now().UTC(),
        DAStartHeight: 0, // Zero DA start height
    }
    
    store := store.NewInMemoryStore()
    // Save state with DAHeight = 0
    state := types.State{
        ChainID:         gen.ChainID,
        InitialHeight:   gen.InitialHeight,
        LastBlockHeight: 0,
        DAHeight:        0, // Zero DA height
    }
    require.NoError(t, store.UpdateState(ctx, state))
    
    syncer := NewSyncer(store, mockExec, mockDA, cache, metrics, config, gen, ...)
    
    // This should not fail with "DA height (0) is lower than DA start height (0)"
    err := syncer.initializeState()
    require.NoError(t, err)
}

✅ Code Quality & Style

  • Follows existing code patterns and Go idioms
  • No additional complexity introduced
  • Minimal change with focused scope

✅ Security & Error Handling

  • No security vulnerabilities introduced
  • Error message remains clear and helpful
  • No sensitive data exposure concerns

✅ Performance

  • No performance impact - single conditional check

📝 Documentation

Minor Improvement Suggested: Add a clarifying comment explaining the zero-height case:

// Validate DA height consistency
// Allow DAHeight = 0 for uninitialized state (fresh nodes or genesis)
if state.DAHeight != 0 && state.DAHeight < s.genesis.DAStartHeight {
    return fmt.Errorf("DA height (%d) is lower than DA start height (%d)", state.DAHeight, s.genesis.DAStartHeight)
}

Conclusion

Status: ✅ Approved with suggestions

The change correctly addresses a validation bug for genesis states. The fix is minimal, focused, and aligned with the codebase's existing patterns.

Recommendations (non-blocking):

  1. Add a comment explaining why zero DA height is allowed
  2. Consider adding a specific regression test for this scenario
  3. Verify that allowing state.DAHeight = 0 when genesis.DAStartHeight > 0 is intentional behavior

The PR is safe to merge as-is, but the suggestions above would improve clarity and prevent potential future issues.


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 modifies the syncer state initialization logic to be more lenient with the DAHeight check. By adding a state.DAHeight != 0 condition, it now allows the syncer to start with a state where DAHeight is zero. This is a sensible change for backward compatibility, as it accommodates states from older versions that might not have this field populated. The change is correct and well-contained.

@codecov
Copy link

codecov bot commented Dec 12, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.65%. Comparing base (b3f2dfb) to head (bb52cfa).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
block/internal/syncing/syncer.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2927      +/-   ##
==========================================
+ Coverage   63.57%   63.65%   +0.08%     
==========================================
  Files          90       90              
  Lines        8571     8571              
==========================================
+ Hits         5449     5456       +7     
+ Misses       2541     2534       -7     
  Partials      581      581              
Flag Coverage Δ
combined 63.65% <0.00%> (+0.08%) ⬆️

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.

@tac0turtle tac0turtle merged commit 578b0f9 into main Dec 15, 2025
33 of 34 checks passed
@tac0turtle tac0turtle deleted the marko/upstream_fix branch December 15, 2025 10:16
alpe added a commit that referenced this pull request Dec 15, 2025
* main:
  fix(syncing): skip forced txs checks for p2p blocks (#2922)
  build(deps): Bump the all-go group across 5 directories with 5 updates (#2919)
  chore: loosen syncer state check (#2927)
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