statesync: send page-encoded storage leaves verbatim (protocol 1.3) - #2483
statesync: send page-encoded storage leaves verbatim (protocol 1.3)#2483maxkozlovsky wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the statesync wire protocol to v1.3 by explicitly marking when a page-encoded server has finished sending the complete contiguous run of storage slots for a page, allowing the client to build covered pages from scratch (skipping per-page trie read/merge). It also threads the requester’s negotiated protocol version through to the C++ server and aligns version numbering semantics.
Changes:
- Add
SYNC_TYPE_UPSERT_STORAGE_PAGE_ENDmarker and client-side tracking (open_run/covered_pages) to safely skip page read-backs when a run closes cleanly. - Carry negotiated protocol version in
monad_sync_requestand gate server emission of page markers onrq.version. - Harden unknown/invalid upsert handling (client returns
falseon reject; server-side upsert classification uses an exhaustive switch to avoid remotely-triggerable aborts).
Verdict: NEEDS CHANGES
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/crates/monad-statesync/src/ffi.rs | Export new upsert type in Rust FFI surface. |
| category/statesync/test/test_statesync.cpp | Extend tests for version negotiation and page-run/coverage semantics. |
| category/statesync/test/test_network_shutdown.cpp | Add regression test ensuring production send path accepts all upsert types. |
| category/statesync/test/fuzz_statesync.cpp | Fuzz injection path for orphan/malformed page-end markers. |
| category/statesync/statesync_version.h | Define major/minor encoded protocol versions (v1.0–v1.3). |
| category/statesync/statesync_version.cpp | Enforce page-key shift invariance for page-groups; update compatibility logic. |
| category/statesync/statesync_server.cpp | Gate server emission of page-end markers by requester version. |
| category/statesync/statesync_server_network.hpp | Replace runtime abort whitelist with exhaustive upsert classification helper. |
| category/statesync/statesync_protocol.hpp | Introduce PageGroups protocol specialization; pass prefix into upsert handling. |
| category/statesync/statesync_protocol.cpp | Implement marker handling, per-prefix run tracking hooks, and failure logging. |
| category/statesync/statesync_messages.h | Add new upsert enum value and carry version in request struct. |
| category/statesync/statesync_client.cpp | Select protocol implementation by declared client decode version; pass prefix through. |
| category/statesync/statesync_client_context.hpp | Add PageRun, open_run, and covered_pages to support coverage tracking. |
| category/statesync/statesync_client_context.cpp | Track/close runs, mark runs split by commit, and skip merges for covered pages. |
| category/execution/ethereum/db/trie_db.hpp | Add storage_read_count() helper for tests measuring avoided reads. |
🤖 Generated with Claude Code
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Traced the page-groups paths end-to-end (open/close run, split_by_commit, covered_pages sanitation on delete/commit) and the hostile-peer sequences the fuzzer + protocol-validation tests exercise. Adversarial orderings I could construct (STORAGE → ACCOUNT_DELETE → PAGE_END; delete/recreate straddling covered_pages; duplicate/oversized markers; page-runs across a per-slot commit boundary) either fall out to read-and-merge via split_by_commit, land on empty on-disk pages so the "build from scratch" outcome is bit-identical, or get scrubbed by the covered_pages.clear() on the next commit. The version-negotiation gates (version >= PAGE_GROUPS on both peer selection and server emit_page_groups) match the wire-compatibility story in the description. Ownership of the new PageRun value type and the 48→56 byte monad_sync_request growth is captured by the static_assert and the regenerated bindgen re-export. No P0/P1 findings.
Verdict: CORRECT
🤖 Generated with Claude Code
4110689 to
9fbf6c9
Compare
8b72fa4 to
309092c
Compare
309092c to
11fca9c
Compare
11fca9c to
da42977
Compare
da42977 to
8d2c672
Compare
A page-encoded statesync client read every touched storage page back out of its own trie and merged into it, because mpt upsert is set-not-merge and writing a page twice loses the slots of the earlier write -- one blocking trie descent per page, on every sync. Protocol 1.3 adds SYNC_TYPE_UPSERT_STORAGE_PAGE: for a peer at 1.3 or newer a page-encoded server emits one record per storage leaf, payload = address followed by the leaf exactly as stored, with no decode and no re-encode. The client decodes it, feeds each slot through the existing storage_update so the account, buffered-storage, incarnation and dual-write paths stay untouched, and records the page as covered; commit() then builds a covered page from empty rather than reading it back. A peer below 1.3 keeps the per-slot expansion loop, so its wire is unchanged. The record is peer input, so the shared storage decoder bounds the key it converts to a bytes32 (an over-long key reached an assert that fires in release), an empty page is refused rather than granted coverage that no slot backs, and the commit window counts slot deltas instead of records, so a 128-slot page cannot buffer 128x the deltas its threshold names. Getting there also required carrying the requesting peer's protocol version to the C++ server, which bft previously dropped, and routing unrecognized upsert types through the existing rejection path instead of a bare assert -- a client below 1.3 can now receive a record type it has no case for, and rejecting it names the record in the log before bft's own assert takes the process down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8d2c672 to
5f6fbd6
Compare
Stacked on #2487 — that lands first; this PR's own change is the second commit.
A page-encoded statesync client read every touched storage page back out of its
own trie and merged into it, because mpt upsert is set-not-merge and writing a
page twice loses the slots of the earlier write — one blocking trie descent per
page, on every sync.
Protocol 1.3 adds
SYNC_TYPE_UPSERT_STORAGE_PAGE: for a peer at 1.3 or newer apage-encoded server emits one record per storage leaf, payload = address
followed by the leaf exactly as stored, with no decode and no re-encode. The
client decodes it, feeds each slot through the existing
storage_updateso theaccount, buffered-storage, incarnation and dual-write paths stay untouched, and
records the page as covered;
commit()then builds a covered page from emptyrather than reading it back. A peer below 1.3 keeps the per-slot expansion loop,
so its wire is unchanged.
The record is peer input, so the shared storage decoder bounds the key it
converts to a bytes32 (an over-long key reached an assert that fires in
release), an empty page is refused rather than granted coverage that no slot
backs, and the commit window counts slot deltas instead of records, so a
128-slot page cannot buffer 128× the deltas its threshold names.
Getting there also required carrying the requesting peer's protocol version to
the C++ server, which bft previously dropped, and routing unrecognized upsert
types through the existing rejection path instead of a bare assert — a client
below 1.3 can now receive a record type it has no case for, and rejecting it
names the record in the log before bft's own assert takes the process down.
Companion monad-bft PR: category-labs/monad-bft#3216