feat(seidb): locate EVM digest mismatches during migration - #4156
feat(seidb): locate EVM digest mismatches during migration#4156blindchaser wants to merge 1 commit into
Conversation
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. |
Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview Toolchain and lint: Workflows and Docker move to Go 1.27.1, Upgrade testing: Adds CI reliability: Integration / EVM CI: Release builds: GoReleaser and Misc: Reviewed by Cursor Bugbot for commit b46b058. 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 #4156 +/- ##
==========================================
- Coverage 66.60% 65.49% -1.12%
==========================================
Files 2196 2079 -117
Lines 169188 157439 -11749
==========================================
- Hits 112692 103107 -9585
+ Misses 56355 54191 -2164
Partials 141 141
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: 318201ee1e
ℹ️ 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".
| // it from the counts. | ||
| MarkerAdjustments []string `json:"marker_adjustments"` | ||
|
|
||
| ZeroCensus evmZeroCensus `json:"zero_census"` |
There was a problem hiding this comment.
When --memiavl-normalization=translator is selected, runMemiavlTranslatorDigest sends raw leaves only through ImportTranslator, so none of the new census hooks run; composite mode likewise records some raw zero values but skips the account-derived census by passing nil at line 700. Nevertheless every report serializes zero_census here, making a translator scan containing zero rows indistinguishable from a genuinely clean scan and leaving composite fields internally incomplete. Collect the census at a common raw-leaf scan choke point or explicitly encode it as unavailable for unsupported modes.
AGENTS.md reference: AGENTS.md:L115-L119
Useful? React with 👍 / 👎.
| // digestSink is where this command's output goes. prose takes the narration — | ||
| // the start banner, scan progress, and the human-readable report. jsonReport is | ||
| // nil in the default text mode and, in JSON mode, takes the encoded report while | ||
| // prose moves to stderr, so stdout carries only the object. | ||
| // |
There was a problem hiding this comment.
This new godoc spends multiple paragraphs explaining the output-routing mechanism and the rationale for retaining narration rather than concisely documenting what digestSink represents. Rewrite it as a one- or two-sentence description and keep any load-bearing rationale inline at the relevant routing decision, as required by the repository's godoc convention.
AGENTS.md reference: AGENTS.md:L79-L90
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-scoped forensic-tooling change: --json renders from the same report() value as the prose (pinned by a test), the narration redirect is clean, and the isZeroAccount → !isLiveAccount() && hasZeroCodeHash() split is genuinely behaviour-preserving including the nil case, so no digest value moves. The one substantive issue is the new zero_census field, which is populated only partially in composite mode and not at all in the flatkv/translator backends while still being emitted as measured-looking zeros.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
zero_censusis emitted unconditionally in JSON but is only ever populated byconsumeSemanticMemiavlLeaf/finalizeSemanticAccounts. The flatkv backend (digestFlatKV) and memiavl translator normalization (runMemiavlTranslatorDigest) route every row throughd.consume, so those runs report an all-zero census that a caller cannot distinguish from "measured, found none" — the prose sidesteps this by suppressing the block on the zero value, but the object does not. Consideromitemptyon a pointer census, or an explicitzero_census_collected/census: nullfor backends that do not measure it. - [suggestion] Several of the new godocs carry rationale rather than description, which
AGENTS.md("Godoc": explain WHAT not WHY; multi-paragraph godocs are rare) asks to move to an inline comment at the line that needs it.sayf's comment is entirely justification for dropping write errors,sink's is entirely justification for being a package variable, anddigestSink/enterJSONModeeach add a second rationale paragraph. The content is worth keeping — it just belongs inline next to_, _ = fmt.Fprintf(...)and thesink.prose = os.Stderrassignment. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| finalizeSemanticAccounts(accounts, d.addLogical) | ||
| d.print(ctx) | ||
| return nil | ||
| finalizeSemanticAccounts(accounts, d.addLogical, nil) |
There was a problem hiding this comment.
[suggestion] Composite mode ends up with a half-measured census. consumeCompositeMemiavl calls d.consumeSemanticMemiavlLeaf (line 793), which passes &d.census and so accumulates ZeroCodeHashRows, EmptyCodeValues and ZeroStorageSlots over the unmigrated memiavl tail — but finalization here passes nil, so ZeroAccounts and LiveAccountsWith/WithoutCodeHashRow stay 0 no matter what the data holds.
The result is a report whose fields disagree about what was counted: "zero_census":{"zero_accounts":0,...,"zero_codehash_rows":N} with N>0 in JSON, and a prose block that prints (because the struct is non-zero) showing zero_accounts=0 beside a non-zero zero_codehash_rows. A scheduled composite-mode caller — the one this PR is built for, since composite is also what carries the migration boundary in normalization — reads a silent zero for something never measured.
Passing &d.census here is not the fix on its own: composite accounts are merged from FlatKV too via mergeCompositeFlatKVAccount, which never sets codeHashRow, so every FlatKV-sourced account would land in LiveAccountsWithoutCodeHashRow. Either complete the accounting on both sources, or make the omission explicit so the account-level fields are not reported as measured zeros (see the omitempty/census: null note in the review).
| cmd.SilenceUsage, cmd.SilenceErrors = true, true | ||
|
|
||
| err := cmd.Execute() | ||
| require.ErrorContains(t, err, "--inspect-bucket") |
There was a problem hiding this comment.
[suggestion] This assertion can pass for the wrong reason: the same branch returns --inspect-bucket does not support --memiavl-open-mode=%q yet (line 589), which also contains --inspect-bucket. It only exercises the --json refusal today because --memiavl-open-mode happens to default to memiavlOpenModeSnapshot; change that default and the test stays green while the refusal it names goes untested. Asserting on --json instead pins the intended error.
![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 2cdbfc5. Configure here.
|
Thanks — three reviewers independently found the composite census bug, and it reproduced exactly as described. Pushed 6e591a1 and 04e0422. The composite census (codex P2, seidroid, Bugbot medium)Confirmed all three legs: Rather than patch the two call sites, the census now lives in one field both levels read (
|
|
Verified the Pod
The new Two honest limits on this run, both recorded in the evidence note: Text and JSON were not shown here to carry the same numbers. Both used The partial-census case this PR also closes is unreachable on this node. A composite digest scans memIAVL leaves only while |
04e0422 to
ca02c28
Compare
blindchaser
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
ca02c28 to
b46b058
Compare
1 participant
Summary
Tests
GOPROXY=direct GOTOOLCHAIN=auto go test ./sei-db/tools/cmd/seidb/operations -count=1GOPROXY=direct GOTOOLCHAIN=auto make fmtcheck