Skip to content

Record a storage schema version, and repair the history encoding - #3408

Closed
cbeck88 wants to merge 17 commits into
testnetfrom
fix/history-schema-repair
Closed

cbeck88 wants to merge 17 commits into
testnetfrom
fix/history-schema-repair

Conversation

@cbeck88

@cbeck88 cbeck88 commented Sep 4, 2026 •

Copy link
Copy Markdown
Collaborator

Draft, for 4.10.1. Supersedes #3404, and is independent of it — this branch is cut from testnet and contains none of that one's commits, so it merges on its own. It shares ideas with #3404 (byte-reversal collision handling, whole-group chunking, read-all-before-write-any batches) but the driver and the resume mechanism differ, and #3404's measurements, adversarial review and defect repros are why this one is shaped the way it is.

Why this exists

A partner running an archive node with --features history upgraded to v4.9.0 and hit a write-performance regression — roughly 500× write amplification (3.6–3.9 GB/s against ~7 MB/s healthy) and a compaction sawtooth that never drained. They diagnosed it themselves, reverted #3323, and have been running that patched build since.

We do not know how long they ran the unpatched build, nor what state their database is in now. They reverted quickly — the report says the signature reproduced within ~60 seconds on a fully-synced node — but "quickly" is not a number, and every entry written during that window is in a different on-disk encoding from everything around it. Nor do we know whether they ran --history before v4.7.5, which determines whether their keys carry a heights row and are therefore immune to the mixing described below.

That uncertainty is the reason for this work. The point is not only to repair a known-bad database, but to be able to tell whether a given database is repairable at all, and to say so before touching it.

The two changes that caused it

#3312 — "Fix history indexing performance" (a108e815) removed all three sites that wrote MappingUpdateHeightsMap, replacing the index with a prefix scan. Self-consistent at the time: every entry was little-endian and the scan decoded little-endian. Writes stopped rewriting an ever-growing vector, so this was a genuine improvement. Shipped in snarkOS v4.7.5 and v4.8.0.

#3323 — "Speed up history reads" (58a1359e) introduced big-endian height keys, using the presence of a heights row as the sentinel for "this key is legacy":

Shipped in snarkOS v4.8.1, v4.9.0 and v4.9.1.

That sentinel was already false for three weeks of production data. Keys written by v4.7.5/v4.8.0 have no heights row and are little-endian, so #3323's else branch treats them as big-endian. Two consequences:

  1. Reads return values from unrelated heights — silently, with a 200.
  2. The write amplification returns. For a key that still carries a row from ≤v4.7.4, a108e815 had frozen that row and made writes O(1). Speed up history reads #3323 sees the frozen row, concludes "legacy", and resumes the read-modify-write. That is what the partner measured.

Seeing it fail

#3404 carries a committed test that demonstrates the corruption directly — a read that is correct before a migration and wrong after it, because an unlisted little-endian entry is byte-identical to a migrated big-endian one:

test_migration_exposes_unlisted_entries_as_wrong_heights

seeded: heights 100 and 1000 listed; an unlisted LE entry at height 256
query at height 65,536 after migration -> Some(256u64)
correct answer                          -> Some(1000u64)

LE(256) and BE(65_536) are the same four bytes. That test is the shortest statement of why a migration driven by the heights row is not enough, and it is worth reading before the rest of this description.

Why the heights map cannot drive this

Three incompatible layouts shipped in three weeks, and nothing on disk records which one wrote a given entry:

snarkOS height encoding MappingUpdateHeightsMap
≤ v4.7.4 little-endian written, lists every entry
v4.7.5, v4.8.0 little-endian not written
v4.8.1+ big-endian for keys with no row; little-endian for keys with one frozen, or appended again

The middle row is the problem. Those entries have no heights row, so a heights-driven migration cannot see them — and on a node that reverted the v4.8.1 change they are most of its history. This migration reads MappingUpdateMap itself; the heights map is only ever deleted, never trusted. test_heights_map_is_ignored_and_dropped proves that against a row which omits a real height, invents two that never existed, and points at a key with no entries.

What is here

Errors are returned, never panicked: snarkOS wraps ledger loading in spawn_blocking!, which turns a panic into anyhow!("[tokio::spawn_blocking] {error}") — so a panicking migration reports "task N panicked" where the error chain should carry the remedy. We observed exactly that on a real ledger.

