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.91% 59.00% -0.92%
==========================================
Files 2288 2202 -86
Lines 189782 180049 -9733
==========================================
- Hits 113706 106230 -7476
+ Misses 65942 64490 -1452
+ Partials 10134 9329 -805
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.
A focused fix for the allow_empty_blocks=false startup deadlock: RPC servers now wait on a shared BaseApp initialized signal, and checkState/synthetic headers serve "latest" state before the first commit. The changes are well-tested and I found no correctness blockers, only a few points worth confirming.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex flagged the sei-tendermint/AGENTS.md edits as a possible prompt-injection attempt. I reviewed them: they are ordinary developer-facing convention updates (non-nil struct-field policy, utils.Recv/Send, scope.Run, proto-field guidance, no-artificial-timeouts) that are part of this repo's own contributor guide and do not attempt to direct the reviewer or alter review behavior. I disagree with treating this as an injection; it reads as legitimate documentation. Worth a quick human glance but not blocking.
- cursor-review.md is empty — the Cursor second-opinion pass produced no output. (REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied.)
- contracts/test/lib.js waitForProposalStatus uses
while (true)with no overall timeout (unlike the old bounded 200-iteration loop and the bashwait_for_proposal_statuswhich takes a timeout). It relies on ensureNotFailed + the kick mechanism to terminate; a proposal that neither reaches the target nor fails will hang until the outer mocha/test timeout. This matches the repo's 'let the test hang' convention but is worth being aware of. - giga_router_validator.go EvmProxy indexes r.cfg.ValidatorAddrs[shardValidator]; a missing key yields a zero-value GigaNodeAddr whose empty Key won't be found in poolOut, silently falling back to local handling. Low risk since shardValidator comes from the committee, but a missing-entry guard would be clearer.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil, nil, err | ||
| } | ||
| height := tmBlock.Block.Height | ||
| ctxHeight := LatestCtxHeight |
There was a problem hiding this comment.
[suggestion] Behavior change worth confirming: for isLatestBlock this now resolves state via LatestCtxHeight (-1 → GetCheckCtx()) instead of the previous tmBlock.Block.Height (which mapped to CreateQueryContext(N), i.e. committed state at the latest height). This aligns latest with the other LatestCtxHeight call sites (CurrentHeader, base fee, chainConfig) and is validated by the new genesis test, but it means eth_call/estimateGas at latest now read the mutable checkState branch, which can carry CheckTx/ante-level effects (e.g. sequence/fee deductions) not present in committed state. Please confirm this is intended for normal (post-first-commit) operation, not just the pre-genesis case.
| // Genesis initialization needs deliver semantics even when populating the | ||
| // check state branch. Running it in CheckTx mode enables mempool-only fee | ||
| // checks and can reject valid genesis txs. | ||
| app.initChainer(app.checkState.ctx.WithIsCheckTx(false), *req) |
There was a problem hiding this comment.
[nit] initChainer now runs a third time (on the checkState branch). Each branch has its own cache multistore over the same empty committed base, so state writes are isolated and safe. Just flagging: this assumes InitGenesis is free of side effects beyond its store (any logging, metrics, or shared in-memory mutation will now fire 3×). Fine as-is if all module genesis handlers are pure state writes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| sdkerrors.ErrInvalidRequest, | ||
| "failed to load state at height %d; %s (latest height: %d)", height, err, lastBlockHeight, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Latest query height mismatches check state
Low Severity
After InitChain, check state is moved to height 1 for CheckTx, but CreateQueryContext(0) before the first commit still returns a context labeled height 0 while reading that same check-state store. Pre-first-commit "latest" queries and mempool checks therefore disagree on block height.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c088f11. Configure here.
There was a problem hiding this comment.
The PR breaks the autobahn AllowEmptyBlocks=false startup deadlock by opening the EVM RPC on init (new Initialized() channel) and serving genesis/checkState for latest-like queries before the first Commit, plus extensive integration-test updates to drive block production via txs. The core change is coherent and well-tested; no blockers found, but a few behavior changes reach beyond autobahn and warrant confirmation.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- InitChain semantics now change for ALL chains, not just autobahn: it runs
initChainera third time oncheckState(WithIsCheckTx(false)), forces checkState BlockHeight to 1 after genesis, and CreateQueryContext returns checkState for height-0/latestbefore the first Commit. The PR description frames InitLastHeader as autobahn-only; please confirm sei-v2 genesis (pre-first-block CheckTx/queries) is unaffected. The added tests only cover the fresh-chain path. - evmrpc/simulate.go: the explicit
header.BaseFee = GetNextBaseFeePerGas(LatestCtxHeight)override was removed. For historical (non-latest) blocks the header BaseFee now comes fromgetHeader(per-block base fee) rather than the latest next-base-fee — an improvement — whilelatestis routed throughsyntheticHeaderFromCtx. New tests only exercise the pre-first-commit scenario; please confirm a running (post-commit) chain still returns the expectedlatestheader Number and BaseFee (the synthetic vs getHeader branch depends on ctxProvider(LatestCtxHeight).BlockHeight() vs the latest committed tm height). - cursor-review.md was empty — the Cursor second-opinion pass produced no output. (Codex produced one finding, incorporated as an inline comment.)
- integration_test/evm_module/scripts/evm_rpc_tests.sh:
bump_chain_to_height 100over-provisions — the comment says fixtures only reference historical block numbers up to 0x2d (45). Producing 100 sequential sync-broadcast blocks noticeably lengthens the test; consider bumping only to the height the fixtures actually require. - sei-cosmos/baseapp/abci.go: the
if app.initChainer == nil { return nil, nil }early return skips thecheckStateheight→1 adjustment, so an app configured without an initChainer keeps checkState at height 0. Edge case (real chains always set an initChainer), but the two states are inconsistent. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| _ = exec.Command("docker", "exec", container, "sh", "-c", script).Run() | ||
| } | ||
|
|
||
| func waitForReceiptBlockNumber(t *testing.T, txHash string) int64 { |
There was a problem hiding this comment.
[nit] waitForReceiptBlockNumber has no deadline of its own — it loops until t.Context() is cancelled, which normally only happens at test end. A liveness regression (tx never gets a receipt) will therefore hang until the suite-wide Go test timeout instead of failing promptly with a clear message. Consider bounding it with a per-call deadline (e.g. derived from haltStableTimeout) so the failure surfaces quickly and points at this tx. (Raised by Codex.)
There was a problem hiding this comment.
Solid, well-tested change that lets the EVM RPC endpoint come up before the first committed block (needed for AllowEmptyBlocks=false / autobahn) by populating check-state at InitChain/InitLastHeader and serving latest-like queries from it. The core baseapp and evmrpc logic is correct and covered by new unit tests; remaining points are non-blocking test-infra/consistency notes.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion pass (cursor-review.md) produced no output, and REVIEW_GUIDELINES.md is empty — no repo-specific guidelines were applied beyond AGENTS.md.
- InitChain now runs initChainer a third time (against checkState with WithIsCheckTx(false)) in addition to deliverState and processProposalState. This is correct for populating the check-state branch, but triples genesis-initialization cost at startup; acceptable as a one-time cost but worth being aware of for chains with large genesis.
- Consistency: the shell helper wait_for_proposal_status (integration_test/utils/_tx_helpers.sh) keeps a bounded timeout_secs, while the JS twin waitForProposalStatus (contracts/test/lib.js) loops unbounded. Both drive progress with kick txs, but the divergent timeout policy is worth aligning or documenting.
- Behavior change in evmrpc CurrentHeader/synthetic header: header Time now derives from the SDK context block time instead of time.Now(). This is deterministic and an improvement, but note that before the first Commit the reported 'latest' block number is checkState height 1 (not a committed block) — intentional for the empty-blocks-false case and validated by tests, but a semantic shift for eth_blockNumber consumers to be aware of.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| const kickAddr = await getKeySeiAddress(kickKeyName) | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
[nit] (from Codex, kept with a note) waitForProposalStatus replaces the previous bounded retry loop with while (true) and no deadline. If block production regresses (e.g. the kick bankSend stops committing), the loop can only be terminated by the outer test-runner's global timeout rather than a focused failure. This is partially aligned with the repo's new testing guidance ("let the test hang"), but note the shell equivalent wait_for_proposal_status still enforces timeout_secs — consider matching them for a clearer failure signal.
| // After genesis initialization completes, CheckTx should use the first | ||
| // executable block height rather than height 0. | ||
| if initHeader.Height == 0 { | ||
| app.checkState.SetContext(app.checkState.ctx.WithBlockHeight(1)) |
There was a problem hiding this comment.
[nit] Setting checkState height to 1 only when initHeader.Height == 0 correctly leaves upgraded chains (InitialHeight > 1) at their real height. Confirm downstream evmrpc consumers are fine reporting latest = height 1 before any block is committed (the new tests cover balance/code/storage; block-number/time semantics rely on this synthetic height).
There was a problem hiding this comment.
The PR correctly breaks the AllowEmptyBlocks=false startup deadlock by signaling RPC readiness from InitChain/Commit/InitLastHeader and making checkState + query paths serve genesis state before the first commit; the design is sound and well-covered by new unit and integration tests. No blockers, but a few base-fee/gas-limit and genesis-init-cost nuances are worth confirming.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (cursor-review.md) produced no output, so only Codex and this review's findings are incorporated.
- The InitChain changes (third initChainer pass into checkState, checkState height set to 1, defer signalInitialized, and the CreateQueryContext height-0/checkState branch) affect ALL chains including sei-v2 at genesis, not just autobahn as the PR description implies. The behavior looks safe (writes are isolated to the discarded checkState cache branch, so it stays non-app-hash-breaking), but it does add a full extra genesis-initialization pass everywhere — please confirm that's acceptable for sei-v2 genesis too.
- evmrpc: the base-fee/gas-limit differences before the first commit only matter during the window where the chain sits at height 0 waiting for the first tx; adding a targeted integration assertion on eth_call/eth_estimateGas gas ceiling in that window would strengthen coverage, but the new Go unit tests already cover the state/header resolution well.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| ctxHeight = LatestCtxHeight | ||
| } | ||
| sdkCtx := b.ctxProvider(ctxHeight) | ||
| header := b.getHeader(ctx, tmBlock) |
There was a problem hiding this comment.
[suggestion] This refactor drops the previous explicit override header.BaseFee = GetNextBaseFeePerGas(ctxProvider(LatestCtxHeight)) that both StateAndHeaderByNumberOrHash and CurrentHeader applied for latest requests. In the normal running case the latest tm block height equals GetCheckCtx().BlockHeight() (H), so this now takes the getHeader(tmBlock) path, which returns GetNextBaseFeePerGas(ctx(H-1)) — i.e. the base fee of the current block H rather than the next block H+1 that the old code reported. This changes the base fee seen by eth_call/eth_estimateGas/CurrentHeader at latest (observable when the base fee is moving between blocks, e.g. under congestion). For historical heights the new per-height base fee is actually more correct than the old always-latest override. Please confirm the current-block-vs-next-block base fee at latest is intentional.
| Number: big.NewInt(height), | ||
| Number: big.NewInt(sdkCtx.BlockHeight()), | ||
| BaseFee: baseFee, | ||
| GasLimit: keeper.DefaultBlockGasLimit, |
There was a problem hiding this comment.
[nit] (Also raised by Codex) syntheticHeaderFromCtx hardcodes keeper.DefaultBlockGasLimit, whereas getHeader derives the gas limit from sdkCtx.ConsensusParams().Block.MaxGas. On a chain whose genesis consensus params set a custom MaxGas, latest-like eth_call/eth_estimateGas will use the wrong gas ceiling during the pre-first-commit window (which, with allow_empty_blocks=false, persists until the first tx lands). Consider deriving the limit from sdkCtx.ConsensusParams() with a DefaultBlockGasLimit fallback, matching getHeader.
| // Genesis initialization needs deliver semantics even when populating the | ||
| // check state branch. Running it in CheckTx mode enables mempool-only fee | ||
| // checks and can reject valid genesis txs. | ||
| app.initChainer(app.checkState.ctx.WithIsCheckTx(false), *req) |
There was a problem hiding this comment.
[suggestion] (Also raised by Codex) This adds a third full initChainer invocation (after deliverState and processProposalState) to populate the checkState branch. For large genesis files this re-reads streamed genesis data and re-runs expensive module init — notably re-delivering genesis txs and re-instantiating Wasm code — increasing genesis startup cost by ~50%. Correctness looks fine since writes land in the checkState cache branch that is discarded at the first Commit, but if the cost is a concern, consider copying the already-computed deliverState genesis writes into checkState instead of re-executing the initChainer. Worth confirming initChainer side effects are fully idempotent across the (now three) runs.
There was a problem hiding this comment.
The core change (starting the EVM RPC endpoint on init rather than after the first finalized block, plus checkState/query wiring to serve genesis state before the first Commit) is sound, but a newly added test-harness helper contains a hex-vs-decimal comparison bug that makes it a no-op and will likely break the evm_rpc fixture tests it's meant to enable.
Findings: 1 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex reported no material issues.
- evmrpc/simulate.go: StateAndHeaderByNumberOrHash previously overwrote header.BaseFee with the latest next-base-fee for every request; the refactor now uses getHeader's per-block base fee (from height-1) for historical blocks. This is arguably more correct, but it is a behavior change for eth_call/estimateGas at historical block references — worth confirming it's intended.
| # Progress-only EVM send: these fixtures need real historical blocks first. | ||
| local tx_hash | ||
| tx_hash=$(run seid tx evm send "$RECIPIENT" 1 --from "$FROM" "${KEYRING_ARGS[@]}" --chain-id sei --evm-rpc "$EVM_RPC_URL" -b sync -y | grep -oE '0x[a-fA-F0-9]{64}' | head -1) | ||
| height=$(wait_for_receipt_field "$tx_hash" blockNumber) |
There was a problem hiding this comment.
[blocker] This loop exits after a single iteration, so bump_chain_to_height 100 only ever mines one block. get_latest_height returns a decimal (e.g. 5), but wait_for_receipt_field ... blockNumber returns a hex quantity (e.g. 0x6, per the eth JSON-RPC spec). On the next loop check [[ "0x6" =~ ^[0-9]+$ ]] is false, so the while terminates immediately. The chain never approaches height 100, so the static .io fixtures with hard-coded historical block numbers up to 0x2d (=45) will query blocks that don't exist yet. Convert the hex receipt height to decimal before assigning to height (e.g. height=$((height_hex))).
There was a problem hiding this comment.
This PR breaks the startup deadlock for evm-only chains running with allow_empty_blocks=false by initializing checkState and signaling EVM-RPC readiness on InitChain/InitLastHeader rather than waiting for the first FinalizeBlock, plus extensive test/harness updates to drive blocks by sending txs. The core baseapp and evmrpc changes are well-structured, well-tested, and non-app-hash-breaking; one minor gas-limit regression is worth addressing.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdandREVIEW_GUIDELINES.mdwere empty/blank, so no Cursor second opinion or repo-specific guidelines were available for this pass; only the single Codex finding was provided.- Behavioral note (not a bug):
StateAndHeaderByNumberOrHash/CurrentHeaderfor latest-like tags now resolve state viaLatestCtxHeight(checkState) and fall back to a synthetic header whenevertmBlock.Block.Height != sdkCtx.BlockHeight(). This is the intended pre-first-commit path, but it will also trigger if the watermark-bounded latest block ever lags checkState height at runtime, yielding a synthetic header numbered at the checkState height rather than the observed block. Confirm this edge is acceptable (tests only cover height-equal and pre-commit cases). - The integration/shell tests now hard-code
keyname=admin(replacingkeys list | .[0]) and assume anadminkey andnode_adminaccount exist in every harness; ensure all CI harness bootstraps provision those names, since a missing key would fail these scripts underset -e. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| baseFee := b.keeper.GetNextBaseFeePerGas(sdkCtx).TruncateInt().BigInt() | ||
| var gasLimit uint64 | ||
| if cp := sdkCtx.ConsensusParams(); cp != nil && cp.Block != nil { | ||
| gasLimit = uint64(cp.Block.MaxGas) //nolint:gosec |
There was a problem hiding this comment.
[suggestion] MaxGas == -1 is valid in Cosmos and means "unlimited", but uint64(cp.Block.MaxGas) converts it to math.MaxUint64, so the synthetic header exposes an invalid/absurd gas limit for latest-like queries before the first Commit. The sibling helper evmrpc/info.go:507 already guards this (if maxGas <= 0 { ... DefaultBlockGasLimit }), and the fallbackToEthHeaderOnly code this replaces used DefaultBlockGasLimit unconditionally — so this is a small regression for the -1 case. Suggest gating on cp.Block.MaxGas > 0 and falling back to keeper.DefaultBlockGasLimit otherwise (same fix applies to the pre-existing cast at line 788).
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
This PR fixes the startup deadlock for evm-only chains with AllowEmptyBlocks=false by initializing checkState (via new InitLastHeader / signalInitialized) and gating EVM RPC start on app initialization rather than the first finalized block, plus supporting changes to before-first-commit query/header resolution and a large integration-test refactor to drive blocks with txs. The core logic is coherent and well covered by new unit tests; I found no blocking correctness issues, only minor/non-blocking notes.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- I disagree with Codex's only finding (sei-tendermint/AGENTS.md:5 flagged as a prompt-injection risk). AGENTS.md is this repo's canonical, checked-in contributor/agent guide (imported by the top-level CLAUDE.md), and the additions are legitimate module conventions (nil-check policy, utils.Recv/Send, typed asserts, no artificial timeouts, proto-field guidance). It is not untrusted injected content and is not a security issue. It is, however, unrelated to the stated AllowEmptyBlocks fix and would be cleaner split into its own PR.
- The Cursor second-opinion review file (cursor-review.md) was empty — that pass produced no output, so its perspective is not represented in this synthesis.
- integration_test/evm_module/scripts/evm_rpc_tests.sh: bump_chain_to_height(100) issues up to 100 sequential -b sync EVM txs before the fixture runs, which can slow CI; also, if a tx never yields a receipt, wait_for_receipt_field times out (30s) and returns empty, and
height=$((height))collapses to 0, so the while-loop can keep looping indefinitely (bounded only by the overall test timeout rather than failing fast). Consider bounding iterations/erroring out. - app/app.go: the two RegisterLocalServices goroutines now block on
<-app.Initialized()andpanicon Start() error. Since the signal fires from InitChain/Commit/InitLastHeader and the channel is closed (not a one-shot send), behavior is correct across genesis, restart, and autobahn paths; worth a brief comment noting the sei-v2 trigger moved from ProcessBlock to Commit so future readers don't re-add the old ProcessBlock signalling. - sei-cosmos/baseapp/abci.go: InitChain now runs the initChainer a third time (on checkState with IsCheckTx=false). This is store-branch-isolated and non-app-hash-breaking, but relies on the genesis initChainer being free of non-store global side effects; a one-line comment stating that assumption would help maintainers.
There was a problem hiding this comment.
This PR breaks the genesis deadlock for EVM-only chains with allow_empty_blocks=false by opening the EVM JSON-RPC endpoint before the first block (via a new Initialized() channel), populating checkState at InitChain, and serving latest-like queries from checkState. The core change is coherent and backed by new unit tests; remaining items are non-blocking test-hygiene and coverage concerns.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Genesis eth_call/gas-estimation fidelity (Codex, downgraded from High): the new unit tests (genesis_latest_state_test.go) use freshChainClient whose Block(0)/BlockByHash return a synthetic height-0 block, so they don't verify that the real Autobahn Block RPC actually accepts an explicit height-0 fetch on the latest-like path (getBlockByNumberOrHash -> blockByNumberRespectingWatermarks with resolved height 0). The real path does appear to be exercised by the autobahn integration suite (testBlockProduction -> sendEvmTxAndWait submits the very first tx at genesis, which goes through
seid tx evm send/gas estimation), but it would be worth confirming that integration coverage actually hits eth_call/eth_estimateGas at height 0 rather than relying only on the mocked unit test. - Cursor produced no second-opinion review (cursor-review.md is empty). Only Codex output was available to merge.
- Behavior note (not a defect): StateAndHeaderByNumberOrHash/CurrentHeader now resolve latest-like tags against ctxProvider(LatestCtxHeight) (checkState) instead of ctxProvider(committedHeight). Post-commit checkState mirrors the committed state so results should match, and the base-fee assertions in simulate_test.go were updated accordingly, but reviewers relying on the old 'latest == last committed query context' semantics for eth_call should be aware of the switch to check-state execution.
- signalInitialized() is now deferred on every Commit (and InitChain) rather than only once; it's guarded by sync.Once so it's correct, just a tiny per-commit overhead — fine as-is.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| const kickAddr = await getKeySeiAddress(kickKeyName) | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
[nit] This while (true) loop has no overall deadline (unlike the shell wait_for_proposal_status, which takes a timeout_secs, and unlike the previous passProposal loop which was bounded to 200 iterations). It only terminates on reaching targetStatus, on REJECTED/FAILED, or on a missing/invalid voting_end_time. If a proposal stays in VOTING_PERIOD and the periodic bankSend "kick" fails to advance tallying (e.g. transient node/tx failure), this hangs indefinitely and the integration test only fails via an outer CI timeout with no useful diagnostic. Consider adding a bounded deadline (like the shell helper) that throws with the last observed status.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e70dd2e. Configure here.
| value: 1n, | ||
| gasPrice: ethers.parseUnits('100', 'gwei'), | ||
| }); | ||
| await advanceTx.wait(); |
There was a problem hiding this comment.
Unsafe receipt wait in test
Medium Severity
The new block-advancing path uses ethers wait() instead of mineTransferBlock / waitForReceipt. Under Autobahn, receipt polling can hit the transient “height not yet available” race and fail the test instead of retrying.
Triggered by learned rule: test: use tryGetReceipt wrapper for Autobahn-safe receipt polling
Reviewed by Cursor Bugbot for commit e70dd2e. Configure here.
There was a problem hiding this comment.
Solid, well-tested change that opens the EVM RPC on initialization so Autobahn can run with AllowEmptyBlocks=false; the interface/wiring is complete and consistent. One non-blocking robustness concern in the latest-like block resolution path (errors are swallowed into a synthetic header) plus a couple of minor nits.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- cursor-review.md was empty — the Cursor pass produced no output, so no second opinion was available from it.
- InitChain now runs the initChainer a third time (against checkState in addition to deliverState and processProposalState). This is intentional to populate the pre-first-commit query branch and is low-risk since each state is a separate store branch, but any non-store side effects in module InitGenesis (event emission, external calls) now execute one extra time — worth a mental note if any genesis hook is not idempotent.
- Historical (non-latest) simulations now derive BaseFee from getHeader (the block's own next-base-fee at height-1) instead of always using the latest next-base-fee. This is arguably more correct, but it is a behavior change for eth_call/estimateGas at historical heights and isn't called out in the PR description.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| sdkCtx := b.ctxProvider(LatestCtxHeight) | ||
| tmBlock, err := b.getBlockByNumberOrHash(ctx, blockNrOrHash) | ||
| if err != nil { | ||
| return sdkCtx, b.syntheticHeaderFromCtx(sdkCtx), true, nil |
There was a problem hiding this comment.
[suggestion] For latest-like refs, any error from getBlockByNumberOrHash — including context cancellation, timeouts, and transient tmClient/backend failures — is converted into a successful synthetic header (nil error). This masks genuine backend failures: an RPC that should surface an error instead returns a simulation computed against local checkState. Consider restricting this fallback to the known pre-first-commit / not-found case (e.g. checking for ErrZeroOrNegativeHeight / a zero latest height) and propagating other errors, and at minimum honoring ctx.Err() so cancellations aren't swallowed. (Raised by Codex.)
| } else { | ||
| header = b.fallbackToEthHeaderOnly(height) | ||
| _, header, _, err := b.resolveStateAndHeaderByNumberOrHash(context.Background(), rpc.BlockNumberOrHashWithNumber(rpc.LatestBlockNumber)) | ||
| if err != nil { |
There was a problem hiding this comment.
[nit] Minor: for latest-like references resolveStateAndHeaderByNumberOrHash never returns a non-nil error (it always falls back to a synthetic header), so this if err != nil branch in CurrentHeader is effectively dead code. Not harmful, but you can drop it or add a comment noting it's defensive.
| // After genesis initialization completes, CheckTx should use the first | ||
| // executable block height rather than height 0. | ||
| if initHeader.Height == 0 { | ||
| app.checkState.SetContext(app.checkState.ctx.WithBlockHeight(1)) | ||
| } |
There was a problem hiding this comment.
🔴 🔴 On a fresh Autobahn chain with allow_empty_blocks=false, eth_call/eth_estimateGas/eth_createAccessList at latest/safe/finalized/pending panic through go-ethereum's RPC recover(). After InitChain bumps checkState.BlockHeight to 1 (sei-cosmos/baseapp/abci.go:87-89) while app.LastBlockHeight() remains 0, the new fallback in resolveStateAndHeaderByNumberOrHash (evmrpc/simulate.go:730-738) returns syntheticHeaderFromCtx(sdkCtx) with header.Number = 1. go-ethereum's doCall (internal/ethapi/api.go:668) then calls b.GetCustomPrecompiles(header.Number.Int64()) = b.ctxProvider(1) = app.RPCContextProvider(1). Because 1 is not LatestCtxHeight(-1), it calls CreateQueryContext(1, false), whose if height > lastBlockHeight guard (abci.go:688) fires (1 > 0) and returns ErrInvalidHeight; RPCContextProvider at app.go:2656 panic(err)s. The client receives "something went wrong: cannot query with height in the future". The new PR shim at abci.go:711 only handles height == 0, not height == 1; GetEVM at simulate.go:664-666 has the same panic paths via chainConfigForHeight(1) and CustomPrecompiles(ctxProvider(1)). This defeats the exact scenario this PR is designed to enable — wallets and dapps that eth_call at latest on startup (MetaMask balance/allowance checks, gas estimation before signing) will fail until a first tx lands.
Extended reasoning...
What the bug is
This PR introduces two changes that interact to produce a user-visible panic:
- InitChain moves
checkStateto height 1 (sei-cosmos/baseapp/abci.go:86-89): afterinitChainerruns oncheckState.ctx, the PR doesapp.checkState.SetContext(app.checkState.ctx.WithBlockHeight(1))so CheckTx sees the first executable block height.LastBlockHeight()continues to returncms.LastCommitID().Version= 0 until the firstCommit. - New synthetic-header fallback for latest-like tags (evmrpc/simulate.go:723-738):
resolveStateAndHeaderByNumberOrHashreturnssyntheticHeaderFromCtx(sdkCtx)when the tm-block lookup errors or whentmBlock.Block.Height != sdkCtx.BlockHeight().syntheticHeaderFromCtxsetsNumber = big.NewInt(sdkCtx.BlockHeight())= 1.
The panic is triggered downstream in go-ethereum's doCall.
Step-by-step proof
Fresh Autobahn chain, allow_empty_blocks=false, after InitChain, before first Commit. Client sends eth_call at latest:
SimulationAPI.Call→export.DoCall→Backend.StateAndHeaderByNumberOrHash(evmrpc/simulate.go:316).resolveStateAndHeaderByNumberOrHashseesisLatestLikeBlockRef=true, setssdkCtx = ctxProvider(LatestCtxHeight)→ checkState withBlockHeight() = 1.getBlockByNumberOrHasheither errors (fresh chain,env.getHeightrejects height 0 withErrZeroOrNegativeHeight) or returns a block withHeight = 0. Either way, the synthetic branch at line 735 fires and returnsheaderwithNumber = 1.StateAndHeaderByNumberOrHashsucceeds.export.DoCallproceeds todoCall(go-ethereum internal/ethapi/api.go:660).- Line 668:
precompiles := vm.ActivePrecompiledContracts(rules, b.GetCustomPrecompiles(header.Number.Int64()))— that isb.GetCustomPrecompiles(1). Backend.GetCustomPrecompiles(evmrpc/simulate.go:801-803):return b.keeper.CustomPrecompiles(b.ctxProvider(1)).b.ctxProviderisapp.RPCContextProvider(app/app.go:2725, 2643). Fori=1(notLatestCtxHeight = -1), it callsapp.CreateQueryContext(1, false).CreateQueryContextat sei-cosmos/baseapp/abci.go:687-694:lastBlockHeight = 0;height (1) > lastBlockHeight (0)→ returnsErrInvalidHeight"cannot query with height in the future". The PR's new shim at line 711 only handleslastBlockHeight == 0 && height == 0, soheight == 1never reaches it.RPCContextProviderat app.go:2655-2657:panic(err).- The panic is caught by
SimulationAPI.Call's defer/recover at simulate.go:186-193, converted to"something went wrong: cannot query with height in the future"and returned as an internal error to the client.
The same panic recurs via GetEVM at simulate.go:664-666 (chainConfigForHeight(1) and CustomPrecompiles(ctxProvider(1))) if execution reaches that path, so all three call sites into ctxProvider(1) on a fresh chain panic identically.
Why the new tests miss it
TestSimulationBackendLatestLikeTagsUseGenesisCheckStateBeforeFirstCommit at evmrpc/genesis_latest_state_test.go only calls backend.StateAndHeaderByNumberOrHash directly. It never proceeds through DoCall → GetCustomPrecompiles → ctxProvider(1), so the panic is not exercised. Even more telling: the test's own ctxProvider closure asserts require.NoError(err) inside CreateQueryContext(height, false) — that assertion would itself fail if the test ever invoked ctxProvider(1) in the current testApp state (app.Setup runs InitChain without Commit, matching the fresh-chain LastBlockHeight=0 scenario).
Impact
Under this PR's Autobahn allow_empty_blocks=false default (sei-tendermint/cmd/tendermint/commands/gen_autobahn_config.go), the fresh-chain window persists indefinitely until someone submits a tx. During that window every eth_call/eth_estimateGas/eth_createAccessList at latest-like tags — the exact tags the PR added tests for — panics through RPCContextProvider(1). Wallets and dapps that read at startup (MetaMask account setup, gas estimation before signing) fail with a generic "something went wrong" error until a first tx lands. Note the primary eth_sendRawTransaction deadlock the PR set out to fix is still resolved — that path does not go through StateAndHeaderByNumberOrHash.
Fix
Any of the three options from the bug description works, each small:
- (a) In
RPCContextProvider, wheni == checkState.BlockHeight()andlastBlockHeight == 0, fall through to theLatestCtxHeightbranch and return checkState. - (b) Extend the existing
lastBlockHeight == 0 && height == 0shim inCreateQueryContext(abci.go:711) to also coverheight == checkState.BlockHeight()during the pre-first-commit window. - (c) Use
LatestCtxHeightinstead of the numeric header height inchainConfigForHeight/GetCustomPrecompileswhenheader.Numbermatches checkState andlastBlockHeightis 0.
Pair the fix with an end-to-end eth_call test on a fresh chain that drives DoCall (not just StateAndHeaderByNumberOrHash) so this regression cannot recur.
wen-coding
left a comment
wen-coding
left a comment
There was a problem hiding this comment.
I think this would work. An alternative is what I mentioned: "process genesis block and then open for business".
InitChain -> Autobahn seals only InitialHeight empty (ignore AllowEmptyBlock for block InitialHeight specifically) -> Commit block InitialHeight -> open RPC -> enforce AllowEmptyBlock for all blocks starting InitialHeight+1
| if isLatestLikeBlockRef(blockNrOrHash) { | ||
| sdkCtx := b.ctxProvider(LatestCtxHeight) | ||
| tmBlock, err := b.getBlockByNumberOrHash(ctx, blockNrOrHash) | ||
| if err != nil { |
There was a problem hiding this comment.
yeah, what seidroid said, should we just match the specific error ErrZeroOrNegativeHeight?
| // a coherent (state, header) view. | ||
| func (b *Backend) resolveStateAndHeaderByNumberOrHash(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (sdk.Context, *ethtypes.Header, bool, error) { | ||
| if isLatestLikeBlockRef(blockNrOrHash) { | ||
| sdkCtx := b.ctxProvider(LatestCtxHeight) |
There was a problem hiding this comment.
Would we still go this path after restart? Then in the future errors will still be swallowed below?
2 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 also stays for backward compatibility. InitLastHeader is only used in autobahn and does not affect sei-v2.
Additionally