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

Claude finished @julienrbrt's task in 1m 52s —— View job

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

• Gather context and read changed files
• Analyze the changes
• Post review feedback

The PR adds 13 lines to processDAInclusionLoop to "unblock" DA inclusion when a node is far behind. There are several significant issues.

🔴 Critical: Logic Bug — unblockDaInclusion is Never Set to true

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

currentDAIncluded is assigned currentHeight - 10000 on the preceding line, so the condition currentDAIncluded > currentHeight-10000 is equivalent to X > X, which is always false. unblockDaInclusion can never be set to true.

As a result, the block runs on every iteration of the inner for loop, unconditionally overriding the persisted currentDAIncluded with currentHeight - 10000. This defeats the purpose of both the unblockDaInclusion flag and the DA included height tracking.

Fix this →

🔴 Critical: uint64 Underflow

Both occurrences of currentHeight - 10000 will wrap around to a very large number if currentHeight < 10000 (e.g. on a fresh node or in early blocks), since currentHeight is uint64. This could cause the DA inclusion loop to attempt to process heights far into the future.

Fix this →

🔴 Critical: Missing Error Guard Causes uint64 Underflow

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")
}
// execution continues with currentHeight == 0
currentDAIncluded = currentHeight - 10000  // ← underflow when currentHeight == 0

When s.store.Height() fails, currentHeight is 0 (zero value), and the code continues without returning or skipping, causing an immediate uint64 underflow. A continue or break is needed after the error log.

Fix this →

🟡 Concurrency: Package-Level Mutable State Without Synchronization

var unblockDaInclusion bool

This is a package-level global with no mutex or atomic protection. If multiple Submitter instances exist (e.g. in tests), they share the same flag. Given that Submitter already uses atomic.Uint64 for daIncludedHeight, this flag should be an atomic.Bool field on the Submitter struct instead.

Fix this →

🟡 Magic Number

10000 appears twice with no explanation. It should be a named constant with a comment explaining what it represents (e.g. is this a number of blocks to skip when a node starts far behind?).

🟠 Missing PR Description

The PR body is empty and the commit messages are fix / fix da inclusion, giving no context about the problem this is solving. Please describe:

• What is the bug being fixed?
• Under what conditions does DA inclusion get stuck?
• What is the intended behavior of unblockDaInclusion?

🟢 No Tests

No tests were added for this new behavior. The processDAInclusionLoop logic should be covered to prevent regressions, especially since the current code has a bug that could go undetected without tests.

Summary

The core fix logic appears to be broken and could cause correctness issues in the DA inclusion loop. I'd recommend revisiting the intended behavior before merging.

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.

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