Skip to content

Migrate legacy history entries on raw bytes - #3404

Closed
cbeck88 wants to merge 4 commits into
testnetfrom
feat/history-migration-raw
Closed

cbeck88 wants to merge 4 commits into
testnetfrom
feat/history-migration-raw

Conversation

@cbeck88

@cbeck88 cbeck88 commented Sep 4, 2026 •

Copy link
Copy Markdown
Collaborator

Draft. An alternative to the migration in #3348, working on raw bytes rather than through the typed Map API. Opened for discussion alongside the findings write-up.

Why raw bytes

A MappingUpdateMap key is (ProgramID, Identifier, Plaintext, HeightBytes) behind a 4-byte [network_id, map_id] context. HeightBytes is [u8; 4], which bincode writes bare with no length prefix, and it is the last tuple field — so the height is the final four bytes of the raw key, and the migration is a suffix byte-reversal that leaves the value bytes untouched.

Going through the typed API instead deserializes a Plaintext and a Value<N> per entry and re-serializes both to identical bytes. Measured, that round-trip costs ~4.1 KB of peak RSS per entry, linear:

entries #3348 peak RSS this branch #3348 rate this branch
111,750 0.42 GiB 0.032 GiB 71k/s 454k/s
894,000 3.45 GiB 0.055 GiB 108k/s 339k/s
1,788,000 — 0.076 GiB — 253k/s

Flat rather than linear — 30–90 MB regardless of depth. Extrapolated to a ~7.7-month archive node (2.9B entries): ~3.2 h and ~0.1 GiB, against ~10.9 TiB for the typed approach. That matters because credits.aleo's bonded/delegated/committee are rewritten in full by replace_mapping on every block, so ~507 keys accrue one history entry each per block — and 507 < MIGRATION_BATCH_SIZE, so batching never splits them.

Other differences: a forward cursor (so a pass skips one tombstone rather than every tombstone left by previous passes), bounded chunks, and collision handling that preserves the property 38b02d3df established without needing a key's whole history in memory.

Review history

An adversarial review of the first commit found two defects, both since fixed in ab9e5b8df. Recording them because the second one shaped the design:

  1. Resume transposed byte-reversal pairs (critical). The colliding write batch and the heights-row deletion were separate writes, so an interruption between them left every entry moved with the row still listing it; the resumed run read each height back through its partner's destination and transposed the pair, silently, for ~109,100 entries on one staking key at today's tip.
  2. The colliding subset was unchunked. It is not "a few" entries — for a key written every block it is #{h <= H : byte_reverse(h) <= H}, growing as H^2/2^24: 109,100 at tip 21.6M, 1,048,578 by tip 67M, previously all held in memory at once.

Both are fixed by the same change: each colliding batch shrinks the heights row to the heights it has not yet moved, in the same atomic write, and batches pack whole partner groups. The only states a crash can leave are "batch not applied, row unchanged" and "batch applied, row already shortened", so a resumed run is never offered a height it has already migrated.

One caveat worth a reviewer's attention. That fix removes the window, not the ambiguity: a migrated colliding entry is byte-identical to its partner's un-migrated source, so a database placed in the old intermediate state by other means would still be transposed. I argue it is unreachable — WriteBatch is atomic, and the only code that could produce that state is the first commit on this branch, which was never released. The repro was updated to exercise the reachable state instead. If that reasoning is too thin, the alternative is a defensive check, and I could not find one that works from the data alone.

A related finding this PR does not fix

test_migration_exposes_unlisted_entries_as_wrong_heights is committed and #[ignore]d, because it documents a constraint on any heights-row-driven migration rather than a bug in this branch.

snarkOS v4.7.5 and v4.8.0 (snarkVM a108e8159..58a1359ea) wrote little-endian history entries while writing no heights rows. Those entries are invisible to a migration driven by the heights row, and they survive it — where an unlisted entry at height h is byte-identical to a migrated entry at byte_reverse(h), a floor seek finds it. A read that was correct before the migration returns a value from an unrelated height after it.

