[db] Add --snapshot-format to choose the layout a dump writes - #2566
maxkozlovsky wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new parameter changes an extern "C" function signature without preserving or coordinating the existing ABI.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds selectable snapshot dump layouts, defaulting to headerless v0 for compatibility with older consumers.
Changes:
- Adds
--snapshot-formatwith v0/v1 validation. - Makes the writer emit headers only for v1.
- Adds v0/v1 dump comparison and restore coverage.
Verdict: NEEDS CHANGES
🤖 Generated with Claude Code
File summaries
| File | Description |
|---|---|
cmd/monad_cli.cpp |
Adds the CLI format option and logging. |
category/execution/ethereum/test/test_db_snapshot.cpp |
Tests both layouts and v0 restoration. |
category/execution/ethereum/db/db_snapshot.h |
Defines formats and updates the dump API. |
category/execution/ethereum/db/db_snapshot.cpp |
Validates formats and conditionally writes headers. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 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.
Review summary
Reviewed the single commit (8f3c86f) adding --snapshot-format to monad-cli and threading monad_snapshot_format through monad_db_dump_snapshot. The change is well-constructed:
- Format validation —
check_snapshot_formatrejects an unknown enumerator before the dump opens the database, and the default-less switch turns a future enumerator into a-Wswitchbuild error at both dispatch sites (db_snapshot.cpp,snapshot_format_nameinmonad_cli.cpp). - Version coupling — the
static_assert(MONAD_SNAPSHOT_FORMAT_V1 == MONAD_SNAPSHOT_STREAM_VERSION)pins the format enum to the stream header version, so they cannot drift apart silently. - Offset invariance —
account_bytes_written_counts from the first record rather than the stream start, so storage-record offsets are identical in v0 and v1 layouts; the loader is unchanged and its magic-sniffing already handles both. - Callers — all in-repo callers of
monad_db_dump_snapshotwere updated; the header is not consumed by the Rust crates, so no bindgen impact. - Tests —
SnapshotFormatV0OmitsStreamHeaderspins v0 = v1 minus headers and restores a v0 dump end to end;Basicnow exercises v0 through the checksum-verifying filesystem loader. One[P3]inline note on a failure-path out-of-bounds read in the new test.
The default of v0 (headerless) and the reader's accept-only-v1-or-headerless policy are both deliberate per the PR description and documented in the enum comment, so they are not flagged.
Verdict: CORRECT
🤖 Generated with Claude Code
A loader takes a stream with or without a header, but a binary that predates the header aborts on one, so a node that always writes headers cuts off every consumer that has not upgraded yet. Give monad-cli --snapshot-format so a publisher can keep writing the pre-header layout (v0, the default) while its own loader still accepts both, and move to v1 once every consumer understands it. A format is stamped verbatim into the header's version field, so the dump rejects one it does not know before writing anything: otherwise a stale enumerator would leave a complete, checksummed snapshot that no reader accepts, and the failure would surface at restore. v0 writes the records alone: stream for stream it is a v1 dump minus the header, which SnapshotFormatV0OmitsStreamHeaders pins by dumping the same database both ways, and which Basic now exercises end to end by restoring a v0 dump of a slot-encoded source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8f3c86f to
e24b251
Compare
A loader takes a stream with or without a header, but a binary that predates the header aborts on one, so a node that always writes headers cuts off every consumer that has not upgraded yet. Give monad-cli --snapshot-format so a publisher can keep writing the pre-header layout (v0, the default) while its own loader still accepts both, and move to v1 once every consumer understands it.
A format is stamped verbatim into the header's version field, so the dump rejects one it does not know before writing anything: otherwise a stale enumerator would leave a complete, checksummed snapshot that no reader accepts, and the failure would surface at restore.
v0 writes the records alone: stream for stream it is a v1 dump minus the header, which SnapshotFormatV0OmitsStreamHeaders pins by dumping the same database both ways, and which Basic now exercises end to end by restoring a v0 dump of a slot-encoded source.