fix: Publisher-mode synchronization option for failover scenario#3222
fix: Publisher-mode synchronization option for failover scenario#3222julienrbrt merged 7 commits intomainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a publisher-mode startup path for sync services during Raft leader failover, refactors syncer lifecycle/status APIs and sync service startup/retry, adjusts Raft startup/election semantics and recovery, improves E2E P2P identities and logging, enables local Go module replaces, and updates docs and changelog. Changes
Sequence Diagram(s)sequenceDiagram
participant Failover as Failover Manager
participant Raft as Raft Node
participant Store as Block Store
participant Sync as Sync Service
participant P2P as P2P Init
Note over Failover,Raft: failover / startup decision
Failover->>Raft: query leader status & raft config
Raft-->>Failover: leader? / persisted config?
alt Leader & stores empty
Failover->>Store: read header/data heights
Store-->>Failover: heights empty
Failover->>Sync: StartForPublishing(ctx)
Sync->>Sync: prepareStart (create syncer, startSubscriber)
Sync->>P2P: startSubscriber (no blocking init)
Sync-->>Failover: publishing started
else Normal startup
Failover->>Sync: Start(ctx)
Sync->>P2P: initFromP2POnce
loop retry until started or ctx.Done
P2P-->>Sync: success/fail
Sync->>Sync: startSyncer if needed
end
Sync-->>Failover: normal started
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 1m 49s —— View job PR Review: Publisher-mode Synchronization for Failover
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3222 +/- ##
==========================================
+ Coverage 61.45% 61.67% +0.22%
==========================================
Files 120 120
Lines 12504 12635 +131
==========================================
+ Hits 7684 7793 +109
- Misses 3959 3968 +9
- Partials 861 874 +13
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:
|
|
alpe
force-pushed
the
alex/sync_race
branch
2 times, most recently
from
ec0ffc4 to
59dc917
Compare
alpe
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/e2e/sut_helper.go (1)
189-195:⚠️ Potential issue | 🟡 MinorClose the per-process logfile handle.
os.Createon Line 192 leaves an open fd that is never closed. E2E suites spawn a lot of processes, so this can leak descriptors and delay flushing the captured logs.Suggested change
logfile, err := os.Create(logfileName) require.NoError(s.t, err) + s.t.Cleanup(func() { _ = logfile.Close() }) errReader = io.NopCloser(io.TeeReader(errReader, logfile)) outReader = io.NopCloser(io.TeeReader(outReader, logfile))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/sut_helper.go` around lines 189 - 195, The per-process logfile created by os.Create (logfile) in the block that checks s.processLogDir() is never closed; wrap the logfile so it is closed when the returned readers are closed by replacing the io.NopCloser(io.TeeReader(..., logfile)) usage with a ReadCloser that closes the underlying logfile on Close, or otherwise ensure logfile.Close() is called (e.g., return a combined io.ReadCloser that delegates Read to the TeeReader and Close to logfile.Close) for both errReader and outReader so the file descriptor is released and buffered data flushed.block/internal/syncing/syncer.go (1)
1204-1213:⚠️ Potential issue | 🔴 CriticalBootstrap the same genesis state as
initializeState().This fallback only sets
ChainID,InitialHeight, andLastBlockHeight, but Line 808 later executes the recovered block againstcurrentState.AppHash. On a fresh node that means raft recovery runs against a different execution baseline than normal startup, which callsexec.InitChainand seedsAppHash,DAHeight, andLastBlockTime. Please reuse that bootstrap path here before applying the raft block.🤖 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 1204 - 1213, The fallback after s.store.GetState should bootstrap the same full genesis state as initializeState() instead of only setting ChainID/InitialHeight/LastBlockHeight; update the recovery path that sets currentState and stateBootstrapped to call the same initialization logic (e.g., run the InitChain/bootstrap sequence used in initializeState) so that currentState.AppHash, DAHeight, LastBlockTime (and any other genesis-seeded fields) are populated before the raft/recovered block is executed; ensure you invoke the same executor/init routine (the InitChain or genesis bootstrap used by initializeState) rather than manually setting only those three fields.pkg/sync/sync_service.go (1)
190-205:⚠️ Potential issue | 🟠 MajorUnwind partially started components on startup errors.
By the time Line 233 returns from
setupP2PInfrastructure, the store, exchange server, and exchange can already be running. IfnewSyncerfails here — orstartSubscriberfails in the new publisher-mode path — these methods return without tearing those pieces down, so a failed start leaks live components into the process.As per coding guidelines, "Be mindful of goroutine leaks in Go code".
Also applies to: 219-225, 231-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/sync_service.go` around lines 190 - 205, The startup may leave store/exchange/exchange-server running if later steps fail (e.g. newSyncer or startSubscriber); update the startup sequence (functions prepareStart / setupP2PInfrastructure / initFromP2PWithRetry) to register and invoke cleanup on error: after setupP2PInfrastructure returns and before calling newSyncer or startSubscriber, capture the created components (store, exchangeServer, exchange or any returned tearDown/Close funcs) and ensure they are stopped when a subsequent error occurs (use a deferred cleanup or explicit teardown call on error paths in the block around prepareStart -> initFromP2PWithRetry -> newSyncer/startSubscriber). Reference symbols to change: prepareStart, setupP2PInfrastructure, initFromP2PWithRetry, newSyncer, and startSubscriber — ensure each path that returns an error calls the appropriate stop/Close/Shutdown methods for the started components to avoid goroutine/resource leaks.
🧹 Nitpick comments (1)
pkg/sync/sync_service_test.go (1)
63-97: Assert remote peer delivery, not just local initialization.This test pays the cost of bringing up
client2, but the only postcondition issvc.storeInitialized. A regression whereStartForPublishinginitializes locally but never broadcasts to peers would still pass. Please add an assertion on peer 2's view of the header so the new publisher-mode path is actually covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/sync_service_test.go` around lines 63 - 97, The test currently only asserts svc.storeInitialized after broadcasting; add an assertion that the remote peer (client2) actually receives the broadcasted header: before calling svc.WriteToStoreAndBroadcast, register or subscribe a handler on client2 to capture incoming P2PSignedHeader messages (using client2's subscribe/handler API), then after WriteToStoreAndBroadcast use require.Eventually to poll/assert that the handler received a header equal to signedHeader (or matching height/DataHash/AppHash). Reference client1/client2, NewHeaderSyncService, StartForPublishing, WriteToStoreAndBroadcast and svc.storeInitialized when locating where to add the subscription and the eventual assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/evm/go.mod`:
- Around line 5-8: Remove the repo-local replace directives from the module
manifest (the replace block that points github.com/evstack/ev-node and
github.com/evstack/ev-node/execution/evm to relative ../../ paths) so the
published module no longer contains repository-specific local wiring; keep the
go.mod clean for downstream consumers and, if local development linking is
needed, use a go.work file or temporary local replace only in your working copy
before publishing.
In `@block/internal/syncing/syncer.go`:
- Around line 1230-1235: The retry uses the identical currentState so
errInvalidState from AssertValidForNextState will always recur; before retrying
the call to trySyncNextBlockWithState, refresh the snapshot/state (e.g. read
s.GetLastState() or obtain the latest snapshot used by AssertValidForNextState)
and pass that refreshed state into trySyncNextBlockWithState instead of reusing
the original currentState, or recompute/validate the next state after calling
s.SetLastState(currentState) so the second call sees the updated global state.
In `@CHANGELOG.md`:
- Around line 12-21: The changelog contains unresolved git conflict markers
(<<<<<<<, =======, >>>>>>>) — remove those markers and produce a single resolved
section (e.g., keep "### Added") combining both bullet entries into the final
list so both PRs stay: the publisher-mode synchronization line [`#3222`] and the
"Improve execution/evm check..." line [`#3221`]; ensure only one header remains
and that the PR links and bullets are intact and properly formatted.
In `@docs/guides/raft_production.md`:
- Around line 93-100: Update the example to use the canonical aggregator flag
and drop the legacy bootstrap flag: replace the --rollkit.node.aggregator=true
occurrence with --evnode.node.aggregator=true, and remove the
--evnode.raft.bootstrap=true line (only keep raft-related flags like
--evnode.raft.enable, --evnode.raft.node_id, --evnode.raft.raft_addr,
--evnode.raft.raft_dir, and --evnode.raft.peers). Ensure the rest of the example
(e.g., --rollkit.p2p.listen_address) stays unchanged unless also intended to be
canonicalized.
In `@node/failover.go`:
- Around line 193-209: The gating logic ignores storeHeight when deciding to
start in publisher mode; change the condition that currently checks headerHeight
and dataHeight to include storeHeight so publishing only starts when the main
block store and both sync stores are empty. In other words, adjust the boolean
check around headerSyncService.Store().Height() and
dataSyncService.Store().Height() (the if that returns false) to also consider
the local storeHeight variable so StartForPublishing is only taken when
storeHeight == 0 && headerHeight == 0 && dataHeight == 0.
In `@pkg/raft/election.go`:
- Around line 93-96: The wait here uses d.node.waitForMsgsLanded which
internally uses context.Background(), so follower startup isn’t cancellable;
update this call to use the Run cancellation context (e.g., pass the existing
ctx) and add a ctx-aware variant on the node (e.g., waitForMsgsLandedCtx(ctx,
timeout) or change waitForMsgsLanded to accept a context) so the call in
election.go uses that context and honors cancellation/role changes; modify the
node implementation (pkg/raft/node.go – waitForMsgsLanded) to accept and
propagate the provided context instead of creating context.Background().
---
Outside diff comments:
In `@block/internal/syncing/syncer.go`:
- Around line 1204-1213: The fallback after s.store.GetState should bootstrap
the same full genesis state as initializeState() instead of only setting
ChainID/InitialHeight/LastBlockHeight; update the recovery path that sets
currentState and stateBootstrapped to call the same initialization logic (e.g.,
run the InitChain/bootstrap sequence used in initializeState) so that
currentState.AppHash, DAHeight, LastBlockTime (and any other genesis-seeded
fields) are populated before the raft/recovered block is executed; ensure you
invoke the same executor/init routine (the InitChain or genesis bootstrap used
by initializeState) rather than manually setting only those three fields.
In `@pkg/sync/sync_service.go`:
- Around line 190-205: The startup may leave store/exchange/exchange-server
running if later steps fail (e.g. newSyncer or startSubscriber); update the
startup sequence (functions prepareStart / setupP2PInfrastructure /
initFromP2PWithRetry) to register and invoke cleanup on error: after
setupP2PInfrastructure returns and before calling newSyncer or startSubscriber,
capture the created components (store, exchangeServer, exchange or any returned
tearDown/Close funcs) and ensure they are stopped when a subsequent error occurs
(use a deferred cleanup or explicit teardown call on error paths in the block
around prepareStart -> initFromP2PWithRetry -> newSyncer/startSubscriber).
Reference symbols to change: prepareStart, setupP2PInfrastructure,
initFromP2PWithRetry, newSyncer, and startSubscriber — ensure each path that
returns an error calls the appropriate stop/Close/Shutdown methods for the
started components to avoid goroutine/resource leaks.
In `@test/e2e/sut_helper.go`:
- Around line 189-195: The per-process logfile created by os.Create (logfile) in
the block that checks s.processLogDir() is never closed; wrap the logfile so it
is closed when the returned readers are closed by replacing the
io.NopCloser(io.TeeReader(..., logfile)) usage with a ReadCloser that closes the
underlying logfile on Close, or otherwise ensure logfile.Close() is called
(e.g., return a combined io.ReadCloser that delegates Read to the TeeReader and
Close to logfile.Close) for both errReader and outReader so the file descriptor
is released and buffered data flushed.
---
Nitpick comments:
In `@pkg/sync/sync_service_test.go`:
- Around line 63-97: The test currently only asserts svc.storeInitialized after
broadcasting; add an assertion that the remote peer (client2) actually receives
the broadcasted header: before calling svc.WriteToStoreAndBroadcast, register or
subscribe a handler on client2 to capture incoming P2PSignedHeader messages
(using client2's subscribe/handler API), then after WriteToStoreAndBroadcast use
require.Eventually to poll/assert that the handler received a header equal to
signedHeader (or matching height/DataHash/AppHash). Reference client1/client2,
NewHeaderSyncService, StartForPublishing, WriteToStoreAndBroadcast and
svc.storeInitialized when locating where to add the subscription and the
eventual assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b5476c3-9ec8-4d67-a4cb-fcde2300edbd
⛔ Files ignored due to path filters (1)
apps/evm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
CHANGELOG.mdapps/evm/go.modblock/internal/syncing/syncer.goblock/internal/syncing/syncer_test.godocs/guides/raft_production.mddocs/learn/config.mdnode/failover.gopkg/config/config.gopkg/config/config_test.gopkg/raft/election.gopkg/raft/election_test.gopkg/raft/node.gopkg/raft/node_test.gopkg/sync/sync_service.gopkg/sync/sync_service_test.gopkg/sync/syncer_status.gopkg/sync/syncer_status_test.gotest/e2e/failover_e2e_test.gotest/e2e/sut_helper.go
block/internal/syncing/syncer.go
Outdated
| err := s.trySyncNextBlockWithState(ctx, event, currentState) | ||
| if err != nil && stateBootstrapped && errors.Is(err, errInvalidState) { | ||
| s.logger.Debug().Err(err).Msg("raft recovery failed after bootstrap state init, retrying once") | ||
| // Keep strict validation semantics; this retry only guards startup ordering races. | ||
| s.SetLastState(currentState) | ||
| err = s.trySyncNextBlockWithState(ctx, event, currentState) |
There was a problem hiding this comment.
The retry cannot clear errInvalidState.
Lines 719-727 validate against the currentState argument, and Line 1235 passes the same snapshot again. s.SetLastState(currentState) only updates the global pointer, so an errInvalidState from AssertValidForNextState will reproduce deterministically on the retry.
🤖 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 1230 - 1235, The retry uses
the identical currentState so errInvalidState from AssertValidForNextState will
always recur; before retrying the call to trySyncNextBlockWithState, refresh the
snapshot/state (e.g. read s.GetLastState() or obtain the latest snapshot used by
AssertValidForNextState) and pass that refreshed state into
trySyncNextBlockWithState instead of reusing the original currentState, or
recompute/validate the next state after calling s.SetLastState(currentState) so
the second call sees the updated global state.
| if err = d.node.waitForMsgsLanded(d.node.Config().SendTimeout); err != nil { | ||
| // this wait can legitimately time out | ||
| d.logger.Debug().Err(err).Msg("timed out waiting for raft messages before follower verification; continuing") | ||
| } |
There was a problem hiding this comment.
Keep follower startup cancellable.
This new wait path ignores Run cancellation because pkg/raft/node.go:145-175 implements waitForMsgsLanded with its own context.Background() timeout. On shutdown or role changes, follower startup can now sit here for up to SendTimeout. A ctx-aware variant would keep this path responsive. As per coding guidelines, "Use context.Context for cancellation in Go".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/raft/election.go` around lines 93 - 96, The wait here uses
d.node.waitForMsgsLanded which internally uses context.Background(), so follower
startup isn’t cancellable; update this call to use the Run cancellation context
(e.g., pass the existing ctx) and add a ctx-aware variant on the node (e.g.,
waitForMsgsLandedCtx(ctx, timeout) or change waitForMsgsLanded to accept a
context) so the call in election.go uses that context and honors
cancellation/role changes; modify the node implementation (pkg/raft/node.go –
waitForMsgsLanded) to accept and propagate the provided context instead of
creating context.Background().
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/guides/raft_production.md (1)
93-100:⚠️ Potential issue | 🟡 MinorRemove the bootstrap flag from the example command.
The documentation now describes
--evnode.raft.bootstrapas a compatibility-only flag (line 36) and states that "startup mode is selected automatically from persisted raft configuration state" (lines 60-62). However, the example still includes--evnode.raft.bootstrap=trueon line 98, making the configuration appear more complex than necessary and contradicting the "optional" guidance.For the primary/happy-path example, omit the bootstrap flag to demonstrate the recommended automatic mode detection.
✂️ Proposed fix
--evnode.raft.enable=true \ --evnode.raft.node_id="node-1" \ --evnode.raft.raft_addr="0.0.0.0:5001" \ --evnode.raft.raft_dir="/var/lib/ev-node/raft" \ - --evnode.raft.bootstrap=true \ --evnode.raft.peers="node-1@10.0.1.1:5001,node-2@10.0.1.2:5001,node-3@10.0.1.3:5001" \ --evnode.p2p.listen_address="/ip4/0.0.0.0/tcp/26656" \As per coding guidelines,
docs/guides/**/*.md: Structure guides with clear step-by-step instructions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/raft_production.md` around lines 93 - 100, The example command still includes the compatibility-only flag --evnode.raft.bootstrap=true which contradicts the guide text that startup mode is selected automatically; remove the --evnode.raft.bootstrap option from the example lines (the stanza containing --evnode.raft.enable, --evnode.raft.node_id, --evnode.raft.raft_addr, --evnode.raft.raft_dir, --evnode.raft.peers, and --evnode.p2p.listen_address) so the primary/happy-path example demonstrates automatic mode detection without the bootstrap flag.
🧹 Nitpick comments (2)
docs/guides/raft_production.md (2)
58-67: Fix markdown formatting issues.Static analysis flagged several markdown formatting violations in this section:
- Line 58: Missing blank line above the heading
- Lines 60-66: Inconsistent spacing after list markers (3 spaces instead of 1)
These don't affect the rendered output in most viewers but should be fixed for consistency and linter compliance.
🔧 Proposed formatting fixes
### 1. Static Peering & Automatic Startup Mode Use static peering with automatic mode selection from local raft configuration: -* If local raft configuration already exists in `--evnode.raft.raft_dir`, the node starts in rejoin mode. -* If no local raft configuration exists yet, the node bootstraps from configured peers. -* `--evnode.raft.bootstrap` is retained for compatibility but does not control mode selection. -* **All configured cluster members** should list the full set of peers in `--evnode.raft.peers`. +* If local raft configuration already exists in `--evnode.raft.raft_dir`, the node starts in rejoin mode. +* If no local raft configuration exists yet, the node bootstraps from configured peers. +* `--evnode.raft.bootstrap` is retained for compatibility but does not control mode selection. +* **All configured cluster members** should list the full set of peers in `--evnode.raft.peers`. * The `peers` list format is strict: `NodeID@Host:Port`. -* **Limitation**: Dynamic addition of peers (run-time membership changes) via RPC/CLI is not currently exposed. -* **Not supported**: Joining an existing cluster as a brand-new node that was not part of the initial static membership. +* **Limitation**: Dynamic addition of peers (run-time membership changes) via RPC/CLI is not currently exposed. +* **Not supported**: Joining an existing cluster as a brand-new node that was not part of the initial static membership.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/raft_production.md` around lines 58 - 67, Add a blank line before the "### 1. Static Peering & Automatic Startup Mode" heading and normalize the list bullets to use a single space after each marker and consistent single-space indentation; update the list items referencing flags like --evnode.raft.raft_dir, --evnode.raft.bootstrap, --evnode.raft.peers and the peers format `NodeID@Host:Port` so each bullet begins with "- " (dash + one space) and remove extra spaces after markers to satisfy markdown linter expectations.
36-36: Clarify what "compatibility flag" means.The description states the bootstrap flag is for "compatibility" but doesn't explain with what or whether it has any effect. Consider being explicit: does the flag still influence behavior, or is it completely ignored? If it's retained only for backward config-file compatibility (parsed but unused), state that clearly.
📝 Suggested clarification
-| `--evnode.raft.bootstrap` | `raft.bootstrap` | Compatibility flag. Startup mode is selected automatically from persisted raft configuration state. | optional | +| `--evnode.raft.bootstrap` | `raft.bootstrap` | Legacy flag (parsed but unused). Startup mode is selected automatically from persisted raft configuration state. | omit |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/raft_production.md` at line 36, The table entry for `--evnode.raft.bootstrap` / `raft.bootstrap` is vague about "compatibility flag"; update the description to explicitly state its current behavior (e.g., if it is retained only for backward compatibility and parsed but ignored at runtime, change the text to "Deprecated compatibility flag — parsed for backward compatibility but ignored; startup mode is selected automatically from persisted Raft configuration state"; if it still affects startup behavior, state that and describe the exact effect). Make the change to the table cell containing `--evnode.raft.bootstrap` / `raft.bootstrap` so readers know whether the flag is effective or merely retained for legacy config parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/guides/raft_production.md`:
- Around line 93-100: The example command still includes the compatibility-only
flag --evnode.raft.bootstrap=true which contradicts the guide text that startup
mode is selected automatically; remove the --evnode.raft.bootstrap option from
the example lines (the stanza containing --evnode.raft.enable,
--evnode.raft.node_id, --evnode.raft.raft_addr, --evnode.raft.raft_dir,
--evnode.raft.peers, and --evnode.p2p.listen_address) so the primary/happy-path
example demonstrates automatic mode detection without the bootstrap flag.
---
Nitpick comments:
In `@docs/guides/raft_production.md`:
- Around line 58-67: Add a blank line before the "### 1. Static Peering &
Automatic Startup Mode" heading and normalize the list bullets to use a single
space after each marker and consistent single-space indentation; update the list
items referencing flags like --evnode.raft.raft_dir, --evnode.raft.bootstrap,
--evnode.raft.peers and the peers format `NodeID@Host:Port` so each bullet
begins with "- " (dash + one space) and remove extra spaces after markers to
satisfy markdown linter expectations.
- Line 36: The table entry for `--evnode.raft.bootstrap` / `raft.bootstrap` is
vague about "compatibility flag"; update the description to explicitly state its
current behavior (e.g., if it is retained only for backward compatibility and
parsed but ignored at runtime, change the text to "Deprecated compatibility flag
— parsed for backward compatibility but ignored; startup mode is selected
automatically from persisted Raft configuration state"; if it still affects
startup behavior, state that and describe the exact effect). Make the change to
the table cell containing `--evnode.raft.bootstrap` / `raft.bootstrap` so
readers know whether the flag is effective or merely retained for legacy config
parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b93e6a52-29c2-4bb1-a32e-10cacf79f60b
📒 Files selected for processing (1)
docs/guides/raft_production.md
| Source: "", | ||
| }) | ||
| } | ||
| err := s.trySyncNextBlockWithState(ctx, event, currentState) |
There was a problem hiding this comment.
this change is effectivelly to use the correct state when we override it at L1207 right?
There was a problem hiding this comment.
yes, before it was re-read in the method and could be empty
| func (syncService *SyncService[H]) startSyncer(ctx context.Context) error { | ||
| if syncService.syncerStatus.isStarted() { | ||
| return nil | ||
| // StartForPublishing starts the sync service in publisher mode. |
There was a problem hiding this comment.
Trying to wrap my head around this. Why is it exactly needed? A node can fetch block 1 over DA and p2p 🤔.
Or is it because the connection happens before DA fetching, effectively deadlocking it?
There was a problem hiding this comment.
There was a race that could lead to a deadlock:
- P2P init waits for block 1 from peers.
- Block 1 production waits for P2P init to finish.
There was a problem hiding this comment.
Ok. I think in all cases we shouldn't need to wait for p2p init to start when we have DA. So we can probably simplify this to the later
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 `@apps/evm/go.mod`:
- Around line 5-8: Add the same replace directive used by apps/evm to the
execution/evm module so it uses the local github.com/evstack/ev-node source
instead of the v1.0.0 registry version: edit the execution/evm module's go.mod
and add a replace entry mapping github.com/evstack/ev-node to the local ../../
path (and similarly map github.com/evstack/ev-node/execution/evm to
../../execution/evm if needed) so both modules resolve the same local codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8102abcd-d222-49a2-bcda-27bfca7d4cf5
⛔ Files ignored due to path filters (1)
apps/evm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
apps/evm/go.mod
| replace ( | ||
| github.com/evstack/ev-node => ../../ | ||
| github.com/evstack/ev-node/execution/evm => ../../execution/evm | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if execution/evm has local replace directives or uses registry versions
echo "=== Checking execution/evm/go.mod for replace directives ==="
rg -n "^replace" execution/evm/go.mod
echo ""
echo "=== Checking what version of ev-node execution/evm requires ==="
rg -n "github.com/evstack/ev-node" execution/evm/go.mod | head -5
echo ""
echo "=== Checking for active go.work file that might reconcile versions ==="
fd -H '^go\.work$' . --max-depth 1Repository: evstack/ev-node
Length of output: 385
Add replace directive to execution/evm/go.mod for version consistency.
execution/evm/go.mod explicitly requires github.com/evstack/ev-node v1.0.0 from the registry (line 7), but apps/evm/go.mod replaces it with the local ../../ path. To avoid implicit version inconsistency and ensure that execution/evm explicitly uses the same local ev-node code as apps/evm, add the matching replace directive to execution/evm/go.mod.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/evm/go.mod` around lines 5 - 8, Add the same replace directive used by
apps/evm to the execution/evm module so it uses the local
github.com/evstack/ev-node source instead of the v1.0.0 registry version: edit
the execution/evm module's go.mod and add a replace entry mapping
github.com/evstack/ev-node to the local ../../ path (and similarly map
github.com/evstack/ev-node/execution/evm to ../../execution/evm if needed) so
both modules resolve the same local codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/sync/sync_service.go`:
- Line 428: The Stop method currently calls
syncService.topicSubscription.Cancel() which can nil-panics if Start failed
before startSubscriber/prepareStart completed; add a nil guard in Stop so it
only calls Cancel when syncService.topicSubscription != nil (and similarly
protect any other dereferences of topicSubscription), ensuring Start/Stop handle
partial startup without panicking; locate symbols Stop, Start, startSubscriber,
prepareStart, and topicSubscription to apply the check.
- Around line 304-318: startSyncer currently forwards the caller's request
context into syncer.Start, which can cause the syncer startup to be cancelled by
request timeouts; modify startSyncer so the function uses a service-scoped
background context when invoking syncer.Start (instead of the incoming ctx) —
e.g., use an existing SyncService field like syncService.ctx or create a
detached context with context.WithCancel tied to the service lifecycle, and pass
that into the closure given to syncerStatus.startOnce when calling syncer.Start;
keep the startOnce usage and error handling the same but ensure the syncer.Start
call is insulated from request cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd1d47db-74de-45a4-b680-de1b903700cf
📒 Files selected for processing (3)
node/failover.gopkg/sync/sync_service.gopkg/sync/sync_service_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- node/failover.go
- pkg/sync/sync_service_test.go
| syncService.wg.Wait() | ||
|
|
||
| // unsubscribe from topic first so that sub.Stop() does not fail | ||
| syncService.topicSubscription.Cancel() |
There was a problem hiding this comment.
Potential nil dereference if Stop called after partial Start failure.
If Start() fails before startSubscriber() completes (e.g., during prepareStart()), then topicSubscription will be nil. Calling Cancel() on nil will panic.
🛡️ Add nil guard
- syncService.topicSubscription.Cancel()
+ if syncService.topicSubscription != nil {
+ syncService.topicSubscription.Cancel()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| syncService.topicSubscription.Cancel() | |
| if syncService.topicSubscription != nil { | |
| syncService.topicSubscription.Cancel() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sync/sync_service.go` at line 428, The Stop method currently calls
syncService.topicSubscription.Cancel() which can nil-panics if Start failed
before startSubscriber/prepareStart completed; add a nil guard in Stop so it
only calls Cancel when syncService.topicSubscription != nil (and similarly
protect any other dereferences of topicSubscription), ensuring Start/Stop handle
partial startup without panicking; locate symbols Stop, Start, startSubscriber,
prepareStart, and topicSubscription to apply the check.
julienrbrt
force-pushed
the
alex/sync_race
branch
from
e817ab6 to
1b8da2e
Compare
2 participants
Overview
E2E HA tests fail sometimes on a race when the leader is waiting for p2p sync complete on a fresh start.
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
Chores