fixed support for AllowEmptyBlocks = false for autobahn#3739
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
pompon0
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3739 +/- ##
==========================================
- Coverage 59.92% 59.02% -0.91%
==========================================
Files 2288 2202 -86
Lines 190180 180350 -9830
==========================================
- Hits 113971 106447 -7524
+ Misses 66049 64562 -1487
+ Partials 10160 9341 -819
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR lets the EVM RPC endpoint open before the first block by populating checkState at InitChain, introducing an IsGenesis context flag, and adding InitLastHeader/Initialized() signaling; the core state-machine and ante changes are internally consistent and backed by substantial new unit tests. No blocking issues found, and Codex's two findings appear to be false positives.
Findings: 0 blocking | 6 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's two findings (genesis account-number and gas-meter handling in app/ante/cosmos_checktx.go) appear to be FALSE POSITIVES. That file's CheckSignatures/SetGasMeter are only invoked by the legacyabci CosmosCheckTxAnte/CosmosDeliverTxAnte paths, which run on real blocks (height >= 1), never on genesis gentx delivery. Genesis gentxs flow through app.BaseApp.DeliverTx -> runTx -> the standard SDK anteHandler (sei-cosmos/x/auth/ante/setup.go + sigverify.go), both of which WERE correctly migrated to ctx.IsGenesis(). Removing the height==0 branches from cosmos_checktx.go is actually required so that real user txs submitted before the first block (checkState at height 0) verify against their true account numbers with a normal gas meter. Recommend the author double-confirm, but genesis initialization should be unaffected.
- REVIEW_GUIDELINES.md and cursor-review.md are both empty: no repo-specific guidelines were available and the Cursor second-opinion pass produced no output. Proceeded with the Codex pass and my own analysis.
- evmrpc/simulate.go: Backend.isLatest calls getBlockNumber, and on the non-latest/historical path StateAndHeaderByNumberOrHash then calls getBlockByNumberOrHash which resolves the same block number again — a redundant tmClient round-trip. Minor; consider threading the already-resolved pointer through instead of re-resolving.
- The PR description states 'the old trigger point in FinalizeBlock also stays for backward compatibility', but the diff removed the httpServerStartSignal defer from ProcessBlock; the backward-compat fallback is now BaseApp.Commit's
defer app.signalInitialized(). Behavior is fine (Commit fires the signal for CometBFT/sei-v2 nodes), but the description is slightly stale. - Large mechanical blast radius: Info() drops its (context, *RequestInfo) params and error return, InitChain() drops its context across the abci.Application interface, Proxy, all mocks and many test call sites, plus deletion of sei-tendermint/rpc/client/mock. The changes look internally consistent and no straggler callers were found, but this leans heavily on CI compile + full test suite for confidence.
- No prompt-injection or suspicious instruction content was found in the diff, commit messages, or PR title/body.
Superseded: latest AI review found no blocking issues.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR enables the EVM RPC endpoint to open before the first block is finalized (needed for autobahn with AllowEmptyBlocks=false), replaces the height-0 "genesis" heuristic in ante handlers with an explicit ctx.IsGenesis() flag, simplifies the Info()/InitChain() ABCI signatures, and adds InitLastHeader. The core change is well-reasoned and backed by good new unit tests plus rewritten integration tests; I found no blocking correctness issues, only minor/non-blocking notes.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output, so its findings were not incorporated.
- Disagree with Codex's High finding on sei-tendermint/AGENTS.md: the added lines are legitimate contributor conventions (structured concurrency, proto-field presence rules, avoiding artificial test timeouts, strongly-typed asserts), not directives aimed at manipulating an automated reviewer. I don't see this as a prompt-injection attempt or a reason to split the PR; safe to keep.
- Correctness of the whole feature hinges on ctx.IsGenesis() being true during the real production InitChain gentx-delivery path (not just in the unit tests that set it manually). SetupWithGenesisValSet and the autobahn integration test exercise validator bootstrap via gentxs, so there is coverage — worth confirming a fresh multi-validator chain still starts, since gentxs carry account number 0 / no fee and now rely solely on IsGenesis rather than BlockHeight()==0.
- Several test doubles now embed the bare client.Client interface (a nil interface value) instead of the deleted mock.Client; any RPC method a given test forgets to override will nil-panic rather than return a mock default. Acceptable for tests but slightly more fragile than the previous recorder-based mock.
- app/ante/cosmos_checktx.go SetGasMeter no longer special-cases height 0 with an infinite gas meter — this is intentional and correct (CheckTx before the first commit should meter gas normally), but it is a behavioral change worth calling out for reviewers focused on the fee/gas path.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| var logger = seilog.NewLogger("evmrpc") | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
[nit] This init() mutates go-ethereum's process-wide default logger via gethlog.SetDefault. Merely importing evmrpc (e.g. from tests or any other package) will silently redirect logging for all geth consumers in the process. Since sei is the sole geth consumer here the practical impact is low, but configuring the geth logger explicitly at application startup (where the RPC servers are wired) rather than as an import side effect would be cleaner and avoids surprising test binaries. (Matches Codex's medium finding.)
| if err != nil { | ||
| return fmt.Errorf("r.data.GlobalBlock(): %w", err) | ||
| } | ||
| app.InitLastHeader((&types.Header{ |
There was a problem hiding this comment.
is this only called when giga is enabled? if so would it cause rpc server to not start if giga is not on?
There was a problem hiding this comment.
in sei-v2 the server starts after first Commit() as it used to. InitLastHeader is an optional call which allows to start the server earlier.
| } | ||
| ctx := app.NewUncachedContext(false, tmproto.Header{Height: max(app.LastBlockHeight(), 1)}) | ||
| return app.StakingKeeper.GetBondedValidators(ctx) | ||
| return app.StakingKeeper.GetBondedValidators(app.GetCheckCtx()) |
There was a problem hiding this comment.
Validators read raced check state
Medium Severity
GetValidators now always reads the bonded set from GetCheckCtx(), including on every Autobahn executeBlock. That shares the mutable check-state multistore with concurrent CheckTx, whereas the old path used an uncached committed context after the first commit.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 809f5a7. Configure here.
There was a problem hiding this comment.
Solid, well-tested PR that fixes the allow_empty_blocks=false deadlock by opening the EVM RPC endpoint on InitChain/InitLastHeader and, importantly, replacing height-based genesis detection with an explicit ctx.IsGenesis() flag (a real correctness/security fix, since under autobahn checkState stays at height 0 while processing real txs). No blockers; a few minor, non-blocking consistency/doc gaps.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- README.md:3 still embeds
assets/SeiLogo.png, which this PR deletes, leaving the repo banner broken (Codex P3). Update the README to drop/replace the image (README.md is not itself in the diff, so this can't be an inline comment). - Fresh-chain "latest" inconsistency (Codex P2): getBlockByNumber returns the synthetic genesis block for
lateston a height-0 chain, but eth_getBlockTransactionCountByNumber("latest") and eth_getBlockReceipts("latest") don't apply the samenumberPtr==nil && LatestCtxHeight height==0handling and will fall through to querying Tendermint block 0. Low impact (only a brand-new chain before its first block), but worth making consistent. - Cursor produced no second-opinion output (cursor-review.md is empty); REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied beyond AGENTS.md.
- evmrpc/filter.go installs a process-global geth default logger from an init() side effect; harmless in the seid binary but a global mutation worth a brief comment noting intent.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil, nil | ||
| } | ||
| if err = a.watermarks.EnsureReceiptHeightAvailable(ctx, block.Block.Height); err != nil { | ||
| if err = a.watermarks.EnsureReceiptHeightAvailable(block.Block.Height); err != nil { |
There was a problem hiding this comment.
[suggestion] Consistency gap on a fresh height-0 chain: getBlockByNumber now returns the synthetic genesis block when numberPtr == nil && LatestCtxHeight height == 0, but this path (and GetBlockReceipts for latest) only special-cases explicit number == 0. For eth_getBlockTransactionCountByNumber("latest") / eth_getBlockReceipts("latest") on a brand-new chain, numberPtr resolves to nil (latest) and the code proceeds to query Tendermint block 0, returning an error/empty instead of the genesis count. Consider mirroring the numberPtr == nil && a.ctxProvider(LatestCtxHeight).BlockHeight() == 0 check here for parity with GetBlockByNumber.
|
|
||
| resp := app.initChainer(app.deliverState.ctx, *req) | ||
| app.initChainer(app.processProposalState.ctx, *req) | ||
| resp := app.initChainer(app.deliverState.ctx.WithIsGenesis(true), *req) |
There was a problem hiding this comment.
[nit] Nice: running the checkState initChainer with WithIsGenesis(true).WithIsCheckTx(false) ensures genesis gentxs get deliver semantics (no mempool-only fee checks) even while populating the check-state branch, and the genesis flag is applied to the derived context only — so app.checkState.ctx.IsGenesis() stays false for subsequent real CheckTx/Simulate at height 0. This is the crux of the height-0 correctness fix; the accompanying TestInitChainGenesisFlagDoesNotLeakToCheckOrSimulateBeforeFirstCommit test covers it well.
| fullTx bool, | ||
| includeSyntheticTxs bool, | ||
| excludeUntraceable bool, | ||
| ) (result map[string]interface{}, returnErr error) { | ||
| ) (result map[string]any, returnErr error) { | ||
| numberPtr, err := getBlockNumber(ctx, a.tmClient, number) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // synthetic genesis block, not the Tendermint block at height 0. | ||
| if number == 0 || (numberPtr == nil && a.ctxProvider(LatestCtxHeight).BlockHeight() == 0) { | ||
| return encodeGenesisBlock(), nil | ||
| } | ||
|
|
||
| // Validate EVM block height for pacific-1 chain | ||
| if numberPtr != nil { |
There was a problem hiding this comment.
🟡 getBlockByNumber was patched to return the synthetic genesis block for latest-style tags (latest/safe/finalized/pending) while checkState is still at height 0, the pre-first-commit window this PR introduces for Autobahn allow_empty_blocks=false. GetBlockTransactionCountByNumber and GetBlockReceipts were not given the same fix — they only special-case the literal block number 0, so on a fresh node they fall through to the watermark-based lookup, which resolves 'latest' to height 0 and errors with ErrZeroOrNegativeHeight instead of returning the genesis-equivalent response.
Extended reasoning...
What the bug is. getBlockByNumber (evmrpc/block.go) was given a genesis-window guard for this PR's core change (starting the EVM RPC server before the first commit):
if number == 0 || (numberPtr == nil && a.ctxProvider(LatestCtxHeight).BlockHeight() == 0) {
return encodeGenesisBlock(), nil
}This correctly handles both an explicit 0x0 request and a latest/safe/finalized/pending tag (which resolves to numberPtr == nil via getBlockNumber) while checkState is still at height 0. GetBlockTransactionCountByNumber and GetBlockReceipts, however, only special-case the literal numeric height:
// GetBlockTransactionCountByNumber
if number == 0 {
return genesisBlockTxCount, nil
}// GetBlockReceipts
if blockNrOrHash.BlockNumber != nil && *blockNrOrHash.BlockNumber == 0 {
return []map[string]any{}, nil
}Neither checks numberPtr == nil && checkState height == 0, so a latest-style request for either falls through to the normal watermark path.
The failure path. For a latest-like tag, getBlockNumber returns numberPtr == nil (evmrpc/utils.go). Both methods then call blockByNumberOrNullForJSONRPC → blockByNumberRespectingWatermarks (evmrpc/watermark_manager.go). With heightPtr == nil, this resolves the height via wm.LatestHeight(ctx), which — per the same PR's Watermarks() change removing the old >0 guards — returns 0 in this window (every source: tm status, ctxProvider(LatestCtxHeight).BlockHeight(), and receiptStore.LatestVersion() all report 0). It then calls blockByNumberWithRetry(ctx, client, &0, 1) → client.Block(ctx, &0). env.getHeight in sei-tendermint (sei-tendermint/internal/rpc/core/env.go) rejects any non-nil height pointer with value <= 0 via ErrZeroOrNegativeHeight. blockByNumberOrNullForJSONRPC (evmrpc/watermark_manager.go) only translates the distinct sentinel ErrBlockHeightNotYetAvailable into (nil, nil) — it does not recognize ErrZeroOrNegativeHeight — so the raw tendermint-layer error propagates all the way out as a genuine RPC error.
Why nothing else catches this. The fix that was applied to getBlockByNumber is method-local; there's no shared helper that both GetBlockTransactionCountByNumber/GetBlockReceipts and getBlockByNumber funnel through for this specific genesis-window decision, so the omission in the two sibling methods isn't caught by anything structural.
Impact. On a freshly started Autobahn node with allow_empty_blocks=false (this PR's target scenario), before any user tx has landed, eth_getBlockByNumber('latest') succeeds and returns the synthetic genesis block, but eth_getBlockTransactionCountByNumber('latest') and eth_getBlockReceipts('latest'|'safe'|'finalized'|'pending') return a raw internal RPC error ("height must be greater than zero"-style) instead of the genesis-equivalent response (0 tx count / empty receipt list). This is a real, if narrow and transient, inconsistency directly in the scenario this PR is designed to serve — it closes automatically once the first tx commits.
Step-by-step proof.
- Fresh Autobahn node starts,
InitChainruns,allow_empty_blocks=false— no block has been finalized yet, socheckState.BlockHeight() == 0andLastBlockHeight() == 0. - A client calls
eth_getBlockTransactionCountByNumber('latest'). number == 0is false (the RPC number here is the sentinel for "latest", not literal 0), so the guard is skipped;getBlockNumberresolvesnumberPtr = nil.blockByNumberOrNullForJSONRPC(ctx, tmClient, watermarks, nil, 1)→blockByNumberRespectingWatermarksresolveswm.LatestHeight(ctx) == 0and callstmClient.Block(ctx, &0).env.getHeighton the tendermint side rejects height<= 0withErrZeroOrNegativeHeight.blockByNumberOrNullForJSONRPConly mapsErrBlockHeightNotYetAvailable, notErrZeroOrNegativeHeight, so the error is returned unchanged fromGetBlockTransactionCountByNumber.- Meanwhile
eth_getBlockByNumber('latest')on the same node at the same time hits the guard ingetBlockByNumber(numberPtr == nil && ctxProvider(LatestCtxHeight).BlockHeight() == 0) and returns the synthetic genesis block successfully — demonstrating the inconsistency. - The identical failure occurs for
eth_getBlockReceipts('latest'), since its literal-only*blockNrOrHash.BlockNumber == 0check also never fires for a nilBlockNumber.
Fix. Mirror the same genesis-window condition used in getBlockByNumber in both methods, e.g. for GetBlockTransactionCountByNumber:
if number == 0 || (numberPtr == nil after resolution && checkState height == 0) {
return genesisBlockTxCount, nil
}and for GetBlockReceipts, extend the numberPtr == nil branch to check ctxProvider(LatestCtxHeight).BlockHeight() == 0 and return []map[string]any{} in that case.
| func (b *Backend) StateAndHeaderByNumberOrHash(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (vm.StateDB, *ethtypes.Header, error) { | ||
| tmBlock, isLatestBlock, err := b.getBlockByNumberOrHash(ctx, blockNrOrHash) | ||
| sdkCtx := b.ctxProvider(LatestCtxHeight) | ||
| zeroExcessBlobGas := uint64(0) | ||
| header := ðtypes.Header{ | ||
| Difficulty: common.Big0, | ||
| Number: big.NewInt(sdkCtx.BlockHeight()), | ||
| BaseFee: b.keeper.GetNextBaseFeePerGas(sdkCtx).TruncateInt().BigInt(), | ||
| GasLimit: keeper.DefaultBlockGasLimit, | ||
| Time: toUint64(sdkCtx.BlockTime().Unix()), //nolint:gosec | ||
| ExcessBlobGas: &zeroExcessBlobGas, | ||
| } | ||
| isLatest, err := b.isLatest(ctx, blockNrOrHash) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| height := tmBlock.Block.Height | ||
| isWasmdCall, ok := ctx.Value(CtxIsWasmdPrecompileCallKey).(bool) | ||
| sdkCtx := b.ctxProvider(height).WithIsEVM(true).WithEVMEntryViaWasmdPrecompile(ok && isWasmdCall) | ||
| if !isLatestBlock { | ||
| // no need to check version for latest block | ||
| if err := CheckVersion(sdkCtx, b.keeper); err != nil { | ||
| if !isLatest || sdkCtx.BlockHeight() > 0 { | ||
| tmBlock, isLatest, err := b.getBlockByNumberOrHash(ctx, blockNrOrHash) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| header.Number = big.NewInt(tmBlock.Block.Height) | ||
| header.Time = toUint64(tmBlock.Block.Time.Unix()) | ||
| header.ParentHash = common.BytesToHash(tmBlock.BlockID.Hash) | ||
| sdkCtx = b.ctxProvider(tmBlock.Block.Height) | ||
| if !isLatest { | ||
| if err := CheckVersion(sdkCtx, b.keeper); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 In evmrpc/simulate.go, both the pre-existing getHeader helper (used by BlockByNumber/HeaderByNumber/CurrentHeader) and the new StateAndHeaderByNumberOrHash path (backing eth_call/eth_estimateGas/eth_createAccessList) set header.ParentHash to the block's own tendermint hash (tmBlock.BlockID.Hash) instead of its parent's hash (tmBlock.Block.LastBlockID.Hash) — the value EncodeTmBlock correctly uses for the client-facing eth_getBlockByNumber parentHash field. The PR duplicates the existing getHeader bug into the new simulation-backend code path, but the practical impact is narrow: no client-facing endpoint returns this header, ParentHash is not consumed by EVM execution, and a naive fix to getHeader risks regressing debug_traceBlock's profiled-tracing parent-resolution fallback, which currently depends on this exact quirk.
Extended reasoning...
What's wrong: getHeader (existing) and the new StateAndHeaderByNumberOrHash (added by this PR) both compute header.ParentHash = common.BytesToHash(tmBlock.BlockID.Hash). tmBlock.BlockID.Hash is the hash of the current Tendermint block, not its parent — the correct parent hash is tmBlock.Block.LastBlockID.Hash, which is exactly what EncodeTmBlock (evmrpc/block.go) uses for the parentHash field returned by eth_getBlockByNumber/eth_getBlockByHash. So within the same package, two header-construction paths disagree about what "parent hash" means, and the synthetic/simulation headers report ParentHash == own hash.
Code path: getHeader backs Backend.BlockByNumber, Backend.HeaderByNumber, and Backend.CurrentHeader (go-ethereum's tracing/gas-oracle internals). The PR's rewritten StateAndHeaderByNumberOrHash duplicates the identical wrong assignment for the new pre-first-commit/"latest" resolution path that backs eth_call, eth_estimateGas, and eth_createAccessList. So the bug's blast radius is extended by this PR into a brand-new code path, even though the underlying mistake predates it.
Why this doesn't break clients today: eth_getBlockByNumber/eth_getBlockByHash go through EncodeTmBlock, which is correct — so the one endpoint clients would use to validate a header chain (header[i].ParentHash == hash(header[i-1])) already returns the right value. The headers built by getHeader/StateAndHeaderByNumberOrHash are internal — they feed EVM block-context construction (where ParentHash isn't consulted; BLOCKHASH is resolved via a separate GetHashFn) and internal tracing/gas-oracle machinery, never serialized back to a caller as chain-linkage data.
A real wrinkle with "just fix it": evmrpc/block_trace_profiled.go's profiledTraceBlock resolves the parent block for debug_traceBlock via:
parent, _, err := api.backend.BlockByNumber(ctx, rpc.BlockNumber(block.Number().Int64()-1))
if parent == nil || parent.Hash() != block.ParentHash() {
parent, _, err = api.backend.BlockByHash(ctx, block.ParentHash())
...
}Because go-ethereum's Header.Hash() (RLP hash) can never equal a Tendermint hash, the parent.Hash() != block.ParentHash() check is always true, so this always falls into the BlockByHash(block.ParentHash()) fallback. Today, with block.ParentHash() == tmBlock.BlockID.Hash (the block's own hash — the bug), that lookup resolves back to the block itself (height N), and StateAtBlock(parent=N) → initializeBlock(block=N) computes exactly the pre-transaction state for block N (committed N-1 + BeginBlock(N)) — i.e., the correct starting state for tracing block N's own transactions. If getHeader's ParentHash were "fixed" to LastBlockID.Hash (the real parent, N-1's hash), this same fallback would resolve to block N-1 instead, and StateAtBlock/initializeBlock would then compute the pre-transaction state for block N-1 — off by one block — silently corrupting debug_traceBlock results. This reliance predates the PR (it's inherent to getHeader), so a correct fix needs to update profiledTraceBlock's parent-resolution logic in lockstep, not just swap the hash source in getHeader.
Step-by-step proof of the value bug itself:
- Tendermint commits block N.
tmBlock.BlockID.Hash= hash of block N.tmBlock.Block.LastBlockID.Hash= hash of block N-1 (the real parent). getHeader(tmBlock)setsheader.ParentHash = common.BytesToHash(tmBlock.BlockID.Hash)→ header for block N reportsParentHash == hash(block N), i.e., its own hash.- Compare to
EncodeTmBlockfor the same block:lastHash := common.HexToHash(block.Block.LastBlockID.Hash.String()); result["parentHash"] = lastHash→ correctly reportshash(block N-1). - So
HeaderByNumber(N).ParentHash != eth_getBlockByNumber(N).parentHashfor the same block N, even though both claim to describe the same field. - The new
StateAndHeaderByNumberOrHash(this PR) does the identical wrong assignment when it falls back to a resolvedtmBlock(non-latest, or latest-with-existing-block), extending the same inconsistency to theeth_call/eth_estimateGas/eth_createAccessListcode path.
Recommendation: Worth fixing for internal consistency (use tmBlock.Block.LastBlockID.Hash), but treat it as a nit rather than a blocking issue: it has no observable client impact today, and any fix to getHeader specifically must be paired with an update to profiledTraceBlock's parent-resolution fallback (or an explicit block-number-based parent lookup) to avoid silently shifting debug_traceBlock's starting state by one block.
There was a problem hiding this comment.
This PR opens the EVM RPC endpoint at height 0 (before the first commit) so an empty-blocks-disabled autobahn chain can bootstrap, by initializing checkState during InitChain, adding an Initialized() signal, gating RPC server start on it, and threading IsGenesis through the ante path; it's well-tested and the core logic is sound. The only substantive issue is an inconsistency (raised by Codex) where getBlockReceipts does not mirror the new latest-tag genesis handling that getBlockByNumber gained.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion inputs: cursor-review.md was empty (no Cursor output to merge). codex-review.md contributed one finding (the getBlockReceipts latest-tag asymmetry), which is confirmed and reported inline.
- InitChain now runs initChainer three times (deliverState, processProposalState, and now checkState). Correctness is fine since each writes to an isolated state branch (no double-application of genesis side effects), but it triples genesis-init CPU cost — worth a note for chains with large genesis files.
- Broad-but-mechanical ABCI change: Application.Info() drops its context/error return and InitChain() drops context across the whole interface, plus a new InitLastHeader(). All implementers, mocks, and callers appear updated consistently; no functional concern, just a large surface to keep in sync on future rebases.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil, err | ||
| } | ||
| // synthetic genesis block, not the Tendermint block at height 0. | ||
| if number == 0 || (numberPtr == nil && a.ctxProvider(LatestCtxHeight).BlockHeight() == 0) { |
There was a problem hiding this comment.
[suggestion] This new latest-tag genesis synthesis (numberPtr == nil && checkState height == 0 → encodeGenesisBlock()) is not mirrored in GetBlockReceipts. Before the first commit, eth_getBlockReceipts("latest"|"safe"|"finalized"|"pending") resolves numberPtr to nil → blockByNumberOrNullForJSONRPC(nil) → WatermarkManager.LatestHeight() returns 0 → client.Block(&0), which fails with an error that is not ErrBlockHeightNotYetAvailable, so the error propagates instead of returning the empty genesis receipts. GetBlockReceipts only special-cases the literal 0x0 number (block.go:306), not the latest-like tags. Consider adding the symmetric numberPtr == nil && a.ctxProvider(LatestCtxHeight).BlockHeight() == 0 guard in GetBlockReceipts, and a test for the latest tag (the added test covers only literal 0x0). (Raised by Codex.)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7959d12. Configure here.
| func SetGasMeter(ctx sdk.Context, gasLimit uint64, paramsKeeper paramskeeper.Keeper) sdk.Context { | ||
| cosmosGasParams := paramsKeeper.GetCosmosGasParams(ctx) | ||
|
|
||
| if ctx.BlockHeight() == 0 { |
There was a problem hiding this comment.
Genesis gentx ante checks incomplete
High Severity
SetGasMeter and CheckSignatures dropped the height-0 genesis special cases without switching to ctx.IsGenesis(), unlike GetGasMeterSetter and the classic SigVerificationDecorator. InitChain now delivers gentxs on an IsGenesis context through CosmosDeliverTxAnte, so gentxs signed with account number 0 can fail signature verification and lose infinite-gas genesis metering.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7959d12. Configure here.
There was a problem hiding this comment.
A large, well-tested refactor that opens the EVM RPC endpoint at InitChain/InitLastHeader to fix the AllowEmptyBlocks=false startup deadlock under Autobahn. The genesis-flag, Initialized-channel, CreateQueryContext, and simulate changes are coherent and covered by tests; the only substantive concern is a behavioral narrowing of the state-earliest watermark for nodes running with SeiDB state store (SS) disabled.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied. Codex contributed a single P1 finding, incorporated below as an inline comment.
- Watermarks() no longer validates the earliest<=latest ordering (the old code returned an error when earliest exceeded latest). This is intentional per the new doc comment (latest<earliest is possible on a fresh chain) and ensureWithinWatermarks() degrades gracefully by rejecting all heights, so no crash — just noting the removed invariant in case any caller relied on the error.
- Watermarks(), EnsureReceiptHeightAvailable(), and ResolveHeight() now dereference m.receiptStore and m.ctxProvider unconditionally (the old nil-guards were removed). Verified both production call sites in evmrpc/server.go always pass a non-nil receiptStore (k.ReceiptStore()) and ctxProvider, so this is safe — but the invariant is now implicit rather than defensively checked.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| if !stateEarliestSet { | ||
| stateEarliest = blockEarliest | ||
| stateEarliest := latest // no historical storage => just the current state. |
There was a problem hiding this comment.
[suggestion] When stateStore is nil (SeiDB SS disabled — a supported, though non-default, configuration), stateEarliest is now pinned to latest, so every historical state query (eth_getBalance, eth_getCode, eth_call, etc.) at a height below latest is rejected as pruned. Previously (stateEarliest = blockEarliest) such a node served historical state down to the block-earliest watermark, and the state-commit (memIAVL) store still physically retains recent versions within its keep-recent window. This is a functional regression for ss-enable=false nodes.
The doc comment ("no historical storage => just the current state") suggests this may be intentional — treating SS-disabled nodes as latest-only. If so, please confirm and ideally document it as a deliberate change; if not, consider deriving stateEarliest from the state-commit store's retained range so recent historical reads keep working. (Flagged as P1 by the Codex pass.)
There was a problem hiding this comment.
Large but largely mechanical change to support autobahn with AllowEmptyBlocks=false: an ABCI InitChain/Info signature refactor, a channel-based server-start replaced by BaseApp.Initialized(), a new IsGenesis context flag, EVM-RPC genesis/before-first-commit state handling, a rewritten WatermarkManager, and test/script updates to drive blocks via txs. One notable concern is a possible historical-state regression in the WatermarkManager for nodes running without SeiDB state store.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- WatermarkManager rewrite now dereferences m.receiptStore unconditionally in Watermarks() (m.receiptStore.LatestVersion()), and drops the former nil guards for ctxProvider/tmClient. Production callers (evmrpc/server.go) always pass k.ReceiptStore(), so this is safe today, but if ReceiptStore() can ever be nil in an embedding/harness setup this becomes a panic on every RPC call rather than a graceful path. Consider asserting non-nil at construction time to make the invariant explicit.
- Codex flagged sei-tendermint/AGENTS.md as a possible prompt-injection attempt. I reviewed it and disagree: the added lines are ordinary module coding conventions (non-nil struct fields, use scope.Run/utils.Recv, no artificial test timeouts, proto field annotations), consistent with the pre-existing file and with the repo's documented use of nested AGENTS.md guides. Not an injection; no action needed.
- The Cursor second-opinion review (cursor-review.md) and REVIEW_GUIDELINES.md were both empty, so no Cursor findings or repo-specific guidelines were incorporated.
- app/app.go GetValidators() was simplified from the height-0/DeliverContext special case to app.StakingKeeper.GetBondedValidators(app.GetCheckCtx()); worth confirming GetCheckCtx() now carries the uncommitted InitChain staking state at height 0 (it should, given InitChain now populates checkState), so the previous panic-avoidance comment/logic is genuinely obsolete.
|
|
||
| if !stateEarliestSet { | ||
| stateEarliest = blockEarliest | ||
| stateEarliest := latest // no historical storage => just the current state. |
There was a problem hiding this comment.
[blocker] Possible historical-state regression for nodes without SeiDB SS (matches Codex). When m.stateStore == nil, stateEarliest is now set to latest, so ensureWithinWatermarks(height, stateEarliest, latest) in ResolveHeight rejects every historical state query (e.g. eth_getBalance/eth_call/eth_getCode/eth_getStorageAt at an old height) as "pruned". Previously the nil-stateStore path set stateEarliest = blockEarliest, allowing historical state to be served from the IAVL commitment multistore via CreateQueryContext. A full/archive node that runs without the state store but retains IAVL history can still serve that state, so this collapses those nodes to latest-only. The comment "no historical storage => just the current state" assumes SS is the only historical source, which isn't true when the SC/IAVL retains versions. Please confirm this is intended for all supported RPC deployments, or key stateEarliest off actual state availability (e.g. blockEarliest) when SS is disabled.
There was a problem hiding this comment.
@sei-will is this relevant? AI is conflicted on whether historical queries can be served with stateStore disabled.
4 participants
Until now the evm RPC endpoint was opened only AFTER the at least 1 block was finalized since startup (no matter if node is at genesis or not). In particular this means that when starting a new evm-only chain disallowing empty blocks we reach a deadlock: a block cannot be constructed, because there are no transactions in the mempool, because no one can submit a transaction, because the evm RPC endpoint is closed, because there is no finalized block, because a block cannot be constructed.
We break this cycle, by adding a method Application.InitLastHeader which effectively initializes checkState on startup, and triggers evm rpc endpoint start. We make InitChain also trigger the evm rpc endpoint start after populating checkState with genesis state and a synthetic header. The old trigger point in FinalizeBlock was moved to Commit(), but stays for backward compatibility. InitLastHeader is only used in autobahn and does not affect sei-v2.
Additionally