A fully correct migration has to enumerate what is actually under each key prefix and classify by encoding, rather than trusting the heights row as an inventory. That is tractable — heights are ≤ ~21.6M < 2^25, so an LE height read as BE almost always exceeds the chain tip — but it is a design change, not a patch.

The same gap causes #3323 to misread that data on v4.8.1+ today, independently of any migration. That is covered in the findings write-up rather than here.

Verification

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

adv_bincode_height_suffix_holds_for_every_plaintext is deliberately active: it checks the assumption the whole approach rests on, over 396 combinations covering every literal type, nested structs and arrays, and both encodings.

This branch also deletes test_get_historical_mapping_value_legacy, since it asserts the legacy read path that this change removes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p

cbeck88 and others added 4 commits September 3, 2026 17:40
Rewrites the pre-#3323 little-endian historical mapping updates to the
big-endian height schema, and removes the dual read/write paths so the
store is single-schema again.

The migration works on raw keys. A MappingUpdateMap key is
(ProgramID, Identifier, Plaintext, HeightBytes) behind a 4-byte
[network_id, map_id] context; HeightBytes is a [u8; 4], which bincode
writes as four raw bytes with no length prefix, and it is the last field
of the tuple. The height is therefore the final four bytes of the raw
key, so the migration is a suffix byte-reversal that leaves the value
bytes untouched. Going through the typed Map API instead would
deserialize a Plaintext and a Value<N> per entry only to re-serialize
both to identical bytes, at ~4 KiB of peak RSS per entry.

That cost is not affordable here. On mainnet the credits.aleo bonded,
delegated and committee mappings are rewritten in full by
replace_mapping on every block, and replace_mapping records a history
entry for every key it writes, so each of those ~507 keys accrues one
entry per block. A node with months of history holds billions of
entries concentrated in a few hundred keys, all of which fall inside a
single batch of 2048.

Three properties follow from that shape:

- The scan advances a forward cursor rather than re-seeking the head of
  the heights map, so a pass skips one tombstone instead of every
  tombstone left by all previous passes.
