Repository navigation
Initialize app from last header when a frozen node restarts - #4447
Conversation
A frozen node commits no blocks after a restart, so nothing called Commit and app.Initialized() never fired. The EVM HTTP and WS servers wait on that signal and never started. In freeze mode, pass the last stored block header to InitLastHeader after the handshake.
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
|
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 #4447 +/- ##
==========================================
+ Coverage 56.75% 56.77% +0.01%
==========================================
Files 2126 2126
Lines 166836 166868 +32
==========================================
+ Hits 94691 94734 +43
+ Misses 72140 72129 -11
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@alexander-sei backport on this one was too hairy so I did it manually -- removing the lable |
PR SummaryMedium Risk Overview In Tests: unit coverage that restart passes the stored last header into the app; Docker integration test that restarts Reviewed by Cursor Bugbot for commit c31603a. 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.
When a node in freeze mode restarts, it now passes the last stored block header to InitLastHeader right after the handshake, so BaseApp sets checkState and signals initialization, and EVM RPC comes up without needing another Commit. I'm approving because the change is correct: freeze mode turns off state sync, so the handshake always runs first and app height matches state.LastBlockHeight; autobahn can't be combined with freeze mode; setCheckState holds its lock; and non-frozen nodes are unaffected. The one other reader (codex) reported nothing, which matches my reading apart from the two test-harness nits below.
2 nits, not posted on the code
integration_test/frozen_rpc_router/router_test.go:147— Runningstart_sei.shagain appends anotherDoneto the sharedbuild/generated/launch.complete. That file then has more lines than there are nodes, which can break any later step that checks for an exact count, such asrelease-upgrade-test.sh's-eq "$NODE_COUNT".integration_test/frozen_rpc_router/router_test.go:107— This test restarts node1, whichTestFrozenRPCRouterReachesEveryNodealso uses, so both tests only pass if they run in file order. Running with-shuffleor a-runsubset could send the router test to a node that is still restarting.
seidroid review · decision approve · session 964dcb912c8346c384c53fb7c6ccd57d · turn resp_claude_0baa7798f04ec4be38fc1e35be444238 · item 6774be652ad35355844485248f75d26e
Findings: 0 blocking | 0 non-blocking | 0 posted inline
| if n.freezeHeight > 0 && state.LastBlockHeight > 0 { | ||
| meta := n.blockStore.LoadBlockMeta(state.LastBlockHeight) | ||
| if meta == nil { | ||
| return fmt.Errorf("freeze mode: block %d is missing from the block store", state.LastBlockHeight) | ||
| } | ||
| n.rpcEnv.App.InitLastHeader(meta.Header.ToProto()) | ||
| } |
There was a problem hiding this comment.
nit: should this not just be in the freeze mode if-statement above or move that check down?
There was a problem hiding this comment.
It's here because higher up it won't work: It has to run after the handshake and after the state is reloaded from the store. Until then the app hasn't been brought up to state.LastBlockHeight, so state.LastBlockHeight isn't the number we want.
WE can moving further down, but the catch is that if handshake fails then we swallow the freezemode error which is argubly a more important log line from an operators perspective.
There was a problem hiding this comment.
Not sure I follow. I'm suggesting removing the if-statement checking for freeze mode to write a log into this if-statement. Anyways, not a blocker
There was a problem hiding this comment.
No worries, let me be clearer: it can go in one block, but it has to be nested because a freeze node that hasn't synced any blocks yet should still log that freeze mode is on. The only cost is the log moving after the handshake. Happy to make that change.
This is the v6.6 version of #4447. A node in freeze mode halts one block below its freeze height and never processes another block. On v6.6 the EVM HTTP and WS servers wait for start signals that only `ProcessBlock` sends, so once a frozen node restarts they never start. The node keeps its data and its Tendermint RPC, but EVM RPC stays down. `frozen-6-7-0` runs v6.6.3 and hits this on every restart. v6.6 has no `InitLastHeader` in the ABCI `Application` interface, so #4447 cannot be cherry-picked as is. `Proxy.InitLastHeader` forwards the call only to apps that implement it, which leaves the interface unchanged. `BaseApp.InitLastHeader` sets checkState from the header, as `Commit` does. `App.InitLastHeader` calls it and then sends the start signals through `signalEVMServersStart`, which `ProcessBlock` now also uses, so each signal is still sent once. `nodeImpl.OnStart` makes the same freeze-only call as on main, with the block meta at `state.LastBlockHeight`. `TestFrozenNodeServesEVMRPCAfterRestart` restarts seid on the height-10 frozen node in the frozen RPC router cluster. Locally it timed out with the v6.6.3 binary and passed with this change. Part of PLT-1370.
|
/backport |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4447-to-release/v6.7
git worktree add --checkout .worktree/backport-4447-to-release/v6.7 backport-4447-to-release/v6.7
cd .worktree/backport-4447-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x d4853e532b878fe34033ff63495521c782baf0fb
git push --force-with-lease |
4 participants
A node in freeze mode halts one block below its freeze height and never commits again. When its process restarts, no
Commitruns, soBaseApp.Initialized()never fires and the EVM HTTP and WS servers, which wait on it inapp/app.go, never start. The node keeps its data and its Tendermint RPC, but EVM RPC stays down from the first restart on. This is PLT-1370.In
nodeImpl.OnStart, right after the handshake, a frozen node with a non-empty chain now loads the block meta atstate.LastBlockHeightand passes its header toApplication.InitLastHeader.BaseAppsets checkState from that header and signals initialization, which is whatCommitdoes after a block. Sending only the start signal was rejected because checkState would keep the empty header fromLoadLatestVersion, and the RPC "latest" context reads it.Nodes not in freeze mode are unchanged. A frozen node that is still catching up now opens EVM RPC at startup instead of after its first block; later commits reset checkState as before.
TestFreezeModeRestartInitializesAppFromLastHeadercovers the node wiring.TestFrozenNodeServesEVMRPCAfterRestartrestarts seid on the height-10 frozen node in the frozen RPC router cluster. Locally it timed out on main and passed with this change.Part of PLT-1370.