Integrate evmonly executor with giga store - #3864
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 #3864 +/- ##
==========================================
- Coverage 59.07% 58.03% -1.05%
==========================================
Files 2305 2208 -97
Lines 196778 185680 -11098
==========================================
- Hits 116249 107761 -8488
+ Misses 69790 68086 -1704
+ Partials 10739 9833 -906
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
a80537b to
66a304c
Compare
1a66532 to
819a564
Compare
PR SummaryHigh Risk Overview Adds Updates evmonly-loadtest to wire Reviewed by Cursor Bugbot for commit 819a564. 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 making the evmonly executor store-only over the giga StateDB interface; the encoder slab arithmetic, storage-clear versioning, commit validation ordering, and error/release paths all check out. One suggestion: MemoryStore's touch-based AccountExists diverges from the account-existence semantics sei-db/state_db/giga/api.go documents.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
--workersin the loadtest now rejects every value except its default of1(cmd/evmonly-loadtest/config.go), leaving a CLI flag with a single legal value. Either drop the flag until the harness can order commits across workers, or keep it and have the harness serializeCommitStateChangesitself so the knob stays meaningful. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| _, codeTouched := latestMemoryStoreValue(s.store.code[addr], s.height) | ||
| firstStorageTouch, storageTouched := s.store.storageTouch[addr] | ||
| s.store.mu.RUnlock() | ||
| if balanceTouched || nonceTouched || codeTouched || storageTouched && firstStorageTouch <= s.height { |
There was a problem hiding this comment.
[suggestion] AccountExists is touch-based rather than value-based: once any balance/nonce/code node or storage touch is recorded for an address, it reports true at every height from that point on. Two consequences diverge from the contract in sei-db/state_db/giga/api.go:
- A block that self-destructs a contract emits
BalanceChange{Balance: 0}, a code delete, and aStorageClear. All three create nodes, andtouchStorageAccountLockedrecords the clear as a storage touch, so the destroyed account reportsAccountExists() == trueforever — not just "in the current block" as the interface doc allows.GetCodeHash(line 451) then returnskeccak256("")where the doc specifies the zero hash for non-existent accounts, i.e. the opposite of EXTCODEHASH /keeper.GetCodeHashbehaviour. TestMemoryStoreTracksZeroValueAndStorageOnlyAccountspins the same shape for an account whose only write isBalance: 0, which a real SC/SS store would not materialize as an existing account.
Nothing consumes this today — gigaSnapshotStateReader only implements GetBalance/GetNonce/GetCode/GetState, and nativeStateDB.Exist derives existence itself — so this is not a correctness bug in the executor path. But MemoryStore is the first and only implementation of gigastore.StateDB and is positioned as the test double for it, so tests written against these semantics will pass here and fail against the production store. Consider resolving existence from the effective values at s.height (non-zero balance, non-zero nonce, non-empty code, or a live storage slot) instead of from touch records, and dropping storageTouch in favour of a height-versioned marker — or, if the touch-based approximation is deliberate, say so in the MemoryStore godoc and note the GetCodeHash deviation.
| } | ||
|
|
||
| func (e *Executor) executePreparedBlock(ctx context.Context, req PreparedBlock) (*BlockResult, error) { | ||
| func (e *Executor) executePreparedBlock(ctx context.Context, req PreparedBlock, source StateReader) (*BlockResult, error) { |
There was a problem hiding this comment.
Is source properly closed after the execution is done? The way how StateSnapshot works require the underlying goroutine to close the reader after usage is done
There was a problem hiding this comment.
Yes. source is only an adapter around the snapshot opened in executePreparedBlockWithStore, and that function registers defer snapshot.Close() immediately after the nil check. Sequential execution is synchronous; on the OCC path, occWorkerPool.Run returns only after errgroup.Wait() has joined every worker. Each worker also releases its nativeStateDB and resets its source before executePreparedBlock returns. Therefore the opening goroutine closes the snapshot only after all executor goroutines have stopped using it, including error paths, and before ExecutePreparedBlock proceeds to the result sink. TestExecutorGigaStoreSnapshotFeedsOCCExecution covers closure on the OCC path.
| @@ -45,6 +45,8 @@ type StateDB interface { | |||
| // | |||
| // Until Close, the underlying resources (e.g. an ephemeral SC snapshot or a | |||
| // pinned SS version) stay alive, even concurrently with later writes/commits. | |||
| // All read methods must be safe for concurrent calls because EVM executor | |||
| // workers may share one snapshot while executing a block. | |||
There was a problem hiding this comment.
We will rename this to StateView in the future PR, just FYI, no action needed for now
2 participants
Summary
CommitStateChangesNamedChangeSetEncoderand preserve storage-prefix clearsMemoryStoreimplementation over the existing immutableStateReaderNamedChangeSetkey/value pairs with contiguous backing allocationsWhy
The evmonly executor now has one persistence model: a giga
Store. The concrete store implementation can vary, but execution no longer has a separate non-giga state path.The first loadtest adapter wrapped the complete native changeset in RLP and decoded it immediately inside
CommitStateChanges. The direct format removes that redundant work while continuing to exercise the real giga interface.Loadtest
Configuration: 400 blocks, 1,000 transactions/block, one ordered block worker, 12 executor workers, zero gas price, and discard result sink. Values are three-run medians in tx/s.
Every run completed 400,000/400,000 transactions successfully with zero execution errors and zero OCC fallbacks.
The pre-MemoryStore comparison is not an equivalent persistence implementation: it executes against
WithStateand discards block state changes, while the Giga run encodes, commits, and retains current and historical state for later snapshots. It is therefore a useful lower bound on commit overhead, not evidence that the Giga interface itself costs 10% in production.In an 800-block snapshot/revert profile,
EncodeMemoryStoreChangeSetandCommitStateChangeseach represented about 0.1% of sampled CPU. Most MemoryStore-specific allocation was the retained versioned storage map. A pointer-free indexed-history experiment did not improve end-to-end throughput and was reverted.Validation
go test ./giga/... ./sei-db/state_db/gigago test -race ./giga/evmonly/...go vet ./giga/evmonly/... ./sei-db/state_db/gigagofmt -s -landgoimports -lgit diff --checkThe full-tree
goimports -l .reports pre-existing untouched generated and test files.