feat(sdk): note-recovery primitives — transport drain, proposal import, public backfill (#357) - #435
Open
haseebrabbani wants to merge 8 commits into
Open
feat(sdk): note-recovery primitives — transport drain, proposal import, public backfill (#357)#435haseebrabbani wants to merge 8 commits into
haseebrabbani wants to merge 8 commits into
Conversation
) * feat(sdk): private-note transport backlog drain recovery primitive (#414) Adds a recovery primitive to both SDKs that rescans the full private-note transport backlog for tracked note tags, regardless of the stored transport cursor: after device loss a fresh store has no cursor, and in a shared store another account's sync may have advanced it past the recovered account's notes. - TS: drainPrivateNoteBacklog(midenClient) wrapping fetchPrivate({mode:'all'}) - Rust: MultisigClient::drain_private_note_backlog() wrapping fetch_all_private_notes() - Structured TransportRecoveryReport (completed | unavailable | failed, imported count, retryable, reason). Transport-disabled/unreachable and the upstream PaginationDidNotTerminate convergence guard are reported, not thrown, so a transport failure never aborts a recovery flow. - Load/tag regression tests: inserting a recovered account (load() / pull_account) tracks its standard note tag; reload stays idempotent. - Rename the misleading add_or_update_account argument imported -> overwrite (matches the upstream add_account parameter it forwards to). - Docs: transport recovery is bounded by transport retention - not a backup. * fix(sdk): review follow-ups for the transport drain primitive (#414) - Reuse the crate's tested transient/permanent classifiers for retryable: Connection cause-chain via is_transient_note_transport_error (endpoint parse/TLS misconfig no longer advertised as retryable), node RPC failures via is_transient_rpc_error (permanent statuses no longer retryable). - Make the NoteTransportError match exhaustive so new upstream variants become compile errors instead of silent defaults. - Honor the Unavailable contract ('nothing was imported'): a connection lost mid-drain after partial progress now reports failed + retryable with the partial count, in both SDKs. - Propagate mid-drain StoreError as Err (broken local store is not a transport outcome). - Replace the Rust commitment-set diff with the same length-delta the TS side uses (drain never removes records; &mut self excludes concurrency). - Dedupe TS error-message extraction into connectivity.errorMessage and classify once (reason comes from the classifier); skip the post-drain store re-count when the transport is disabled. - Pin the TS classification fragments against the shipped WASM binary (tests/recovery-fragments.test.ts) so an SDK bump that rewords the upstream error text fails CI instead of silently misclassifying. - Keep src/testing (fake-indexeddb device helper) out of the npm tarball. - Correct the add_or_update_account doc: the overwrite flag is inert today. * test(sdk): live testnet round trip for the transport drain (#414) Ignored by default (network-dependent); run with: cargo test -p miden-multisig-client --lib live_testnet -- --ignored Relays a private note over the real testnet transport, recovers it into a fresh store via drain_private_note_backlog, checks idempotence, and checks the Unavailable report for a custom node endpoint with no derivable transport. * fix(sdk): address PR #423 review feedback - TS: rethrow local-store failures raised inside the drain instead of folding them into a transport report, matching the Rust StoreError branch and the documented contract. isLocalStoreError is deliberately narrow: the WASM 'storage error' chain prefix, IndexedDB/Dexie store error names, and AbortError only when its message points at an IndexedDB transaction/database abort (network aborts stay transport-classified). 'storage error' added to the WASM fragment drift guard. - TS: real advanced-cursor non-regression coverage in the WASM behavioral test - seed the raw cursor row past the backlog, drain, assert the note is still recovered and the seeded bytes survive. The public settings API runs values through the JS<->WASM codec (not the store's raw encoding), so the test reads/writes the IndexedDB row directly. - TS: harden errorMessage against non-string message properties ({ message: 42 } no longer crashes classification). - Tests: drop the 'load path' framing from the tag test names/comments - they pin the store-side insert=>tag invariant, not load()/pull_account themselves.
# Conflicts: # packages/miden-multisig-client/package-lock.json # packages/miden-multisig-client/package.json
…erge main brought in the v0.16 upstream bump (#340 / v0.17.0-rc.1), which removed the explicit full-drain API the #414 primitive used: - fetch_all_private_notes() is gone; 0.16 moved full-drain semantics into sync_note_transport()'s covered-tag bookkeeping. Both SDKs now clear the covered-tags marker (NOTE_TRANSPORT_COVERED_TAGS_KEY) and run the transport sync, which re-drains every tracked tag from the start with a local cursor — same recovery semantics (full rescan, global-cursor non-regression, idempotent). The TS side probes with the incremental fetch first because 0.16's syncNoteTransport silently no-ops when the transport is disabled, and the drain must report that as unavailable. - Test fixtures ported to the 0.16 protocol surface: P2idNote::builder(), AuthSingleSig::new(Approver::new(..)), with_component for auth, send_private_note_with_block_hint, mandatory P2ID note asset, and the sendPrivate scanAfterBlockNum option.
* feat(sdk): import proposal-embedded notes as a recovery primitive (#415) Adds import_notes_from_proposals (Rust) / importNotesFromProposals (TS): rebuilds store records from the note bytes embedded in v2 consume_notes proposals plus a node-fetched inclusion proof, so recovery works for private notes without the node holding the body (validated by spike #412). Per unique embedded note: decode, skip notes the store already tracks (matched by details commitment / recipient digest + asset fingerprint so metadata-less records are recognized), fetch proofs in one batch, import individually (upstream batches are atomic), and classify the outcome. Uncommitted notes are recorded as expected with their tag tracked so a later sync picks them up; chain-nullified notes are recorded as consumption history and reported already-consumed. No per-note problem aborts the batch. Includes the fixes from the PR #424 Copilot review. * feat(sdk): Multisig.importNotesFromProposals reusing client RPC settings Review follow-up on PR #424: the standalone helper required passing the Miden RPC endpoint although the loaded client already knows it (neither the MidenClient facade nor the raw WASM client exposes its endpoint, so a standalone function cannot derive it). The new Multisig method reuses the client's endpoint and resolved retry configuration, and syncs pending proposals from GUARDIAN when none are passed. The standalone export stays for callers holding a raw WASM client or proposals from another source; docs now lead with the method. Also drops a stray README section that a shared rerere resolution replayed from another branch's conflict.
…416) * feat(sdk): historical public-note backfill by tag recovery primitive (#416) Adds the third recovery primitive from #357: a tag-scoped historical scan that discovers public notes normal forward sync would skip (global cursor already past their blocks, or a fresh store) and imports them individually with their on-chain inclusion proofs, never touching the global sync height. - Rust: MultisigClient::backfill_public_notes_by_tag(account_id, from, to) in new client/backfill.rs, genesis/tip defaults; TS: backfillPublicNotesByTag(midenClient, { accountId, midenRpcEndpoint, fromBlock?, toBlock?, rpc? }) in new publicNoteBackfill.ts. - Shared PublicBackfillReport { scanned range, discovered, skippedPrivate, outcomes, uncovered, retryable, reason } reusing NoteImportOutcome with a new 'backfill' source; scan failures are reported (uncovered ranges), not thrown, so a partial scan never aborts a recovery flow, and report-level retryable also reflects retryable per-note outcomes. - The node's per-request pagination cap is handled by client-side range splitting with a bounded request budget; TS classifies it by the pinned 'rpc pagination error' WASM fragment (drift-guarded). TS block bounds are validated as u32 integers (JS numbers wrap mod 2^32 at the WASM boundary) and NoteId handles are minted fresh per retry attempt (the bridge consumes call arguments). - Store lookups key on details commitments / id+details keys so metadata-less records are not re-imported; unlike the proposal import, a proof-less Expected record is upgraded in place with the fetched proof — forward sync never revisits its block, so skipping would strand it. - Extracts the shared store-lookup, with-proof import, and consumed-state re-check helpers out of the proposal import, now used by both recovery primitives; private tag matches are counted and skipped, tag collisions and duplicate discoveries tolerated per the issue spec. - Offline Rust coverage drives the full success path against the upstream MockChain (dirty-store scenario, collision/private partition, pagination split and unsplittable fallback, Expected-record upgrade); TS unit twins mirror every branch. Docs in MULTISIG_SDK.md and both READMEs. * fix(sdk): screen backfill discoveries for relevance; address PR #429 review (#416) Relevance screening (review finding): normal sync routes every tag match through the NoteScreener before storing, so a stranger's note that merely collides with an account's tag never enters the store — but the backfill imported every public tag match, letting anyone grief a recovered account with permanent store pollution and per-note import RPC. Now, exactly like sync, every genuinely new discovery is screened before import and rejected matches are counted in a new skipped_irrelevant/skippedIrrelevant report field (already-tracked records, including the proof-less Expected upgrade path, bypass the screen — they are material the user chose to track): - Rust screens with the execution-based NoteScreener (get_batch_consumability_for_account), covering all note scripts; screening requires the account tracked in the store, which recovery via pull_account guarantees. - TS screens statically against the well-known P2ID/P2IDE script roots and storage layouts (the WASM surface does not expose the screener — upstream ask); custom-script notes are conservatively skipped, documented as a divergence. A real-WASM drift-guard test (tests/backfill-relevance.test.ts) pins the root and storage-layout assumptions against notes built by the shipped SDK. Fixed a latent bug this surfaced: WASM Word.toString() returns '[object Object]' — root comparison now uses toHex(). Review comments (Ze): - Rust modules renamed to match the TS filenames: proposal_note_import.rs, public_note_backfill.rs. - TS recovery primitives moved into src/recovery/ (transportDrain, proposalNoteImport, publicNoteBackfill) so the #357 family is grouped. - New Multisig.backfillPublicNotesByTag({ fromBlock?, toBlock? }) reusing the client's endpoint and retry configuration (same shape as importNotesFromProposals from #424); docs and README examples use it. - Both READMEs group the three primitives under one 'Recovering Notes After Device Loss' section with when-to-use context.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Comment |
Review feedback on the recovery-primitives PR: - New upper-level method running all three strategies as one flow — Rust MultisigClient::recover_notes(Option<NoteRecoveryOptions>), TS Multisig.recoverNotes(options?). Strategies (transport drain, proposal import, public backfill) are individually selectable; the flow ends with a normal verifying sync. A strategy that cannot run at all lands in report.problems instead of aborting, each primitive's report is carried through unchanged, and the combined report aggregates imported/retryable. The flow is idempotent. - backfill_public_notes_by_tag now takes Option<PublicBackfillOptions> instead of two positional Option<BlockNumber> arguments, so the options can grow without signature churn. - Issue/PR numbers dropped from user-facing documentation (READMEs, MULTISIG_SDK.md, rustdoc/TSDoc). Wiring: the demo gains an 'n — Recover notes' menu action printing the combined report; smoke-web gains a recoverNotes step (window.smoke API + Account-panel button) that refreshes the notes panel afterwards. Shared Rust test fixtures consolidated into client/test_support.rs (offline clients, mock transport, wallet/note builders), replacing the per-module copies.
…n across SDKs Review fixes on the recovery primitives: - The drain snapshots the covered-tags bookkeeping before clearing it and restores it (Rust: merged with any partial progress; TS: verbatim, the WASM surface exposes the value only opaquely) when the drain fails. Without this, one permanently undecodable relay blob in a tag's transport history left every tag uncovered, making every subsequent normal sync re-attempt and fail the same backfill on a client that synced fine before the attempt. - The drain now runs one transport sync per 64 tracked candidate tags (upstream MAX_BACKFILL_TAGS_PER_SYNC caps each call), so a shared store tracking more than 64 tags no longer gets a false Completed with tags past the cap never scanned. - TS drain failure classification mirrors the Rust classifier: node-RPC failures mid-import report failed (not unavailable), permanently misconfigured transport endpoints (invalid uri / tls / certificate / unsupported scheme wording) are not retryable, and transport network errors stay unavailable+retryable. The new Display-prefix fragments and the covered-tags settings key are pinned by the WASM drift guard. - The TS not-committed proposal import uses NoteFile.fromExpectedNote (details + sync-hint tag), mirroring Rust's NoteFile::ExpectedNote: upstream registers a note-source tag it removes once the note commits, instead of the permanent user-source tag addTag left behind (which the transport backfill would also re-drain). - detailsKeyOf's collision-safety claim corrected: the fingerprint covers fungible assets only (the WASM surface exposes no non-fungible accessor or commitment), a latent gap until upstream exposes a complete key. - PublicBackfillReport.outcomes documented with its real invariant: one outcome per screened-in public note, imported or not (discovered - skipped_private - skipped_irrelevant).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #414, closes #415, closes #416. Advances #357 — together with the transaction-history endpoint (#413, merged via #420) this completes the recovery primitives from the #357 plan.
Problem
After key-based recovery the local Miden store starts empty, and in a store shared with other accounts the global cursors may already be past the blocks and transport batches holding a recovered account's notes. Normal forward sync never revisits either, so pending notes are silently lost — the "hours of sync, still missing notes" scenario #357 describes. Spike #412 validated the recovery approach for each note class on a live network before implementation.
The three primitives (both SDKs, semantic parity)
MultisigClient::drain_private_note_backlog(), TSdrainPrivateNoteBacklog(midenClient). Rescans the full transport backlog for every tracked tag regardless of the stored cursor, without ever regressing it. Transport problems are reported (completed/unavailable/failed+ retryability), never thrown, so a transport outage can't abort a recovery flow. Bounded by transport retention — a best-effort rescan, not a backup.MultisigClient::import_notes_from_proposals(&proposals), TSmultisig.importNotesFromProposals(). Rebuilds importable notes from the bytes v2 consume-notes proposals embed (consume_notes proposal verification depends on the caller's local Miden IndexedDB — should it? #229) plus a node-fetched inclusion proof, so it works for private notes too. Per-note outcomes (imported/already-present/already-consumed/not-committed/invalid/failed), dedup across proposals, and no per-note problem aborts the batch.MultisigClient::backfill_public_notes_by_tag(account_id, from, to), TSmultisig.backfillPublicNotesByTag(). Tag-scoped historical scan (genesis→tip by default) that imports discovered public notes with their on-chain inclusion proofs, never touching the global sync height. Cost scales with matches, not range length (Spike: note-recovery feasibility benchmarks (genesis tag-scan, transport drain, proposal-note reconstruction) #412: genesis→tip over ~1.7M blocks in seconds). Handles the node's pagination cap by client-side range splitting, reports unscannable ranges instead of failing, and — like normal sync — screens every new discovery for relevance before import so tag collisions can't pollute the store.Cross-cutting design
NoteImportOutcomewith asourcediscriminator) and shared store-dedup/import/consumed-reclassification helpers between the proposal import and the backfill.Known limitations
NoteScreener(custom-script notes are conservatively skipped; upstream exposure would close the gap with Rust).