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.30% 60.27% -1.04%
==========================================
Files 2178 2074 -104
Lines 190788 178692 -12096
==========================================
- Hits 116968 107707 -9261
+ Misses 62796 60957 -1839
+ Partials 11024 10028 -996
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 Tests and the load harness wire Tendermint / Autobahn centralizes storage: the node opens the configured block DB under one manager, Reviewed by Cursor Bugbot for commit af5a2d7. 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.
| blocks. `ResultSink` runs after both stores succeed; a persistence error does | ||
| not roll back the state commit. | ||
|
|
||
| `MemoryStorageManager` supplies the non-persistent state and receipt |
There was a problem hiding this comment.
[suggestion] MemoryStorageManager does not exist anywhere in the tree — grep -r MemoryStorageManager matches only this line. The PR removed the EVM-only in-memory manager in favour of bootstrap.NewGigaStorageManagerWithStores, so this paragraph documents a type readers cannot find. Reword to describe MemoryStore and MemoryReceiptStore as the two non-persistent implementations installed into a bootstrap.GigaStorageManager.
Relatedly, "Neither is a production implementation" is a bit strong now that MemoryReceiptStore backs the Autobahn EVM-only app rather than only tests.
| if err := stateStore.CommitStateChanges(blockNumber, changesets); err != nil { | ||
| return nil, fmt.Errorf("commit state changes for block %d: %w", req.Context.Number, err) | ||
| } | ||
| if err := receiptStore.SetReceipts(newReceiptContext(ctx, blockNumber), records); err != nil { |
There was a problem hiding this comment.
[suggestion] Receipts are written after CommitStateChanges, and that ordering makes a receipt failure unrecoverable rather than retryable: the state store has already advanced its height, so re-executing the same block fails (MemoryStore.CommitStateChanges rejects blockNum <= currentHeight at memory_store.go:231, and the giga store is height-ordered too). A cancelled context during shutdown reaches this exact path, since MemoryReceiptStore.SetReceipts returns ctx.Err() after the commit already landed.
Swapping the two writes removes the trap: receipt writes are keyed by tx hash and idempotent, so writing them first and leaving CommitStateChanges as the last (height-advancing) step means a receipt failure leaves the block fully retryable, and a state-commit failure leaves only receipts that the retry overwrites. If the current order is deliberate, the README sentence "a persistence error does not roll back the state commit" should also say the block cannot then be re-executed.
|
|
||
| // 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] Taking the concrete *bootstrap.GigaStorageManager pulls sei-db/bootstrap (littblock, flatkv, controller, the checkpoint scheduler) into giga/evmonly, which previously depended only on sei-db/state_db/giga and sei-db/proto. It also forced a test-shaped constructor, NewGigaStorageManagerWithStores, into the production bootstrap package, where every current caller passes a nil block store just to hold two in-memory stores.
The executor only needs two methods. Declaring them in evmonly — e.g. type StorageManager interface { StateDB() gigastore.StateDB; ReceiptDB() receipt.ReceiptStore } — keeps *bootstrap.GigaStorageManager a valid argument (it already satisfies that shape), lets tests and the load harness pass a two-field struct, and drops the bootstrap dependency along with the extra constructor. It would also make the README's "satisfies this contract" wording literally true.
| receipt *evmtypes.Receipt | ||
| } | ||
|
|
||
| // MemoryReceiptStore retains receipts in memory by transaction hash and block. |
There was a problem hiding this comment.
[suggestion] MemoryReceiptStore grows without bound: blocks and byTxHash retain every receipt (with its logs) for the process lifetime, and nothing calls PruneHistory — NewGigaStorageManagerWithStores never starts a StorageGarbageCollector, so the store joins no prune cycle. Before this PR the EVM-only runtime dropped receipts with the block result, so a long Autobahn Docker load run now carries a new steady-state memory cost proportional to total transactions rather than to live state.
Worth either capping retention in the store (a KeepRecent-style floor applied inside SetReceipts) or having the EVM-only app call PruneHistory on commit. A note on this godoc that retention is unbounded absent an external collector would at least make the constraint visible at the call site.
| storageManager, | ||
| nodeOptions..., | ||
| ) | ||
| case config.ModeSeed: |
There was a problem hiding this comment.
[suggestion] The seed branch never sets storageManagerTransferred and never closes the manager, so a successful makeSeedNode return would leak the block store that prepareApplication opened (holding the littDB directory lock for the node's lifetime).
This is not reachable today: EVMOnlyInMemory requires AutobahnConfigFile (validateNodeSetupConfig), and makeSeedNode passes utils.None[*proxy.Proxy]() to createRouter, which fails with "autobahn requires app" whenever AutobahnConfigFile != "" — so the error path always runs the deferred close. But the ownership protocol this PR introduces then depends on an invariant two functions away. Rejecting EVMOnlyInMemory in seed mode inside validateNodeSetupConfig, or closing the manager before returning the seed node, makes it hold locally.
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-db/bootstrap ./sei-tendermint/internal/evmonlyapp ./sei-tendermint/nodegofmtandgoimportson every touched Go fileNotes
make fmtcheckcannot start locally because the pinned golangci-lint executable was built with Go 1.24, below the repository target of Go 1.25.6.sei-tendermint/nodetest package reaches an unrelated local-port collision inTestFreezeModeDisablesMempoolTraffic; the targeted node setup tests pass independently.