Repository navigation
op-node: do not clobber persisted sequencer state on Init - #23332
Tyagiquamar wants to merge 1 commit into
Conversation
Sequencer Init with sequencing stopped no longer writes SequencerStopped to admin persistence, so a prior admin_startSequencer survives restart. Fixes ethereum-optimism#12854
|
/ci authorize f159506 |
| } | ||
| s.metrics.SetSequencerState(false) | ||
| s.active.Store(false) | ||
| // Do not persist on startup: admin state was loaded via LoadPersisted before Init. |
There was a problem hiding this comment.
Claude: On develop, I don't think #12854 happens unless op-conductor is enabled, and with conductor this change does harm.
LoadPersisted (op-node/config/config.go:93) runs before Init and replaces SequencerStopped with the saved value. A saved started therefore already leads to Init(true). The e2e test TestLoadSequencerStateOnStarted_Started (op-e2e/system/conductor/system_adminrpc_test.go) covers exactly this scenario (saved started plus --sequencer.stopped) and expects the node to start sequencing. v1.9.5 loads config in the same order.
The only way to reach Init(false) with started on disk is the conductor override in op-node/service.go:129-132. There, conductor owns the state, and today's overwrite keeps the file honest. With this change, a node that was once leader keeps started on disk through every restart. If an operator later turns conductor off, that node starts sequencing on boot with no leader check, which risks two sequencers producing conflicting unsafe blocks.
Could you confirm whether the reporter had conductor enabled? If there's a real gap, I think it belongs in the config/override layer rather than in Init.
| s.metrics.SetSequencerState(false) | ||
| s.active.Store(false) | ||
| // Do not persist on startup: admin state was loaded via LoadPersisted before Init. | ||
| return nil |
There was a problem hiding this comment.
Claude: This also drops a deliberate startup check that the state file can be written. Before #10991, startup persisted the initial state on purpose: "This ensures persistence can write the state correctly and that the state file exists."
Without it, a bad --rpc.admin-state path only shows up when admin_startSequencer runs. With conductor, every node boots stopped, so that failure would hit during a leader handover.
It also changes behaviour: a first boot with --sequencer.stopped no longer creates {"sequencerStarted":false}. A later restart without the flag would then start sequencing instead of honouring the earlier stopped state. Starting is still persisted through forceStart, so startup handling is now inconsistent between the two cases.
| return nil | ||
| } | ||
| s.metrics.SetSequencerState(false) | ||
| s.active.Store(false) |
There was a problem hiding this comment.
Claude: nit: this does nothing. Init runs once at startup and active already starts as false.
| return os.WriteFile(l.file, []byte(`{"sequencerStarted":false}`), 0o644) | ||
| } | ||
|
|
||
| func TestInitStoppedDoesNotOverwritePersistedStartedState(t *testing.T) { |
There was a problem hiding this comment.
Claude: This test uses a fake listener that writes its own JSON file and never goes through LoadPersisted, ActiveConfigPersistence or the service.go setup. It shows that the SequencerStopped() call was removed (it fails on develop with expected: 0, actual: 1), but not that a restart keeps state. A real regression test would sit next to the existing TestLoadSequencerStateOnStarted_* e2e tests.
| func TestInitStoppedDoesNotOverwritePersistedStartedState(t *testing.T) { | ||
| log := testlog.Logger(t, log.LevelInfo) | ||
| dir := t.TempDir() | ||
| stateFile := dir + "/state" |
There was a problem hiding this comment.
Claude: nit: filepath.Join(dir, "state")
|
Verified on current develop (02ca4b5). I am not pushing a further Init change. The reported bug does not reproduce on develop. LoadPersisted (op-node/config/config.go:93-104) runs before Init and replaces SequencerStopped with the saved value. A saved sequencerStarted:true therefore reaches Init(true) from driver.Start (op-node/rollup/driver/driver.go:195), and forceStart writes started again. TestLoadSequencerStateOnStarted_Started covers saved started plus SequencerStopped=true and expects the sequencer to already be running. Conductor does not clobber a saved started file either. service.go:129-132 forces SequencerStopped=true after LoadPersisted, but Config.Check (op-node/config/config.go:156-159) then refuses startup when conductor is enabled and the admin state file is not unset. Init never runs, so the file stays as it was and the process exits in config check. The Init(false) SequencerStopped() call is still the startup write. driver.Start reports a failure there as "persist initial sequencer state". Removing it means a bad --rpc.admin-state path shows up only on a later start or stop, and a first boot with --sequencer.stopped no longer creates sequencerStarted:false. A later restart without that flag would start sequencing. That is why I am leaving Init as it is on develop. |
|
Thanks @Tyagiquamar for checking this so carefully. You're right about the |
2 participants
Summary
SequencerStopped()fromSequencer.Initwhen the node starts with sequencing inactive; startup now only syncs in-memory state.Init(..., false)does not rewrite persistedsequencerStarted: true.Fixes #12854
Test plan
golang:1.26-bookwormfrom repo root:go test -count=1 -run TestInitStoppedDoesNotOverwritePersistedStartedState ./op-node/rollup/sequencing/ -v(pass, 0.250s)golang:1.26-bookworm:gofmt -lon changed files (clean)