feat(seidb): Add JSON output to evm-logical-digest and inspect a FlatKV migration in flight - #4166
blindchaser wants to merge 4 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR SummaryLow Risk Overview Digest and inspect reports are built from shared Inspect now supports Reviewed by Cursor Bugbot for commit b64ef6b. 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).
|
blindchaser
changed the title
![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.
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 b46b058. Configure here.
| } | ||
| account := getSemanticAccount(accounts, keyBytes) | ||
| copy(account.codeHash[:], rawVal) | ||
| account.codeHashRow = true |
There was a problem hiding this comment.
Account filter misses balance fragments
Medium Severity
consumeSemanticMemiavlLeafFiltered applies allowAccountKey to nonce and code-hash rows but not to EVMKeyBalance. A prefix inspect of the account bucket still inserts every funded out-of-shard address into the accounts map, so the new early filter does not actually drop those fragments before buffering.
Reviewed by Cursor Bugbot for commit b46b058. Configure here.
![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: b46b058e69
ℹ️ 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".
| fmt.Printf("key=%X logical=%X\n", physKey, logical) | ||
| entry.Meta = meta | ||
| } | ||
| a.entries = append(a.entries, entry) |
There was a problem hiding this comment.
With --list --list-limit<=0 in the existing text mode, every matching key is now converted to hex and retained in a.entries even though the entry is immediately printed and the slice is never used. Large account or storage listings therefore change from constant-memory streaming to memory proportional to the entire result and can OOM before the scan completes; only append to entries when producing the JSON object.
Useful? React with 👍 / 👎.
| if acc.inspectBucket == flatkvBucketAccount { | ||
| accounts = make(map[string]*semanticAccountDigestState) | ||
| } | ||
| if err := consumeCompositeFlatKV(source.opened, acc.addLogical, accounts, acc.matchesAccountPhysicalKey, nil); err != nil { |
There was a problem hiding this comment.
When --backend composite --inspect-bucket misc is run during or after migration, this FlatKV scan feeds migration/migration-boundary or migration/migration-version into the inspect accumulator. The global digest explicitly XORs those FlatKV-only rows out in miscForCompare, so identical logical state can have matching global misc digests but mismatching inspect shards, preventing the new inspection flow from locating the real discrepancy.
Useful? React with 👍 / 👎.
| if allowAccountKey != nil && !allowAccountKey(ktype.EVMPhysicalKey(keys.EVMKeyNonce, keyBytes)) { | ||
| return nil |
There was a problem hiding this comment.
For semantic account inspection with a narrow --key-prefix, the new filter is applied to nonce and code-hash fragments but not to the balance case below, which still calls getSemanticAccount for every address. On a large replay containing many out-of-prefix balance rows, the command therefore buffers nearly the full account population and can exhaust memory despite sharding; apply the account-key filter once before buffering any account fragment rather than repeating it selectively.
AGENTS.md reference: AGENTS.md:L115-L119
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid refactor: rendering is split from accumulation so the prose and JSON forms are provably rendered from one report struct, the isZeroAccount rewrite is behavior-preserving, and the composite-source extraction closes handles on every early return. No blockers — the findings are a text-mode memory regression on unlimited --list, an account filter that balance rows bypass, an omitempty ambiguity in the new JSON contract, and a CHANGELOG entry pointing at the wrong PR.
Findings: 0 blocking | 7 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The package doc block (
evm_logical_digest.go:112-164) still documents only the pre-PR surface: there is no usage example for--json, nor for--backend composite --inspect-bucket/--memiavl-open-mode=replayin inspect mode. Since composite inspect is the headline capability in the PR title ("locate EVM digest mismatches during migration"), the one place an operator reads to learn the tool now omits it. A# Shard the account bucket across a drain in flight:example alongside the existing inspect examples would close it. - [suggestion]
--find-hashoutput never reaches the JSON report.addLogicalemitsFOUND-HASHthroughdigestOut.sayf(evm_logical_digest.go:330), which is stderr narration under--json.--find-hashis the tool's primitive for pinpointing the single diverging row, so a scheduled caller that reads only stdout — exactly the caller--jsonexists for — gets the digest but not the located entry. Consider collecting matches into afound_hash_entriesfield onevmDigestJSONin addition to narrating them. - [suggestion]
inspectCompositeMigrateEVMpassesnilfor both progress callbacks (evm_logical_digest.go:1252 and 1262), so a composite inspect over mainnet-sized FlatKV plus the memiavl tail emits no progress at all until it finishes, whiledigestCompositeMigrateEVMover the same two scans narrates every 20M rows. These are multi-hour scans; reusing the digest path's callbacks (or a shared one) would keep the two modes symmetric. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| fmt.Printf("key=%X logical=%X\n", physKey, logical) | ||
| entry.Meta = meta | ||
| } | ||
| a.entries = append(a.entries, entry) |
There was a problem hiding this comment.
[suggestion] a.entries is appended unconditionally, including in text mode where the very next lines stream the same pair to digestOut. Before this PR, --list was pure streaming with constant memory; now every listed entry is retained as two freshly allocated hex strings (%X of the physical key and of the logical value) for the lifetime of the run.
With the default --list-limit 1000 this is negligible, but --list-limit 0 means unlimited, and that is the documented way to dump a full bucket. A storage-bucket dump on mainnet then holds a evmInspectEntryJSON per matching row — ~200 bytes of hex per 32-byte key/value pair — with nothing bounding it.
Guard the append on the same condition that already guards the prose:
if digestOut.jsonReport != nil {
a.entries = append(a.entries, entry)
} else if meta != "" {
digestOut.sayf("key=%X logical=%X %s\n", physKey, logical, meta)
} else {
digestOut.sayf("key=%X logical=%X\n", physKey, logical)
}| account := getSemanticAccount(accounts, keyBytes) | ||
| copy(account.codeHash[:], rawVal) | ||
| account.codeHashRow = true | ||
| case keys.EVMKeyBalance: |
There was a problem hiding this comment.
[suggestion] The EVMKeyBalance case does not consult allowAccountKey, unlike the EVMKeyNonce (line 1957) and EVMKeyCodeHash (line 1972) cases just above. Every balance row therefore calls getSemanticAccount and allocates a semanticAccountDigestState in accounts, for every address in the unmigrated memiavl tail, regardless of the prefix filter.
The emitted result is still correct — finalizeSemanticAccounts routes each account through consume, and consumeLogical re-applies the same prefix test — so this is not a wrong-output bug. But the PR describes the filter as dropping "out-of-shard account fragments before buffering", and since essentially every live account carries a balance row, the filter buys almost nothing: accounts still grows to the full address space. That defeats the point of sharding an account inspect on a node too large to scan in one pass.
Applying the same two-line guard here would fix it. TestInspectAccountPrefixFilterSkipsOutOfRangeMemiavlAccounts only feeds nonce rows, so it passes either way — extending it with a balance row for the skipped address (and asserting len(accounts) == 1) would pin the buffering claim rather than just the match count.
| ShardNextBytes int `json:"shard_next_bytes"` | ||
| Matched uint64 `json:"matched"` | ||
| List bool `json:"list"` | ||
| Listed int `json:"listed,omitempty"` |
There was a problem hiding this comment.
[suggestion] omitempty on Listed and ListLimit drops both fields when they are 0, and 0 is meaningful for each: listed: 0 means "listed nothing matched" and list_limit: 0 means "unlimited" (per the flag help, <=0 means unlimited). A consumer reading report.listed gets undefined precisely in the zero-match case, which is the interesting outcome when hunting a mismatch, and cannot tell an unlimited run from a field that was never applicable.
This also cuts against the contract the PR pins elsewhere: TestDigestJSONNamesTheMarkerAdjustmentsBehindTheMiscBucket asserts marker_adjustments encodes as [] rather than null specifically so callers need no presence check. Dropping omitempty from these two ints (they are already gated by the list boolean in the same object) would make the inspect report consistent with that.
| ## Unreleased | ||
|
|
||
| ### Improvements | ||
| * [#4156](https://github.com/sei-protocol/sei-chain/pull/4156) feat(seidb): `evm-logical-digest` can emit both digest and inspect reports as one JSON object on stdout, while progress and warnings go to stderr. Inspect mode now supports the mid-migration composite EVM view and semantic memiavl replay, so operators can shard and locate mismatched EVM keys during a FlatKV drain. Digest replay opens memiavl read-only without changelog repair, so an observer cannot truncate a live node's changelog. |
There was a problem hiding this comment.
[suggestion] Two things to fix in this entry:
- The link points at PR Add JSON output to evm-logical-digest and let inspect read a drain in flight #4156, which is closed; this is PR feat(seidb): Add JSON output to evm-logical-digest and inspect a FlatKV migration in flight #4166. Both the number and the URL need updating.
- "Digest replay opens memiavl read-only without changelog repair, so an observer cannot truncate a live node's changelog" describes behavior that already exists on the base branch —
openMemiAVLReplayReadOnlyand its no-repair doc comment are unchanged by this diff, anddigestMemIAVLReplayalready routed through it. The new part is that inspect mode can now reach that same open path. Rewording to credit only what this PR adds keeps the entry accurate for anyone bisecting the behavior later.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4166 +/- ##
==========================================
- Coverage 66.66% 65.54% -1.12%
==========================================
Files 2200 2080 -120
Lines 169431 157734 -11697
==========================================
- Hits 112954 103394 -9560
+ Misses 56336 54199 -2137
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The --json report is a machine-readable stdout channel, but a refused run put a bare error line there as well: cobra reports to stderr, then main duplicated it onto stdout. A scheduled caller piping stdout to a parser saw a parse error, and the tool's own SEI_LOG_OUTPUT warning named the wrong cause, since setting that variable does not move this line. main.Execute is the one point every subcommand returns through, so the stream is corrected there rather than in the command that happens to have --json today.
1 participant
Summary
Adds a
--jsonflag toseidb evm-logical-digest, and extends inspect mode towork during a FlatKV migration.
Getting JSON out required splitting rendering from accumulating: both forms now
render from one report struct per run (
evmDigestJSON/evmInspectJSON), sono number is computed twice and the prose and the JSON cannot drift. A
package-level
digestSinkholds the two destinations, so one assignmentredirects every line the scan helpers emit.
sei-db/tools/cmd/seidb/operations/evm_logical_digest.go:--jsonemits the report as one JSON line on stdout and moves the narrationto stderr. Storage-layer logging is raised to error level, because
seilogwrites to stdout and fixes its destination at process start. A warning fires
when
SEI_LOG_OUTPUTstill points at stdout; it goes to the narration,never into the report.
marker_adjustmentsnames the migration marker rows XORed out of the miscbucket. That list is the only thing distinguishing a mid-migration reading
from a completed one, since the misc digest is adjusted in both.
change. It is one nullable field shared by the row-level and account-level
counters, so a half-counted census is unreachable.
nilreads as "notmeasured" in both forms, not as all-zero.
semanticAccountDigestStaterecords whether a code-hash row was present, soa stored all-zero row is distinguished from an absent one.
--backend composite(FlatKV rows plus memiavl rowspast the migration boundary) and
--memiavl-open-mode=replay, reusing theexisting no-repair read-only open. Translator normalization and
--detailsstorage inspect still require snapshot mode and now reject replay with their
own messages.
filter, and an optional progress callback, so digest and inspect share one
scan. The filter drops out-of-shard account fragments before buffering.
FlatKV rows, which cannot observe whether memiavl held a code-hash row.
Test plan
sei-db/tools/cmd/seidb/operations/evm_logical_digest_test.gothe run context match between prose and JSON, for both digest and inspect
reports. JSON output is one line and carries no prose.
counter levels (table over nil and non-nil); an untaken census omitted from
both forms.
boundary row, misc bucket equal in both, and empty encodes as
[]notnull.SEI_LOG_OUTPUTis unset, absent when it isredirected, and never in the report buffer.
matching a reference accumulator; an account prefix filter skips out-of-range
addresses.
Existing tests take the new census argument and otherwise assert unchanged
behavior.