Snapshot Engine + flatKV integration: phase 1 - #3902
Conversation
PR SummaryHigh Risk Overview rootmulti now owns block height for the state-commit path: Snapshot lifecycle tests and behavior shift from Reviewed by Cursor Bugbot for commit 407bf3b. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
This comment was marked as low quality.
This comment was marked as low quality.
| // Set sets the value for the given key at the current version. | ||
| // | ||
| // A write to a shard that is out of service is refused: it would land in versioned data that no |
There was a problem hiding this comment.
What does out of service mean for a shard? When would a shard become out of service?
There was a problem hiding this comment.
Two scenarios qualify as "out of service":
- The shard is shutdown. Only reachable if you call shutdown concurrently with an operation that touches a shard (which is illegal).
- The shard has crashed. Only possible when the DB experiences a crash. DB failures are fatal, so we should never try to recover them.
Updated the doc to spell this out.
| // Post-Cosmos this goes away along with rootmulti: a single call will supply a block's writes and | ||
| // commit them, and nothing will ask for a hash mid-block. | ||
| func (s *CommitStore) RootHash() []byte { | ||
| if err := s.commitPendingBlock(); err != nil { |
There was a problem hiding this comment.
is looks RootHash commits the pending block. then later composite commit increments FlatKV again? is there a double commits on flatkv?
There was a problem hiding this comment.
This is intentional on my part. The core problem is that comsos wants the hash before the commit, but the proper shape of the storage engine is to provide a hash only after the block is finalized+committed.
I've set it up so that if we call Commit() multiple times for the same block number, later commits become no-ops. If you try to write changes to the block after the commit, you get an error. Previously it was technically possible to write changes after we get the hash but before the commit, but this would either mean the hash is actually an invalid hash for that block.
Worth a discussion in parking lot.
| if c.MetadataDBConfig.DataDir == "" { | ||
| c.MetadataDBConfig.DataDir = filepath.Join(workDir, metadataDir) | ||
| } | ||
| applyPebbleMetricsConfig(c) |
There was a problem hiding this comment.
Config.Fsync is not passed to the five snapshot-engine FlushSync fields. Since FlushSync defaults to false, programmatic callers that set Fsync=true still get unsynchronized normal flushes. can we propagate this value to all five store configurations and add a test.
There was a problem hiding this comment.
good point, fixed
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 0e3dee4. Configure here.
| // advanced, so it would conclude the chain had moved to the next block and commit one that never | ||
| // existed. A later run that does carry writes is handed down instead of refused here — whether the | ||
| // block is still open to them is the commit store's to judge, and flatkv refuses a sealed one. | ||
| if len(changeSets) == 0 && rs.flushedVersion == currentVersion { |
There was a problem hiding this comment.
FlatKV only refuses when the batch is stamped at the already-committed height N. The writer stamps N+1, so it silently defers instead. Someone reading this comment would conclude the non-empty case is caught downstream, and it isn't.
And the safety property currently rests on a convention — "nothing writes to the multistore in the preCommitHandler" — rather than an invariant. Whoever adds the next preCommitHandler hook, or a module that writes during it, gets silent height drift rather than an error. Given the PR's own stated principle about guarding at the choke point, turning flushedVersion == currentVersion && len(changeSets) > 0 into an error in flush() would make it checkable. That's a small, self-contained follow-up, not something that needs to hold up phase 1.
Might be worth adding a comments for context
| } | ||
|
|
||
| // LastCommitInfo returns the last commit info | ||
| func (cs *CompositeCommitStore) LastCommitInfo() *proto.CommitInfo { |
There was a problem hiding this comment.
This PR moves the FlatKV seal earlier: flatKVWorkingHash now calls flatKV.Commit(version) from inside GetWorkingHash, which runs in FinalizeBlock. memIAVL still commits later, in ABCI Commit. So for the whole gap between those two ABCI calls, FlatKV is at block N while memIAVL is at N-1.
The result is a CommitInfo labeled Version: N-1, carrying memIAVL's store hashes at N-1, with an evm_lattice entry holding block N's hash. That combination corresponds to no committed state at any height. Note the contrast with flatKVWorkingHash, which does compare hashed != version and panics — the height check exists exactly where the race can't happen, and is dropped where it can.
It's reachable. rootmulti.Query recomputes commit info live on the latest path, under no lock, and RPC nodes serve queries concurrently with block execution.
if latest { // latest never needs historical LoadVersion clone store = types.Queryable(commitment.NewStore(rs.scStore.GetChildStoreByName(storeName))) commitInfo = convertCommitInfo(rs.scStore.LastCommitInfo()) commitInfo = amendCommitInfo(commitInfo, rs.storesParams) }
The race technically predates the PR (memIAVL and FlatKV committed sequentially inside composite.Commit, so they disagreed for a few microseconds), but this widens it from an instruction window inside one function to the full inter-ABCI-call gap, on every block.

Describe your changes and provide context
Integrate flatKV and the new snapshot engine.
As part of the pipelining refactor, our goal is to move three things off of the main execution thread:
In order to limit the size of this PR, this PR only moves flushing of data off the main thread, although it does lay the groundwork for moving hashing and checkpointing off-thread.