Skip to content

Conversation

@julienrbrt
Copy link
Member

Closes: #3016

Opus did a pretty good job. I'll be improving it.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedFeb 9, 2026, 2:36 PM

@claude
Copy link
Contributor

claude bot commented Feb 9, 2026

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


Code Review Complete ✓

Tasks

  • Read and analyze changed files
  • Review code quality & architecture
  • Review error handling & resilience
  • Review security considerations
  • Review performance & resource efficiency
  • Review testing & reliability
  • Review documentation & developer experience
  • Compile and post comprehensive feedback

Overview

This PR implements a catch-up from base feature that allows the single sequencer to replay missed DA epochs after extended downtime, producing blocks with only forced inclusion transactions (matching base sequencer behavior) before resuming normal operation. The implementation spans 862 additions across 3 files with comprehensive test coverage (683 lines of tests).

Overall Assessment: Strong implementation with excellent test coverage. The catch-up mechanism is well-designed, but there are several areas requiring attention around error handling, edge cases, and production readiness.


1. Code Quality & Architecture

✅ Strengths

  • Clean separation of concerns: Catch-up logic is well-encapsulated in the sequencer with minimal changes to the syncer
  • Idiomatic Go: Proper use of atomic.Pointer, context handling, and standard patterns
  • Clear state machine: The catch-up boolean flag clearly delineates operational modes
  • Good abstraction: The catch-up behavior integrates naturally into the existing GetNextBatch flow

⚠️ Issues & Recommendations

Critical: Potential timestamp reuse bug (pkg/sequencers/single/sequencer.go:363-365)

timestamp := time.Now()
if c.catchingUp && !c.currentDAEndTime.IsZero() {
    timestamp = c.currentDAEndTime
}

Problem: currentDAEndTime is set once per epoch fetch and reused for all blocks in that epoch. If multiple blocks are produced from the same epoch, they'll all have identical timestamps, violating monotonicity assumptions.

Impact: Could cause block validation failures, state hash mismatches, or execution layer issues.

Recommendation: Track which block within the epoch this is, or use a monotonically increasing timestamp like c.currentDAEndTime.Add(time.Duration(blockIndex) * time.Second).

Fix timestamp reuse →


Major: Race condition in checkpoint updates (pkg/sequencers/single/sequencer.go:326-352)

if daHeight > 0 || len(forcedTxs) > 0 {
    c.checkpoint.TxIndex += forcedTxConsumedCount
    if c.checkpoint.TxIndex >= uint64(len(c.cachedForcedInclusionTxs)) {
        c.checkpoint.DAHeight = daHeight + 1
        c.checkpoint.TxIndex = 0
        c.cachedForcedInclusionTxs = nil
        c.SetDAHeight(c.checkpoint.DAHeight)
    }
    if err := c.checkpointStore.Save(ctx, c.checkpoint); err != nil {
        return nil, fmt.Errorf("failed to save checkpoint: %w", err)
    }
}

Problem: Multiple goroutines could potentially call GetNextBatch concurrently. The checkpoint update is not atomic - there's a gap between reading TxIndex, computing the new value, and persisting it. This could lead to:

  1. Lost checkpoint updates
  2. Transactions being marked as consumed when they haven't been processed
  3. Checkpoint corruption after crash

Recommendation: Add a mutex around checkpoint read-modify-write operations, or use a channel-based serialization approach for GetNextBatch calls.

Fix race condition →


Moderate: DA height initialization inconsistency (pkg/sequencers/single/sequencer.go:195-208)

if daHeight > 0 && c.checkpoint.DAHeight == 0 {
    c.checkpoint = &seqcommon.Checkpoint{
        DAHeight: daHeight,
        TxIndex:  0,
    }
    if c.fiRetriever != nil {
        c.fiRetriever.Stop()
    }
    c.fiRetriever = block.NewForcedInclusionRetriever(...)
}

Problem: This initialization path:

  1. Stops and recreates fiRetriever on the hot path (every GetNextBatch call until checkpoint is initialized)
  2. Doesn't persist the new checkpoint immediately, risking inconsistent state after crash
  3. Has unclear semantics - why would checkpoint.DAHeight be 0 if daHeight is non-zero?

