Cleanup MIP8 migration code: assume single page-encoded timeline in runloop_monad and statesync - #2551
Cleanup MIP8 migration code: assume single page-encoded timeline in runloop_monad and statesync#2551Chen-Yifan wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core execution/statesync invariants and a C ABI surface area, so a final human review (plus green CI) is needed to confidently validate correctness and compatibility.
Pull request overview
This PR finishes the “single page-encoded timeline” assumption across runloop_monad and statesync by removing the remaining dual-db/dual-timeline branching and asserting invariants at entry points. It also updates the runloop C ABI and test/fuzz coverage to match the new page-only behavior.
Changes:
runloop_monad: drop the secondary DB parameter and remove dual-writefor_each_dbpaths; assert page-encoding + no active secondary timeline.- statesync: client/server now operate page-only (server expands page leaves into slot upserts; client finalization requires MIP-8-active target revision) and remove secondary timeline plumbing.
- tests/fuzz: collapse fixtures to page-only and update hardcoded expected roots; fuzz only the page-encoded path.
File summaries
| File | Description |
|---|---|
| cmd/monad/main.cpp | Updates runloop/statesync wiring and comments to reflect page-only runloop_monad and page-required statesync server. |
| category/execution/runloop/runloop_monad.hpp | Removes the secondary_db parameter from runloop_monad API and documents the page-only invariant. |
| category/execution/runloop/runloop_monad.cpp | Removes dual-db writes/metadata updates, hardwires commit_block(..., secondary_db=nullptr), and adds entry asserts for page-only + single-timeline. |
| category/execution/runloop/runloop_interface_monad.h | Replaces primary/secondary state-root getters with a single monad_runloop_get_state_root API. |
| category/execution/runloop/runloop_interface_monad.cpp | Opens a page-encoded primary only, removes secondary timeline setup/usage, and implements the unified state-root getter. |
| category/statesync/statesync_server_context.cpp | Asserts the server context is constructed with a page-encoded TrieDb. |
| category/statesync/statesync_server.cpp | Removes slot-vs-page branching; always expands decoded storage pages into slot-format upserts on the wire. |
| category/statesync/statesync_client_context.hpp | Removes secondary timeline members and documents the page-only + MIP-8 target requirement. |
| category/statesync/statesync_client_context.cpp | Enforces page-only + no-secondary invariant and simplifies commit/roll-forward logic to single-timeline. |
| category/statesync/statesync_client.cpp | Simplifies finalize flow to single DB, and asserts target revision is MIP-8 active before comparing state roots. |
| category/statesync/test/test_statesync.cpp | Collapses pre/post-MIP8 fixture into a single page-only fixture; updates expected state roots and removes dual-db helpers. |
| category/statesync/test/fuzz_statesync.cpp | Removes slot-vs-page fuzzing split and fuzzes only the page-encoded path (DEVNET / MIP-8 active). |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3dce9dc to
89f4566
Compare
…unloop_monad and statesync runloop_monad and the statesync client and server now require a single page-encoded db and assert it on entry, instead of carrying an optional secondary timeline and branching on encoding. - runloop_monad: drop the secondary_db parameter and the for_each_db dual writes; assert db.is_page_encoded() and no active secondary. - statesync client: drop the secondary db and slot storage builder; commit() is page-only, finalize() rolls forward one db. Drop the chain_config parameter from the C ABI and the Rust StateSyncCtx, since the target revision no longer selects an encoding. - statesync server: always expand page leaves into slot upserts; the server context asserts a page-encoded TrieDb. - runloop C library: open a page-encoded primary only; replace the primary/secondary state root getters with monad_runloop_get_state_root. - tests: collapse the pre/post-mip8 typed fixture into one page-encoded fixture, drop chain and timestamp plumbing, update hardcoded state roots. runloop_monad_ethblocks keeps its secondary_db for pre-mip8 replay. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
89f4566 to
739e608
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the single-timeline/page-encoded cleanup across runloop_monad, statesync client/server, the C ABI, and the tests.
The overall direction is sound and the diff is internally consistent: runloop_monad and the statesync client/server now assert their preconditions instead of branching on encoding, the removed chain_config parameter is cleaned out of the C header, the Rust StateSyncCtx, and all in-repo callers, runloop_monad_ethblocks retains its dual-db path as described, and the identifier/temporal-comment sweeps come up clean (no stale references to secondary_tdb, for_each_db in the touched paths, or the removed C functions). The intrusive UpdateList lifetime discipline in the flattened commit() is preserved (deques outlive the upsert), and main.cpp still opens the db via the kind-driven ctor, so the new asserts in runloop_monad genuinely reject non-migrated dbs on the production path.
One blocking issue (inline): MonadRunloopImpl opens the primary with the machine-taking mpt::Db ctor, which force-stamps the persisted state_machine_kind to monad. That makes the new is_page_encoded() assert a tautology in this entry point and mutates a legacy slot-encoded db's metadata on open, which can make the db unreadable/corrupt for subsequent kind-driven opens. Switching to the kind-driven mpt::Db{OnDiskDbConfig} ctor (as main.cpp and the statesync client do) resolves it.
Verdict: NEEDS CHANGES
🤖 Generated with Claude Code
MonadRunloopImpl used the Db ctor that takes a StateMachine, which stamps the machine's kind into the pool metadata on every open and would relabel an existing pool of the other encoding. Use the kind-reading ctor instead and register the state machine factories in monad_runloop_new, so the encoding always comes from monad-mpt --create --state-machine. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@claude re-review please |
There was a problem hiding this comment.
Re-reviewed after 2027be8, which addresses the one blocking finding from the previous review.
The fix is correct: MonadRunloopImpl now opens the primary with the kind-driven mpt::Db{OnDiskDbConfig} ctor (same as cmd/monad/main.cpp and the statesync client), so the persisted state_machine_kind is read rather than overwritten, and MONAD_ASSERT(triedb.is_page_encoded()) genuinely rejects a non-migrated db instead of being a tautology. The factory registration in monad_runloop_new happens before the impl is constructed (the only construction path for MonadRunloopImpl), register_state_machine tolerates repeat registration, and the updated header comment documents the new precondition on db_path. The remaining machine-taking mpt::Db ctor uses in the diff are all test-fixture creation with .append = false, where stamping the kind is the intent.
No new findings. One hygiene note: the PR is now two commits; per the repo convention, squash before merging.
Verdict: CORRECT
🤖 Generated with Claude Code
runloop_monad and the statesync client and server now require a single page-encoded db and assert it on entry, instead of carrying an optional secondary timeline and branching on encoding.
runloop_monad_ethblocks keeps its secondary_db for pre-mip8 replay.