Replace benchmark DB wrapper - #4112
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4112 +/- ##
==========================================
- Coverage 61.24% 59.97% -1.28%
==========================================
Files 2190 2087 -103
Lines 191985 179471 -12514
==========================================
- Hits 117587 107629 -9958
+ Misses 62978 61563 -1415
+ Partials 11420 10279 -1141
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 entire Reviewed by Cursor Bugbot for commit 2a626f5. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Swapping the benchmark onto giga.NewStateDB is the right direction and the deletions are clean (no dangling references), but the new path leaves the state WAL with no pruner, so long runs grow it without bound. A few leftovers from the removed wrapper abstraction (dead block-number counter, stale doc, orphaned ss/offload package) and the loss of all commit/read round-trip coverage are worth cleaning up.
Findings: 1 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] Test coverage regressed with no replacement:
wrappers/flatkv_wrapper_test.go,state_store_wrapper_test.goandwrappers_test.gocovered commit/read round-trips and are deleted, while the only remaining test ofDatabase(transaction_test.go) drives a stubreadTrackingViewwhoseGetalways misses and whoseGetBlockHeightis hardcoded to 0. Nothing now exercises the two behaviours this PR actually introduces — commit →reopenView→ read the value back, and startup height recovery viaview.GetBlockHeight()+1on a reopened DB. A single test against a realgiga.NewStateDBovert.TempDir()would cover both. - [suggestion]
sei-db/state_db/ss/offload(kafka.go, aws_msk_iam.go, types.go) has no importers anywhere in the tree after this PR —wrappers/historical_offload_wrapper.gowas its only consumer, andhistorical_offload_test.gowas its only end-to-end exercise. Either delete the package alongside the wrapper or note why it is being kept. - [suggestion]
CryptoSimConfig.HashLagBlocks's doc (cryptosim_config.go:88) still ends with "A database that publishes no block hashes waits on nothing." That case no longer exists:Database.hashesis now unconditionally set andawaitBlock()is always called. The sibling comment on thehashesfield was updated for this; this one was missed. - [suggestion]
NewCryptoSimnow overwritesconfig.FlatKVConfig.DataDirandconfig.StateStoreConfig.EVMDBDirectoryunconditionally, so a value set for either in a config file is silently ignored; andStateStoreConfig.DBDirectoryis not read at all on the giga path (openSSuses onlyEVMDBDirectory).config/basic-config.json, which is documented as the reference listing every knob at its default, still advertisesDBDirectory. Worth dropping the dead key, or ignoring the supplied values only when they are empty. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] A bricked finalization manager does not stop the hash engine (
sei-db/state_db/sc/flatkv/finalization_manager.go):runcancels only the finalizer's own context, whilehashCombiner.publish(lthash/hash_combiner.go:126) is released only by the engine's context. OnceHashChanSize(default 1024) fills, the combiner blocks forever, and bothCommitStore.FlushHashes()andstopHashing()wait onengine.Flush()before ever reaching the finalizer's latched error, soClose()hangs. Codex reported this as introduced by this PR;finalization_manager.gois not in the diff, so it is base-branch behaviour. Low exposure from cryptosim specifically, since every teardown path cancels the store's context before callingClose. - [suggestion]
NewCryptoSim(sei-db/state_db/bench/cryptosim/cryptosim.go) leaks the opened state DB whenNewRecieptStoreSimulator(~line 189) orc.setup()(~line 224) fails — both return without callingdb.Close(), unlike theNewDatabasefailure path just above them. Because the state DB now holds the state WAL directory's exclusive lock, a leaked handle makes an in-process retry fail to open the WAL rather than just leaking file descriptors.
|
@seidroid review |
There was a problem hiding this comment.
The state WAL now has a pruner — giga.NewStateDB is paired with a controller.StorageGarbageCollector over db.PrunableStores() with ExternalPruning set on both configs, matching bootstrap.GigaStorageManager, and teardown stops the collector before closing the stores. Remaining items are non-blocking: no test drives a real Giga-backed Database, and a few leftovers from the removed wrapper abstraction are still in the tree.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No test exercises the two behaviours this PR introduces.
wrappers/flatkv_wrapper_test.go,state_store_wrapper_test.goandwrappers_test.gocovered commit/read round-trips and are deleted; the only remainingDatabasetest drives a stubreadTrackingViewwhoseGetalways misses and whoseGetBlockHeightis hardcoded to 0, so neither commit →reopenView→ read-the-value-back nor startup height recovery viaview.GetBlockHeight()+1on a reopened DB is covered. One test against a realgiga.NewStateDBovert.TempDir()(commit a block, read it back throughDatabase.Get, close, reopen, assert the next commit lands at height+1) would cover both. Run it underscripts/ramtest.shper AGENTS.md, since it opens stores. - [suggestion]
sei-db/state_db/ss/offload(kafka.go, aws_msk_iam.go, types.go) has no importer anywhere in the tree after this PR —wrappers/historical_offload_wrapper.gowas its only consumer andhistorical_offload_test.goits only end-to-end exercise, and both are deleted. Either delete the package alongside the wrapper or note why it is kept. - [suggestion]
CryptoSimConfig.HashLagBlocks's doc (cryptosim_config.go:88) still ends with "A database that publishes no block hashes waits on nothing." That case no longer exists:RegisterHashListenerlost itsregisteredreturn,Database.hashesis set unconditionally, andawaitBlock()is always called. The sibling comment on thehashesfield was updated for this; this one was missed. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
NewCryptoSim(sei-db/state_db/bench/cryptosim/cryptosim.go:210 and :245) returns without releasing the opened state DB whenNewRecieptStoreSimulatororc.setup()fails, unlike the two failure paths just above it; thesetup()path does not evencancel(). Same shape inDatabase.Close, which returns early whenFinalizeBlockfails and so never reachesCloseWithoutFinalizing. Because the state DB holds the state WAL directory's exclusive lock, a leaked handle makes an in-process retry fail to open the WAL. This PR widens what leaks on those paths (the collector goroutine and the pinned SC view join the DB handle) but the missing cleanup is base-branch behaviour. - [suggestion] A bricked finalization manager does not stop the hash engine (
sei-db/state_db/sc/flatkv/finalization_manager.go):runcancels only the finalizer's own context, whilehashCombiner.publish(lthash/hash_combiner.go:126) is released only by the engine's context. OnceHashChanSize(default 1024) fills, the combiner blocks forever, and bothCommitStore.FlushHashes()andstopHashing()wait onengine.Flush()before reaching the finalizer's latched error, soClose()hangs. Not in this diff. Exposure from cryptosim is low because every teardown path cancels the store's context before callingClose, andblockHashWaiter.takeHashbounds its own wait at 5 minutes.
| changeSets = append(changeSets, &proto.NamedChangeSet{ | ||
| Name: wrappers.EVMStoreName, | ||
| Name: keys.EVMStoreKey, |
There was a problem hiding this comment.
[suggestion] BlockNumberCounterKey is still write-only. Its sole reader was the database.Get(BlockNumberCounterKey()) block removed from NewDataGenerator, replaced by view.GetBlockHeight() + 1 in NewDatabase — which is the better source, since it is what the store will actually accept. grep over the package now finds only this write plus the util.go builder and the data_generator.go const.
Dropping this changeset entry along with blockNumberCounterKey/BlockNumberCounterKey() removes a per-block write and, more importantly, removes a persisted value a later reader could mistake for authoritative — the old code stored the block being committed and then re-used that same number on restart, which the new height source quietly fixed.
| dbConfig = config.StateStoreConfig | ||
| case wrappers.SSHistoricalOffload: | ||
| dbConfig = config.HistoricalOffload | ||
| config.FlatKVConfig.DataDir = config.DataDir |
There was a problem hiding this comment.
[suggestion] These two assignments overwrite FlatKVConfig.DataDir and StateStoreConfig.EVMDBDirectory unconditionally, so a value set for either in a config file is silently ignored (LoadConfigFromFile uses DisallowUnknownFields, so EVMDBDirectory is accepted and then discarded rather than rejected). Overwriting only when the supplied value is empty would keep both knobs meaningful.
Two related loose ends while you are here:
- The layout no longer matches the node's that the comment below cites:
DefaultGigaStorageConfigputs flatkv atdata/state_commit/flatkvand SS atdata/state_store/evm/{backend}, whereas this puts flatkv at the data-dir root and nests SS inside it. Functionally harmless (traverseSnapshotsskips non-snapshot dirs), but it makes per-storeduand separate-device mounts awkward on a harness whose point is to reproduce node storage behaviour. StateStoreConfig.DBDirectoryis not read on the giga path (openSSuses onlyEVMDBDirectory), yetconfig/basic-config.json— documented as listing every knob at its default — still advertises it. Worth dropping alongside theEVMDBDirectoryentry this PR already removed.
Superseded: latest AI review found no blocking issues.
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.
Reviewed by Cursor Bugbot for commit 2a626f5. Configure here.
| if closeErr := db.Close(); closeErr != nil { | ||
| fmt.Printf("failed to close the state DB during error recovery: %v\n", closeErr) | ||
| } | ||
| } |
There was a problem hiding this comment.
Cleanup skips open state view
Medium Severity
releaseStorage closes the garbage collector and state DB without first closing the view that NewDatabase opened. After that constructor succeeds, receipt-store and setup failures take this path, so the store is torn down while a reservation is still held. That can block or fail Close, leaving the state WAL lock taken and blocking an in-process retry.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 2a626f5. Configure here.


Describe your changes and provide context
Replaces the legacy DB wrapper used in benchmarking with the new giga interface. Deletes a large number of legacy benchmarks that used the wrapper, as they are no longer actively used.