Skip to content

feat: remove peaks from blocks table#33

Open
TomasArrachea wants to merge 12 commits into
nextfrom
tomasarrachea-mmr-peaks-store
Open

feat: remove peaks from blocks table#33
TomasArrachea wants to merge 12 commits into
nextfrom
tomasarrachea-mmr-peaks-store

Conversation

@TomasArrachea

@TomasArrachea TomasArrachea commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Adapts the web-client to the changes introduced in the rust client 0xMiden/miden-client#2100

@TomasArrachea TomasArrachea force-pushed the tomasarrachea-mmr-peaks-store branch from dcf092c to 651e1d7 Compare April 28, 2026 19:48
@TomasArrachea TomasArrachea force-pushed the tomasarrachea-mmr-peaks-store branch from 651e1d7 to 895014e Compare April 28, 2026 19:55
[Table.OutputNotes]: indexes("noteId", "recipientDigest", "stateDiscriminant", "nullifier"),
[Table.NotesScripts]: indexes("scriptRoot"),
[Table.StateSync]: indexes("id"),
[Table.BlockchainCheckpoint]: indexes("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 is going to lose sync state for every existing browser user 😬

The stateSyncblockchainCheckpoint rename was applied directly inside V1_STORES (this line), but the Dexie version integer is still 1 (line 164 — only this.dexie.version(1).stores(V1_STORES);). With no version(2).stores(...).upgrade(tx => ...) block, Dexie will silently mutate the schema diff for existing DBs and the old stateSync table content is gone. The on('populate') hook only fires on first DB creation, so upgraded users get an empty blockchainCheckpoint and getSyncHeight returns null — the Rust side then almost certainly treats that as a fresh client and re-syncs from genesis, dropping every cached MMR node.

The ensureClientVersion nuke only triggers on a major.minor change (the same-major-minor check in schema.ts:577-583), so a 0.15.0-alpha → 0.15.0 release bypasses it. So patch-level bumps are the silent-data-loss case.

The file's own docs explicitly forbid this pattern — lines 130, 142, 162 of this same file say things like "Never modify a previous version block." 🙃

Fix: keep V1_STORES as it was, then add:

this.dexie.version(2).stores({
  // remove peaks from blockHeaders, switch the store name
  blockHeaders: indexes("blockNum", "hasClientNotes"),
  blockchainCheckpoint: indexes("id"),
  stateSync: null, // drop the old table
}).upgrade(async tx => {
  const old = await tx.table("stateSync").get(1);
  const oldHeader = old ? await tx.table("blockHeaders").get(old.blockNum) : null;
  await tx.table("blockchainCheckpoint").put({
    id: 1,
    blockNum: old?.blockNum ?? 0,
    partialBlockchainPeaks: oldHeader?.partialBlockchainPeaks ?? new Uint8Array(),
  });
});

And add a schema.test.ts case that seeds a v1 DB and asserts the upgrade carries the peak forward.

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.

This change is not meant to be released in a patch version, rather on the next v0.15 release. So is it not okay to expect db to be nuked?

}
}

export async function getCurrentBlockchainPeaks(dbId: string) {

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.

Quick heads up — this new getCurrentBlockchainPeaks function has zero test references (I grepped all *.test.ts files under crates/idxdb-store/src/ts/). The coverage gate for this crate is 95% lines/branches/functions/statements (crates/idxdb-store/src/vitest.config.ts:thresholds), so CI will fail until this is covered.

Three tests cover all the branches cheaply: empty DB (no record → {blockNum: 0, peaks: ""}), populated record (round-trips the bytes), and error path (logWebStoreError invoked).

});

it("returns the persisted blockNum after updating stateSync", async () => {
it("returns the persisted blockNum after updating blockchainCheckpoint", async () => {

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.

While you're updating fixture names here — line 72 of this file (minimalStateUpdate) still uses the old field name flattenedPartialBlockChainPeaks: emptyFlattenedVec(). The prod code was renamed to newPeaks: Uint8Array (see sync.ts:179, 199, 314, 334, 350).

Because vitest doesn't typecheck test files (they're typically excluded from tsconfig.json:include), the destructure in applyStateSync silently gets newPeaks: undefined in every test, and since no test asserts on the persisted peaks after the write, the entire peaks write path is being exercised with undefined bytes and passing. That's why all 43 sync tests stay green — they're not actually proving the write.

Fix: rename the field at lines 72, 416, 439, 462, 474 and add one assertion in an existing test that reads blockchainCheckpoint.get(1) after applyStateSync and verifies the peaks bytes round-trip.

}
).blockchainCheckpoint.update(1, {
blockNum: blockNum,
partialBlockchainPeaks: newPeaks,

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.

Two small things on this updateSyncHeight block:

  1. The peaks update is gated on current.blockNum < blockNum (line 343) — meaning a backward-going sync silently drops the new peaks. That's almost certainly the right behavior (you don't want older peaks overwriting newer ones), but now that peaks and blockNum are coupled in one row, it deserves a one-line comment so a future reader doesn't read "peaks write is conditional on height comparison" as a bug: // Peaks travel with blockNum: skipping the height update also skips the peaks update, by design.

  2. The whole body is wrapped in try { ... } catch (error) { logWebStoreError(...) }. If logWebStoreError ever doesn't re-throw, this peaks/height write silently fails while the rest of the rw transaction commits — same broken-state class as the missing migration. Worth either confirming logWebStoreError always re-throws (and adding a comment that says so) or removing the try/catch so Dexie aborts the whole transaction on failure.

if (!record) {
return {
blockNum: 0,
peaks: uint8ArrayToBase64(new Uint8Array()),

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.

When the record exists but record.partialBlockchainPeaks is empty (the populate default for a fresh DB), this returns {blockNum: 0, peaks: ""} — but record.blockNum may actually be non-zero on disk (e.g. after a future migration that initializes peaks lazily). Fine for the Rust side today since empty peaks → empty forest, but you're throwing away info.

Cheaper to be honest about what's on disk:

if (!record || record.partialBlockchainPeaks.length === 0) {
  return { blockNum: record?.blockNum ?? 0, peaks: uint8ArrayToBase64(new Uint8Array()) };
}

Comment thread crates/idxdb-store/src/sync/mod.rs Outdated
}

let mmr_peaks_nodes: Vec<Word> = Vec::<Word>::read_from_bytes(&peaks_idxdb.peaks)?;
MmrPeaks::new(Forest::new(peaks_idxdb.block_num as usize), mmr_peaks_nodes)

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.

Lossy-looking cast — block_num is u32, cast to usize here. Safe on 32/64-bit wasm targets today, but the sqlite-store equivalent uses usize::try_from(forest).expect("u64 should fit in usize") defensively. Copying that idiom keeps the invariant explicit and survives a future widening of block_num:

MmrPeaks::new(
    Forest::new(usize::try_from(peaks_idxdb.block_num).expect("u32 fits in usize")),
    mmr_peaks_nodes,
)

Comment thread crates/idxdb-store/src/js/schema.js Outdated
* older DBs. Set to `0.15.0-alpha.5`: the release that moved MMR peaks out of
* the `blockHeaders` table into `blockchainCheckpoint` (renamed from `stateSync`).
*/
export const MIN_COMPATIBLE_CLIENT_VERSION = "0.15.0-alpha.5";

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.

I think we can drop the alpha now that we have a proper release.

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