move flatKV hashing to a background thread - #4085
Conversation
PR SummaryHigh Risk Overview Cosmos still needs synchronous AppHash inputs, so the composite store adds Hash logging is opened in The lthash package is reworked around gather/combine workers (replacing Reviewed by Cursor Bugbot for commit 9608f1e. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9608f1e. Configure here.
| if published.BlockNumber == height { | ||
| checksum := published.Global.Checksum() | ||
| return checksum[:], nil | ||
| } |
There was a problem hiding this comment.
Published hash race strands stream
Medium Severity
awaitHeight may answer from PublishedHash without taking that block off HashChan. The finalizer stores latest and reports to the logger before publish, so a concurrent read can observe the new height while the stream entry is still unsent. The comments already note that leaving a published hash queued strands it for good.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9608f1e. Configure here.
| Value: value, | ||
| LastValue: old[string(key)], | ||
| Delete: value == nil, | ||
| }) |
There was a problem hiding this comment.
Diff nil treated as deletion
High Severity
The gatherer sets Delete from value == nil when converting a view diff into KeyMutation. FlatKV must not treat nil and empty []byte as the same: empty writes are real values, and protobuf already collapses empty to nil. Inferring deletion from a nil value can MixOut a live key and diverge LtHash / AppHash.
Triggered by learned rule: sei-db flatkv: changeset deletion is Delete flag, not nil Value — protobuf erases empty vs nil
Reviewed by Cursor Bugbot for commit 9608f1e. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moving flatKV hashing off the commit thread is a well-structured refactor (pipelined gather/hash/combine engine, a finalization manager that writes each block's metadata into the same batch as its data, and a new golden-hash regression test), but the new hash stream applies backpressure to Commit while several live paths never consume it — most notably a paused EVM migration, which is the default post-upgrade state, and writable WAL replay. Both stall commit permanently.
Findings: 2 blocking | 3 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
FinalizationManager(357 lines) andflatKVHashCacheare both new, concurrency-heavy and consensus-critical, and neither has a dedicated unit test (lthash.HashEnginedoes, inhash_engine_test.go). Worth direct tests for: the finalizer's discard/abandon paths, thehash.BlockNumber != pending.blockNumberout-of-step guard, the latched-failure behaviour, and the cache's clamp-to-committed / "no longer available" / stream-drain-before-published-hash ordering. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| if cs.shouldAppendLatticeHash() { | ||
| return cs.appendEvmLatticeHash(ci, cs.flatKVWorkingHash(version)) | ||
| return cs.appendEvmLatticeHash(ci, cs.mustLatticeHash(version)) |
There was a problem hiding this comment.
[blocker] flatKV's hash stream is only drained through mustLatticeHash → flatKVHashCache, and both call sites (here and refreshLastCommitInfo) are gated on shouldAppendLatticeHash(). Commit however calls cs.flatKV.Commit(version) unconditionally whenever flatKV != nil, so in MigrateEVM with the migration not yet started the store commits a block — and publishes a hash — every block with nothing reading it.
That state is the default, not a corner case: app/migration/params.go sets DefaultNumKeysToMigratePerBlock = 0 ("leaves the migration paused"), and with migrationBatchSize == 0 MigrationManager.ApplyChangeSets skips the boundary advance entirely (advanceMigration := firstBatchInBlock && m.migrationBatchSize > 0), so neither MigrationBoundaryKey nor MigrationVersionKey is ever written and migrationStarted stays false until governance raises the param.
With the defaults (HashChanSize: 1024, FinalizationQueueSize: 64) the chain halts after ~1088 blocks in that state: publishedHashChan fills, FinalizationManager.run blocks in publish, messageChan fills, and Offer — reached from sealBlock inside flatkv.Commit — blocks forever.
The drain needs to be unconditional for a committing flatKV rather than tied to the AppHash-participation gate.
| s.hashLogger, | ||
| ) | ||
|
|
||
| if s.readOnly { |
There was a problem hiding this comment.
[blocker] The self-drain is conditioned on s.readOnly, but a writable store also hashes blocks with no consumer attached: openTo → replayIntoMutableStore → catchUpFromWAL → replayBlocks → applyAndCommit → sealBlock, which offers every replayed block to the finalizer. The composite store's flatKVHashCache is not created until the first hash question, which is long after LoadVersion returns, so nothing reads publishedHashChan during replay. Past HashChanSize + FinalizationQueueSize (1024 + 64 by default) blocks, Offer blocks forever and replay never returns — before it can reach the FlushHashes in requireAlignedDataDBs.
Replays that long are reachable: Rollback reopens at a snapshot base and replays forward to the target (up to SnapshotInterval, default 10000), as do rebuildIfAnyDataDBIsUnreachable → rebuildWorkingCopy and openTo(version) for an older height. So seid rollback and post-corruption repair can hang instead of failing.
The same gap hits every writable standalone user, since HashChan is now handed out but nobody takes it: sei-db/state_db/bench/wrappers/db_implementations.go, the seidb operations helpers, giga's test StateDB, and the flatkv test helpers themselves (commitAndCheck/rootHash call FlushHashes but never dequeue), so any of them committing >1024 blocks deadlocks.
Draining whenever no external consumer has claimed the stream (rather than only for readOnly) would cover replay and every standalone caller in one place.
| case message := <-g.scheduledBlockChan: | ||
| request, ok := message.(*hashRequest) | ||
| if !ok { | ||
| continue |
There was a problem hiding this comment.
[suggestion] teardown skips every non-hashRequest message with continue, which silently swallows a queued flushRequest without closing its doneChan. A HashEngine.Flush() whose request lands on scheduledBlockChan just as the engine's context is cancelled then blocks forever on <-request.doneChan, since nothing else ever closes it — and FlushHashes is reachable from verifyLtHashInternal, outOfBandSnapshot and reloadLocalMeta.
FinalizationManager.abandonMessage already gets this right (case *finalizationFlushRequest: close(request.doneChan)); mirroring that here would make the two teardown paths consistent.
|
|
||
| // Controls the number of workers in the dedicated lattice-hash pool used to | ||
| // compute per-module LtHashes during ApplyChangeSets. The worker count is | ||
| // HashEngineConfig configures the pipeline that hashes each committed block. |
There was a problem hiding this comment.
[suggestion] HashEngineConfig was inserted into the middle of LtHashThreadsPerCore's existing two-part comment, so the godoc for both fields is now garbled: HashEngineConfig reads "Controls the number of workers in the dedicated lattice-hash pool used to compute per-module LtHashes during ApplyChangeSets. The worker count is / HashEngineConfig configures the pipeline...", and LtHashThreadsPerCore is left starting mid-sentence ("LtHashThreadsPerCore * runtime.NumCPU() (clamped to at least 1)..."). Dropping the two orphaned lines above and restoring a leading sentence on LtHashThreadsPerCore fixes both.
| // | ||
| // A read-only store gets them too: it replays blocks to reach its target height, and each replayed block | ||
| // is hashed against the one before it exactly as a committed block is. | ||
| func (s *CommitStore) startHashing() error { |
There was a problem hiding this comment.
This seems to be a bug that Writable WAL replay could deadlock past the stream's dept:
startHashing only attaches a drain when s.readOnly (flatkv/store.go:1130-1139). A writable store also hashes every block it replays: openTo → replayIntoMutableStore → catchUpFromWAL → replayBlocks → applyAndCommit → sealBlock, which calls finalizer.Offer and hashEngine.ScheduleHash for each block. Nothing reads publishedHashChan during replay — the composite store's flatKVHashCache isn't created until the first hash question, long after LoadVersion returns — so FinalizationManager.publish blocks, messageChan fills, and Offer/ScheduleHash block forever.
| // flatkv — see latticeHash — so by the time this runs flatkv may already sit at version, and a | ||
| // height derived from its own state would land on the next block and commit one that never existed. | ||
| // Handing it the height the caller means lets flatkv recognise the block it already committed. | ||
| func (cs *CompositeCommitStore) Commit(version int64) (int64, error) { |
There was a problem hiding this comment.
A paused EVM migration commits and publishes with nobody reading could lead to the chain halts:
Commit calls cs.flatKV.Commit(version) unconditionally whenever flatKV != nil (composite/store.go:819), but the only thing that drains the stream is mustLatticeHash → flatKVHashCache, and both of its call sites (store.go:1134 and refreshLastCommitInfo at :1197) are gated on shouldAppendLatticeHash(). In MigrateEVM with the migration not yet started, that gate is closed, so flatKV commits and publishes a hash every block with nothing reading it.
| } | ||
|
|
||
| // Flush blocks until the engine has published a hash for every block scheduled so far. | ||
| func (he *HashEngine) Flush() error { |
There was a problem hiding this comment.
Some corner case: HashEngine.Flush() can hang forever on shutdown:
blockGatherer.teardown skips every non-hashRequest message with continue (lthash/block_gatherer.go:85-88), which silently swallows a queued flushRequest without closing its doneChan. A Flush() whose request lands on scheduledBlockChan just as the engine's context is cancelled then blocks forever on <-request.doneChan
2 participants
Describe your changes and provide context
Move flatKV hashing to background threads