- A key's history streams in bounded chunks and is never materialized.
- Heights whose byte-reversal is also an update height for the same key
  are migrated together in one batch, reading everything before writing
  anything; BE(h') and LE(h) are the same four bytes exactly when
  h' == byte_reverse(h). Every other height collides with nothing and
  streams freely. Palindromes such as 65_792 are their own partner.

A key's heights row is deleted only once all of its entries have moved,
so an interrupted run repeats that key. An entry whose source is gone is
confirmed already-migrated by checking its destination exists; if
neither exists the migration fails rather than dropping history.

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

Four tests, three of which fail against the migration as it stands and are
marked #[ignore] with the defect they demonstrate. They are committed rather
than held back because each one is the cheapest available statement of a
problem that is otherwise easy to reason past:

- adv_resume_after_colliding_batch_swaps_values: the colliding write batch and
  the heights-row deletion are separate database writes, so an interruption
  between them leaves every entry moved and the row still present. On the
  resumed pass a pair (a, b) with b == byte_reverse(a) is read back through
  its own aliased destination and the two values are transposed. At today's
  mainnet tip this is ~109,100 entries for a single staking key, silent.

- adv_colliding_subset_is_unbounded: the colliding subset is the one path not
  passed through CHUNK_SIZE. It is not "a few" entries as the comment claims;
  for a key written on every block it is #{h <= H : byte_reverse(h) <= H},
  which grows as H^2/2^24 -- 109,100 at tip 21.6M, 1,048,578 at tip 67M.

- test_migration_exposes_unlisted_entries_as_wrong_heights: snarkOS v4.7.5 and
  v4.8.0 wrote little-endian entries while writing no heights rows, so a
  heights-driven migration cannot see them. They survive it, and an unlisted
  entry at height h is byte-identical to a migrated entry at byte_reverse(h).
  A read that was correct before the migration returns a value from an
  unrelated height after it.

- adv_bincode_height_suffix_holds_for_every_plaintext passes, and is kept
  active. It checks the assumption the whole approach rests on -- that the
  height is the final four bytes of the raw key -- over 396 combinations
  covering every literal type, nested structs and arrays, and both encodings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhrVwfiKhGgcRQXUTZfC5p
test_get_historical_mapping_value_legacy asserted that keys with a heights row
are served by binary search over that row. This branch removes that read path
deliberately -- after migration there are no heights rows, and every key is
served by a big-endian floor seek -- so the test can no longer pass.

Deleting it rather than adapting it: there is nothing left for it to assert
that the migration tests do not already cover, and leaving it failing would
have hidden the next real regression. It failed silently until now because the
migration work was verified with a filtered test command that excluded it.

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

Closes the two defects the adversarial review found in the previous commit.

A migrated colliding entry is byte-identical to its partner's un-migrated
source, so the data alone cannot say whether a pair has already moved. The
previous code left that ambiguity observable: the colliding batch and the
heights-row deletion were separate writes, so an interruption between them left
every entry moved with the row still listing it. The resumed run then read each
height back through its partner's destination and transposed the pair --
silently, and for ~109,100 entries on a single staking key at today's tip.

Each colliding batch now shrinks the heights row to the heights it has not yet
moved, in the same atomic write. The only states a crash can leave are "batch
not applied, row unchanged" and "batch applied, row already shortened", so a
resumed run is never offered a height it has already migrated and the aliasing
is never observable.

That also removes the reason the colliding subset had to be migrated in one
batch, which was the second defect: it is not "a few" entries. For a key
written on every block it is #{h <= H : byte_reverse(h) <= H}, growing as
H^2/2^24 -- 109,100 at tip 21.6M and 1,048,578 by tip 67M, previously all held
in memory at once. Batches now pack whole partner groups up to CHUNK_SIZE.
Keeping partners together is what makes the surviving row hold only whole
pairs, so the isolated/colliding split stays stable across a resume.

The two repros are updated accordingly and un-ignored. Note that
adv_resume_after_colliding_batch_swaps_values now reproduces the *reachable*
interrupted state (pair moved, row already shortened) rather than the state it
was written against (pair moved, row intact). The latter is no longer reachable
from this code, but it remains genuinely ambiguous if manufactured: a database
left in it by the previous commit's logic would still be transposed. That
commit was never released, so no such database exists outside this branch.

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

cbeck88 commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

closing in favor of #3408

@cbeck88 cbeck88 closed this Sep 5, 2026
14MR pushed a commit to 14MR/snarkVM that referenced this pull request Oct 5, 2026
`cargo audit -D warnings` fails on staging, and has done since these two
releases were yanked from crates.io. Neither is a security advisory and neither
needs a source change; the lockfile simply pins versions that no longer exist as
far as the registry is concerned. Because a yank is a registry-side event, this
started failing without any commit to this repository, and it fails every open
PR rather than any particular one.

    Crate: chacha20  Version: 0.10.1  Warning: yanked
    Crate: wnaf      Version: 0.14.0  Warning: yanked
    error: 2 denied warnings found!

Both have semver-compatible successors, so `cargo update -p chacha20 -p wnaf`
is the whole change:

- chacha20 0.10.1 -> 0.10.2, reached through `rand`
- wnaf 0.14.0 -> 0.14.1, reached through `k256` and `snarkvm-console-algorithms`

wnaf 0.14.1 declares `primefield`, which the lockfile already carries, so no
package is added to the graph and no manifest changes.

chacha20 0.10.2 is the same version already used by snarkOS and by snarkVM's
testnet branch; this brings staging in line with them. wnaf has not been updated
on any branch yet, and is on its own why testnet and aleo-labs-protocol#3404 still fail.

After this, `cargo audit -D warnings` exits 0 and
`cargo check --workspace --all-targets --all-features` still passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qb8dzi6WAXBSCeyEkpBvrc
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.

1 participant