Repository navigation
fix(evmrpc): floor earliest state height at genesis InitialHeight (SEI-10383)
#3798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2a7cd32
2a5ad54
e1f3406
a43b79a
72e5e27
eb25ef9
9781962
9862c57
8705546
995a1fe
c3ff11b
5d4ddba
2e66b15
d704bf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import ( | |
| "github.com/ethereum/go-ethereum/rpc" | ||
| "github.com/sei-protocol/sei-chain/sei-cosmos/client" | ||
| sdk "github.com/sei-protocol/sei-chain/sei-cosmos/types" | ||
| genesistypes "github.com/sei-protocol/sei-chain/sei-cosmos/types/genesis" | ||
| "github.com/sei-protocol/sei-chain/sei-db/db_engine/types" | ||
| "github.com/sei-protocol/sei-chain/sei-db/ledger_db/receipt" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/rpc/coretypes" | ||
|
|
@@ -44,6 +45,16 @@ func NewWatermarkManager( | |
| } | ||
| } | ||
|
|
||
| // genesisInitialHeight returns the chain's genesis InitialHeight, sourced from | ||
| // the local client's in-memory genesis doc, falling back to | ||
| // genesis.DefaultGenesisInitialHeight when the client cannot supply it. | ||
| func (m *WatermarkManager) genesisInitialHeight() int64 { | ||
| if h := m.tmClient.GenesisInitialHeight(); h > 0 { | ||
| return h | ||
| } | ||
| return genesistypes.DefaultGenesisInitialHeight | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] The |
||
| } | ||
|
Comment on lines
+48
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 This PR adds a cheap, RPC-free Extended reasoning...What the bug is. This PR introduces |
||
|
|
||
| // Watermarks returns the earliest block height, earliest state height, and | ||
| // latest height that are safe to serve. Earliest heights are inclusive. | ||
| // It is possible that block latest < block earliest, in case there are no blocks yet. | ||
|
|
@@ -66,6 +77,19 @@ func (m *WatermarkManager) Watermarks(ctx context.Context) (int64, int64, int64, | |
| latest = min(latest, m.stateStore.GetLatestVersion()) | ||
| stateEarliest = m.stateStore.GetEarliestVersion() | ||
| } | ||
|
|
||
| // Floor the earliest state height at genesis. A never-pruned store reports | ||
| // its earliest version as 0 (the earliest-version key is only written by | ||
| // pruning or state-sync), which would otherwise resolve `earliest` to the | ||
| // tip via CreateQueryContext's height-0 coercion. Guarding on | ||
| // stateEarliest < latest leaves the pre-commit window (no blocks yet, where | ||
| // stateEarliest == latest) reading the genesis checkState, and the clamp to | ||
| // latest keeps the floor from ever exceeding the safe latest. Using the real | ||
| // InitialHeight (not a literal 1) keeps this correct for chains started | ||
| // above height 1, whose version 1 was never committed. | ||
| if stateEarliest < latest { | ||
| stateEarliest = max(stateEarliest, min(m.genesisInitialHeight(), latest)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Worth calling out a user-visible consequence beyond the Erroring is clearly better than the current behavior (silently serving tip state), and the PR calls this out as intentional hardening. But note it leaves the block and state surfaces inconsistent for the same tag: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Behavior change worth confirming as intended: on SS-enabled nodes this also raises the floor used by That matches what SS-disabled nodes already do (
If erroring is the intended contract, a test pinning it (alongside |
||
| } | ||
| return blockEarliest, stateEarliest, latest, nil | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] nit: the
h > 0fallback toDefaultGenesisInitialHeight(1) is a silent substitution that is wrong in the one case it exists for. IfGenDocis unavailable on a chain whose realInitialHeightis >1, the floor becomes 1 andearliestresolves to a never-committed version — reintroducing thefailed to load state at height 1failure this PR explicitly set out to avoid by not hardcoding 1.In practice this is unreachable:
rpcEnv.GenDocis always populated (sei-tendermint/node/node.go:234) andValidateAndCompletenormalizesInitialHeightto ≥1, so the branch is defensive only. Two small suggestions: (a) since the value is immutable for the node's lifetime, resolve it once inNewWatermarkManagerinstead of on everyWatermarks()call (which is every read path); and (b) note the trade-off in the comment — falling back to 1 vs. returning 0 to disable the floor are both defensible, but the current choice silently contradicts the interface doc onLocalClient.GenesisInitialHeight("or 0 if it is unavailable") and the PR description.