Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 3m 57s —— View job PR Review: feat(sequencer): catchup from base
Overall AssessmentThis PR tackles a real and important problem — sequencer consistency after downtime — and the design is sound. The epoch-based catch-up state machine is well-structured, the tests are thorough, and the syncer's epoch-start-alignment fix is critical and correct. Below are findings ranging from a blocking bug to minor style notes. Critical1.
|
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 60.91% 60.98% +0.06%
==========================================
Files 113 113
Lines 11617 11724 +107
==========================================
+ Hits 7077 7150 +73
- Misses 3742 3772 +30
- Partials 798 802 +4
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.
|
i believe this pr justifies an adr |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSequencers gain a catch‑up mode: on restart they query the DA head, detect multi‑epoch gaps, replay missed epochs using only DA forced‑inclusion transactions with DA‑derived timestamps, advance execution finalized heights when running as a BasedSequencer, and resume normal sequencing once caught up. Changes
Sequence Diagram(s)sequenceDiagram
participant Seq as Sequencer
participant DA as DA Layer
participant FI as Forced‑Inclusion Retriever
participant Exec as Execution Layer
participant P2P as Network
Seq->>DA: GetLatestDAHeight(ctx)
DA-->>Seq: latestDAHeight
alt Gap > 1 epoch
Seq->>Seq: Enter CatchUp Mode
loop For each missed epoch
Seq->>FI: RetrieveForcedInclusion(epoch)
FI-->>Seq: forcedTxs + epochTimestamp
Seq->>Exec: ProduceBlock(forcedTxs, timestamp)
Exec-->>Seq: blockProduced
Seq->>Exec: SetFinal(height)
Seq->>Seq: Advance checkpoint
end
Seq->>DA: GetLatestDAHeight(ctx)
DA-->>Seq: latestDAHeight
alt Caught up
Seq->>Seq: Exit CatchUp Mode
else Still behind
Seq->>Seq: Continue CatchUp
end
else Gap <= 1 epoch
Seq->>Seq: Normal sequencing (mempool allowed)
end
Seq->>P2P: Broadcast / Sync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/sequencers/single/sequencer.go (1)
483-504:⚠️ Potential issue | 🟠 MajorCatch‑up detection can be permanently disabled on a transient DA error.
updateCatchUpStatesetscatchUpDonebefore callingGetLatestDAHeight, so a one‑off error means the sequencer will never re‑check catch‑up for the rest of its lifecycle.💡 Suggested fix to allow retry after DA errors
func (c *Sequencer) updateCatchUpState(ctx context.Context) { if c.catchUpState.Load() != catchUpUnchecked { return } - // Optimistically mark as done; overridden to catchUpInProgress below if - // catch-up is actually needed. - c.catchUpState.Store(catchUpDone) - epochSize := c.genesis.DAEpochForcedInclusion if epochSize == 0 { + c.catchUpState.Store(catchUpDone) return } - currentDAHeight := c.checkpoint.DAHeight - daStartHeight := c.genesis.DAStartHeight - 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 } + + currentDAHeight := c.checkpoint.DAHeight + daStartHeight := c.genesis.DAStartHeight // At head, no catch-up needed if latestDAHeight <= currentDAHeight { + c.catchUpState.Store(catchUpDone) return } @@ if missedEpochs <= 1 { c.logger.Debug(). Uint64("checkpoint_da_height", currentDAHeight). @@ Uint64("latest_epoch", latestEpoch). Msg("sequencer within one epoch of DA head, no catch-up needed") + c.catchUpState.Store(catchUpDone) return } // More than one epoch behind - enter catch-up mode c.catchUpState.Store(catchUpInProgress)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sequencers/single/sequencer.go` around lines 483 - 504, updateCatchUpState currently marks c.catchUpState as catchUpDone before calling c.daClient.GetLatestDAHeight, so a transient DA error can permanently disable catch‑up checks; change the flow in updateCatchUpState so you do not store catchUpDone until after GetLatestDAHeight succeeds and you’ve determined no catch‑up is needed (or, alternatively, on GetLatestDAHeight error, do not change c.catchUpState or explicitly revert it to catchUpUnchecked); reference updateCatchUpState, c.catchUpState, catchUpDone, catchUpInProgress and c.daClient.GetLatestDAHeight to locate and implement this fix.
🧹 Nitpick comments (3)
block/internal/executing/executor.go (1)
637-642:e.ctxvsctxinProduceBlock.
SetFinalis called withe.ctxwhile almost every other primary block operation in this method uses the passedctxparameter. The existing broadcast calls (Lines 619, 624) also usee.ctx, so this is following the established pattern—but it creates a subtle inconsistency where a timeout or deadline on the caller'sctxdoesn't propagate toSetFinal.♻️ Suggested alignment
- if err := e.exec.SetFinal(e.ctx, newHeight); err != nil { + if err := e.exec.SetFinal(ctx, newHeight); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/executing/executor.go` around lines 637 - 642, In ProduceBlock, e.exec.SetFinal is being called with e.ctx which prevents the caller's ctx deadline/cancelation from applying; change the call to use the passed ctx parameter (i.e., call e.exec.SetFinal(ctx, newHeight)) so SetFinal honors the ProduceBlock caller context, ensuring consistency with other operations in ProduceBlock (and keep e.ctx usage only where intentional, e.g., long-lived background broadcasts).block/internal/da/tracing_test.go (1)
57-57: Missing tracing tests forGetLatestDAHeight.The new
GetLatestDAHeightmethod ontracedClientadds span creation, error recording, and ada.latest_heightattribute — all of which are untested. Every other traced method (Submit,Retrieve,Get) has both a success and an error test.GetLatestDAHeighthas neither.🧪 Suggested test additions
+func TestTracedDA_GetLatestDAHeight_Success(t *testing.T) { + mock := &mockFullClient{ + getLatestDAHeightFn: func(_ context.Context) (uint64, error) { + return 42, nil + }, + } + client, sr := setupDATrace(t, mock) + + height, err := client.GetLatestDAHeight(context.Background()) + require.NoError(t, err) + require.Equal(t, uint64(42), height) + + spans := sr.Ended() + require.Len(t, spans, 1) + span := spans[0] + require.Equal(t, "DA.GetLatestDAHeight", span.Name()) + require.Equal(t, codes.Unset, span.Status().Code) + testutil.RequireAttribute(t, span.Attributes(), "da.latest_height", int64(42)) +} + +func TestTracedDA_GetLatestDAHeight_Error(t *testing.T) { + mock := &mockFullClient{ + getLatestDAHeightFn: func(_ context.Context) (uint64, error) { + return 0, errors.New("network unavailable") + }, + } + client, sr := setupDATrace(t, mock) + + _, err := client.GetLatestDAHeight(context.Background()) + require.Error(t, err) + + spans := sr.Ended() + require.Len(t, spans, 1) + span := spans[0] + require.Equal(t, codes.Error, span.Status().Code) + require.Equal(t, "network unavailable", span.Status().Description) +}You would also need to add
getLatestDAHeightFntomockFullClientand updateGetLatestDAHeightto invoke it when non-nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/da/tracing_test.go` at line 57, Add unit tests for tracedClient.GetLatestDAHeight mirroring other traced method tests: one test asserting span creation, recorded `da.latest_height` attribute and no error on success, and one test asserting span error recording and proper error propagation on failure; update mockFullClient by adding a getLatestDAHeightFn field and change its GetLatestDAHeight method to call getLatestDAHeightFn when non-nil (so tests can inject success and error behaviors); place the new tests in tracing_test.go alongside existing Submit/Retrieve/Get tests and use the same tracing assertions used elsewhere to verify span creation, attributes, and error recording.test/e2e/evm_force_inclusion_e2e_test.go (1)
590-961: Consider waiting for process exit after SIGTERM to reduce flakiness.
Several phases stop and restart nodes on the same ports with only a fixed sleep. A deterministic wait (process wait or RPC-down poll) will make these E2E tests more stable under load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/evm_force_inclusion_e2e_test.go` around lines 590 - 961, The test currently sends syscall.SIGTERM to seqProcess, fnProcess and basedSeqProcess and then sleeps; replace the fixed sleeps with deterministic waits for the process to exit or the RPC to become unavailable: after signaling seqProcess/fnProcess/basedSeqProcess call the process Wait method (or use sut.AwaitNodeDown / poll the node RPC with endpoints.GetRollkitRPCAddress()/GetFullNodeRPCAddress() until it is down) before reusing ports or restarting the node, and apply this change wherever seqProcess, fnProcess or basedSeqProcess is stopped to eliminate racey restarts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/da/interface.go`:
- Around line 20-21: The comment above the GetLatestDAHeight method contains a
stray double period (".."); edit the comment for GetLatestDAHeight in
interface.go to use a single period (or remove the trailing punctuation) so it
reads: "// GetLatestDAHeight returns the latest height available on the DA
layer.".
In `@block/internal/syncing/syncer.go`:
- Around line 776-818: Update the block comment to reflect actual behavior:
state.DAHeight stepping (newState.DAHeight) is used to pace safe state
persistence/progression across restarts and does not control which epoch
VerifyForcedInclusionTxs checks, because VerifyForcedInclusionTxs is called with
event.DaHeight and uses that value to fetch/verify forced inclusion
transactions; explicitly state that during sequencer catch-up the code advances
newState.DAHeight by one epoch per block to avoid large persistence jumps, and
add a clear note that this creates a verification gap (forced txs from missed DA
epochs are verified against event.DaHeight/the submission epoch) and document
what external guarantee or mechanism (e.g., deterministic replay, sequencer
constraints, or other safeguard) must ensure catch-up correctness or that this
behavior is intentional.
In `@docs/adr/adr-019-forced-inclusion-mechanism.md`:
- Around line 472-492: Add a language specifier to the fenced code block that
begins with "Sequencer offline during epochs 100-150 (5 epochs of 10 blocks
each)" so markdownlint MD040 is satisfied; change the opening backticks to
```text (i.e., replace ``` with ```text) for that example block in
adr-019-forced-inclusion-mechanism.md.
- Around line 755-756: Replace the awkward sentence "P2P nodes only do not
proceed to any verification." in the NOTE paragraph with a clear phrasing such
as "P2P-only nodes do not perform any verification, because DA inclusion happens
later than block production and DA hints are therefore added to broadcasted
blocks afterward." Update the surrounding sentence to keep the causal
explanation intact and ensure the NOTE reads fluently.
---
Duplicate comments:
In `@pkg/sequencers/single/sequencer.go`:
- Around line 483-504: updateCatchUpState currently marks c.catchUpState as
catchUpDone before calling c.daClient.GetLatestDAHeight, so a transient DA error
can permanently disable catch‑up checks; change the flow in updateCatchUpState
so you do not store catchUpDone until after GetLatestDAHeight succeeds and
you’ve determined no catch‑up is needed (or, alternatively, on GetLatestDAHeight
error, do not change c.catchUpState or explicitly revert it to
catchUpUnchecked); reference updateCatchUpState, c.catchUpState, catchUpDone,
catchUpInProgress and c.daClient.GetLatestDAHeight to locate and implement this
fix.
---
Nitpick comments:
In `@block/internal/da/tracing_test.go`:
- Line 57: Add unit tests for tracedClient.GetLatestDAHeight mirroring other
traced method tests: one test asserting span creation, recorded
`da.latest_height` attribute and no error on success, and one test asserting
span error recording and proper error propagation on failure; update
mockFullClient by adding a getLatestDAHeightFn field and change its
GetLatestDAHeight method to call getLatestDAHeightFn when non-nil (so tests can
inject success and error behaviors); place the new tests in tracing_test.go
alongside existing Submit/Retrieve/Get tests and use the same tracing assertions
used elsewhere to verify span creation, attributes, and error recording.
In `@block/internal/executing/executor.go`:
- Around line 637-642: In ProduceBlock, e.exec.SetFinal is being called with
e.ctx which prevents the caller's ctx deadline/cancelation from applying; change
the call to use the passed ctx parameter (i.e., call e.exec.SetFinal(ctx,
newHeight)) so SetFinal honors the ProduceBlock caller context, ensuring
consistency with other operations in ProduceBlock (and keep e.ctx usage only
where intentional, e.g., long-lived background broadcasts).
In `@test/e2e/evm_force_inclusion_e2e_test.go`:
- Around line 590-961: The test currently sends syscall.SIGTERM to seqProcess,
fnProcess and basedSeqProcess and then sleeps; replace the fixed sleeps with
deterministic waits for the process to exit or the RPC to become unavailable:
after signaling seqProcess/fnProcess/basedSeqProcess call the process Wait
method (or use sut.AwaitNodeDown / poll the node RPC with
endpoints.GetRollkitRPCAddress()/GetFullNodeRPCAddress() until it is down)
before reusing ports or restarting the node, and apply this change wherever
seqProcess, fnProcess or basedSeqProcess is stopped to eliminate racey restarts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
CHANGELOG.mdapps/evm/server/force_inclusion_test.goblock/internal/da/client.goblock/internal/da/forced_inclusion_retriever.goblock/internal/da/interface.goblock/internal/da/tracing.goblock/internal/da/tracing_test.goblock/internal/executing/executor.goblock/internal/syncing/block_syncer.goblock/internal/syncing/syncer.goblock/internal/syncing/syncer_forced_inclusion_test.goblock/internal/syncing/tracing.goblock/internal/syncing/tracing_test.godocs/adr/adr-019-forced-inclusion-mechanism.mdpkg/config/config.gopkg/sequencers/single/sequencer.gopkg/sequencers/single/sequencer_test.gopkg/store/cached_store.gotest/e2e/evm_force_inclusion_e2e_test.gotest/mocks/da.gotest/testda/dummy.go
💤 Files with no reviewable changes (1)
- pkg/store/cached_store.go
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/adr/adr-019-forced-inclusion-mechanism.md (1)
756-757: Grammatical issue in the NOTE — previously flagged."P2P nodes only do not proceed to any verification" reads awkwardly. Consider: "P2P-only nodes do not perform forced inclusion verification."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-019-forced-inclusion-mechanism.md` around lines 756 - 757, The NOTE sentence is awkward; replace "P2P nodes only do not proceed to any verification" with a clearer phrasing such as "P2P-only nodes do not perform forced inclusion verification." Update the sentence in adr-019-forced-inclusion-mechanism.md (the NOTE block) to use "P2P-only nodes" and "perform forced inclusion verification" so the intent is unambiguous.
🧹 Nitpick comments (3)
docs/adr/adr-019-forced-inclusion-mechanism.md (1)
449-501: Catch-up mode documentation is clear and well-structured.The problem/solution framing, key behaviors list, and step-by-step example provide a solid explanation of the catch-up mechanism. The benefits section clearly ties back to the PR objectives.
One minor note: the example at lines 472-493 uses plain-text formatting without a fenced code block. For readability and consistency with the rest of the ADR (which uses fenced blocks for examples), consider wrapping it in a
```textblock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-019-forced-inclusion-mechanism.md` around lines 449 - 501, The example block under "Sequencer Catch-Up Mode" is plain text and should be fenced for consistency; wrap the example (the paragraph starting with "Sequencer offline during epochs 100-150..." through "Normal operation resumes:") in a ```text fenced code block so it matches the ADR's other examples and improves readability—update the section containing the example and ensure the fence surrounds the entire example including the step-by-step numbered list and the final "Normal operation resumes" lines.test/e2e/evm_force_inclusion_e2e_test.go (1)
977-977:WithFullNode()option is used but no full node is started in this test.
setupCommonEVMEnvis called withWithFullNode(), which likely allocates additional Docker containers and ports for a full node. SinceTestEvmBasedSequencerBaselineE2Eonly runs a single based sequencer, this may waste test resources. Consider whether you can drop the option, or if the full node ports/endpoints are actually needed downstream (e.g., for the engine/eth URLs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/evm_force_inclusion_e2e_test.go` at line 977, The test calls setupCommonEVMEnv with WithFullNode() but never starts/uses a full node, so remove the unnecessary resource allocation: modify TestEvmBasedSequencerBaselineE2E to call setupCommonEVMEnv without the WithFullNode() option; if the test actually needs engine/eth endpoints later, instead ensure a full node is started and its URLs are propagated (or provide mocked endpoints) before keeping WithFullNode() — locate the call to setupCommonEVMEnv and the WithFullNode() symbol to make the change.block/internal/syncing/syncer.go (1)
797-808: Safety clamp at Line 805-807 is good but the condition is worth documenting further.The clamp
if nextEpochStart > event.DaHeightprevents overshooting, which is a sensible safety net. However, this condition can be hit legitimately — not just as an edge case — when the gap is between one and two epochs (e.g.,gap = epochSize + 1where the current DA height is near the end of an epoch). Consider adding a more specific log level (warn instead of debug) or a metric counter so operators can detect if this clamp fires frequently, which could indicate misconfigured epoch sizes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer.go` around lines 797 - 808, The safety clamp inside syncer.go (inside the epoch-gap handling around nextEpochStart computed from types.CalculateEpochBoundaries using newState.DAHeight, s.genesis.DAStartHeight and epochSize) can legitimately trigger when gap is between one and two epochs; update the code in the if nextEpochStart > event.DaHeight block to (1) add a brief comment explaining this legitimate scenario (gap == epochSize+1 near epoch boundary), (2) replace or raise the log level to warn (use processLogger.Warnf or the existing logger) instead of debug so operators see it, and (3) increment a metric/counter (e.g., nextEpochClampCounter or NextEpochClampTotal) to track frequency of this clamp; keep the clamp assignment nextEpochStart = event.DaHeight as-is for safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/syncing/syncer.go`:
- Around line 793-818: The current logic advances newState.DAHeight by one epoch
when a large gap is detected, which can misalign verification when multiple
blocks carry partial forced-inclusion txs from the same DA epoch (see
sequencer.GetNextBatch and MaxBytes). Instead of advancing only one epoch,
compute the DA epoch start that contains event.DaHeight and set
newState.DAHeight to that epoch start (use
types.CalculateEpochBoundaries(event.DaHeight, s.genesis.DAStartHeight,
epochSize) to get the epoch start) so all blocks from the same epoch verify
against the same DA epoch; keep the safety clamp to not exceed event.DaHeight.
In `@test/e2e/evm_force_inclusion_e2e_test.go`:
- Around line 738-745: The short fixed 1s sleeps after sending SIGTERM to
seqProcess and fnProcess can allow restarts to race with shutdown (syncer's
Stop() uses a 5s drain); replace the sleeps by waiting for the processes to exit
(call seqProcess.Wait() and fnProcess.Wait() and assert no error) or increase
the sleep to >5s so the drain timeout is exceeded before Phase 4 restart; update
the code around seqProcess.Signal(syscall.SIGTERM) and
fnProcess.Signal(syscall.SIGTERM) to perform the chosen wait and failing test on
Wait() errors.
- Around line 863-864: The reconnections reassign seqClient and fnClient so the
earlier defer seqClient.Close() and defer fnClient.Close() close only the
original clients; fix by explicitly closing the previous client before
reassigning (call oldSeq := seqClient; if oldSeq != nil { oldSeq.Close() }
before seqClient = ethclient.Dial(...)) or by using a temporary variable for the
new dial and then replacing the variable and closing the old one, and ensure a
final deferred Close on the last client instance; update both places where
ethclient.Dial is called (references: seqClient, fnClient, ethclient.Dial, and
the existing defer seqClient.Close()/defer fnClient.Close()).
- Around line 753-768: The based sequencer restart call using sut.ExecCmd(ev
mSingleBinaryPath, ...) to start the node as aggregator and based sequencer
(variable basedSeqProcess) omits the required signer passphrase flag; add the
--evnode.signer.passphrase_file flag with the same passphrase file variable used
elsewhere in this test file (the one passed during init/start for other
aggregator/based sequencer runs) so the node can load signing keys when started
with --evnode.node.aggregator=true and --evnode.node.based_sequencer=true.
---
Duplicate comments:
In `@docs/adr/adr-019-forced-inclusion-mechanism.md`:
- Around line 756-757: The NOTE sentence is awkward; replace "P2P nodes only do
not proceed to any verification" with a clearer phrasing such as "P2P-only nodes
do not perform forced inclusion verification." Update the sentence in
adr-019-forced-inclusion-mechanism.md (the NOTE block) to use "P2P-only nodes"
and "perform forced inclusion verification" so the intent is unambiguous.
---
Nitpick comments:
In `@block/internal/syncing/syncer.go`:
- Around line 797-808: The safety clamp inside syncer.go (inside the epoch-gap
handling around nextEpochStart computed from types.CalculateEpochBoundaries
using newState.DAHeight, s.genesis.DAStartHeight and epochSize) can legitimately
trigger when gap is between one and two epochs; update the code in the if
nextEpochStart > event.DaHeight block to (1) add a brief comment explaining this
legitimate scenario (gap == epochSize+1 near epoch boundary), (2) replace or
raise the log level to warn (use processLogger.Warnf or the existing logger)
instead of debug so operators see it, and (3) increment a metric/counter (e.g.,
nextEpochClampCounter or NextEpochClampTotal) to track frequency of this clamp;
keep the clamp assignment nextEpochStart = event.DaHeight as-is for safety.
In `@docs/adr/adr-019-forced-inclusion-mechanism.md`:
- Around line 449-501: The example block under "Sequencer Catch-Up Mode" is
plain text and should be fenced for consistency; wrap the example (the paragraph
starting with "Sequencer offline during epochs 100-150..." through "Normal
operation resumes:") in a ```text fenced code block so it matches the ADR's
other examples and improves readability—update the section containing the
example and ensure the fence surrounds the entire example including the
step-by-step numbered list and the final "Normal operation resumes" lines.
In `@test/e2e/evm_force_inclusion_e2e_test.go`:
- Line 977: The test calls setupCommonEVMEnv with WithFullNode() but never
starts/uses a full node, so remove the unnecessary resource allocation: modify
TestEvmBasedSequencerBaselineE2E to call setupCommonEVMEnv without the
WithFullNode() option; if the test actually needs engine/eth endpoints later,
instead ensure a full node is started and its URLs are propagated (or provide
mocked endpoints) before keeping WithFullNode() — locate the call to
setupCommonEVMEnv and the WithFullNode() symbol to make the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
block/internal/syncing/syncer.godocs/adr/adr-019-forced-inclusion-mechanism.mdtest/e2e/evm_force_inclusion_e2e_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/adr-019-forced-inclusion-mechanism.md`:
- Around line 449-501: The example in the "Sequencer Catch-Up Mode" section has
an off-by-one inconsistency: keep the offline range, counts, and later ranges
aligned by making the missed-epochs count match the stated range. Update the
example so that if the sequencer was offline "epochs 100-150 (5 epochs of 10
blocks each)" then the missed epochs line reads "Missed epochs: 5" and the
catch-up ranges start at 101 and end at 150 (and the exit condition becomes
ErrHeightFromFuture at 151); adjust the restart/latest DA/checkpoint values in
that example text to match these same bounds (references: the "Sequencer
Catch-Up Mode" heading and the example paragraph describing
epochs/checkpoint/latest DA/missed epochs).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
block/internal/syncing/syncer.go (1)
794-818: Add explicit source guard for DA height update to match code patterns elsewhereThe comment on lines 775–776 states this update applies only to DA events, but the code lacks a corresponding
event.Source == common.SourceDAguard. While P2P events fromp2pHandlerdo not setDaHeight(it defaults to 0), making the condition effectively safe, similar DA-specific operations elsewhere in this file (e.g., line 631, line 755) use explicit source guards. For consistency and defensive clarity, add the guard:🔧 Suggested fix
- if event.DaHeight > newState.DAHeight { + if event.Source == common.SourceDA && event.DaHeight > newState.DAHeight {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer.go` around lines 794 - 818, The DA height update block that sets newState.DAHeight based on event.DaHeight should only run for DA-origin events; wrap the existing logic (the check if event.DaHeight > newState.DAHeight, the epochSize/gap logic using s.genesis.DAEpochForcedInclusion and types.CalculateEpochBoundaries, the s.logger.Debug() call and the assignment to newState.DAHeight) in an explicit guard checking event.Source == common.SourceDA so only DA events can change newState.DAHeight, keeping the epoch-start clamping behavior and logging unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/syncing/syncer.go`:
- Line 752: Update the comment text that currently reads "The checks is actually
only performed on DA only enabled nodes." to correct the grammar: change "checks
is" to "check is" so it reads "The check is actually only performed on DA only
enabled nodes." Locate the comment string in syncer.go (the line containing "The
checks is actually only performed on DA only enabled nodes.") and replace it
with the corrected phrase; keep surrounding punctuation and capitalization
unchanged.
---
Nitpick comments:
In `@block/internal/syncing/syncer.go`:
- Around line 794-818: The DA height update block that sets newState.DAHeight
based on event.DaHeight should only run for DA-origin events; wrap the existing
logic (the check if event.DaHeight > newState.DAHeight, the epochSize/gap logic
using s.genesis.DAEpochForcedInclusion and types.CalculateEpochBoundaries, the
s.logger.Debug() call and the assignment to newState.DAHeight) in an explicit
guard checking event.Source == common.SourceDA so only DA events can change
newState.DAHeight, keeping the epoch-start clamping behavior and logging
unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/adr/adr-019-forced-inclusion-mechanism.md (1)
470-493: MD040 is resolved; consider adding a code block fence for consistency with other examples.The previous review comment flagged MD040 (missing language specifier) at line 472 as unaddressed. The current catch-up example (lines 470–493) is formatted as plain prose without a code block fence, which means MD040 no longer applies.
However, all other examples in this document—the Smoothing Example (lines 386–405) and Grace Period Examples (lines 556–616)—are wrapped in fenced code blocks. For consistency, consider wrapping the catch-up example in a
```textcode block:Suggested formatting for consistency
**Example**: +```text Sequencer offline during epochs 100-150 (5 epochs of 10 blocks each) Full nodes (base sequencing) produced blocks with forced txs only Sequencer restarts: 1. Checkpoint DA height: 100 2. Latest DA height: 150 3. Missed epochs: 5 (more than 1) 4. Enter catch-up mode Catch-up process: - Epoch 101-110: Produce blocks with forced txs only, use epoch timestamps - Epoch 111-120: Continue catch-up... - ... - Epoch 141-150: Still catching up - Epoch 151: ErrHeightFromFuture -> exit catch-up mode Normal operation resumes: - Include both forced txs and mempool txs - Use current timestamps +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-019-forced-inclusion-mechanism.md` around lines 470 - 493, Wrap the catch-up example block under the "Catch-up process" / "Sequencer restarts" example in a fenced code block with a language specifier (use ```text) to match the other examples (Smoothing Example, Grace Period Examples) and resolve MD040; ensure the opening fence appears before "Sequencer offline during epochs..." and the closing fence after "Use current timestamps".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/adr/adr-019-forced-inclusion-mechanism.md`:
- Around line 470-493: Wrap the catch-up example block under the "Catch-up
process" / "Sequencer restarts" example in a fenced code block with a language
specifier (use ```text) to match the other examples (Smoothing Example, Grace
Period Examples) and resolve MD040; ensure the opening fence appears before
"Sequencer offline during epochs..." and the closing fence after "Use current
timestamps".
3 participants
Closes: #3016
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Configuration
Tests