Recover shielded cache and cancel stale synchronization - #109
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are verified runtime-breaking bugs in the updated sync/store code paths (undefined controller references in sync cancellation handling, and a possible null object-spread in putNotesMap() when writing sync meta).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens shielded synchronization by anchoring cached scan state to a verifiable chain hash, rebuilding invalid/legacy caches on reorgs, and preventing stale async sync work from writing status/cache after lock/profile/network changes (issues #106–#108).
Changes:
- Anchors shielded cache metadata to a
(blockHeight, blockHash)and rebuilds caches from genesis when the anchor is invalid or unverifiable. - Commits discovered notes and their cursor/anchor atomically in IndexedDB, and propagates a shared AbortController through preflight/reconciliation/streaming to cancel stale work.
- Adds/updates tests covering reconciliation failure surfacing, rollback/reorg recovery, atomic cache commits, and cancellation semantics.
File summaries
| File | Description |
|---|---|
| src/shared/walletEngine.js | Adds chain-anchored cache validation, shared abort/cancellation, and revised sync/reconciliation flow. |
| src/shared/walletEngine.test.js | Expands test coverage for rollback, reconciliation failures, and cancellation/epoch-guard behavior. |
| src/shared/shieldedStore.js | Adds abort-signal guards, atomic clear operations, and transactional note+meta commits. |
| src/shared/shieldedStore.test.js | Adds tests for transactional note+meta commits and aborted-write behavior. |
| src/build.worker-config.test.js | Updates expectation to match the refactored createAddressSyncer(network) usage. |
| CHANGELOG.md | Documents the fixes for #106–#108. |
Review details
Suppressed comments (2)
src/shared/walletEngine.js:2326
controller.abort()is referenced inside the ReadableStream stale-epoch handling, but the localcontrollerwas removed when switching to the sharedsignal. This will throw aReferenceErrorwheneverisStale()becomes true.
This issue also appears on line 2366 of the same file.
if (isStale()) {
try {
controller.abort();
} catch {}
try {
src/shared/walletEngine.js:2369
- In the async-iterator scan path, the stale-epoch branch still calls
controller.abort(), but the localcontrollerwas removed in this refactor. That will throw aReferenceErrorwhenisStale()becomes true.
}
await processChunk(value);
if (shouldStop) break;
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!entries.length && !syncMeta) return 0; | ||
| const previousMeta = syncMeta ? await getShieldedMeta(networkKey, walletId, profileIndex) : null; | ||
| signal?.throwIfAborted(); | ||
|
|
||
| // Discovered notes must never be committed without their matching chain anchor. | ||
| await new Promise((resolve, reject) => { | ||
| const tx = db.transaction([STORE_NOTES], "readwrite"); | ||
| const tx = db.transaction(syncMeta ? [STORE_NOTES, STORE_META] : [STORE_NOTES], "readwrite"); | ||
| tx.oncomplete = () => resolve(true); | ||
| tx.onerror = () => reject(tx.error || new Error("Failed to write notes")); | ||
|
|
||
| if (syncMeta) { | ||
| tx.objectStore(STORE_META).put({ | ||
| ...previousMeta, ...syncMeta, ownerKey: ok, networkKey: String(networkKey), | ||
| walletId: String(walletId || ""), profileIndex: Number(profileIndex), updatedAt: Date.now(), | ||
| }); |
There was a problem hiding this comment.
Confirmed the abort-only transaction issue: aborting after the write request succeeds reproduced a hanging promise. Write transactions now handle onabort as well as onerror; the regression passes. The null-spread claim is a false positive: { ...null } is valid JavaScript. That expression is unchanged, and an explicit first-notes/no-existing-metadata regression and the Chrome full-rebuild check pass. Also removed the stale local-controller references mentioned in the review summary; their ReferenceErrors were caught, and shared invalidation already aborts the operation. Reader cleanup now lives in finally, with a blocked-reader cancellation test. Full suite: 601 passing.
| console.log("rollback probe", { | ||
| force, tip: "9", status: engine.getShieldedStatus().state, | ||
| cursor: engine.getShieldedStatus().cursorBookmark, | ||
| scannedFrom: notes.mock.calls.map(([, options]) => options.from.asUint().toString()), | ||
| cachedNotes: (await store.getNotesMap(NETWORK_KEY, WALLET_ID, 0)).size, | ||
| }); |
There was a problem hiding this comment.
Removed the probe logging and strengthened this test to verify the lagging-node error, no scan, and preservation of both the cached note and persisted cursor.
There was a problem hiding this comment.
🟡 Changes recommended
IndexedDB write transactions in shieldedStore.js are not aborted when the sync AbortSignal is cancelled, so stale cache clears/writes can still commit after lock/profile/network changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/shared/shieldedStore.js:244
putNotesMap()writes notes+meta in an IndexedDB transaction, but aborting the passed AbortSignal after the transaction starts will not stop the pending IDB requests. That means a lock/profile/network change can still commit stale cache writes after cancellation. Hook the signal totx.abort()and clean up the listener on completion/error so the transaction is reliably cancelled.
await new Promise((resolve, reject) => {
const tx = db.transaction(syncMeta ? [STORE_NOTES, STORE_META] : [STORE_NOTES], "readwrite");
tx.oncomplete = () => resolve(true);
tx.onerror = tx.onabort = () => reject(tx.error || new Error("Failed to write notes"));
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
|
Follow-up review addressed in f744217: cancellation now calls native |
There was a problem hiding this comment.
🔵 Needs a closer look
It makes substantial changes to shielded synchronization, caching, and cancellation semantics where a final human review should validate the safety/performance tradeoffs and edge cases.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/shared/walletEngine.js:2295
assertAnchor()performs anetwork.query(...)round-trip and is invoked for every notes chunk (await assertAnchor()insideprocessChunk). If w3sper emits many small chunks, this becomes an RPC-per-chunk pattern and can materially slow sync / stress the node. Consider reducing validation frequency (e.g., validate once before scanning and once after, or at a coarser interval / on detected inconsistencies) while still ensuring you never commit notes under an unverified anchor.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Final advisory review: the per-chunk header round trip is an intentional safety-first tradeoff, now documented in f406bd5. The real Testnet full-scan check is not a large-chain throughput benchmark. Batching verified commits is the upgrade path if latency dominates; this still needs human review before merge. |
There was a problem hiding this comment.
🟡 Changes recommended
The Vite load() hook in w3sperStreamCompat() uses strict id equality that can miss real Vite ids (e.g., query-suffixed/normalized paths), which can prevent the backport from being applied.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/12 changed files
- Comments generated: 1
- Review effort level: Lite
| load(id) { | ||
| if (id !== target) return null; | ||
| const pkg = JSON.parse(readFileSync(path.join(path.dirname(entry), "../package.json"), "utf8")); |
There was a problem hiding this comment.
Confirmed the version-query case through the real Vite development transform pipeline: it returned the original reader before this fix. The loader now normalizes path separators and accepts version/HMR/import queries while leaving raw, URL and other asset requests alone. The regression passes; all 615 tests and Chrome/Firefox/Tauri builds pass. Emitted production JS bundles remain byte-identical to the already browser-tested build.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core shielded-sync correctness, persistence, and build-time SDK patching in ways that warrant careful final human review despite strong test coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
vite.local-w3sper.js:26
w3sperStreamCompat().load()does synchronous disk IO and JSON parsing on every module load (reads package.json and the backported source each time). In Vite dev/HMR this hook can be hit repeatedly, so this adds avoidable overhead; the version check and backport source can be read once when the plugin is constructed and then reused.
- Files reviewed: 10/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
An empty unspent store and zero cursor do not establish that a legacy cache is empty. Reset both note stores before recovery so reconciliation cannot restore orphaned spent notes under a new anchor, while preserving pending reservations. Cover missing and zero-cursor metadata, ordinary and forced sync, empty and replacement chains, and subsequent reconciliation. Refs #106, #109.
fbae2ed to
4772b5a
Compare
Summary
Fixes #106, fixes #107, fixes #108.
Published SDK compatibility
The published
@dusk/w3sper1.7.0-rc.0 drops upstream read failures. Its reader is already fixed upstream but no newer SDK version is published. Backport that upstream module verbatim, retaining its MPL notice, through the existing Vite SDK integration for Chrome, Firefox and Tauri. The hook is scoped to the installed SDK file, checks the package version, and excludes SDK dev prebundling. Remove the backport when upgrading to a release containing the fix. No new dependencies, protocol/WASM changes, disabled validation, or larger chunk-size default.Validation
Current rebased candidate:
4772b5aRebased the recovery work and spent-only-cache follow-up onto main
1f05366, retaining commit author identities, author dates, messages and trailers; original history is backed up locally. Only the changelog conflicted, and both sets of entries were retained.npm ci --no-audit --no-fund, then 720 tests / 60 files passed with configured utility-subset coverage thresholds.9948212, whose tree equals this candidate. The existing non-gating Codecov upload still failed for a missing token (Token required because branch is protected), despite its successful action-step conclusion.The engine/store regressions use the actual repository functions with mocked W3sper/network and
fake-indexeddb. Renderer checks and frontend builds do not establish native-browser cache recovery, real extension signing, genuine chain scanning, proof generation, consensus-induced reorg behavior or native-desktop execution. The spent-only finding is conditional on the seeded persisted state, not a claim that every wallet naturally reaches it or that an invalid note could be accepted on-chain.Earlier validation — before this rebase and spent-only follow-up
These results describe the original recovery branch through
fbae2ed, against the older8906af8baseline. They were not rerun on4772b5aand are not current-candidate E2E evidence.Historical performance and limits
The following measurements use the earlier recovery branch and
8906af8baseline, not the rebased candidate.At 100k notes the original three-run medians were approximately 74–79s for this PR versus 145–154s on reviewed main. This is the normal two-profile configuration: the PR scans the selected profile, while main checks both yet does not advance the other profile's cursor. It is a workflow improvement, not a 2× faster ownership algorithm. Results do not establish mainnet-scale, cross-device or cross-browser performance.
Per-chunk header checks remain. SDK prefetch overlaps them with ownership work; removing validation showed no clear benefit at 0/100 ms added delay. Full-rescan recovery and waiting for a behind node have real availability costs. Anchors are node-provided consistency checks, not independent chain verification. Already committed transactions cannot be undone by cancellation.