feat(evmonly): run load tests on Giga storage - #4088
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 #4088 +/- ##
==========================================
- Coverage 61.24% 60.28% -0.97%
==========================================
Files 2190 2079 -111
Lines 191985 179206 -12779
==========================================
- Hits 117587 108032 -9555
+ Misses 62978 60821 -2157
+ Partials 11420 10353 -1067
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview The EVM-only executor now requires both a state store and a
Reviewed by Cursor Bugbot for commit 8dab03b. 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.
Solid, well-tested refactor that routes EVM-only executor receipts through the shared receipt.ReceiptStore and moves Autobahn block-store ownership into a GigaStorageManager that the node closes after giga.Run. No blockers; the notable items are a README reference to a type that doesn't exist, an unrecoverable state/receipt ordering on the persistence path, and the executor now depending on the concrete bootstrap manager.
Findings: 0 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
newReceiptContextbuilds ansdk.Contextwith a nilMultiStore. That is safe for both currentReceiptStorebackends (the write path never touches the store, andlegacyReceiptFromKVStoreguards on a nilstoreKey), but nothing records the assumption. A one-line comment onnewReceiptContextstating that the returned context carries no store and is only valid for backends constructed without a legacystoreKeywould keep the next caller from hitting a nil deref viactx.KVStore. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The three fixes from the last round landed correctly: receipts are now written before the height-advancing state commit (with tests covering both failure directions), seed mode rejects EVMOnlyInMemory at config validation, and the README no longer references a nonexistent type. Storage ownership through New → makeNode → closeGigaStorageManager is leak-free on every path I traced; the remaining items are the two unresolved design/retention suggestions plus two new ones on the injected-manager contract.
Findings: 0 blocking | 5 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
newReceiptContext(giga/evmonly/receipt.go) still builds ansdk.Contextwith a nilMultiStoreand carries no comment recording that. It is safe for both current backends (receiptStore.SetReceiptsnever touches the multistore, andMemoryReceiptStoreignores it), but the executor now accepts anyreceipt.ReceiptStoreviaWithStorageManager, so a one-line note that the returned context has no store would keep the next backend from hitting a nil deref throughctx.KVStore. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
codchen
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ 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 93687db. Configure here.
|
@seidroid review |
4 similar comments
|
@seidroid review |
|
@seidroid review |
|
@seidroid review |
|
@seidroid review |
There was a problem hiding this comment.
Solid follow-up: the EVM-only executor now persists non-balance state through FlatKV and receipts through the manager-owned littidx store, MemoryStore/MemoryReceiptStore are demoted to unit-test doubles, and the NewGigaStorageManagerWithStores test-shaped constructor is gone (the PR has no net sei-db diff). No blockers; the remaining items are restart semantics of the now-persistent EVM-only app, a fail-closed gap when a FlatKV state store is used without a balance store, and two convention/dependency points.
Findings: 0 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
newReceiptContext(giga/evmonly/receipt.go) still builds ansdk.Contextwith a nilMultiStoreand carries no comment saying so. It is safe for the two backends in play —littReceiptStore.SetReceiptsnever touches the multistore and itsGetReceiptfallback guards on a nilstoreKey— but the executor accepts anyreceipt.ReceiptStore, so a one-line note that the returned context has no store would keep the next backend from hitting a nil deref throughctx.KVStore. - [suggestion]
giga/evmonly/cmd/evmonly-loadtest/pipeline.gocommits generated genesis at height 1, butstate.changeSet()emits only nonce/code/storage pairs, so a pure-transfer run with all-zero nonces producesnilchangesets and the genesis commit is an empty block. That works (FlatKV accepts an empty contiguous block), but neitherflatkv_changeset_test.gonor the loadtest tests cover the empty-changeset encode → commit path, which is the default workload's shape. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| func evmOnlyInMemoryGasLimit(req *abci.RequestInitChain) (uint64, error) { | ||
| func (a *evmOnlyApplication) seedInitialStateVersion(initialHeight int64) error { | ||
| stateStore := a.storage.SC() | ||
| if stateStore == nil || initialHeight == 1 { |
There was a problem hiding this comment.
[suggestion] The initialHeight == 1 short-circuit skips the guard in exactly the case where it is needed. State, receipts and blocks now live in data/autobahn (step4 no longer passes --persistent-state-dir=), so that directory survives a container/process restart, while evmOnlyState — committedHeight, appHash, parentHash — and the whole PlaceholderBalanceStore are rebuilt empty by NewEVMOnlyApplication. On a restart of a genesis-height-1 chain, Info() reports height 0, InitChain runs again, this function returns early without noticing that SC().GetLatestVersion() is already N, and the first FinalizeBlock fails at CommitStateChanges(1, ...) because FlatKV requires contiguous versions (store_apply.go:45). Codex flags the same path.
It fails closed rather than diverging, but the diagnostic lands two layers away from the cause. Dropping the initialHeight == 1 clause makes the existing "EVM-only state is already at height %d before InitChain" error fire for the common case too (a fresh store returns 0, so height 1 still passes), and SetInitialVersion is already equivalent to a no-op there. A close/reopen test would pin whichever behaviour you choose — refuse to start on a populated directory, or recover committedHeight/appHash from storage.
There was a problem hiding this comment.
step4 no longer passes --persistent-state-dir=, so data/autobahn survives a restart, while committedHeight, appHash and the whole PlaceholderBalanceStore are rebuilt empty by NewEVMOnlyApplication. On restart, storage recovers itself correctly and comes back on height N; the app reports height 0, InitChain reruns, seedInitialStateVersion returns early, and FinalizeBlock(1) executes. executePreparedBlockWithStore writes receipts before the height-advancing state commit.
Block 1 now holds a receipt from an execution that was never committed, produced against a fresh zero-balance placeholder.
| } | ||
|
|
||
| func (r gigaSnapshotStateReader) GetBalance(addr common.Address) *big.Int { | ||
| if r.balances != nil { |
There was a problem hiding this comment.
[suggestion] The balance store is optional here, but for the FlatKV-backed state store it is mandatory: flatKVStateView.GetBalance panics (sei-db/state_db/sc/flatkv/state_view.go:107), and encodeFlatKVChangeSet silently drops changes.Balances. So WithStorageManager(manager, NewFlatKVChangeSetEncoder(...)) without a matching WithBalanceStore panics inside the state view on the first balance read, rather than failing with an executor-level error like the sibling errMissingStateStore / errMissingReceiptStore checks.
Both production call sites pass WithBalanceStore today, so this is a latent misconfiguration rather than a live bug — but it is a guard that belongs at the choke point. Either have executePreparedBlockWithStore reject a nil balanceStore when the state view cannot serve balances, or bundle the encoder and the placeholder store into one option so they cannot be supplied apart.
| return giga, ownedBlockStore, nil | ||
| } | ||
|
|
||
| func selectAutobahnBlockStore( |
There was a problem hiding this comment.
[suggestion] The second return value uses a bare nil as "not owned by the caller", which buildGigaRouter propagates and createRouter then tests with if blockStore != nil, plus a closeAutobahnBlockStore helper that exists only to nil-check. sei-tendermint/AGENTS.md asks for utils.Option for optional values and says not to add defensive nil-checks in internal logic — and this file already uses utils.Option[atypes.BlockStore] for exactly this value on the way in (managed) and on the way out of createRouter.
Returning utils.Option[atypes.BlockStore] for the owned store would make ownership explicit at every hop and let closeAutobahnBlockStore collapse into if store, ok := owned.Get(); ok.
|
|
||
| // WithStorageManager selects the stores used for state and receipt persistence. | ||
| // The encoder converts executor-native state changes into the state store's format. | ||
| func WithStorageManager(manager *bootstrap.GigaStorageManager, encoder NamedChangeSetEncoder) Option { |
There was a problem hiding this comment.
[suggestion] Repeating from the last round, with the case now narrower: WithStore and WithReceiptStore are exported here, so WithStorageManager is a two-line convenience wrapper — and it is the only reason giga/evmonly imports sei-db/bootstrap, pulling littblock, flatkv, controller, the checkpoint scheduler and autobahn/blockstore into the package (and into the standalone evmonly-loadtest binary).
Both production call sites already have the manager in hand and could call WithStore(manager.StateDB(), encoder) plus WithReceiptStore(manager.ReceiptDB()) directly, or pass a locally-declared two-method interface. That drops the import and removes the if manager != nil branch, which currently swallows a nil manager into a much later errMissingStateStore.
| if err != nil { | ||
| return fmt.Errorf("configure storage manager: %w", err) | ||
| } | ||
| storage, err := bootstrap.NewGigaStorageManager(ctx, storageConfig.WithFullNodeMode()) |
There was a problem hiding this comment.
We should start with ValidatorMode since GigaSS hasn't really been wired up yet
There was a problem hiding this comment.
Agreed. EVM-only now starts from WithValidatorMode() so GigaSS stays disabled. Because validator mode also disables ReceiptDB, the shared EVM-only validator config explicitly re-enables it so executor receipts remain disk-backed.
b930957 to
547ee42
Compare
9c13e70 to
8dab03b
Compare
4 participants
Summary
receipt.ReceiptStoreowned by the existingbootstrap.GigaStorageManagereth_getTransactionReceiptfrom the manager-owned receipt store, with finalized block hashes supplied by the Autobahn-aware block readernullfor unknown or not-yet-committed transactionsMemoryStoreandMemoryReceiptStoreas documented unit-test implementations onlyevm-only-in-memorytoevm-onlyStorage boundary
This branch is rebased onto the current
main, including #4101, and has no net changes undersei-db. It consumes the existing manager APIs:StateDB(),ReceiptDB(), andBlockStore(). EVM-only uses a shared validator storage configuration: FlatKV, littidx receipts, and littblock remain enabled, while GigaSS is disabled because EVM-only execution does not use it. The standalone harness opens the block store but has no consensus layer to populate blocks.Testing
scripts/ramtest.sh ./giga/evmonly/... ./sei-tendermint/internal/evmonlyapp/... -count=1scripts/ramtest.sh ./giga/evmonly/... ./sei-tendermint/internal/evmonlyapp/... -race -count=1go test -race ./giga/evmonly/rpc -count=1scripts/ramtest.sh ./sei-tendermint/node/... -run TestSelectAutobahnBlockStoreOwnership\|TestPrepareApplicationEVMOnly\|TestValidateNodeSetupConfig -count=1scripts/ramtest.sh ./sei-tendermint/config/... ./cmd/autobahn-e2e/... -count=1GOWORK=off go test -c -tags autobahn_integration ./integration_test/autobahnmake autobahn-evmonly-integration-test— four local Docker validators finalized 4,000 raw EVM transfers in 1.192 s (3,356 tx/s), every validator reached at least height 12, and every validator served a persisted receipt with validated block and transaction fieldsgolangci-lint run --timeout 10m— 0 issuesgolangci-lint fmt --diffgofmtandgoimportson every touched Go filebash -n docker/localnode/scripts/step4_config_override.shgit diff --quiet origin/main -- sei-dbmake fmtcheckinvokes a cachedgo runlinter binary built with Go 1.24 and rejects the repository Go 1.25.6 target locally; the equivalent installedgolangci-lint fmt --diffcheck passes.