feat(flatkv): introduce module-prefix physical keys across all FlatKV DBs#3229
feat(flatkv): introduce module-prefix physical keys across all FlatKV DBs#3229blindchaser merged 7 commits intomainfrom
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 #3229 +/- ##
=======================================
Coverage 59.26% 59.26%
=======================================
Files 2068 2069 +1
Lines 169695 169663 -32
=======================================
- Hits 100572 100556 -16
+ Misses 60334 60319 -15
+ Partials 8789 8788 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 172fcfa983
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // prefixModuleKeys, avoiding double-prefixing. | ||
| return []*types.SnapshotNode{{ | ||
| Key: key, | ||
| Key: originalKey, |
There was a problem hiding this comment.
This now strips the <module>/ namespace and emits only originalKey, but importer writes all incoming nodes under the evm changeset, so non-EVM legacy entries (for example bank/foo) reimport as evm/foo. That can overwrite real EVM legacy keys with the same raw key and corrupt state-sync/import results for any module data newly routed into legacyDB.
Useful? React with 👍 / 👎.
| if len(keyBytes) != AddressLen { | ||
| return nil, nil | ||
| } | ||
| return readFromDB(EVMPhysicalKey(EVMKeyAccount, keyBytes), s.accountWrites, s.accountDB, vtype.DeserializeAccountData, "accountDB") |
There was a problem hiding this comment.
The read path now always looks up account rows using the new prefixed physical key format (evm/...), but this commit does not include a migration or fallback for existing databases that still store rows in the old unprefixed layout. On upgrade, previously committed entries become unreadable until rewritten, which changes Get/Has behavior and can produce incorrect execution on live state.
Useful? React with 👍 / 👎.
| // Clears the delete flag — an explicit SetValue is a write, not a deletion, | ||
| // even when value is empty ([]byte{} is a valid Cosmos module value). |
There was a problem hiding this comment.
Ah, I didn't know this. Good thing to catch!
| // Emit the original key (without module prefix). The importer feeds nodes | ||
| // back through ApplyChangeSets which re-applies the prefix via | ||
| // prefixModuleKeys, avoiding double-prefixing. |
There was a problem hiding this comment.
Not a request to change this PR, since this preserves the existing behavior. But we should discuss import/export as a larger group.
I think we need to change the way we export data. Instead of exporting the top level application key-value pairs, I think we need to export the raw keys-value pairs including all of the metadata. This is because we need to pass metadata fields such as block height modified and serialization version during a state sync if we want to calculate the same state hash, and these values are not currently contained within the data we currently export.
sei-db/state_db/sc/flatkv/keys.go
Outdated
| // and original key. Returns ok=false if no "/" separator is found. | ||
| func StripModulePrefix(physicalKey []byte) (moduleName string, originalKey []byte, ok bool) { |
There was a problem hiding this comment.
Is there a reason why we don't return an error if we don't find a "/" character? Should this be something that bubbles up as an error to the user if there is a malformed key?
There was a problem hiding this comment.
good call. changed to return error instead of ok bool
sei-db/state_db/sc/flatkv/keys.go
Outdated
| // StripEVMPhysicalKey extracts the EVM key kind and stripped key from a | ||
| // physical DB key. This is the inverse of EVMPhysicalKey for export paths. | ||
| // For account keys the returned kind is EVMKeyAccount (evm.EVMKeyNonce). | ||
| func StripEVMPhysicalKey(physicalKey []byte) (kind evm.EVMKeyKind, strippedKey []byte, ok bool) { |
There was a problem hiding this comment.
If you promote StripModulePrefix() to return an error, then you should also promote this function to return an error.
There was a problem hiding this comment.
done. propagates the error from StripModulePrefix
| for _, cs := range changeSets { | ||
| if cs.Name == evm.EVMStoreKey { | ||
| evmChangeSets = append(evmChangeSets, cs) | ||
| } else { | ||
| if cs.Changeset.Pairs == nil { | ||
| continue | ||
| } | ||
| modMap, ok := nonEVMByModule[cs.Name] | ||
| if !ok { | ||
| modMap = make(map[string][]byte, len(cs.Changeset.Pairs)) | ||
| nonEVMByModule[cs.Name] = modMap | ||
| } | ||
| for _, pair := range cs.Changeset.Pairs { | ||
| if pair.Delete { | ||
| modMap[string(pair.Key)] = nil | ||
| } else { | ||
| modMap[string(pair.Key)] = pair.Value | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we pull this logic into sortChangeSets()? Perhaps it would make sense to also add modules to keys, so we'd want to rename the function to sortAndAddPrefix() or something.
| if len(keyBytes) != AddressLen { | ||
| return nil, nil |
There was a problem hiding this comment.
we shouldn't ignore malformed keys
| } | ||
| } | ||
| if err = deserializeBatchResults(storageBatch, storageOld, vtype.DeserializeStorageData, "storageDB"); err != nil { | ||
| return nil, nil, nil, nil, err |
There was a problem hiding this comment.
nit, perhaps wrap error
There was a problem hiding this comment.
agreed. getAccountData, getStorageData, and getCodeData now return fmt.Errorf with expected vs actual key length instead of nil, nil
| } | ||
| } | ||
| if err = deserializeBatchResults(accountBatch, accountOld, vtype.DeserializeAccountData, "accountDB"); err != nil { | ||
| return nil, nil, nil, nil, err |
There was a problem hiding this comment.
nit, perhaps wrap error
There was a problem hiding this comment.
wrapped all four deserializeBatchResults calls with context
| } | ||
| } | ||
| if err = deserializeBatchResults(codeBatch, codeOld, vtype.DeserializeCodeData, "codeDB"); err != nil { | ||
| return nil, nil, nil, nil, err |
There was a problem hiding this comment.
nit, perhaps wrap error
| } | ||
| } | ||
| if err = deserializeBatchResults(legacyBatch, legacyOld, vtype.DeserializeLegacyData, "legacyDB"); err != nil { | ||
| return nil, nil, nil, nil, err |
There was a problem hiding this comment.
nit, perhaps wrap error
|
Some suggestions:
|
| nonEVMByModule map[string]map[string][]byte, | ||
| ) { | ||
| evmPrefix := evm.EVMStoreKey + "/" | ||
| for kind, m := range changesByType { |
There was a problem hiding this comment.
This allocates a new map and string-concatenates every key on every block. For storage-heavy blocks with tens of thousands of entries, this is a non-trivial allocation. Consider prepending the prefix earlier in the pipeline (inside sortChangeSets) to avoid the extra copy pass, or using a key builder that avoids repeated string concatenation.
* main: feat(flatkv): introduce module-prefix physical keys across all FlatKV DBs (#3229)
3 participants
Summary
Introduces module-prefixed physical keys across all FlatKV data DBs, enabling non-EVM Cosmos modules to be stored in legacyDB and preparing for future DB consolidation by ensuring key uniqueness across namespaces.
Physical key format:
"module/" + type_prefix + stripped_key. EVM keys become"evm/" + kind_byte + addr_or_slot, non-EVM Cosmos keys become"<module>/" + original_key.ktype/ktype.go(new package): Defines physical key encoding —ModulePhysicalKey,EVMPhysicalKey,StripModulePrefix,StripEVMPhysicalKey,PrefixEnd. DefinesEVMKeyAccountas the canonical kind for merged account rows. Replaces the oldkeys.goin the flatkv package.common/evm/keys.go: AddsEVMKeyPrefixBytehelper.BuildMemIAVLEVMKeyrefactored to use it, eliminating a redundant allocation.store_apply.go:ApplyChangeSetsnow splits incoming changesets into EVM vs non-EVM; non-EVM modules are routed into legacyDB under their module prefix.classifyAndPrefixreplaces the former two-passsortChangeSets+prefixModuleKeys, converting keys to physical format viaktypehelpers in a single pass.store_read.go: Extracts genericreadFromDBthat checks pending writes then falls back to DB, replacing four per-DB getter implementations.getLegacyData/getLegacyValuenow require amoduleNameparameter.getAccountData,getStorageData,getCodeDatavalidate key length before constructing physical keys.store_write.go: ReplacesprepareBatchCodeDB,prepareBatchStorageDB,prepareBatchLegacyDBwith a single genericprepareBatch[T]. ExtractscollectPendingReadsanddeserializeBatchResultsgenerics to deduplicatebatchReadOldValues. Commit ordering fix: global metadata is now persisted to metadataDB before updating in-memorycommittedVersion/committedLtHash, so a metadata write failure cannot leave the store in an inconsistent state.api.go:Storeinterface updated —Get,Has,GetBlockHeightModifiednow take amoduleNameparameter.Iteratordocs updated to reflect physical key format forKey()and dual-format acceptance forSeekGE/SeekLT.vtype/legacy_data.go: Replaces value-length inference for deletion with an explicitMarkDeleted()flag andisDeletefield. The previous implementation treatedlen(value) == 0as a deletion, but Cosmos SDK modules (bank, staking, etc.) legitimately write empty values ([]byte{}) to the store — this is standard Cosmos convention and not a deletion. Now that non-EVM modules are routed into legacyDB, the old heuristic would silently drop valid empty-value writes.MarkDeleted()makes the delete intent explicit, andSetValue(even withnilor[]byte{}) clears the delete flag, preserving Cosmos semantics.