Recommendation:

  • Move this initialization to NewSequencer or a dedicated initialization method
  • Persist the checkpoint immediately after creation
  • Add comments explaining when this condition is expected

Fix initialization →


Minor: Magic number for catch-up threshold (pkg/sequencers/single/sequencer.go:533-535)

catchUpThreshold := epochWallDuration
if catchUpThreshold < 30*time.Second {
    catchUpThreshold = 30 * time.Second
}

Recommendation: Extract 30*time.Second as a named constant with documentation explaining the rationale. Consider making it configurable.


2. Error Handling & Resilience

✅ Strengths

  • Proper error wrapping with context (fmt.Errorf with %w)
  • Graceful handling of ErrHeightFromFuture to exit catch-up mode
  • Comprehensive error checking throughout

⚠️ Issues & Recommendations

Critical: Silent failure on checkpoint save (pkg/sequencers/single/sequencer.go:342-344)

if err := c.checkpointStore.Save(ctx, c.checkpoint); err != nil {
    return nil, fmt.Errorf("failed to save checkpoint: %w", err)
}

Problem: If checkpoint save fails, we return an error but the in-memory state (c.checkpoint.TxIndex, c.cachedForcedInclusionTxs) has already been modified. On retry, the sequencer will process the same transactions again, potentially causing:

  1. Duplicate transaction inclusion
  2. Checkpoint corruption
  3. Inconsistent state between memory and disk

Recommendation:

  • Persist checkpoint BEFORE modifying in-memory state
  • Or implement a rollback mechanism
  • Or use a transactional approach where memory state is only updated after successful persist

Fix checkpoint save →


Major: Context cancellation ignored during fetch (pkg/sequencers/single/sequencer.go:429-502)

The fetchNextDAEpoch function doesn't check ctx.Done() before potentially expensive operations:

  • DA retrieval (could block for minutes if DA layer is slow)
  • Transaction validation loop
  • Checkpoint persistence

Recommendation: Add context checks at strategic points to allow graceful cancellation.


Moderate: Filtering errors are swallowed (pkg/sequencers/single/sequencer.go:264-272)

filterStatuses, err := c.executor.FilterTxs(ctx, allTxs, req.MaxBytes, maxGas, forcedTxCount > 0)
if err != nil {
    c.logger.Warn().Err(err).Msg("failed to filter transactions, proceeding with unfiltered")
    // Fall back to using all txs as OK
    filterStatuses = make([]execution.FilterStatus, len(allTxs))
    for i := range filterStatuses {
        filterStatuses[i] = execution.FilterOK
    }
}

Problem: If filtering fails, we proceed with ALL transactions marked as OK. This could include:

  • Invalid/malformed transactions
  • Transactions exceeding size limits
  • Transactions that violate gas limits

This could lead to executor failures or blocks that can't be processed.

Recommendation:

  • Return an error if filtering fails (safer approach)
  • Or implement a fallback size-only filter
  • At minimum, add metrics to track filtering failures

3. Security

✅ Strengths

  • Proper validation of forced inclusion transactions via FilterTxs
  • Clean separation between catch-up and normal modes prevents mempool leakage
  • Checkpoint-based recovery prevents replay attacks

⚠️ Issues & Recommendations

Moderate: Epoch validation gap (pkg/sequencers/single/sequencer.go:514-560)

The updateCatchUpState function uses event.Timestamp directly without validation:

if event == nil || event.Timestamp.IsZero() {
    return
}

Problem: The timestamp comes from external DA sources and could be manipulated. A malicious DA or compromised node could:

  1. Set timestamps far in the past to keep nodes in catch-up mode indefinitely
  2. Set timestamps in the future to exit catch-up prematurely

Recommendation: Add sanity checks:

  • Validate timestamp is within reasonable bounds relative to local time
  • Cross-check with multiple DA sources if possible
  • Add metrics to track unusual timestamp patterns

Minor: Hash function not specified (block/internal/syncing/syncer.go:916-920)

func hashTx(tx []byte) string {
    hash := sha256.Sum256(tx)
    return hex.EncodeToString(hash[:])
}

Observation: SHA256 is fine for transaction hashing, but consider:

  • Documenting why SHA256 was chosen (collision resistance requirements)
  • Ensuring this matches the hashing used elsewhere in the system
  • Considering whether a faster hash (like blake3) would suffice

