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.21% 60.32% -0.89%
==========================================
Files 2196 2085 -111
Lines 193395 179810 -13585
==========================================
- Hits 118386 108475 -9911
+ Misses 63793 61179 -2614
+ Partials 11216 10156 -1060
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
Reviewed by Cursor Bugbot for commit b930957. 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.
| } | ||
|
|
||
| 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.
3 participants
Summary
receipt.ReceiptStoreowned by the existingbootstrap.GigaStorageManagerMemoryStoreandMemoryReceiptStoreas documented unit-test implementations onlyevm-only-in-memorytoevm-onlyStorage boundary
This PR has no net changes under
sei-db. It consumes the existing manager APIs:StateDB(),ReceiptDB(), andBlockStore(). The standalone harness opens the full production storage manager, including the block store, but has no consensus layer to populate blocks.Testing
scripts/ramtest.sh ./giga/evmonly/... ./sei-tendermint/internal/evmonlyapp/... ./sei-tendermint/config/... ./cmd/autobahn-e2e/... -count=1scripts/ramtest.sh ./sei-tendermint/node/... -run 'TestSelectAutobahnBlockStoreOwnership|TestPrepareApplicationEVMOnly|TestValidateNodeSetupConfig|TestBuildGigaConfig|TestPreparePersistentStateDir' -count=1scripts/ramtest.sh ./giga/evmonly/... ./sei-tendermint/internal/evmonlyapp/... -race -count=1go test -tags autobahn_integration -c ./integration_test/autobahngolangci-lint run --timeout 10m— 0 issuesgolangci-lint fmt --diffgofmtandgoimportson every touched Go filebash -n docker/localnode/scripts/step4_config_override.sh docker/localnode/scripts/step5_start_sei.shgit diff --quiet origin/main...HEAD -- sei-db