Skip to content

Replace benchmark DB wrapper - #4112

Merged
yzang2019 merged 24 commits into
mainfrom
cjl/replace-db-wrapper-2
Sep 10, 2026
Merged

Replace benchmark DB wrapper#4112
yzang2019 merged 24 commits into
mainfrom
cjl/replace-db-wrapper-2

Conversation

@cody-littley

Copy link
Copy Markdown
Contributor

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.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 10, 2026, 1:23 AM

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.97%. Comparing base (c9c9549) to head (2a626f5).

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 173 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cody-littley
cody-littley marked this pull request as ready for review September 9, 2026 18:18
@cody-littley
cody-littley marked this pull request as draft September 9, 2026 18:19
@cursor

cursor Bot commented Sep 9, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Large deletion of benchmark infrastructure and a behavior change in how cryptosim opens, prunes, and commits state; production paths are mirrored but regressions would mainly show up in perf/CI smoke rather than node runtime.

Overview
Cryptosim no longer selects a pluggable Backend via bench/wrappers. It opens state the same way a node does: giga.NewStateDB with FlatKV, optional historical StateStore, checkpoint and external pruning settings, plus a StorageGarbageCollector over PrunableStores(). Commits go through CommitStateChanges(blockNum, changeSets); reads use a StateView refreshed after each block instead of the old apply/commit wrapper path. Config drops Backend / historical offload; CheckpointConfig and PruningConfig are new. Block height comes from the store view, not a persisted block counter.

The entire sei-db/bench/wrappers layer (MemIAVL, FlatKV-only, SS composite, Kafka offload, NoOp, etc.) and sei-db/state_db/bench (SC/SS micro-benchmarks, write-set replay) are removed, along with related cryptosim JSON configs and tests. CI benchmark smoke now runs ./sei-db/bench/... instead of ./sei-db/state_db/bench/....

Reviewed by Cursor Bugbot for commit 2a626f5. Bugbot is set up for automated code reviews on this repo. Configure here.

@cody-littley
cody-littley changed the base branch from main to cjl/flatkv-hash-refactor-2 September 9, 2026 18:19
@cody-littley
cody-littley marked this pull request as ready for review September 9, 2026 18:26
seidroid[bot]
seidroid Bot previously requested changes Sep 9, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go and wrappers_test.go covered commit/read round-trips and are deleted, while the only remaining test of Database (transaction_test.go) drives a stub readTrackingView whose Get always misses and whose GetBlockHeight is hardcoded to 0. Nothing now exercises the two behaviours this PR actually introduces — commit → reopenView → read the value back, and startup height recovery via view.GetBlockHeight()+1 on a reopened DB. A single test against a real giga.NewStateDB over t.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.go was its only consumer, and historical_offload_test.go was 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.hashes is now unconditionally set and awaitBlock() is always called. The sibling comment on the hashes field was updated for this; this one was missed.
  • [suggestion] NewCryptoSim now overwrites config.FlatKVConfig.DataDir and config.StateStoreConfig.EVMDBDirectory unconditionally, so a value set for either in a config file is silently ignored; and StateStoreConfig.DBDirectory is not read at all on the giga path (openSS uses only EVMDBDirectory). config/basic-config.json, which is documented as the reference listing every knob at its default, still advertises DBDirectory. 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): run cancels only the finalizer's own context, while hashCombiner.publish (lthash/hash_combiner.go:126) is released only by the engine's context. Once HashChanSize (default 1024) fills, the combiner blocks forever, and both CommitStore.FlushHashes() and stopHashing() wait on engine.Flush() before ever reaching the finalizer's latched error, so Close() hangs. Codex reported this as introduced by this PR; finalization_manager.go is 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 calling Close.
  • [suggestion] NewCryptoSim (sei-db/state_db/bench/cryptosim/cryptosim.go) leaks the opened state DB when NewRecieptStoreSimulator (~line 189) or c.setup() (~line 224) fails — both return without calling db.Close(), unlike the NewDatabase failure 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.

Comment thread sei-db/bench/cryptosim/cryptosim.go
Comment thread sei-db/bench/cryptosim/database.go Outdated
Base automatically changed from cjl/flatkv-hash-refactor-2 to main September 9, 2026 19:04
@cody-littley

Copy link
Copy Markdown
Contributor Author

@seidroid review

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go and wrappers_test.go covered commit/read round-trips and are deleted; the only remaining Database test drives a stub readTrackingView whose Get always misses and whose GetBlockHeight is hardcoded to 0, so neither commit → reopenView → read-the-value-back nor startup height recovery via view.GetBlockHeight()+1 on a reopened DB is covered. One test against a real giga.NewStateDB over t.TempDir() (commit a block, read it back through Database.Get, close, reopen, assert the next commit lands at height+1) would cover both. Run it under scripts/ramtest.sh per 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.go was its only consumer and historical_offload_test.go its 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: RegisterHashListener lost its registered return, Database.hashes is set unconditionally, and awaitBlock() is always called. The sibling comment on the hashes field 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 when NewRecieptStoreSimulator or c.setup() fails, unlike the two failure paths just above it; the setup() path does not even cancel(). Same shape in Database.Close, which returns early when FinalizeBlock fails and so never reaches CloseWithoutFinalizing. 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): run cancels only the finalizer's own context, while hashCombiner.publish (lthash/hash_combiner.go:126) is released only by the engine's context. Once HashChanSize (default 1024) fills, the combiner blocks forever, and both CommitStore.FlushHashes() and stopHashing() wait on engine.Flush() before reaching the finalizer's latched error, so Close() hangs. Not in this diff. Exposure from cryptosim is low because every teardown path cancels the store's context before calling Close, and blockHashWaiter.takeHash bounds its own wait at 5 minutes.

Comment thread sei-db/bench/cryptosim/database.go Outdated
changeSets = append(changeSets, &proto.NamedChangeSet{
Name: wrappers.EVMStoreName,
Name: keys.EVMStoreKey,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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: DefaultGigaStorageConfig puts flatkv at data/state_commit/flatkv and SS at data/state_store/evm/{backend}, whereas this puts flatkv at the data-dir root and nests SS inside it. Functionally harmless (traverseSnapshots skips non-snapshot dirs), but it makes per-store du and separate-device mounts awkward on a harness whose point is to reproduce node storage behaviour.
  • StateStoreConfig.DBDirectory is not read on the giga path (openSS uses only EVMDBDirectory), yet config/basic-config.json — documented as listing every knob at its default — still advertises it. Worth dropping alongside the EVMDBDirectory entry this PR already removed.

@seidroid
seidroid Bot dismissed their stale review September 9, 2026 20:00

Superseded: latest AI review found no blocking issues.

* main:
  Fix flaky/environment-dependent tests (macOS) (#4115)
  ci: drop unused Chrome/Microsoft apt sources before apt-get update (#4114)
  Move storage bench to sei-db root (#4109)
  Validate block part Merkle proof totals (CON-412) (#4105)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2a626f5. Configure here.

@yzang2019
yzang2019 added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 43be435 Sep 10, 2026
151 of 159 checks passed
@yzang2019
yzang2019 deleted the cjl/replace-db-wrapper-2 branch September 10, 2026 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants