Skip to content

chore: hotfix to unstuck da inclusion on old betas#3197

Draft
julienrbrt wants to merge 3 commits intomainfrom
julien/hotfix-da-inclusion
Draft

chore: hotfix to unstuck da inclusion on old betas#3197
julienrbrt wants to merge 3 commits intomainfrom
julien/hotfix-da-inclusion

Conversation

@julienrbrt
Copy link
Copy Markdown
Member

Overview

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 24, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedMar 31, 2026, 2:22 PM

@julienrbrt julienrbrt changed the title Julien/hotfix da inclusion chore: hotfix to unstuck da inclusion on old betas Mar 24, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 24, 2026

Claude finished @julienrbrt's task in 2m 8s —— View job


Code Review: block/internal/submitting/submitter.go

  • Gather context and read repo instructions
  • Read changed file and diff
  • Review code quality, error handling, resilience, and correctness
  • Post review feedback

This hotfix adds logic to "unstick" DA inclusion processing on old betas by skipping ahead to near the current chain height. The intent is sound, but the implementation has two critical bugs and a few other issues.


🔴 Critical: Logic condition is always false — flag never flips

File: submitter.go:332–335

currentDAIncluded = currentHeight - 10000
if currentDAIncluded > currentHeight-10000 {   // ← always false
    unblockDaInclusion = true
}

currentDAIncluded was just assigned currentHeight - 10000, so the comparison currentDAIncluded > currentHeight - 10000 is literally X > X, which is always false. unblockDaInclusion will never be set to true.

Consequence: The entire if !unblockDaInclusion { ... } block runs on every iteration of the inner for loop. This means:

  1. s.store.Height() is called on every single iteration (performance regression)
  2. currentDAIncluded is overridden to currentHeight - 10000 on every iteration, which may mask actual progress

The condition should compare against the original stored currentDAIncluded, not the freshly overwritten local variable. Likely intended logic:

if !unblockDaInclusion {
    unblockDaInclusion = true  // set unconditionally — only run this block once
    currentHeight, err := s.store.Height(s.ctx)
    if err != nil {
        s.logger.Error().Err(err).Msg("...")
    } else if currentHeight > 10000 && currentDAIncluded < currentHeight-10000 {
        currentDAIncluded = currentHeight - 10000
    }
}

Fix this →


🔴 Critical: uint64 underflow on store error

File: submitter.go:327–331

currentHeight, err := s.store.Height(context.Background())
if err != nil {
    s.logger.Error().Err(err).Msg("failed to get current height for da inclusion unblocking")
}
currentDAIncluded = currentHeight - 10000  // ← runs even if err != nil

When s.store.Height() returns an error, currentHeight is 0 (zero value for uint64). The subtraction 0 - 10000 underflows to 18446744073709541616. On the next line, nextHeight := currentDAIncluded + 1 becomes 18446744073709541617. The subsequent s.store.GetBlockData(s.ctx, nextHeight) call will always fail (no such block), but we've now broken the inner loop's progress tracking for this tick.

The fix is to continue or break on error, or skip the assignment:

if err != nil {
    s.logger.Error().Err(err).Msg("...")
    unblockDaInclusion = true  // don't retry on next iteration
} else if currentHeight > 10000 && currentDAIncluded < currentHeight-10000 {
    currentDAIncluded = currentHeight - 10000
    // also persist this? See note below
}

🟡 Warning: Package-level variable is not thread-safe and affects all instances

File: submitter.go:307

var unblockDaInclusion bool

This is a package-level (global) variable. If multiple Submitter instances exist in a process (e.g., in tests), they share this flag, and the first one to run will prevent others from applying the hotfix. Additionally, it is accessed from a goroutine without synchronization (sync/atomic or a mutex). In practice, processDAInclusionLoop is a single goroutine, so there's no data race in production — but this is fragile and will break test isolation (the test file uses unblockDaInclusion state implicitly).

Consider making it an instance field on Submitter and initializing it in the constructor.


🟡 Warning: context.Background() instead of s.ctx

File: submitter.go:327

currentHeight, err := s.store.Height(context.Background())

This should be s.ctx so that node shutdown cancels the call. Using context.Background() means the store operation may block even after the submitter has been stopped.


🟡 Note: Skip-ahead height is not persisted

When currentDAIncluded is overwritten to currentHeight - 10000, the new value is held only in the local variable. It is not persisted to the store via putUint64Metadata(...) and not written to the atomic via s.SetDAIncludedHeight(...). On the next ticker tick, currentDAIncluded is re-fetched from s.GetDAIncludedHeight() at line 322, which still returns the old (stuck) value — starting the cycle over. The hotfix only has effect within a single ticker period.

If the intent is a one-time skip, the new currentDAIncluded should be persisted (at least via s.SetDAIncludedHeight) so it survives to the next tick. However, given the flag is never set to true anyway (bug #1), this is moot until that is fixed.


Summary

Severity Issue
🔴 Critical unblockDaInclusion is never set to true — hotfix has no lasting effect
🔴 Critical uint64 underflow if store.Height() fails
🟡 Warning Package-level global variable — not thread-safe, breaks test isolation
🟡 Warning context.Background() instead of s.ctx
🟡 Note Skip-ahead height not persisted — effect limited to one tick

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 24, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 21174540-9c33-4bc1-9143-9d48e2292d44

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch julien/hotfix-da-inclusion

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

1 participant