4. Performance & Resource Efficiency

✅ Strengths

  • Efficient caching of forced inclusion transactions
  • Atomic operations for concurrent access
  • Sequential processing prevents excessive memory growth

⚠️ Issues & Recommendations

Major: Unbounded memory growth in pending map (block/internal/syncing/syncer.go:1051-1065)

s.pendingForcedInclusionTxs.Range(func(key, value any) bool {
    pending := value.(pendingForcedInclusionTx)
    if _, ok := blockTxMap[pending.TxHash]; ok {
        s.pendingForcedInclusionTxs.Delete(key)
    } else {
        stillPending = append(stillPending, pending)
    }
    return true
})

Problem: If the sequencer never includes forced transactions (malicious behavior), or if there's a bug preventing cleanup, the pendingForcedInclusionTxs map will grow indefinitely, eventually causing OOM.

Recommendation:

  • Add a maximum size limit with proper error handling/metrics when exceeded
  • Implement periodic cleanup of entries past the grace boundary (they've already been detected as malicious)
  • Add memory usage metrics for this map

Add bounds →


Moderate: Repeated DA epoch fetches (pkg/sequencers/single/sequencer.go:438-462)

If RetrieveForcedIncludedTxs fails with a transient error (network timeout, etc.), the sequencer will retry the fetch. However, there's no backoff mechanism, potentially causing:

  • Excessive load on DA layer
  • Wasted CPU cycles
  • Slow progress through catch-up

Recommendation: Implement exponential backoff for DA fetch retries.


Minor: Inefficient transaction lookups (block/internal/syncing/syncer.go:1043-1047)

blockTxMap := make(map[string]struct{})
for _, tx := range data.Txs {
    blockTxMap[hashTx(tx)] = struct{}{}
}

Observation: Building this map on every block could be expensive for large blocks. Consider:

  • Caching the map if blocks are reprocessed
  • Using a more efficient data structure (bloom filter for negative lookups)
  • Pre-computing hashes in the Data structure itself

5. Testing & Reliability

✅ Strengths

  • Excellent test coverage (683 lines of new tests)
  • Comprehensive test scenarios covering:
    • Multi-epoch catch-up
    • Grace period exit conditions
    • Checkpoint persistence and crash recovery
    • Mempool skipping during catch-up
    • DA timestamp usage
    • Forced inclusion filtering
  • Good use of table-driven tests where appropriate
  • Proper test isolation with mocks

⚠️ Issues & Recommendations

Major: Missing concurrency tests

The implementation uses atomic operations and has potential race conditions (as noted above), but there are no tests exercising concurrent access patterns:

  • Multiple goroutines calling GetNextBatch simultaneously
  • Checkpoint updates during forced retriever operations
  • Cache access during catch-up mode transitions

Recommendation: Add concurrency tests using go test -race and explicit goroutine spawning to validate thread safety.

Add concurrency tests →


Moderate: Missing edge case tests

Some edge cases lack explicit test coverage:

  1. Zero-length epoch: What happens if DAEpochForcedInclusion is 0?
  2. Clock skew: What if local time is far ahead/behind DA timestamps?
  3. Corrupted checkpoint: What if checkpoint data on disk is corrupted?
  4. DA layer returns duplicate transactions: Are duplicates properly deduplicated?
  5. Very large forced inclusion transaction: Does size validation work correctly?

Recommendation: Add tests for these scenarios.


Minor: Test naming inconsistency

Most tests follow TestSequencer_<Feature>_<Scenario> pattern, but a few use different conventions (e.g., TestSequencer_QueueLimit_Integration). Standardize naming for consistency.


6. Documentation & Developer Experience

✅ Strengths

  • Excellent inline comments explaining complex logic (especially around catch-up state transitions)
  • Clear function and variable naming
  • Good docstrings on public functions

⚠️ Issues & Recommendations

Major: Missing architecture documentation

The catch-up mechanism is a significant feature change, but there's no high-level documentation explaining:

  • When catch-up mode is triggered
  • Why it's necessary (relationship to base sequencer)
  • How it interacts with forced inclusion
  • What happens to mempool transactions during catch-up
  • How long catch-up typically takes

Recommendation: Add a docs/catchup-mode.md or update CLAUDE.md with this information.

Add documentation →


Moderate: Unclear checkpoint semantics (pkg/sequencers/single/sequencer.go:48-49)

checkpoint      *seqcommon.Checkpoint
// Cached forced inclusion transactions from the current epoch
cachedForcedInclusionTxs [][]byte

Issue: The relationship between checkpoint.TxIndex and cachedForcedInclusionTxs is not immediately clear:

  • Does TxIndex track consumed transactions or remaining transactions?
  • Is it an index into the cache, or an absolute count?
  • What happens if the cache is cleared but TxIndex is non-zero?

Recommendation: Add detailed comments explaining the invariants and semantics.


Minor: Logging could be improved (pkg/sequencers/single/sequencer.go:240-244)

c.logger.Debug().
    Uint64("checkpoint_da_height", c.checkpoint.DAHeight).
    Int("forced_txs", len(forcedTxs)).
    Msg("catch-up mode: skipping mempool transactions")

Observation: This is only logged at debug level. Consider:

  • Logging catch-up mode entry/exit at INFO level for better observability
  • Adding metrics for catch-up duration
  • Including catch-up progress (e.g., "processed epoch 103 of 110")

Minor: Comment typo (block/internal/syncing/syncer.go:775)

// When a sequencer restarts after extended downtime, it produces "catch-up"

Good comment, but consider expanding it to explain the specific scenario: "...it produces catch-up blocks that were produced by base sequencers during its downtime".


7. Additional Observations

Positive: DA Height Gap Handling (block/internal/syncing/syncer.go:775-801)

The logic to advance DA height by one epoch at a time during catch-up is well-designed and properly documented. This prevents the forced inclusion verifier from checking the wrong epoch.

Concern: No metrics for catch-up operations

Consider adding:

  • sequencer_catchup_active (gauge, 0/1)
  • sequencer_catchup_epochs_remaining (gauge)
  • sequencer_catchup_duration_seconds (histogram)
  • sequencer_catchup_txs_processed_total (counter)

Concern: No alerting guidance

Operators should be alerted when:

  • Catch-up mode is active for extended periods (>1 hour)
  • Catch-up fails to make progress (DA height not advancing)
  • Forced inclusion transactions are past grace boundary

Summary of Recommendations by Priority

Must Fix (Before Merge)

  1. Timestamp reuse bug - Could cause block validation failures
  2. Checkpoint race condition - Could cause state corruption
  3. Silent checkpoint save failure - Could cause transaction duplication
  4. Add concurrency tests - Required to validate thread safety

Should Fix (High Priority)

  1. Unbounded memory growth in pending forced inclusion map
  2. DA height initialization on hot path
  3. Filtering error handling - Too permissive fallback
  4. Architecture documentation - Critical for maintainability

Nice to Have (Medium Priority)

  1. Context cancellation checks in fetch paths
  2. DA fetch retry backoff
  3. Epoch timestamp validation
  4. Edge case test coverage
  5. Catch-up metrics and alerting

Optional (Low Priority)

  1. Magic number extraction
  2. Test naming consistency
  3. Logging improvements
  4. Performance optimizations (map lookups, etc.)

Conclusion

This is a well-implemented feature with excellent test coverage and clear code structure. However, there are several critical issues around concurrency, error handling, and edge cases that should be addressed before merge. The timestamp reuse bug and checkpoint race condition are the most urgent concerns.

The PR demonstrates strong engineering practices overall, particularly in testing and code clarity. Once the critical issues are resolved, this will be a solid addition to the codebase.

Recommendation: Request changes to address critical issues, then approve after fixes.


@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 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

  • Sequencer Catch-up Mechanism: Introduced a new mechanism allowing the sequencer to "catch up" on missed Data Availability (DA) epochs after extended downtime.
  • Forced Inclusion Priority: During catch-up, the sequencer temporarily processes only forced inclusion transactions, ensuring consistency with base sequencing nodes and temporarily skipping mempool transactions.
  • DA Height Synchronization: Modified syncer.go to incrementally advance the DAHeight by one epoch during catch-up, preventing issues with forced inclusion transaction verification.
  • Timestamp Alignment: Block timestamps generated during catch-up are now aligned with the DA epoch's end timestamp for historical accuracy.
  • Comprehensive Testing: Added extensive unit tests covering various catch-up scenarios, including detection, mempool skipping, timestamp usage, exit conditions, and multi-epoch replay.

🧠 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
  • block/internal/syncing/syncer.go
    • Updated the logic for DAHeight updates to handle large discrepancies between event.DaHeight and newState.DAHeight. When a significant gap (more than one epoch) is detected, newState.DAHeight is now advanced by exactly one epoch per block, rather than jumping directly to event.DaHeight. This ensures correct verification of forced inclusion transactions during sequencer catch-up.
    • Added detailed comments explaining the new DAHeight update strategy and its importance for catch-up blocks and forced inclusion verification.
  • pkg/sequencers/single/sequencer.go
    • Added catchingUp (boolean) and currentDAEndTime (time.Time) to the Sequencer struct to manage the catch-up state and store the DA epoch's end timestamp.
    • Modified GetNextBatch to conditionally skip fetching transactions from the mempool when the sequencer is in catchingUp mode. This ensures that only forced inclusion transactions are included in batches during catch-up.
    • Updated GetNextBatch to use sequencer.currentDAEndTime as the block timestamp when catchingUp is true, aligning block timestamps with the historical DA epoch.
    • Introduced a public IsCatchingUp() method to query the sequencer's current catch-up status.
    • Modified fetchNextDAEpoch to store the forcedTxsEvent.Timestamp in currentDAEndTime and to call a new updateCatchUpState method to manage the catchingUp flag. It also now exits catch-up mode if StatusHeightFromFuture is returned by the DA client.
    • Implemented a new private method updateCatchUpState that determines whether the sequencer should be in catch-up mode based on the timestamp of the fetched DA epoch. It enters catch-up if the epoch timestamp is significantly in the past (more than one epoch's wall-clock duration) and exits if it's recent.
  • pkg/sequencers/single/sequencer_test.go
    • Added a comprehensive suite of tests (TestSequencer_CatchUp_...) to validate the new catch-up functionality. These tests cover:
    • Detection of old epochs and entering catch-up mode.
    • Skipping mempool transactions during catch-up.
    • Using DA epoch timestamps for blocks during catch-up.
    • Exiting catch-up mode when the DA head is reached (either a recent epoch or HeightFromFuture).
    • Scenarios where catch-up is not triggered (recent epoch, no forced inclusion configured).
    • Multi-epoch replay scenarios.
    • Correct advancement of the internal checkpoint (DAHeight) during catch-up.
Activity
  • The pull request addresses and closes issue https://github.com/evstack/ev-node/issues/3016.
  • The author indicates that this work builds upon a previous attempt by "Opus", aiming for improvement.
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.

@codecov
Copy link

codecov bot commented Feb 9, 2026

Codecov Report

❌ Patch coverage is 75.32468% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.79%. Comparing base (a64c35b) to head (17fcc48).

Files with missing lines Patch % Lines
block/internal/syncing/syncer.go 22.22% 13 Missing and 1 partial ⚠️
pkg/sequencers/single/sequencer.go 91.52% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3057      +/-   ##
==========================================
+ Coverage   61.64%   61.79%   +0.14%     
==========================================
  Files         111      111              
  Lines       11120    11192      +72     
==========================================
+ Hits         6855     6916      +61     
- Misses       3526     3537      +11     
  Partials      739      739              
Flag Coverage Δ
combined 61.79% <75.32%> (+0.14%) ⬆️

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

@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 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.

Comment on lines +362 to +365
timestamp := time.Now()
if c.catchingUp && !c.currentDAEndTime.IsZero() {
timestamp = c.currentDAEndTime
}
Copy link
Contributor

Choose a reason for hiding this comment

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

security-high high

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.

Comment on lines +60 to +63
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
Copy link
Contributor

Choose a reason for hiding this comment

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

security-medium medium

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.

Comment on lines 533 to 535
if catchUpThreshold < 30*time.Second {
catchUpThreshold = 30 * time.Second
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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

Choose a reason for hiding this comment

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

medium

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.

Suggested change
logger := zerolog.New(zerolog.NewConsoleWriter())
logger := zerolog.New(zerolog.NewTestWriter(t))

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.

[FEATURE] Sequencer catchup on restart from base

1 participant