Add freeze mode for historical EVM RPC - #3910
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3910 +/- ##
==========================================
- Coverage 59.47% 58.47% -1.00%
==========================================
Files 2323 2227 -96
Lines 198389 187864 -10525
==========================================
- Hits 117982 109847 -8135
+ Misses 69198 67641 -1557
+ Partials 11209 10376 -833
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview Configuration adds Block sync stops applying blocks before the freeze height and hands off without auto-remediation past the boundary. Consensus enters a frozen path: no new rounds, WAL catchup/double-sign checks skipped when frozen, Node startup rejects freeze if app/state/block store already reached H, disables state sync when freeze is on, and blocks Autobahn and seed mode. Tests and config fuzz/golden coverage were extended for the new key. Reviewed by Cursor Bugbot for commit 0a8aaab. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Freeze mode is implemented soundly: the boundary is enforced at both the blocksync handoff and the consensus updateToState transition, the startup guards (app/state/block-store height, initial height, MaxInt64) correctly prevent the handshaker from ever replaying the freeze block, and forcing stateSync = false before the blocksync reactor is constructed keeps BlockSync/waitSync consistent. No blocking defects found; the notes below are a dead-in-production eager freeze check, a silent state-sync downgrade, a stale characterization doc, and a gap in blocksync test coverage.
Findings: 0 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Operator note is slightly optimistic: while frozen the mempool is never
Updated, so gossiped and submitted txs accumulate and are never purged (this fork has nopurgeExpiredTxsoutsideUpdate). Oncemempool.sizeis reached,CheckTxrejects everything, so "transaction submission remains available" holds only until the mempool fills. Worth stating in the PR/operator docs. - No test covers the incompatible-configuration rejections that the PR description lists (grpc-only, seed mode, Autobahn).
TestValidateFreezeHeightcovers only the height guards, andTestWithFreezeHeightonly the option plumbing. - While frozen,
enterNewRoundreturns before the tracing bookkeeping, socs.heightSpanfor the last executed height is neverEnd()ed. Only one dangling span, and only when tracing is enabled — noting for completeness rather than as something to fix. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-cosmos/server/start.go:319discardsconfig.ValidateBasic's error and logs a fixed "minimum-gas-prices is empty" warning regardless of which check actually failed, so the pruning-everything + state-sync-snapshots rejection is effectively unenforced at startup. This is why the PR has to callValidateFreeze()separately at line 311; the underlying swallowed error predates this PR.
| // It must be called before the state starts. | ||
| func (cs *State) SetFreezeHeight(height uint64) { | ||
| cs.freezeHeight = height | ||
| cs.markFrozen(nextHeightForState(cs.state), cs.state.LastBlockHeight) |
There was a problem hiding this comment.
[suggestion] This eager markFrozen never fires at the only production call site. makeNode calls SetFreezeHeight immediately after NewState, and NewState leaves cs.state as the zero sm.State — so nextHeightForState(cs.state) is 0 (LastBlockHeight+1 == 1 → InitialHeight, which is 0), and markFrozen returns at the uint64(nextHeight) < cs.freezeHeight guard every time.
Startup freezing actually works through Run() → updateStateFromStore() → updateToState() → markFrozen(), which is reached before the cs.frozen.Load() checks in Run, so behavior is correct. But the consensus test helper calls updateStateFromStore() before SetFreezeHeight (common_test.go:536), so TestStateFreezesAfterTargetBlock exercises this call under an initialization order production never uses — a regression here would not be caught.
Either drop the call and let updateToState be the single choke point (matching the guard-at-the-choke-point rule in AGENTS.md), or keep it and reword the godoc, which currently reads as though this is what arms the freeze at startup.
|
|
||
| // Determine whether we should attempt state sync. | ||
| stateSync := cfg.StateSync.Enable && !onlyValidatorIsUs(state, pubKey) | ||
| if stateSync && opts.freezeHeight > 0 { |
There was a problem hiding this comment.
[suggestion] Every other incompatible configuration in this PR is rejected (halt-height/halt-time, grpc-only, seed mode, Autobahn), but [statesync] enable = true is silently downgraded at Info level. Note this branch only matters for a node with no local state — an existing node is already caught by the state.LastBlockHeight > 0 check below — which is exactly the case where the fallback is most surprising: the operator gets a sync from genesis instead of a snapshot restore, discoverable only from one log line.
Suggest rejecting with an error for consistency with the other mode checks (or at minimum logging at warn/error), so freeze-height + state sync fails fast rather than after hours of syncing.
|
|
||
| consensus.Global.RecordConsMetrics(first) | ||
| blocksSynced++ | ||
| if handoff, frozen := s.frozenHandoff(pool, state, blocksSynced, stateSynced); frozen { |
There was a problem hiding this comment.
[suggestion] This is the call site that matters operationally — stopping an in-flight sync exactly one block before the boundary — and it is untested. TestPoolRoutineHandsOffAtFreezeHeight only reaches the entry-guard at line 471 (it passes a state already at freezeHeight-1, so poolRoutine returns before the loop and never touches blockExec/store).
A test that applies a block and asserts the handoff carries blocksSynced == 1 with state.LastBlockHeight == freezeHeight-1 would also pin the off-by-one that the whole feature rests on (freeze before executing freezeHeight, not after).
| "cannot enable state sync snapshots with '%s' pruning setting", storetypes.PruningOptionEverything, | ||
| ) | ||
| } | ||
| return c.ValidateFreeze() |
There was a problem hiding this comment.
[nit] This adds two more rejection conditions to ValidateBasic, which makes FuzzConfigValidateBasic's doc comment stale — it opens "pins the two conditions that reject an otherwise parseable app.toml" and enumerates only min-gas-prices and pruning-everything + snapshots (config_fuzz_test.go:693). That fuzz surface also never varies freeze-height, so the new conditions sit outside it; they are pinned only by the TestValidateBasic table rows.
In a package where these comments are the characterization record, worth updating the count/enumeration (and ideally threading freeze-height/halt-height through the fuzz inputs).
| if err != nil { | ||
| return nil, fmt.Errorf("LoadStateFromDBOrGenesisDocProvider(): %w", err) | ||
| } | ||
| if err := validateFreezeHeight(opts.freezeHeight, genDoc.InitialHeight, state.LastBlockHeight, blockStore.Height(), proxyApp.Info().LastBlockHeight); err != nil { |
There was a problem hiding this comment.
[nit] Consider swapping these two checks. In Autobahn/giga mode the CometBFT block and state stores stay at 0 while the app height is N, so a giga node configured with freeze-height <= N fails here with "application height N has already reached freeze height H" instead of the accurate "freeze height is not supported with Autobahn" on the next line. Putting the mode rejection first gives the operator the actionable message in every case.
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
No blockers but I highly recommend addressing the condition check inside auto restart if behind loop.
| if r, ok := s.consReactor.Get(); ok { | ||
| logger.Info("switching to consensus reactor", "height", handoff.height, "blocks_synced", handoff.blocksSynced, "state_synced", handoff.stateSynced, "max_peer_height", handoff.maxPeerHeight) | ||
| r.SwitchToConsensus(handoff.state, handoff.blocksSynced > 0 || handoff.stateSynced) | ||
| if s.shouldFreeze(handoff.state) { |
There was a problem hiding this comment.
What if the handoff state is not already in freeze boundary?
IIUC, that means:
- the auto restart mechanism starts and runs for the lifetime of the process
- then when we reach freeze height at H-1 because pool's max peer height keeps growing regardless (status request keeps broadcasting) we will end up restarting.
We can avoid this by checking the freeze boundary inside autoRestartIfBehind loop based on self height.
| @@ -375,6 +378,9 @@ func (s *syncController) run(ctx context.Context) error { | |||
| if r, ok := s.consReactor.Get(); ok { | |||
| logger.Info("switching to consensus reactor", "height", handoff.height, "blocks_synced", handoff.blocksSynced, "state_synced", handoff.stateSynced, "max_peer_height", handoff.maxPeerHeight) | |||
| r.SwitchToConsensus(handoff.state, handoff.blocksSynced > 0 || handoff.stateSynced) | |||
There was a problem hiding this comment.
Switch to consensus means we still write to wal right? not a huge issue but worth gating any state writes in freeze mode in case it results in hands on involvement to bring the node out of freeze mode.
Operationally, we want the freeze mode to basically be noop for consensus.
2 participants
Summary
Add a
freeze-heightnode mode that keeps the existing binary and RPC services running while stopping block sync and consensus at an upgrade boundary.freeze-height = Htreats H as the first block the node must not execute. The node commits and serves state through H-1, transitions consensus to H/NewHeight, and remains alive without proposing, signing, or advancing further.Motivation
Historical RPC nodes need to retain the binary behavior that was active before an upgrade. Shutting the node down at the upgrade height also shuts down EVM HTTP and WebSocket RPC, while allowing the old binary to execute the upgrade block can panic or produce responses using code that does not match that historical state.
Changes
freeze-heightto app configuration and theseid startCLI.Operator impact
Operators can set either:
or:
The node will continue serving EVM HTTP/WebSocket RPC against state at block 123455. Transaction submission remains available, but accepted transactions cannot be committed while the node is frozen.
Validation
go test -race ./sei-tendermint/node ./sei-tendermint/internal/consensus ./sei-tendermint/internal/blocksync -run 'Test(ValidateFreezeHeight|WithFreezeHeight|StateFreezesAfterTargetBlock|PoolRoutineHandsOffAtFreezeHeight)$' -count=1go test --count=0 ./sei-cosmos/server/config ./sei-cosmos/server ./sei-tendermint/node ./sei-tendermint/internal/consensus ./sei-tendermint/internal/blocksync ./cmd/seid/cmdgo veton the affected packagesgofmt,goimports, andgit diff --checkon all changed filesHistorical backports
Each branch below is exactly one backport commit above its corresponding release tag.
v6.5andv6.6map to the repository'sv6.5.0andv6.6.0tags.codex/freeze-rpc-v5.5.2b6814c179codex/freeze-rpc-v5.5.5b8f17db9ecodex/freeze-rpc-v5.6.04472fa6a8codex/freeze-rpc-v5.6.2e3f99ff9dcodex/freeze-rpc-v5.7.029461d2d9codex/freeze-rpc-v5.7.1ab7103415codex/freeze-rpc-v5.7.2d26eecc99codex/freeze-rpc-v5.7.46f831399bcodex/freeze-rpc-v5.7.5f0939f828codex/freeze-rpc-v5.8.07e9a4785acodex/freeze-rpc-v5.9.085e0848facodex/freeze-rpc-v6.0.04a074671dcodex/freeze-rpc-v6.0.19ce41f1eecodex/freeze-rpc-v6.0.272c896adccodex/freeze-rpc-v6.0.348804fe29codex/freeze-rpc-v6.0.4641a82159codex/freeze-rpc-v6.0.52a7774679codex/freeze-rpc-v6.0.646c28a3accodex/freeze-rpc-v6.1.044c28c9f0codex/freeze-rpc-v6.1.41dfc51696codex/freeze-rpc-v6.2.0647c607aecodex/freeze-rpc-v6.3.0e2aa7be2fcodex/freeze-rpc-v6.4.08161b645ccodex/freeze-rpc-v6.5.00f17410ffcodex/freeze-rpc-v6.6.0228b35bd3For tags before
v6.3.0,sei-cosmosandsei-tendermintwere external archived modules. Those backport commits are self-contained: they import the matching historical dependency trees and point the branch's Go replacements to those local copies.