feat(seidb): composite + replay digest modes and migration-boundary omission for evm-logical-digest#3711
feat(seidb): composite + replay digest modes and migration-boundary omission for evm-logical-digest#3711blindchaser wants to merge 7 commits into
Conversation
…mission for evm-logical-digest Port the evolved evm-logical-digest tooling to main: - `--backend composite` (with `--flatkv-dir` / `--memiavl-dir`): digests the full mid-migration EVM logical view as the union of flatkv rows plus the memiavl rows not yet migrated past the boundary, so an in-progress node can be compared against a memiavl-only truth node. - `--memiavl-open-mode snapshot|replay`: snapshot (default, fast: sequential scan of the completed snapshot kvs file) vs replay (opens a read-only DB, replays the changelog to --height, walks the mmap tree) for heights with no on-disk snapshot. Documented the speed trade-off and when to prefer each. - Omit the FlatKV-only `migration/migration-boundary` cursor from the legacy bucket comparison, mirroring the existing migration-version handling, so an in-progress (composite) node compares apples-to-apples against completed / memiavl-only nodes. Extracted legacyForCompare() and added a regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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 #3711 +/- ##
==========================================
- Coverage 59.88% 58.35% -1.53%
==========================================
Files 2283 2187 -96
Lines 189429 178165 -11264
==========================================
- Hits 113431 103965 -9466
+ Misses 65906 64932 -974
+ Partials 10092 9268 -824
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.
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 b7cea4f. 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: b7cea4f9c6
ℹ️ 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".
| if err := d.consume(k, iter.Value()); err != nil { | ||
| return err |
There was a problem hiding this comment.
When FlatKV contains any non-EVM module rows, this loop now feeds every RawGlobalIterator row into d.consume; RawGlobalIterator walks all data DB keys, while the memiavl side only scans the evm tree. Those bank/staking/etc. rows are folded into the legacy bucket, so FINAL_DIGEST can diverge for identical EVM state; the removed shouldIncludeFlatKVEVMLogicalDigestKey filter was the guard here, and the new composite FlatKV scan follows the same unfiltered pattern.
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds composite and replay digest modes plus migration-boundary marker omission to the seidb evm-logical-digest diagnostic tool. The changes are well-documented and covered by regression tests; no blocking correctness issues found. One asymmetry (raised by Codex) is worth noting but is non-blocking for the tool's intended migrate_evm use.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Legacy-bucket cross-backend asymmetry (Codex, re-scoped): removing
shouldIncludeFlatKVEVMLogicalDigestKeymakesdigestFlatKV/inspectFlatKVconsume ALL FlatKV rows, and FlatKV'slegacyDBroutes non-EVM modules (bank/staking/etc.) perCommitStore.routePhysicalKey. The memiavl side still scans only the EVM tree, so on a node where non-EVM modules have been migrated into FlatKV thelegacybucket digest/count would be polluted and no longer comparable to memiavl (the marker-adjustment lines become meaningless). This does NOT affect the primary account/code/storage comparison, and in the migrate_evm deployment this PR targets FlatKV holds only EVM data (consistent with the PR's byte-for-byte validation), so I disagree with Codex's blocker framing for a diagnostic tool. Suggest either re-adding an EVM-scope guard to the legacy bucket or documenting that the legacy comparison is only valid when FlatKV contains EVM state exclusively. - Test coverage: the deleted
TestShouldIncludeFlatKVEVMLogicalDigestKeyis not replaced by any test exercisingdigestFlatKV's handling of non-EVM/legacy rows, so the behavior change above is untested. - Second-opinion passes:
cursor-review.mdis empty (Cursor produced no output);REVIEW_GUIDELINES.mdis empty (no repo-specific guidelines applied).
Collapse the snapshot vs replay and semantic vs translator memiavl digest duplication behind a shared evmLeafSource abstraction (one snapshot kvs-file scanner + one replay-tree scanner) and two shared cores (runMemiavlSemanticDigest / runMemiavlTranslatorDigest). The inspect memiavl paths and the composite unmigrated-tail consume are folded onto the same scanners. Behavior is unchanged except the snapshot translator progress log now carries `mode=translator` for consistency with the other modes; FINAL_DIGEST / bucket output is identical. File shrinks ~190 lines. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore the EVM-module ingestion filter on the FlatKV scan paths (digestFlatKV, consumeCompositeFlatKV, inspectFlatKV). FlatKV's RawGlobalIterator yields every module's rows and routes non-EVM modules into the legacy bucket, while a memiavl node walks the EVM tree alone; without the filter the legacy bucket and FINAL_DIGEST diverge from a memiavl-only node once FlatKV holds non-EVM Cosmos state. The FlatKV-only migration markers are still let through and XORed out in legacyForCompare. Addresses Cursor Bugbot review. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A well-scoped enhancement to the seidb evm-logical-digest diagnostic tool (composite + replay modes, migration-boundary marker omission) that refactors the memiavl leaf-scanning into a shared evmLeafSource abstraction and adds regression tests. No correctness or security issues found; one non-blocking usability limitation carried over from the Codex pass.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Composite mode requires a valid memiavl snapshot/replay even for a completed migration where no memiavl rows are consumed (see inline comment on line 451) — completed composite nodes should ideally be digestible from FlatKV alone. Reported by Codex; non-blocking for a diagnostic tool.
- The Cursor second-opinion review file (
cursor-review.md) was empty, so no Cursor findings were incorporated. - The Codex pass noted it could not run the tests locally (sandbox lacked the Go 1.25.6 toolchain; local Go was 1.24.13), so the added tests were not executed by that reviewer.
- Consider a brief note in the flag help / docs that
--backend compositeinsnapshotmode fails fast ifsnapshot-<height>/evmis absent even when the migration is already complete, so operators know to pick a height with an on-disk snapshot or use replay mode. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| var memReplayDB *memiavl.DB | ||
| var memEvmSnapshotDir string | ||
| var memVersion int64 | ||
| switch memiavlOpenMode { |
There was a problem hiding this comment.
[suggestion] This switch opens/validates the memiavl snapshot (resolve dir + read version) or replay DB unconditionally, before the boundary.Status() != migration.MigrationComplete guard at line 484. For a completed EVM migration no memiavl rows are ever consumed, yet the command still fails here if --memiavl-dir lacks snapshot-<height>/evm or cannot replay that height. Consider skipping the memiavl open when the boundary is already MigrationComplete so completed composite nodes can be digested from FlatKV alone. (Raised by Codex.)
- Extract memiavl-open-mode / normalization string literals (snapshot, replay, semantic, independent, translator) into named constants (goconst). - Change inspectAccumulator.listed to int and drop the uint64->int conversion in the list-limit check (gosec G115). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A well-scoped, thoroughly-documented refactor of the seidb evm-logical-digest diagnostic tool: adds a composite backend, a replay memiavl open mode, and folds the FlatKV-only migration-boundary marker out of the legacy comparison. The change is a read-only debugging tool (non-app-hash-breaking) with sound abstractions and good test coverage for the marker/merge logic; only minor UX and follow-up notes.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied beyond AGENTS.md.
- Balance handling (semanticAccountDigestState.balance + logicalPayload copying it) is a consistency change, not a bug: the EVM keyspace has no balance key, so the memiavl semantic path leaves balance zero, and the pre-existing digestFlatKV path already included ad.GetBalance(). It's worth a one-line note in the code that this makes the composite/flatkv account payload consistent with digestFlatKV; if FlatKV ever begins storing non-zero EVM balances, composite/flatkv account digests would diverge from the zero-balance memiavl-semantic truth node (a pre-existing property of digestFlatKV, not newly introduced here).
- In digestCompositeMigrateEVM, for a completed migration (boundary.Status()==MigrationComplete) the memiavl side is never scanned, yet the code still resolves the snapshot dir / reads the snapshot version (snapshot mode) or fully opens a read-only replay DB (replay mode) beforehand. For a completed node in replay mode this opens an expensive memiavl DB that is never used — consider deferring the memiavl open until after the completeness check.
- No end-to-end test exercises the new composite or replay paths (they require a real on-disk memiavl DB/snapshot). The added unit tests cover legacyForCompare and mergeCompositeFlatKVAccount well; an integration-style test would be nice-to-have but is understandably heavy for this tooling.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| memiavlNormalization, _ := cmd.Flags().GetString("memiavl-normalization") | ||
| memiavlOpenMode, _ := cmd.Flags().GetString("memiavl-open-mode") | ||
| if inspectBucket != "" { | ||
| if memiavlOpenMode != memiavlOpenModeSnapshot { |
There was a problem hiding this comment.
[nit] This guard rejects any non-snapshot --memiavl-open-mode before the backend is known, so --backend flatkv --inspect-bucket ... --memiavl-open-mode replay fails even though the memiavl open mode is irrelevant to a FlatKV inspect (matches Codex's finding). The default is snapshot, so this only bites users who explicitly pass the flag alongside --inspect-bucket, but scoping the check to the memiavl/composite inspect paths (or gating on backend == "memiavl") would avoid the spurious rejection. Low priority.
Document the mid-migration workflow (flatkv migrated rows ∪ memiavl not-yet-migrated rows) and when to use --memiavl-open-mode replay, which was missing from the Usage block despite composite being the tool's main mode for migrating nodes. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A well-structured refactor of the offline seidb evm-logical-digest tool that adds composite (mid-migration) and memiavl replay modes plus migration-boundary marker omission, with good tests and documentation. The one substantive concern (shared with Codex) is that replay mode does not assert the DB actually reached the requested height; it is an offline diagnostic and the reached version is printed, so this is non-blocking but worth fixing.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
- Test coverage regression: TestShouldIncludeFlatKVEVMLogicalDigestKey was deleted, even though shouldIncludeFlatKVEVMLogicalDigestKey was moved and extended to also let migrationBoundaryPhysKey through. Consider re-adding a test that asserts both marker keys (version + boundary) and an EVM key are included while non-EVM module rows are excluded.
- Balance is now plumbed into semanticAccountDigestState.logicalPayload from the flatkv side in composite mode, while the pure-memiavl semantic path leaves it zero. This is consistent today only because the migrate_evm keyspace carries no balance in memiavl (so flatkv-migrated balances are also zero); if that ever changes, a completed/composite node's account digest would diverge from a memiavl-only node. A brief comment noting this invariant would help future readers.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return err | ||
| return nil, fmt.Errorf("open memiavl read-only replay: %w", err) | ||
| } | ||
| return db, nil |
There was a problem hiding this comment.
[suggestion] Replay mode does not verify the DB reached the requested height. memiavl.OpenDB(height, …) seeks the nearest snapshot ≤ height and calls MultiTree.Catchup, which returns nil (no error) when the retained WAL tail is behind height (multitree.go:388-391). So for a positive height whose changelog has been truncated/retained short of it, this returns a DB at an older version, and both digestMemIAVLReplay and composite replay will silently digest a lower height than requested — producing a misleading cross-node comparison. The report does print requestedHeight and the actual version, which mitigates it, but consider adding an explicit guard here for height > 0, e.g. if height > 0 && db.Version() != height { db.Close(); return nil, fmt.Errorf("memiavl replay reached version %d, requested height %d", db.Version(), height) }. (Also raised by Codex as P1.)
There was a problem hiding this comment.
A well-structured extension of the seidb evm-logical-digest debug tool adding a composite backend, memiavl replay open-mode, and migration-boundary marker omission, with a large shared-scanner refactor. The logic is correct and the change is non-consensus (tooling only); the only notable gap is missing end-to-end test coverage for the composite path.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage gap (also raised by Codex): the new composite path has no end-to-end test. The added tests cover only legacyForCompare marker omission and flatkv+memiavl account-fragment merging; the boundary IsMigrated filtering, memiavl-unmigrated inclusion, marker omission within a composite run, and final digest composition are untested and could regress silently. Non-blocking since this is a debug/ops tool that was validated manually against the pacific-1 fleet, but a table test building a partial-migration boundary and asserting
flatkv ∪ unmigrated memiavl == memiavl-only digestwould lock in the core invariant. - Second-opinion inputs: cursor-review.md is empty (Cursor produced no output) and REVIEW_GUIDELINES.md is empty/missing (no repo-specific review standards were available to apply).
- Snapshot-mode composite on a completed migration (boundary Status == MigrationComplete) still calls resolveMemIAVLEvmSnapshotDir + readMemIAVLSnapshotVersion even though the memiavl side is never scanned; on a completed node whose memiavl snapshot at --height has been pruned this errors out spuriously. Minor, since a completed node is better digested with --backend flatkv.
- The new balance field in semanticAccountDigestState / logicalPayload is currently inert: there is no EVM balance key kind yet (flatkv writes nil balance, memiavl has no balance leaf), so all paths keep balance zero. Harmless and forward-compatible, but worth a one-line comment noting it only becomes meaningful once a balance key kind exists, to avoid future confusion about cross-backend account-bucket equality.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
There was a problem hiding this comment.
A well-structured refactor of the diagnostic evm-logical-digest tool adding composite/replay modes and migration-boundary marker omission, with good tests. One legitimate correctness gap (composite mode never enforces that the two backends are read at the same version) is worth guarding but is non-blocking for a diagnostic tool.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage was partial: cursor-review.md and REVIEW_GUIDELINES.md are both empty (no Cursor findings and no repo-specific guidelines to apply). Codex produced exactly one finding, incorporated below.
- Snapshot mode requires a memiavl snapshot at --height even when the flatkv migration is already complete (boundary.Status()==MigrationComplete), a case where memiavl is never scanned (see the skip at line 504). resolveMemIAVLEvmSnapshotDir/readMemIAVLSnapshotVersion run unconditionally at lines 473-480, so a completed node with no memiavl snapshot at that height fails needlessly. Consider resolving the snapshot lazily only when the boundary is incomplete.
- Behavior change: logicalPayload now includes the 32-byte balance (previously forced to zero). This is only populated on the flatkv side (mergeCompositeFlatKVAccount); memiavl semantic leaves never set balance, so it stays zero there. Since EVM-store account balance is effectively always zero in this keyspace this keeps digests consistent, but it's worth confirming a real flatkv node never carries a non-zero GetBalance() that would then diverge from a pure-memiavl digest.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| default: | ||
| return fmt.Errorf("unknown --memiavl-open-mode %q (want snapshot|replay)", memiavlOpenMode) | ||
| } | ||
| printDigestStart(ctx) |
There was a problem hiding this comment.
[suggestion] Composite mode reads memVersion (from the snapshot or the replay DB) but never compares it against opened.Version() before unioning flatkv rows with memiavl rows. With --height 0 (flatkv resolves to its latest version, memiavl to the current symlink) or when a node's snapshot rewrite lags the tip, the two backends can be at different chain heights, silently producing a digest for a state that never existed at a single height — which defeats the tool's apples-to-apples comparison purpose. Suggest rejecting a mismatch here (e.g. if opened.Version() != memVersion { return fmt.Errorf(...) }) before printDigestStart, or at minimum documenting that the operator must verify the two version fields in the output match.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
Summary
Ports the evolved
seidb evm-logical-digesttooling tomainso mid-migration nodes can be compared correctly:--backend composite(--flatkv-dir/--memiavl-dir): digests the full mid-migration EVM logical view = flatkv rows ∪ memiavl rows not yet migrated past the boundary. Lets an in-progressmigrate_evmnode be compared against a memiavl-only truth node (the flatkv-only backend alone would miss EVM keys still in memiavl).--memiavl-open-mode snapshot|replay:snapshot(default, fast — sequential scan of the completed snapshot kvs file; requires an on-disk snapshot at--height) vsreplay(opens a read-only DB, replays the changelog to--height, then walks the mmap tree; ~10x slower — for heights whose snapshot rewrite lags the tip). Doc/flag help spell out the trade-off and when to prefer each.migration/migration-boundarycursor (present only while a migration is in flight) is now XORed out of the legacy bucket for the final comparison, mirroring the existingmigration/migration-versionhandling. Without this, an in-progress composite node shows a spurious +1 legacy entry vs a completed / memiavl-only node. ExtractedlegacyForCompare()and added a regression test covering clean / completed / in-progress shapes.Context
Validated against the live pacific-1 flatkv fleet: at height 218270000, account/code/storage digests matched byte-for-byte across a memiavl-only node, a completed composite node, and two in-progress composite nodes; the only difference was the unfiltered boundary row, which this change removes.
Test plan
go test ./sei-db/tools/cmd/seidb/operations/go build ./sei-db/tools/cmd/seidb/...Made with Cursor