A storage schema version. A metadata map (last DataID variant, per that enum's append-only rule) records the version a database was last written under, plus migration resume state. RocksDB::open refuses a database from a newer build instead of misreading it, and otherwise applies migrations in order, advancing the version only once each completes. It runs whatever cargo features are set, since a migration works on raw keys under a prefix — a build that never opens the typed map can still repair it. It only protects forward from this release; older builds do not consult it.

Migration v0 → v1. A suffix byte-reversal on raw keys: HeightBytes is a [u8; 4] written bare by bincode as the final field, so the value bytes never move. The typed API would deserialize a Plaintext and a Value per entry only to re-serialize both identically, at ~4.1 KB peak per entry — unaffordable where a few hundred staking keys accrue one entry each per block forever.

Both legacy paths removed. The write path is the half that matters: it read the heights row, appended, and rewrote it on every update. No rows survive the migration, so removing it is what actually ends the amplification.

Measurements — and a correction

An xhigh review found that the sweep below measures the wrong axis, and I have confirmed it. Peak memory is O(entries in the largest single key), not flat: scan_heights's vector, its bincode copy, the lookup HashSet and the colliding/isolated partition each hold one element per entry of the key being migrated. The sweep scaled key count while holding depth at <= 4,000, so it never varied the dimension that drives memory.

At a constant ~900k entries, varying only the shape:

shape entries peak growth
447 keys x 2,000 894,000 0.076 GiB
20 keys x 45,000 900,000 0.110 GiB
5 keys x 180,000 900,000 0.118 GiB

So a real 6.5M-height staking key is on the order of low single-digit GB, not the ~0.1 GiB originally claimed here. Still far below the ~10.9 TiB the typed approach would need, and still workable, but the earlier figure was wrong and the benchmark needs a depth axis before any number here should be trusted.

Throughput, which the sweep does measure correctly: 300-550k entries/s.

Two things a reviewer should attack

Resume semantics. An xhigh review found six defects here, all sharing one cause: progress was recorded in a write separate from the data it described. write_chunk now puts the cursor in the same WriteBatch as its deletes and inserts, which required the work order to be deterministic in the recorded heights alone so a resumed run rebuilds the identical batch list and skips committed batches whole. Three metadata orderings were inverted for the same reason. Every substantive defect found in this work so far has been in the interruption path, and most were found by someone other than me. Specifically here: a key's original heights are recorded to metadata before anything moves, because once partly migrated a key's entries no longer say what they were — a moved entry sits at BE(h) == LE(byte_reverse(h)) and reads back as a plausible height of its own. And migrate writes a completion sentinel before returning, since the runner advances the version afterwards and a crash in between would otherwise re-run a finished migration over big-endian data.

Classifying a mixed database. An earlier revision refused any database containing big-endian entries. Running it against a real three-format ledger showed that was wrong: it panicked on an entry that byte-reversed to height 53,442, and the reason is structural. A key first written during the v4.7.5/v4.8.0 window has no heights row — that is what defines the window — so when a v4.8.1+ build took over it took the big-endian branch for exactly those keys. Single keys hold both encodings, and that is the expected shape of any node that ran v4.7.5/v4.8.0 and then upgraded, so refusing the mixture would refuse the databases this exists to fix.

Entries are classified per key against the chain tip, read from the committee store — CurrentRound then RoundToHeight, both in this same database and reachable by the raw prefix access the migration already uses. A reading above the tip cannot be a height, which settles almost every entry.

Where both readings are plausible:

  • a palindromic suffix reads as the same height either way, so there is nothing to decide;
  • a key with no big-endian entry was never touched by a v4.8.1+ build, so everything under it is little-endian;
  • a key with no little-endian entry was only ever written big-endian;
  • a key showing neither — a mapping written exactly once, at an ambiguous height — is judged against the ledger as a whole, which usually excludes one encoding;
  • only a ledger genuinely holding both, on a key that shows neither, leaves the entry undecidable, and it is reported rather than guessed at.

That fourth case was found by review, and it mattered: without it such an entry was assumed little-endian, so a correct big-endian entry was deleted and rewritten at a height it never had — on ledgers that had nothing to migrate, while --check reported them as safe.

There is deliberately no boundary heuristic. An earlier revision placed such entries by which side of the encoding changeover they fell on; it produced two classification defects and earns very little once the bound is a real tip rather than a constant, so it was removed. (Thanks to @snarkworld for catching that this description still described it.)

What the residual case costs

Measured on real ledgers built from stock binaries:

ledger tip undecidable
genesis-synced, three formats 76,517 6
20.8M rung, v4.8.0 → v4.9.1 20,847,646 2,887

The count is approximately mixed keys × little-endian window ÷ 128, so it grows linearly with how long an operator ran v4.8.0 before upgrading. The second figure came from a five-hour window; a month-long one would be far larger.

Deciding these needs evidence the storage layer does not have — whether a given block actually updated a given mapping key, which is recorded in that block's finalize operations. A phase to do that was written and removed as unjustified complexity; these measurements are the argument for building it, and the decision is open.

Operator impact

Worth carrying into release notes; there is no CHANGELOG in this repo.

A node will not start on a ledger whose history has not been migrated. It reports the schema version it found, the one it needs, and the command to run. This is the intended behaviour: the alternative is the node reading its own history incorrectly, silently, which is the fault being fixed.

This applies regardless of whether the build has the history feature. The gate is on the data, not on the build. A build that never reads history could stamp the schema version without migrating and would never notice — but that stamp is what a later history-enabled build consults, and it would then skip the migration and misread every entry. So a node carrying unmigrated history must migrate it even if it has no interest in it.

The operator most likely to be caught out is one who ran --history at some point, stopped, and now runs a build without it. If that population turns out to be real rather than hypothetical, a --drop-history mode that discards the maps and stamps the version would be the humane answer; it is not built, because it destroys data.

Ledgers with no history are unaffected — one seek finds nothing, the version is stamped in passing, and the node starts normally. A node that never enabled the feature will not notice this change.

Migration is not automatic. It can rewrite billions of entries and take hours, so it is a deliberate operator action:

snarkvm-migrate-db --check <ledger-dir>   # read-only; can this be migrated, and how long?
snarkvm-migrate-db <ledger-dir>           # do it, with the node stopped

--check is safe against a copy of a production ledger. The migration is resumable: interrupt it and run it again.

Verification

cargo test -p snarkvm-ledger-store --features history,rocks — 128 passed, 0 failed, 6 ignored.

Not yet run against a database produced by real binaries. A three-format ledger built from stock v4.7.4 → v4.8.0 → v4.9.1 exists for exactly that and is the next step.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p

cbeck88 and others added 2 commits September 4, 2026 15:11
…ry encoding

Nothing on disk has ever recorded which schema wrote a ledger. Three
incompatible layouts of the historical mapping updates shipped within three
weeks and can only be told apart by inspecting byte patterns, which is why a
node can silently misread its own history. This adds the missing record and the
first migration that uses it.

Storage version
---------------
A new metadata map (the last DataID variant, per that enum's append-only rule)
holds the schema version the database was last written under, plus the resume
state of an in-progress migration. RocksDB::open now refuses a database written
by a newer build rather than misreading it, and otherwise applies migrations in
order, advancing the version only once each finishes.

This runs whatever cargo features are enabled: a migration works on raw keys
under a map's prefix, so a build that never opens the typed map can still repair
it, and where a map is absent its migration finishes immediately. It only
protects forward from this release, since older builds do not consult the record.

Migration v0 -> v1: historical mapping updates
----------------------------------------------
Rewrites every historical mapping update to a big-endian height key.

Driven by MappingUpdateMap itself rather than by MappingUpdateHeightsMap. The
heights map is never read, only deleted -- which matters because snarkOS v4.7.5
and v4.8.0 wrote little-endian entries while writing no heights rows at all. A
heights-driven migration is blind to those, and on a node that reverted the
v4.8.1 change they are most of its history.

The work is done on raw keys. A MappingUpdateMap key ends in HeightBytes, a
[u8; 4] that bincode writes bare, so re-encoding a height is a suffix
byte-reversal that leaves the value untouched. Going through the typed API would
deserialize a Plaintext and a Value per entry only to re-serialize both to
identical bytes, at ~4 KiB of peak memory per entry -- unaffordable on an archive
node, where a few hundred staking keys accrue one entry each per block forever.
Measured at 447 keys: ~300-550k entries/s with peak memory flat at 18-74 MB
across a 16x range in history depth.

Two properties the tests pin down:

- Byte-reversal partners (BE(h) == LE(byte_reverse(h))) are migrated together in
  one batch, reading every value before writing any, so neither overwrites the
  other. Palindromic heights are their own partner.
- A key's original heights are recorded to metadata before anything moves.
  Once partly migrated a key's entries no longer say what they were, since a
  moved entry reads back as a plausible height of its own, so an interrupted run
  must not re-derive its work from the data.

A database containing big-endian entries cannot be separated from little-endian
ones by inspection, so rather than guess at a mixture this panics and asks for a
resync from genesis. Detection relies on a height being small while its
byte-reversal usually is not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
With the v0 -> v1 migration repairing the encoding before the store opens, only
one layout is ever present, and the dual paths can go.

The write path is the important half. For a key carrying a heights row, it read
that row, appended, and rewrote it on every single update -- O(n) per write and
O(n^2) cumulative. A partner running an archive node measured roughly 500x write
amplification from this (3.6-3.9 GB/s against ~7 MB/s on a healthy node) and a
sawtooth of pending compaction that never drained. Since no heights rows survive
the migration, removing the path is what actually ends that.

Also adds a test that encoded mapping-key bodies are prefix-free, which the
migration relies on to group a key's entries: were one body a prefix of another,
the first key's entries would sort on either side of the second's and its run
would be split. It holds because a Plaintext is length-delimited; this checks it
against the encoder rather than trusting it.

test_get_historical_mapping_value_legacy is deleted rather than adapted, since
it asserts the read path being removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
cbeck88 and others added 3 commits September 4, 2026 15:50
… with its data

Two changes, both forced by evidence rather than review taste.

Handle the mixture instead of refusing it
-----------------------------------------
Running the previous commit against a real three-format ledger panicked on an
entry whose little-endian reading was 3,268,411,392 -- byte-reversed, height
53,442, written big-endian.

The premise that a key carrying a heights row keeps receiving little-endian
writes holds only for keys that already had a row. A key first written during
the v4.7.5/v4.8.0 window has no row -- that is the defining property of that
window -- so when a v4.8.1+ build took over it took the big-endian branch for
exactly those keys. Single keys therefore hold both encodings, and this is the
expected shape of any node that ran v4.7.5/v4.8.0 and then upgraded, not an edge
case. Refusing it would refuse the databases this repair exists to fix.

Entries are now classified per key. Where only one reading is a plausible
height, the answer is certain. Where both are, three cases: a key with no
big-endian entry was never touched by a v4.8.1+ build, so everything under it is
little-endian; a key with no little-endian entry was only ever written
big-endian; and a key holding both is decided by the boundary between them,
since every little-endian entry predates every big-endian one. Only an entry
that fits both sides is reported, which needs all three of an unambiguous entry
in each encoding and an ambiguous one straddling them.

The cursor commits with the data it describes
---------------------------------------------
An xhigh review found six defects, all in the interruption path, and they share
one cause: progress was recorded in a write separate from the data it described.

write_chunk now puts the cursor in the same WriteBatch as its deletes and
inserts. That required the work order to be deterministic in the recorded
heights alone, so a resumed run rebuilds the identical batch list and skips
committed batches whole -- rather than reconciling a count against a list
processed in a different order, which silently skipped one height and bailed on
another. It also removes the replay that swapped byte-reversal partners back,
the same defect a previous review had already found once in a different form.

Three metadata orderings are inverted for the same reason: a key's heights and
the cursor naming that key are written together before any of its data moves;
the completion sentinel and the disposal of the working state commit together;
and the schema version and the finished migration's cursor commit together. Each
of those, split, left a database that either bricked on the next start or
restarted a completed migration over converted data.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
…s remaining gaps

Four items from an xhigh review, none affecting the migration's behaviour.

The typed MappingUpdateHeightsMap is removed from the trait, from both backends,
and from all eight atomic-lifecycle calls. Nothing had read or written it since
the legacy paths came out, yet it was still opened at startup and carried
through every atomic scope, doing bookkeeping on a keyspace the migration
empties. Three doc blocks that still described it as a "legacy sentinel"
directing reads down an O(n) path are rewritten. The DataID variant stays, since
that enum can never drop one, and the migration still needs the map id to
compute the prefix it deletes.

Errors are returned rather than asserted. snarkOS wraps ledger loading in
spawn_blocking!, which converts a panic into anyhow!("[tokio::spawn_blocking]
{error}") -- so a panicking migration reports "task N panicked" where the error
chain should carry the remedy, and the operator's instruction to resync reaches
only stderr. Observed exactly that on a real ledger run.

Progress is reported per batch rather than per key. On the shape this exists for
-- a few hundred keys with millions of entries each -- one key is hundreds of
batches, so reporting between keys goes silent for precisely as long as the
interval is meant to prevent.

The test harness now opens RocksDB with production's options, including the
fixed 4-byte prefix extractor. With one installed, raw_iterator runs in
prefix-seek mode, where iteration beyond the seeked key's prefix is not
guaranteed; the migration seeks to longer keys, walks off the end of a map by
design, and range-deletes across a prefix boundary. All of that was previously
validated only against default options.

The benchmark's documentation now states that peak memory is O(entries in the
largest single key) and that BENCH_HEIGHTS is the axis to vary. An earlier sweep
scaled key count at fixed depth, which holds the only dimension that drives
memory constant and reports a flatness that is an artifact of the sweep.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
An entry can read as a plausible height under either encoding, on a key that
holds both. Storage cannot choose between them: the two candidate blocks are
equally consistent with the bytes on disk.

Rather than guess, the migration records every such entry -- its key body, the
four height bytes, and both candidate heights -- and then fails, reporting the
count and the first entry's two readings. An operator learns precisely which
entries are at issue instead of being told to resync a multi-terabyte archive
on faith, and the same list is what a later phase would read back.

Deciding these needs evidence the storage layer does not have: whether a given
block actually updated a given mapping key is recorded in that block's finalize
operations, which the ledger can answer and this crate cannot. A phase to do
that was written and removed again. It would have meant a trait implemented by
the ledger, a second entry point running after Ledger::load, and version
tracking spanning both phases -- real complexity for a case not yet observed in
any real database. The undecidable case needs a key first written in the
v4.7.5/v4.8.0 window, updated again under v4.8.1+, holding an entry at one of
the byte patterns that is plausible either way, with both readings straddling
the changeover. That intersection may well be empty in practice.

The count a failing migration reports is the evidence that would justify
building it. The metadata slot the list is written to is kept and documented to
that effect, so a future reader knows why a write-only record exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
@cbeck88

cbeck88 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

The bar this is aiming at

Stating the goal explicitly, since it has shifted twice during this work and the code now reflects a deliberate choice rather than a default.

After this change, a history node either answers every query correctly and performantly, or the database is reported as unrepairable and the operator resyncs. There is no third state.

In particular there is no "mostly repaired, a few entries still misread". That is the state we are trying to end — it is what a node running v4.8.1+ is in today, silently, and quietly leaving a residue of it behind would be the same bug with a smaller blast radius. A wrong answer that looks like a right answer is worse than a refusal.

Within that bar, avoiding a resync is strongly preferred. An archive node resyncing mainnet from genesis is a multi-week operation, so "resync" is a real cost to be spent only when the alternative is silent wrongness.

So the shape is: repair what can be repaired from the database alone, and stop with a precise report when something cannot be.

What that means concretely

Almost everything is repairable. An entry's encoding is usually decidable from its bytes: a height is small, and the byte-reversal of a small number is usually large, so only one reading is a plausible height. Where both are plausible, the key itself usually settles it — a key with no big-endian entry was never touched by a v4.8.1+ build, and a key holding both has its two encodings in disjoint height ranges, because every little-endian entry predates every big-endian one.

What is left over is narrow: an entry at one of the byte patterns that is plausible either way, on a key holding both encodings, with its two readings straddling the changeover. For those, the bytes on disk are genuinely consistent with two different heights and no amount of storage-layer reasoning separates them.

The migration records each one — key body, height bytes, both candidate heights — and then fails, naming the count and the first entry's two readings. The operator learns exactly what is wrong instead of being told to resync a multi-terabyte ledger on faith.

Why it does not go further

These are decidable with evidence the storage layer does not have: whether a given block actually updated a given mapping key is recorded in that block's finalize operations. The ledger can answer that; this crate cannot.

A phase to do it was written and then removed. It needed a trait implemented by the ledger, a second entry point running after Ledger::load, and schema-version tracking spanning both phases. That is real complexity, and the case it serves has not been observed in any real database yet — it requires a key first written in the v4.7.5/v4.8.0 window, updated again under v4.8.1+, holding an entry at an ambiguous byte pattern, with both readings straddling the boundary.

The count a failing migration reports is exactly the evidence that would justify building it. The metadata slot the list is written to is kept and documented to that effect, so a future reader understands why a write-only record exists.

Status

A run against a real three-format ledger (stock v4.7.4 → v4.8.0 → v4.9.1, synced from genesis) is in flight to produce that count. It is a measurement rather than a pass/fail: whether the residue is one entry or thousands, and whether it is one byte pattern or many, determines whether the resolver phase is worth building or the approach needs rethinking.

Worth recording that the same ledger has already falsified two earlier designs on their first run — including one that refused any database containing big-endian entries, which turned out to be the expected shape rather than an edge case.

cbeck88 and others added 3 commits September 4, 2026 16:53
Answers the question an operator actually has -- "will my node come up, and how
long will it take?" -- without the only alternative currently available, which is
to attempt the repair and find out.

    snarkvm-history-check ~/.aleo/storage/ledger-0

    schema version   v0 -- repair needed
    mapping keys     412,908
    entries          2,847,205,445 (2,847,193,004 little-endian, 12,441 big-endian)
    undecidable      0

    This ledger can be repaired in place. Estimated 158.2 minutes.

The database is opened with RocksDB's read-only mode, so writing nothing is a
property of the API rather than a promise -- which matters when the thing being
inspected is a partner's production data. It is also opened with the same
prefix extractor the node uses, since that setting changes iteration behaviour
and inspecting under different options would not be inspecting the same
database.

`inspect` shares its classification with the migration, so the two cannot
drift: the answer given here is the one the repair itself would reach, not a
second implementation that might disagree.

Where entries cannot be attributed to a height the report says so plainly, names
how many, and lists the candidate height pairs -- so "you must resync" arrives
with its evidence attached rather than as an assertion.

This also makes iterating on the migration cheaper. The repair mutates as it
goes, so every trial run against a real ledger previously consumed a fresh
multi-terabyte restore; the count can now be had in minutes, repeatedly, from
the same copy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
…re writing

Three changes that belong together, prompted by a review finding that the
migration could turn its own "unrepairable" verdict into a silent success.

Migration moves out of the node
-------------------------------
Bringing a schema forward can rewrite billions of entries and take hours, which
is not something a node should do as a side effect of starting. An operator
wants to run it when they choose, against a copy first, watching it, able to
stop and resume it -- the arrangement every other system that migrates schemas
arrives at.

The node now does one point lookup and refuses a schema it does not understand,
naming the tool to run. A ledger with no history to migrate is stamped in
passing, so a node that never enabled the feature is never prompted for work
that does not exist. `snarkvm-migrate-db` performs the migration;
`snarkvm-history-check` reports what it would do, read-only.

That also settles a review finding that the migration ran inside
`RocksDB::open`, which every `open_map` call re-enters, and before the
network-ID guard. A point lookup repeating a few dozen times is free where range
deletes and cursor writes were not.

Everything is decided before anything is written
------------------------------------------------
The whole map is classified first. If any entry cannot be attributed to a
height, the migration stops having modified nothing at all -- so a ledger
reported as unrepairable stays exactly as it was, and no restart can convert
that verdict into a silent success. Previously the completion sentinel was
written before the refusal, and simply restarting the node would skip the check,
stamp the schema version, and delete the evidence.

The plausibility bound is derived, not assumed
----------------------------------------------
A reading above the chain tip cannot be a height, and that is what separates the
two encodings. The tip is not known to the storage layer, so it is derived:
exactly one of an entry's readings is its real height, so the tip is at least the
smaller of them, and the largest such value over every entry is a bound no real
height exceeds. Every entry therefore keeps a plausible reading, and a correct
ledger can never be condemned.

Judged against the previous fixed constant, an entry was ambiguous whenever its
height's low byte was small -- one in thirty-two, and 3,409 undecidable entries
on a real 76,514-block ledger. Judged against the derived tip, only entries whose
*both* readings are real heights are ambiguous; below a tip of 65,536 no such
entry exists at all.

This also retires the boundary heuristic that placed ambiguous entries by which
side of the encoding changeover they fell on. It was the source of two
classification defects and earns nothing once the bound is real.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
A suffix that reads as the same height under both encodings is not ambiguous.
[0, 1, 1, 0] is height 65,792 either way, and the big-endian key for 65,792 is
that same suffix -- so whichever build wrote it, the entry is already where it
belongs. Reporting these as undecidable condemned ledgers over entries that
require no decision at all.

On the real three-format ledger this was half of what remained after the tip
derivation: 12 undecidable entries, of which 6 were the palindrome 65,792 and 6
were genuine cross-pairs (256 little-endian against 65,536 big-endian). Below a
tip of 76,517 only four suffixes can read as a plausible height both ways, and
one of them is this palindrome; at mainnet's tip there are 331 of them, all
removed from the undecidable population for free.

They are counted as already migrated rather than moved, since a write that
produces the bytes already present is not worth issuing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
cbeck88 and others added 2 commits September 5, 2026 10:23
One migration binary with two modes rather than two binaries, matching how a
schema tool is usually shaped: ask what it would do, then let it do it.

    snarkvm-migrate-db --check <ledger-dir>    read-only; what would this do?
    snarkvm-migrate-db <ledger-dir>            do it

Both modes run the same plan, so the report and the migration cannot disagree
about whether a ledger is repairable or how much work it is. --check opens the
database read-only, so writing nothing is a property of the API rather than a
promise, which matters when the thing being inspected is a partner's production
data. Without it the node must be stopped, since RocksDB permits one writer.

A ledger that cannot be migrated is refused in both modes with the same message
and a non-zero exit, so a script cannot mistake "unrepairable" for "done".

Also: --help now exits zero, and the node's own message points at the single
tool.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
Left at RocksDB's default of unlimited, the tool opened every SST of the ledger
it was scanning and kept each one's index and filter blocks resident. On a
1.7 TiB ledger that exhausted the default 1024 file-descriptor limit outright --
the tool failed immediately with "Too many open files" against exactly the size
of ledger it exists to serve -- and where it did run, it reached 4.6 GB of
resident memory for work that needs a few hundred megabytes.

The node sets this bound to 8192 because it serves random reads. This tool makes
one sequential pass and gains almost nothing from keeping files open, so 256
keeps it comfortably inside the `ulimit -n` an operator's shell will have,
without needing them to raise it first.

Found by running against a real 229M-entry ledger; a synthetic fixture has too
few SST files to show either symptom.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p

@snarkworld snarkworld left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 2e7edf8. Two defects reproduced locally against this branch, plus clippy is failing; details inline.

Blocking

  1. The storage version is never stamped after a migration (ljedrz's comment above). Confirmed with a test: seed one entry, migrate(), read StorageVersion back → 0, has_history → true. The node refuses forever and rerunning the tool prints success while doing nothing (it returns early on the CURSOR_COMPLETE sentinel).
  2. observed_tip is a lower bound on the chain tip, not an upper bound on real heights, so the newest entries can be misclassified as already migrated. Confirmed with a test (inline on observed_tip).
  3. cargo clippy -p snarkvm-ledger-store --features rocks --all-targets -- -D warnings fails: dead MetadataKey::StorageMigrationHandoff, dead delete_metadata, dead Encoding, and enum_variant_names on MetadataKey.

Should be addressed or acknowledged
4. The PR description says a mixed key is resolved "by the boundary between" its LE and BE entries. classify_entries has no boundary logic; every ambiguous entry on a mixed key is reported as undecidable (inline).
5. check_storage_version is not gated on the history feature, so a build that never reads the map is still forced to migrate before it can start. Fine if intentional, but it needs a release note.
6. The tool infers the network id from the directory suffix; dev ledgers are .ledger-{network}-{id} so it picks up the dev id (inline).
7. The map is scanned three times per run: plan() in the tool, plan() again inside migrate(), then classify_entries per key (inline).

Process
8. New binary plus a new tracing-subscriber dependency; per AGENTS.md both need explicit sign-off.

No consensus exposure found: storage only, and the appended DataID variant respects the append-only rule.

match (saw_little, saw_big) {
(_, false) => little.push(as_little),
(false, true) => big += 1,
(true, true) => undecidable.push(Undecidable { little: as_little, big: as_big }),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says a key holding both encodings is "placed by the boundary between them, since every little-endian entry predates every big-endian one" and only entries fitting both sides are reported. There is no boundary check here: any ambiguous entry on a key with saw_little && saw_big is undecidable. That is safer than described, but it refuses more ledgers than the description claims, and reviewers evaluating the residual-case argument are evaluating logic that is not in the code. Either implement the boundary test (LE reading valid iff as_little < min(certain BE), BE reading valid iff as_big > max(certain LE), decide when exactly one holds) or update the description.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, and it is the description that is wrong rather than the code. The boundary heuristic existed in an earlier revision, produced two classification defects, and was removed once the plausibility bound became real — at which point it earned nothing. The description was not updated with it.

I am correcting the PR body rather than implementing the boundary test. Your reading of the tradeoff matches mine: refusing more ledgers is the safer error, and with the tip now read rather than inferred the ambiguous population is small enough that the heuristic would be recovering very little at the cost of the logic that went wrong twice.

Thank you for catching that reviewers were being asked to evaluate an argument that was not in the code.

//! interrupted run reads the list back and continues from the index, never re-deriving what it has
//! already moved.

use super::{MapID, MetadataKey, PREFIX_LEN, ProgramMap, get_metadata, metadata_key};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: crate convention is std first, external crates second, crate-local third. This file has super first, then serde, then anyhow, then std.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1055c3e — imports reordered to std, external, crate-local.

}

// Nothing recorded, and nothing to record: stamp it and carry on.
if !history_migration::has_history(database, network_id)? {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is not gated on the history feature, so a build that never opens the typed map is still refused until the operator migrates data it does not read. The PR body presents that as a feature ("a build that never opens the typed map can still repair it"), which is fine, but it is a startup-blocking behavior change for non-history nodes with old history data on disk and should be called out in the release notes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional, and on reflection it is a correctness requirement rather than a convenience — so I have recorded the reasoning in the code (0858472) rather than leaving it to be inferred.

The gate is on the data, not on which features the build was compiled with. A build that never reads history could stamp the version without migrating and would never notice the difference. But that stamp is exactly what a later history-enabled build consults, and it would then skip the migration and read little-endian entries as big-endian — silent wrongness, which is the failure this whole change exists to end. Blocking a non-history node that carries unmigrated history is the price of the version meaning what it says.

You are right that it needs calling out. There is no CHANGELOG in this repo, so I have added an Operator impact section to the PR description; tell me if release notes are kept somewhere I have not found and I will move it.

Worth naming the sharp edge for whoever writes the release notes: the operator most likely to be surprised is one who ran --history at some point, stopped, and now runs a build without it. They have no interest in the history data, yet must migrate it before the node will start. If that turns out to be a real population rather than a hypothetical one, the humane answer is probably a --drop-history mode on the tool that deletes the maps and stamps the version — it is a few lines, but it discards data, so I would not add it speculatively.

Comment thread ledger/store/src/bin/migrate_db.rs Resolved
Comment thread ledger/store/Cargo.toml
[dependencies.tracing]
workspace = true

[dependencies.tracing-subscriber]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New dependency (and a new [[bin]] above). Per AGENTS.md both need explicit approval; flagging so it is a conscious decision rather than an implicit one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, and we should discuss whether we want snarkvm-db-migrate to be a standalone binary (so that the operator explicitly runs migration) or something that runs when the node starts up unconditionally.

usually in webapps you want to have a human watching when migration happens, because if migration goes wrong you can get corrupted data base or data loss. for a blockchain it might not be important to do that because all this data is highly replicated anyways by design, and backed up on CDNs etc.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed off on both the [[bin]] and the tracing-subscriber dependency — thank you for flagging them rather than letting them pass implicitly.

The larger question they point at is genuinely open, and worth settling before this merges: should the migration be a standalone binary at all, or a snarkos ledger migrate subcommand?

The case for standalone, which is what is here now:

  • No node machinery — no config, keys, or network. It is a data tool operating on a directory.
  • The migration logic lives in snarkvm-ledger-store and works on raw prefixes, so a binary beside it has no dependencies a node would drag in.
  • Easy to run against a copy, which is how anyone sensible will first use it.

The case for a subcommand, which I now think is stronger than I judged when I built this:

  • Distribution. Operators receive snarkOS releases. They do not build snarkvm-ledger-store with --features rocks, and a binary that lives in this repo has no route to them without one being invented.
  • Discovery. snarkos --help is where someone looks after the node tells them to migrate. A tool named in an error message but absent from the binary they have is a poor handoff.
  • I had thought a compliance constraint favoured standalone, on the grounds that a partner told us they cannot run non-stable builds. Re-reading it, that constraint is about patched builds, not about snarkOS as such — a stable release carrying a ledger migrate subcommand is precisely what they can run. So that argument does not hold, and I should not have leaned on it.

What does not change either way: the work belongs in snarkvm-ledger-store where the storage layout is, and the node refuses to start on a schema it does not understand rather than migrating implicitly. The question is only which binary the entry point hangs off.

I have no strong preference and would rather defer to whoever owns the release and packaging story. If the answer is a subcommand, the change is small — the two entry points are thin wrappers over plan() and migrate().

@vicsn vicsn Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I think it makes sense for migrations not to be run automatically/unconditionally:

  • As Chris said, we may want to have a human in the loop in case of corruption
  • We don't want a quorum of validators to ever directly activate this and to stall consensus for the duration of the migration

As an exception, the design in #3329 is run automatically/unconditionally but it's a special case: because it only prunes data there is no interference with consensus. And anyhow we can decide later on the final architecture for that PR.

That leaves the question of snarkVM binary v.s. snarkOS migrate. One more advantage of the snarkOS migrate approach is that this could potentially also cover migrations of BFTPersistentStorage. If BFTPersistentStorage would be moved into snarkVM this benefit would be void, but that would be yet another huge work package.

So I'm in favour of a snarkOS migrate command - for which most logic can live in snarkVM, "close" to the db logic.

cbeck88 and others added 2 commits September 7, 2026 11:28
Both blocking findings from review, reproduced locally before fixing.

The schema version was never recorded
-------------------------------------
Moving the migration out of the node removed the runner that advanced
StorageVersion, and nothing replaced it. The migration did its work and recorded
nothing: the node refused to start forever, and rerunning the tool returned
early on the completion sentinel and printed success while doing nothing.

The version is now written in the same batch that disposes of the migration's
working state, and the sentinel is gone -- the version is the record, and it is
the same one the node checks. The cursor is deleted rather than left holding a
marker, so a later migration reading it cannot mistake this one's leavings for
its own progress. Covered by a test that asserts the record exists rather than
merely that a second run is harmless, which is what the previous test checked
and why this went unnoticed.

The tip was inferred, and the inference was unsound
---------------------------------------------------
`observed_tip` took the largest of each entry's smaller reading. That is a lower
bound on the chain tip, not an upper bound on real heights, though the doc
comment claimed the latter. An entry above it has its true reading rejected and
its byte-reversed reading accepted, so it is classified as already migrated and
left in place -- silently readable at a height it never had. Since the staking
keys are written every block, the entries at the tip would all be misplaced
together.

The tip was never out of reach: the committee store shares this database, so
CurrentRound and RoundToHeight give it directly through the same raw prefix
access the migration already uses. The inference and its constant are gone.

Worth recording how this survived: the test asserted the property that could be
proved -- that no entry is implausible under both readings -- while the doc
claimed the property that was needed. Both statements were about the bound, and
only one of them was true.

Also from review: clippy failures from code left dead by earlier restructuring;
dev-mode ledgers named `.ledger-{network}-{id}` had the dev id read as the
network, which quietly migrated nothing and reported success, so the id is now
parsed explicitly and range-checked; and the tool no longer scans the whole map
to print a status line for a ledger that is already migrated or resuming, nor
plans twice on a fresh run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
Crate convention, from review.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
@cbeck88
cbeck88 marked this pull request as ready for review September 7, 2026 17:33
cbeck88 and others added 3 commits September 7, 2026 11:36
The schema version is stamped in passing only when there is no history to
migrate, and that condition is deliberately about the data rather than about
which features the build was compiled with.

A build that does not read history could stamp the version without migrating and
never notice. But the stamp is what a later history-enabled build consults, and
it would then skip the migration and read little-endian entries as big-endian.
Blocking a non-history node that carries unmigrated history is the price of the
version meaning what it says.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
The check previously reported a count of undecidable entries. That says whether
a ledger is repairable, but not whether the residue is worth resolving, nor how
the ledger came to be in that state.

It now also reports the height span of each encoding, the number of keys holding
both, and how many keys the undecidable entries fall on.

The big-endian span is the useful one: it is the window during which a newer
build was running, readable from the data rather than from anyone's
recollection of what ran when. On a node that upgraded and then reverted, that
window is precisely what determines how much of its history is mixed -- and the
operator no longer has to remember it.

Mixed keys matter for the same reason: only a key holding both encodings can
produce an undecidable entry, so that count, not the total, is the population at
risk. And undecidable entries falling on two keys is a very different problem
from the same number spread across thousands.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
Pushed in b67adbc with clippy failing. My check echoed success unconditionally
rather than reporting the command's exit status -- the exact shape of mistake
the stress-testing notes warn about, where the shell reports the echo's status
and not the command's.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
continue;
}
match (saw_little, saw_big) {
(_, false) => little.push(as_little),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low: Ambiguous heights are silently assigned the wrong encoding

The absence of an unambiguous big-endian entry does not prove these ambiguous entries are little-endian: a key can contain an older unambiguous LE entry and a newer ambiguous BE entry. For example, with a tip above 19,005,440, a BE update at height 19,005,440 also reads as the plausible LE height 8,705; an attacker who submits that mapping update can cause migration to relocate it to height 8,705, after which historical views expose the value before it existed. Treat entries whose encoding cannot be established individually as undecidable, or require authoritative metadata before choosing an encoding.

@veria-ai

veria-ai Bot commented Sep 7, 2026

Copy link
Copy Markdown

PR overview

This pull request records a storage schema version and adds migration logic to repair historical data encoding in the RocksDB ledger store.

One security issue remains open in the history migration logic. Under specific chain-height conditions, an attacker-controlled mapping update with an ambiguous encoding could be migrated to an incorrect earlier height, causing historical views to expose the value before it existed. No issues have yet been addressed.

Open issues (1)

Fixed/addressed: 0 · PR risk: 5/10

Review found that `classify_entries` assumed little-endian for an entry with no
unambiguous evidence in either direction, which is the case for a mapping
written exactly once at a height whose two readings are both plausible.

Reproduced: a key written once by a v4.9.x build, big-endian at height 256.
`--check` reported `little=1 big=0 undecidable=0` -- the ledger looked entirely
safe -- and the migration then deleted the entry and rewrote it at height
65,536, a height it never had. The damage lands on ledgers that ran only v4.8.1+
and had nothing to migrate at all, and at mainnet tip roughly one suffix in 256
qualifies, so any single-update mapping key is exposed.

The key's own entries settle it where they can. Where the key offers nothing,
the ledger as a whole usually does: if no entry anywhere is unambiguously
big-endian then no v4.8.1+ build ever wrote here. Only a ledger genuinely
holding both, on a key that shows neither, is undecidable -- and that is
reported rather than guessed at.

Also from the same review:

- `--check` on a half-migrated ledger classified a partly-moved map and reported
  "must be resynced" about a ledger that would have finished correctly. It now
  recognises an interrupted migration and says to finish it.
- The tool's "already migrated" test compared the schema version against zero
  rather than the version this build requires, so it would have refused to run
  every future migration while the node refused to start.
- The "nothing to migrate" shortcut in `check_storage_version` asked a question
  specific to v0 -> v1. It now asks each outstanding migration whether it has
  work, so a future migration about some other map cannot be skipped on a ledger
  without history.
- The resume cursor counts entries committed, which only names a batch boundary
  under the batching that produced it. The batching parameters are recorded
  alongside it and checked, rather than a changed CHUNK_SIZE silently replaying
  committed work and wedging the ledger permanently.
- The destination-occupancy check issued a point lookup per entry; it is now one
  batched read, which was the dominant read cost of a batch.
- `byte_reverse` was `u32::swap_bytes` written out longhand.
- A comment still justified an error arm as unreachable on the strength of the
  entry-derived tip, which was removed when the tip began being read.

Deferred, with the reason recorded in Cargo.toml: `tracing-subscriber` belongs
outside the library's dependency graph, but moving it behind a bin-scoped
feature would change the build command already in a partner's hands, and
dropping it would leave a multi-hour migration printing nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
@cbeck88

cbeck88 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Updated through 29021f266. A second xhigh review found a defect worse than anything previously identified here, so recording it plainly.

The one that mattered

classify_entries assumed little-endian for an entry with no unambiguous evidence in either direction — which is the case for a mapping written exactly once, at a height whose two readings are both plausible.

Reproduced before fixing: a key written once by a v4.9.x build, big-endian at height 256.

before   height 256 -> "correct"
--check  little=1  big=0  undecidable=0     <- ledger reported entirely safe
after    height 256 -> None,  height 65,536 -> "correct"

A correct entry, deleted and rewritten at a height it never had, with the check reporting no problem. The exposure is on ledgers that ran only v4.8.1+ and had nothing to migrate at all, and at mainnet tip roughly one suffix in 256 qualifies, so any single-update mapping key was at risk.

Fixed by falling back to ledger-wide evidence: a key's own entries settle it where they can, and where a key shows nothing, the ledger usually does — if no entry anywhere is unambiguously big-endian then no v4.8.1+ build ever wrote there. Two regression tests pin both directions.

Also fixed

  • --check on a half-migrated ledger classified a partly-moved map and reported "must be resynced" about a ledger that would have finished correctly.
  • The tool compared the schema version against zero rather than the version the build requires, so it would have refused to run every future migration.
  • The "nothing to migrate" shortcut asked a question specific to v0 → v1; it now asks each outstanding migration whether it has work.
  • The resume cursor's batching parameters are recorded and checked, rather than a changed CHUNK_SIZE silently replaying committed work and wedging the ledger.
  • The destination check is one batched read rather than a point lookup per entry.
  • byte_reverse was u32::swap_bytes longhand.

Deferred, deliberately

tracing-subscriber in the library's feature graph. The objection is right. The clean fix is a bin-scoped feature, but the build command for snarkvm-migrate-db is already in a partner's hands for them to run --check against their own ledger, and changing it now would strand them. Dropping the dependency instead would leave a multi-hour migration printing nothing. Restored with the reason recorded in Cargo.toml, to be revisited once they have reported back.

Packaging. Review notes that the node's refusal message names a binary that ships in no snarkOS artifact — an operator whose node will not start is told to run something they cannot obtain. That is a strong argument for a snarkos ledger migrate subcommand, but it is a decision for reviewers rather than something to change under a partner mid-run.

133 tests, clippy clean.

…at could lie

Both from migrating a real 234-million-entry ledger end to end.

The estimate was seven times optimistic. `ENTRIES_PER_SECOND = 300_000` came
from synthetic benchmarks on a freshly built database; the measured rate on a
real ledger is ~44,000/s, sustained. The tool advertised "Estimated 13.0
minutes" for a run that took ninety. An operator plans a maintenance window
around that number, so it is now taken from the real migration and rounded down
to 40,000.

The big-endian span no longer carries an inference. It said "a build writing
that format ran for roughly N blocks", and on a ledger written only by v4.8.0 --
which no big-endian writer had ever touched -- it reported a 2.3-million-block
window. Entries whose two readings are identical are counted as already
migrated, because moving them is a no-op, and they carry heights of their own,
which is enough to fabricate a span. The spans themselves are facts and are
kept; that reading of them was not, and it was load-bearing for operators
reconstructing their upgrade history.

For the record, from the same run: 234,244,900 entries across 52,539 keys in
5,341.8s, peak RSS 1.12 GB. A SIGKILL at 900s mid-write, then a resume, produced
a byte-identical bonded table to the uninterrupted run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
Comment on lines +60 to +63
/// Note this only protects forward from the release that introduced it: builds older than that do
/// not consult the record at all. Three incompatible historical-mapping layouts shipped within
/// three weeks with nothing on disk to distinguish them, which is the situation this exists to
/// prevent recurring.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 => history_migration::has_history(database, network_id),
other => bail!("No storage migration is defined for schema v{other}"),
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mild preference for the following setup:

  • call it fn requires_migration
  • currently there is no compile-time check whether we're including all possible versions and we have to manually remember to add entries here. We could:
    • replace const STORAGE_VERSION: u32 = 1 by an enum StorageVersions similar to what we do for enum ConsensusVersions.
    • match here on the enum, for the latest one we should always return Ok(false)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that seems reasonable

return Ok(());
}

// Nothing recorded, and nothing to record: stamp it and carry on.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah some of the comments are wierd and it's partly the code getting manipulated by tools that don't respect the comments

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly focused on correctness so far but let's get the comments right too. There was still a correctness bug yesterday I think

@cbeck88

cbeck88 commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Note that, I tested this extensively against actual mainnet chain data, running 4.7.5, then 4.8.0 then 4.9, then this, and it seems to sync properly and not cause damage even when there are incorrect endian entries. But that doesn't mean it doesn't have bugs.

Ultimately resyncing is going to ensure that you don't have corrupted data. I'm unsure how important it is to avoid that. But it seems like if it's easy to help our partner avoid that, it's a good idea. How we draw the line, like, at what point we give up and say they have to resync, is up for debate of course.

@cbeck88

cbeck88 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

these are the tests that got ran over the weekend:

  3. Real ledgers — where everything was actually learned

  ┌─────────────────────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
  │               run               │                                                                       outcome                                                                       │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ v4.8.1 node                     │ wrong version: writes BE, nothing to migrate                                                                                                        │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ v4.7.4 on a rung                │ RocksDB format_version wall — can't open a modern ledger                                                                                            │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ genesis v4.7.4 → v4.8.0 →       │ built the three-format ledger; measured the frozen bonded window — finding A proven on real data                                                    │
  │ v4.9.1                          │                                                                                                                                                     │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ migration #1 (ab9e5b8df)        │ completed, after-table byte-identical → proved the heights-map design strands the v4.8.0 window                                                     │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ migration #2 (66ba0d64b)        │ panicked → falsified "refuse any mixed database"; mixed is the expected shape                                                                       │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ migration #3 (cdc3cf9a9)        │ failed on a genuinely undecidable entry                                                                                                             │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ --check sweeps                  │ 3,409 → 12 → 6 undecidable as the constant became a derived then a real tip                                                                         │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ realistic-tip ledger (20.8M)    │ 2,887 undecidable; derived mixed keys × LE window ÷ 128; found the FD/memory bug that killed the tool on a 1.7 TiB ledger                           │
  ├─────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ final run (29021f266)           │ 234M entries / 52,539 keys in 90 min, 1.12 GB peak, exit 0. SIGKILL at 900s → resume → byte-identical bonded table. Node starts and serves          │
  │                                 │ correctly.                                                                                                                                          │
  └─────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

@vicsn

vicsn commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #3449

@vicsn vicsn closed this Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants