balances and bools - #4093
Conversation
PR SummaryHigh Risk Overview Balance storage is wired end-to-end: new Tests and random/composite SS paths are updated for balance-only accounts, coordinated deletes of merged account fields, and the new presence semantics (cleared/deleted slots and code read as absent, not empty). Reviewed by Cursor Bugbot for commit 792b4af. 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 #4093 +/- ##
==========================================
- Coverage 61.31% 60.64% -0.67%
==========================================
Files 2183 2111 -72
Lines 191331 184181 -7150
==========================================
- Hits 117310 111694 -5616
+ Misses 62942 61924 -1018
+ Partials 11079 10563 -516
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 792b4af. Configure here.
| s.store.mu.RUnlock() | ||
| if ok { | ||
| return value.value | ||
| return value.value, true |
There was a problem hiding this comment.
Zero overlay balance reported present
Medium Severity
The memory-store overlay reports a balance as present for any latest write, including a zeroed or deleted value. FlatKV and the new read contract treat a zero balance as not stored, so GetBalance's ok disagrees across implementations when an account is drained or a balance is removed.
Reviewed by Cursor Bugbot for commit 792b4af. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-tested groundwork: the new 0x21 balance key kind is wired consistently through FlatKV's account row (apply/merge, read, six-lane iterator, import translator, SS snapshot split, digest tool), and the (value, ok) state-view API change is mechanical with no production consumer of the bool yet. No blockers found; four non-blocking issues, mostly around contract consistency and the newly-created SS balance sub-DB on existing evm-ss-separate-dbs stores.
Findings: 0 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The "a zero account field means the field is absent" rule is now restated at six independent sites:
accountFieldValue(store_read.go),flatKVStateView.GetBalanceand.GetCodeHash(state_view.go),buildAccountCodehashLaneandbuildAccountBalanceLane(store_iteration.go), andconvertFlatKVNodes(ss/composite/store.go). Adding balance required touching five of them; a seventh account field would require all six. Per AGENTS.md ("Guard at the choke point, never at each caller"), consider a singlevtype-level field-presence accessor (e.g.AccountData.Field(kind) ([]byte, bool)) that every projection path routes through, so the rule cannot be forgotten at a new call site. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
CommitStore.GetBlockHeightModified(sei-db/state_db/sc/flatkv/store_read.go:113) already had this shape forEVMKeyCodeHashbefore this PR: an account row with a zero code hash reportsfound=true(whileGetreports the same key absent), and the height returned is the row's shared height, so a nonce-only write moves the code hash's reported "last modified" height. The PR extends the same behaviour to balance.
| return sd.GetBlockHeight(), true, nil | ||
|
|
||
| case keys.EVMKeyNonce, keys.EVMKeyCodeHash: | ||
| case keys.EVMKeyNonce, keys.EVMKeyCodeHash, keys.EVMKeyBalance: |
There was a problem hiding this comment.
[suggestion] Adding EVMKeyBalance here inherits two behaviours that disagree with Get for the same key:
- Presence:
Getroutes throughaccountFieldValue, which reports a zero balance as absent.GetBlockHeightModifiedreportsfound=truefor a balance key on any live account row, so a nonce-only account (or one whose balance was just zeroed, i.e. deleted) answers(rowHeight, true, nil)for a balance thatGet/GetBalanceboth call missing. - Height: the height is the account row's, shared by all three merged fields, so a later nonce or code-hash write silently changes the balance's reported last-modified height.
There are no production callers of GetBlockHeightModified today (only the giga.LiveStateStore interface declaration), so this is latent rather than live. Routing the account case through the same field-presence check Get uses — returning (-1, false, nil) when the named field is zero — would keep the two reads consistent. Note the code-hash half of this predates the PR; balance is the new instance.
| StoreCode, | ||
| StoreStorage, | ||
| StoreMisc, | ||
| StoreBalance, |
There was a problem hiding this comment.
[suggestion] Adding StoreBalance to AllEVMStoreTypes() makes NewEVMStateStore open a sixth sub-DB directory (<dir>/balance) when evm-ss-separate-dbs = true. On an existing store that directory is created fresh, so its GetLatestVersion() is 0 — and EVMStateStore.GetLatestVersion() takes the min across managedDBs (store.go:174-185). RecoverCompositeStateStore (ss/composite/store.go:686-694) derives startVersion from that min, so the first startup after this change replays the whole available changelog into the EVM store, re-applying already-present versions and walking SetLatestVersion back up from the WAL's first entry.
The flag is non-default and documented as experimental (docs/migration/giga_store_migration.md), so this is a slow/noisy upgrade rather than data loss. Still worth handling explicitly — e.g. seed a newly created sub-DB's latest-version marker from the store's existing max on first open, or have recovery ignore sub-DBs that are empty.
| baseNonce := s.store.base.GetNonce(address) | ||
| // The base reader reports no presence of its own, so a missing account is indistinguishable from | ||
| // one whose nonce is zero. | ||
| return baseNonce, baseNonce != 0 |
There was a problem hiding this comment.
[suggestion] This contradicts the contract the PR just wrote for EVMStateView: "an account that exists with a nonce of 0 reads as (0, true)" (sei-db/state_db/giga/state_view.go:50-51), and the method doc "whether addr has an account". An account present only in the base reader with nonce 0 (e.g. funded but never having sent a tx) reads as (0, false) here.
AccountExists on this same type (line 387) already solves the base-presence problem by type-asserting the base reader for an AccountExists(common.Address) bool method; GetNonce could reuse it instead of inferring presence from baseNonce != 0.
The mirror-image case is GetBalance (line 422): an explicitly written zero balance returns (0, true), whereas flatKVStateView.GetBalance returns (0, false) for a stored zero. So the two implementations of one interface disagree on the zero cases in opposite directions. No caller branches on the bool yet, so nothing is broken today — but the first one that does will get implementation-dependent behaviour. Either align the implementations or state in the interface doc that a zero value may be reported as present or absent for fields whose zero encodes absence.
| codeKeyPrefix = []byte{0x07} | ||
| codeHashKeyPrefix = []byte{0x08} | ||
| nonceKeyPrefix = []byte{0x0a} | ||
| balanceKeyPrefix = []byte{0x21} |
There was a problem hiding this comment.
[suggestion] This is the second declaration of 0x21; x/evm/types.BalanceKeyPrefix is the first, and the block comment above ("mirrored from x/evm/types.go") is the only thing tying them together. TestEVMKeyPrefixesAreDistinct checks the mirrored prefixes are distinct from each other but not that they still match their originals, so a future prefix change in x/evm/types reroutes balance keys to the misc lane with a green test suite.
sei-db test packages can import x/evm/types (see sei-db/state_db/ss/composite/recovery_test.go:374 using evmtypes.NonceKeyPrefix), so a drift test asserting each EVMKeyPrefixByte(kind) equals the corresponding evmtypes.*KeyPrefix[0] is available and would cover the whole mirrored set, not just balance.
| return value, value != nil | ||
|
|
||
| case keys.EVMKeyNonce, keys.EVMKeyCodeHash: | ||
| case keys.EVMKeyNonce, keys.EVMKeyCodeHash, keys.EVMKeyBalance: |
There was a problem hiding this comment.
GetBlockHeightModified and Get disagree about whether a balance key exists.
Get on a balance key for an account whose balance is zero returns (nil, false), but GetBlockHeightModified on the same key returns (height, true).
Is that an issue?
| changesByType[keys.EVMKeyNonce], | ||
| changesByType[keys.EVMKeyCodeHash], | ||
| nil, // TODO: update this when we add a balance key! | ||
| changesByType[keys.EVMKeyBalance], |
There was a problem hiding this comment.
Before this PR the balance bytes were always zero, because we passed nil for the balance argument so "a row exists" implied "nonce ≠ 0 or codeHash ≠ 0".
This PR replaces that nil with changesByType[keys.EVMKeyBalance] (store_apply.go:129), so a balance write alone now creates a row, and "a row exists" no longer implies anything about the nonce.
Question is will this behavior change cause any issues?
|
Marking this PR as a draft until we figure out proper semantics |
2 participants
Describe your changes and provide context
This PR updates FlatKV in two important ways:
ok boolreturn parameter