Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 1m 54s —— View job Code Review: Sequencer Catch-up FeatureI've completed a comprehensive review of this PR implementing the sequencer catch-up mechanism. The implementation addresses an important gap in handling sequencer downtime, but there are several critical issues that must be addressed before merging. Critical Issues1. Data Race on Catch-up State Fields 🔴 [sequencer.go:67-70]Issue: The
While Impact: Data race leading to potential timestamp corruption and undefined behavior. Fix Required: Replace currentDAEndTime atomic.Pointer[time.Time]Then update reads/writes accordingly: // Write (line 485):
c.currentDAEndTime.Store(&forcedTxsEvent.Timestamp.UTC())
// Read (line 370):
if daEndTime := c.currentDAEndTime.Load(); daEndTime != nil {
timestamp = daEndTime.Add(-time.Duration(remainingForcedTxs) * time.Millisecond)
}2. Non-Monotonic Block Timestamps During Catch-up 🔴 [sequencer.go:366-378]Issue: The timestamp logic has a fundamental flaw that will cause execution layer rejection:
Impact: Most blockchain execution layers (EVM, Cosmos SDK, etc.) require strictly monotonically increasing block timestamps. Duplicate timestamps will cause execution to reject blocks and halt the chain. Example Failure Scenario: Fix Required: This needs a more sophisticated timestamping strategy:
3. Insufficient Test Coverage for Edge Cases 🟡The test suite (64.54% coverage) is missing critical scenarios: Missing Tests:
Existing Test Gap [sequencer_test.go:1406-1462]: Recommendation: Add tests for:
High Priority Issues4. Magic Number for Catch-up Threshold [sequencer.go:569]Issue: The 30-second threshold is hardcoded without clear justification: if missedEpochs <= 1 {
// Within the current or next epoch — normal operationProblem: This logic is purely epoch-based (missedEpochs), the 30-second value mentioned in Gemini's review doesn't appear in the final code. However, the epoch-based detection could still trigger false positives if DA epoch size is small. Recommendation: Add configuration parameter for catch-up threshold or document why epoch-based detection is sufficient. 5. Syncer DA Height Advancement Logic Complexity [syncer.go:780-806]Issue: The DA height advancement logic in the syncer is complex and has subtle edge cases: if epochSize > 0 && gap > epochSize {
// Advance by one epoch
_, epochEnd, _ := types.CalculateEpochBoundaries(...)
nextEpochStart := epochEnd + 1
if nextEpochStart > event.DaHeight {
nextEpochStart = event.DaHeight // Safety clamp
}
newState.DAHeight = nextEpochStart
}Concerns:
Recommendation:
Medium Priority Issues6. GetLatestDAHeight Called Without Retry [sequencer.go:548]Issue: Network call to latestDAHeight, err := c.daClient.GetLatestDAHeight(ctx)
if err != nil {
c.logger.Warn().Err(err).Msg("failed to get latest DA height for catch-up detection, skipping check")
return // Silently skips catch-up detection
}Problem: Transient network failures will prevent catch-up detection entirely, leaving the sequencer in normal mode when it should be catching up. Impact: Sequencer might produce blocks with mempool transactions during what should be catch-up mode, creating divergence from base sequencing behavior. Recommendation: Add retry logic with exponential backoff, similar to other DA calls. 7. DA Client GetLatestDAHeight Missing Error Mapping [client.go:304-316]Issue: New func (c *client) GetLatestDAHeight(ctx context.Context) (uint64, error) {
header, err := c.headerAPI.NetworkHead(headCtx)
if err != nil {
return 0, fmt.Errorf("failed to get DA network head: %w", err)
}
// ...
}Compare to Recommendation: Return a 8. Checkpoint Persistence Could Fail Silently During Catch-up [sequencer.go:349]Issue: Checkpoint save errors during catch-up are returned but not specially handled: if err := c.checkpointStore.Save(ctx, c.checkpoint); err != nil {
return nil, fmt.Errorf("failed to save checkpoint: %w", err)
}Problem: During catch-up with rapid block production, checkpoint failures could indicate disk/DB issues that should trigger different behavior than normal mode. Recommendation: Add monitoring/metrics for checkpoint save failures during catch-up mode specifically. Low Priority / Suggestions9. Inconsistent Logger Usage in Tests [sequencer_test.go:1245, 1309, 1457, 1676]Multiple tests use Recommendation: Use 10. Documentation Could Be Clearer on Catch-up Exit ConditionsThe code has two exit conditions for catch-up:
Recommendation: Add more detailed comments explaining both exit paths and their implications. Positive Aspects ✅
SummaryThis PR implements a critical feature for sequencer resilience, but has two blocking issues that must be fixed:
Additionally, several high-priority improvements are recommended around error handling, testing, and edge case coverage. The syncer-side DA height advancement logic is well-designed and correctly handles the catch-up block verification challenge. RecommendationDo not merge until the two critical issues are resolved and additional tests are added for multi-block epochs and timestamp monotonicity. |
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 implements a critical "catch-up" feature for the sequencer, designed to enhance its resilience and consistency following periods of inactivity or downtime. By intelligently replaying missed Data Availability (DA) epochs, the sequencer can resynchronize its state, ensuring that all forced inclusion transactions are processed in the correct order and with accurate timestamps, thereby maintaining a consistent view of the blockchain's history, even when operating in a decentralized environment. This mechanism allows the sequencer to seamlessly transition back to normal operation without compromising data integrity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3057 +/- ##
==========================================
+ Coverage 61.92% 61.97% +0.04%
==========================================
Files 111 111
Lines 11113 11218 +105
==========================================
+ Hits 6882 6952 +70
- Misses 3492 3527 +35
Partials 739 739
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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust catch-up mechanism for the sequencer, designed to handle restarts after extended downtime. While the overall approach for consuming and verifying catch-up blocks in the syncer, including the incremental advancement of DAHeight, is well-implemented and tested, the implementation of catch-up mode in the single sequencer has significant flaws. Specifically, it produces non-monotonic block timestamps when multiple blocks are generated for a single DA epoch or when empty epochs are encountered, which will likely cause the execution layer to reject blocks and halt the chain. Additionally, there is a data race on the new catch-up state fields due to a lack of synchronization primitives. Minor suggestions for code clarity and testing experience were also noted.
| timestamp := time.Now() | ||
| if c.catchingUp && !c.currentDAEndTime.IsZero() { | ||
| timestamp = c.currentDAEndTime | ||
| } |
There was a problem hiding this comment.
During catch-up mode, the sequencer uses the timestamp from the last fetched DA epoch as the block timestamp. However, multiple blocks can be produced for a single DA epoch (e.g., if the epoch contains many transactions that don't fit in one block). In this case, all blocks produced for that epoch will have the exact same timestamp (c.currentDAEndTime). Furthermore, if an empty DA epoch is encountered, c.currentDAEndTime is not updated, causing the next block to reuse the timestamp from the previous non-empty epoch.
Most blockchain execution layers (like EVM) require that each new block has a timestamp strictly greater than its parent. If the sequencer produces multiple blocks with the same timestamp, the execution layer will reject them, halting the block production process and causing a Denial of Service.
pkg/sequencers/single/sequencer.go
Outdated
| catchingUp bool | ||
| // currentDAEndTime is the DA epoch end timestamp from the last fetched epoch. | ||
| // Used as the block timestamp during catch-up to match based sequencing behavior. | ||
| currentDAEndTime time.Time |
There was a problem hiding this comment.
The fields catchingUp and currentDAEndTime are accessed concurrently without proper synchronization. GetNextBatch and fetchNextDAEpoch (which update these fields) can be called by the block production loop, while IsCatchingUp() can be called concurrently by other components (e.g., RPC, monitoring, or metrics).
In Go, concurrent read/write access to non-atomic fields is a data race, which leads to undefined behavior. Specifically, a race on time.Time (which is a struct) can result in a corrupted timestamp being read, potentially leading to invalid blocks and a Denial of Service.
pkg/sequencers/single/sequencer.go
Outdated
| if catchUpThreshold < 30*time.Second { | ||
| catchUpThreshold = 30 * time.Second | ||
| } |
There was a problem hiding this comment.
The value 30*time.Second is used as a minimum threshold for the catch-up logic. To improve readability and maintainability, consider defining this as a named constant at the package level, for example: const minCatchUpThreshold = 30 * time.Second, and using that constant here. This makes the intent of the value clearer and centralizes its definition.
| // This test uses maxBytes to limit how many txs are fetched, triggering the unprocessed txs scenario. | ||
| func TestSequencer_CatchUp_DetectsOldEpoch(t *testing.T) { | ||
| ctx := context.Background() | ||
| logger := zerolog.New(zerolog.NewConsoleWriter()) |
There was a problem hiding this comment.
Using zerolog.New(zerolog.NewConsoleWriter()) in tests can lead to verbose output in the console during test runs. It's generally better to use a logger that integrates with the testing framework, such as zerolog.New(zerolog.NewTestWriter(t)), which only shows logs for failed tests, or zerolog.Nop() if logs are not needed. This practice is followed in other new tests in this file (e.g., TestSequencer_CatchUp_UsesDATimestamp). Applying this consistently would improve the developer experience. This comment also applies to lines 1309, 1457, and 1676.
| logger := zerolog.New(zerolog.NewConsoleWriter()) | |
| logger := zerolog.New(zerolog.NewTestWriter(t)) |
1 participant
Closes: #3016
Opus did a pretty good job. I'll be improving it.