Add balance support to flatKV - #4101
Conversation
PR SummaryMedium Risk Overview Commit and import paths now merge balance changes with nonce/codehash updates ( Tooling and tests extend the composite random oracle, export/import round-trips, snapshot rollback, and Reviewed by Cursor Bugbot for commit 1a086bc. 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).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4101 +/- ##
==========================================
- Coverage 61.19% 60.62% -0.57%
==========================================
Files 2196 2107 -89
Lines 193277 183335 -9942
==========================================
- Hits 118272 111144 -7128
+ Misses 63783 61704 -2079
+ Partials 11222 10487 -735
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d7152b0. Configure here.
There was a problem hiding this comment.
Wires the new 0x21 balance key kind through flatkv's read, write, iterate, import/export, SS-split and digest paths; the merge/zero-is-absent semantics are consistent with the existing codehash handling and are well covered by tests. One non-blocking upgrade concern: adding StoreBalance to AllEVMStoreTypes changes the on-disk sub-DB set in SS separate-DB mode.
Findings: 0 blocking | 1 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| StoreMisc = commonevm.EVMKeyMisc // Catch-all: codesize, address mappings, receipts, etc. | ||
| // StoreBalance is reserved for future migration; balances currently use tendermint store | ||
| StoreBalance EVMStoreType = 100 | ||
| StoreBalance = commonevm.EVMKeyBalance |
There was a problem hiding this comment.
openDBs creates a fresh balance/ Pebble DB at version 0 whenever SeparateEVMSubDBs is on. GetLatestVersion is the minimum across managedDBs, so a store that was at height N reports 0 after upgrade with this change.
There was a problem hiding this comment.
All SS changes have been removed in this PR
|
@seidroid review |
There was a problem hiding this comment.
The flatkv side of the new 0x21 balance kind is thorough and well tested (read/write/merge/iterate/import-export/digest all covered). However, ParseEVMKey is a repo-wide router, and two of its consumers in the SS/composite layer were not updated for the new kind: the SS EVM store has no sub-DB for EVMKeyBalance and silently drops it, and convertFlatKVNodes drops the balance field when translating flatkv account rows into snapshot nodes.
Findings: 2 blocking | 2 non-blocking | 1 posted inline
Blockers
convertFlatKVNodes(sei-db/state_db/ss/composite/store.go:509-533) drops balances. A flatkv account row arrives with physical prefix 0x0a, soParseEVMKeyclassifies itEVMKeyNonce, and that branch emits a nonce node plus a codehash node when the hash is non-zero — but nothing for the balance field, which this PR makes writable. On a state-sync restore that feeds a flatkv snapshot into the composite SS store (Import → convertFlatKVNodes, store.go:619), every account balance is silently lost; the restored SS has correct nonces and codehashes and zero balances. The mirrored helpercollectFlatKVEVMin sei-cosmos/storev2/rootmulti/flatkv_helpers_test.go:454 replicates the same conversion for the flatkv↔memiavl equivalence assertions and would need the same addition, otherwise that test keeps passing against the lossy conversion. No writer produces 0x21 keys yet, so this is latent today — but it is silent data loss the moment one lands.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] Balance-only accounts widen the phantom-nonce divergence.
buildAccountNonceLane(store_iteration.go:380) emits a zero nonce for every live row, unlike the codehash and new balance lanes which skip their zero value — so an account held up only by its balance yields a0x0a||addr → 0entry that memiavl does not have.addBalanceOnlyAccountin store_iteration_test.go now asserts exactly that. Today this is harmless because codehash-only-with-zero-nonce accounts are rare, but once balances are written a funded EOA that has never sent a transaction is the common case, and each one becomes a phantom nonce row in iteration, in the exporter, and inconvertFlatKVNodesoutput. The composite random framework never generates the case (newRandomEVMEntrycase 0 always writes a nonce), so nothing currently catches it. Worth settling the intended semantics before a balance writer lands rather than after. - 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
GetBlockHeightModified(sei-db/state_db/sc/flatkv/store_read.go:113-121) answers with the merged account row's block height for whichever field the key names, so a nonce-only update at height N reports the codehash as modified at N too — and now the balance as well. The same branch returnsfound=truefor a field that is zero, whileGeton that key returnsfound=falsefor the same row, so the two reads disagree on existence. This already applied to codehash on the base branch; the PR extends the existing behaviour to a third field rather than introducing it, and no test covers the cross-field case for either.
|
@seidroid review |

Describe your changes and provide context
Support reading/writing account balances in FlatKV.
Note that SS does not currently support an EVM balance key. That's out of scope for this PR and still needs to be added. Nothing writes this key type yet, so it doesn't break production code.