feat(evmonly): persist executor receipts - #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.26% 60.33% -0.94%
==========================================
Files 2188 2083 -105
Lines 192258 179709 -12549
==========================================
- Hits 117793 108427 -9366
+ Misses 63301 61149 -2152
+ Partials 11164 10133 -1031
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 A concurrency-safe Tendermint node startup centralizes Autobahn storage: the configured block DB lives on the manager, Reviewed by Cursor Bugbot for commit 5de8bc1. 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.
| receipt *evmtypes.Receipt | ||
| } | ||
|
|
||
| // MemoryReceiptStore retains receipts in memory by transaction hash and block. |
There was a problem hiding this comment.
[suggestion] Still unaddressed from the last round, and Codex flags it independently: MemoryReceiptStore grows without bound. blocks and byTxHash retain every receipt (with its logs) for the process lifetime, and nothing ever calls PruneHistory — NewGigaStorageManagerWithStores does not run startGarbageCollector, so the store joins no prune cycle even though ExternalPruning() reports true.
Before this PR the EVM-only runtime dropped receipts with the block result, so a long Autobahn Docker load run now carries a steady-state memory cost proportional to total transactions rather than to live state. Either cap retention inside SetReceipts (a KeepRecent-style floor) or have the EVM-only app call PruneHistory on commit. Failing that, a sentence on this godoc saying retention is unbounded absent an external collector would at least make the constraint visible where the store is constructed.
| } | ||
|
|
||
| // NewGigaStorageManagerWithStores returns a manager that owns the supplied stores. | ||
| func NewGigaStorageManagerWithStores( |
There was a problem hiding this comment.
[suggestion] A manager built here is a different animal from one built by NewGigaStorageManager, and the godoc says none of it: cfg is nil, no recovery runs, no garbage collector starts, and stateDB stays nil while stateStore is set — so StateDB(), StateWAL(), SC() and SS() all return nil, prunableStores() skips the injected state store because it gates on stateDB, and recoverStores would nil-deref on m.stateDB.RollbackTo(target).
Nothing reaches those paths today (only OpenDBWithRecovery calls recoverStores, and only the config-driven constructor calls that), but GigaStorageManager is the production storage owner, so the next caller has no way to know which half of its surface is live. Worth stating the contract on this godoc — which accessors are valid, and that retention/recovery are the caller's problem — or guarding recoverStores on a nil stateDB.
|
|
||
| // 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, unchanged: taking the concrete *bootstrap.GigaStorageManager pulls sei-db/bootstrap (littblock, flatkv, controller, the checkpoint scheduler, autobahn/blockstore) into giga/evmonly, which previously depended only on sei-db/state_db/giga and sei-db/proto — including for the standalone evmonly-loadtest binary.
The executor uses exactly two methods. Declaring them here — type StorageManager interface { StateStore() gigatypes.StateDB; ReceiptDB() receipt.ReceiptStore } — keeps *bootstrap.GigaStorageManager a valid argument, lets withTestStores and the load harness pass a two-field struct instead of routing through NewGigaStorageManagerWithStores(nil, ...), and drops the bootstrap dependency. (The part of the earlier comment about the constructor being test-shaped no longer holds — openAutobahnStorageManager is a production caller now — but the dependency direction is the same.)
| func NewEVMOnlyInMemoryApplication( | ||
| chainID uint64, | ||
| validators []abci.ValidatorUpdate, | ||
| blockStore *blockstore.Store, |
There was a problem hiding this comment.
[suggestion] All three call sites in app_test.go pass nil here, but the godoc does not say nil is allowed. sei-tendermint/AGENTS.md requires utils.Option for optional values and disallows passing nil as a function value unless explicitly documented. A nil block store is also semantically load-bearing — it produces a manager that createRouter then rejects with "autobahn requires a storage manager with a block store" — so either take utils.Option[*blockstore.Store] or state on the godoc that nil means "no block store, tests only".
1 participant
Summary
receipt.ReceiptRecordformat and persist them through the existingreceipt.ReceiptStoreinterfacebootstrap.GigaStorageManagerand remove the EVM-only in-memory manager implementationmanager.BlockStore()giga.Runexits, including construction and startup failure cleanupTesting
go test ./giga/evmonly/...go test ./sei-db/bootstrapgo test ./sei-db/ledger_db/receiptgo test ./sei-tendermint/internal/evmonlyappgo test ./sei-tendermint/internal/p2pgo test ./sei-tendermint/node -run "TestPrepareApplication|TestValidateNodeSetupConfig|TestBuildGigaConfig|TestPreparePersistentStateDir" -count=1go test -race ./giga/evmonly ./sei-tendermint/internal/evmonlyappgo vet ./giga/evmonly/... ./sei-tendermint/nodegolangci-lint run --timeout 10m— 0 issuesgolangci-lint fmt --diffgofmtandgoimportson every touched Go fileNotes
sei-tendermint/nodetest package reaches an unrelated local-port collision inTestFreezeModeDisablesMempoolTraffic; the targeted node setup tests pass independently.