0xMiden / 0xMiden/note-transport-service

Private notes silently lost after transport sequence resets — PR #97 heals starvation but not the dead band; no client-side recovery path

Open
#101 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
3
Forks
10
Avg merge
2h 23m
Merged PRs (30d)
4

Description

## Summary

Private note payloads exist in exactly one place before consumption: this service. The client paginates `FetchNotes` with a persisted cursor that is, server-side, a raw SQLite `AUTOINCREMENT` value. When the server's sequence space regresses below a client's persisted cursor, that client fetches **zero notes** and the server echoes the stale cursor straight back (silent starvation). Once the sequence later regrows past the stale cursor, notes that landed in the skipped range are delivered to **no one** (dead band).

This is a confirmed, reproduced defect, filed with a proposed remediation. The server-side cursor bug is partially tracked by PR #97 — but #97 does **not** close the full class: the dead band survives it, and there is no client-side recovery path. The mechanism below is verified from shipped source and reproduced against the real gRPC handler; the fix (from "Proposal" onward) is a proposed direction, labelled as such, not a prescription. Where a claim could not be verified it is labelled rather than dropped.

## What is proven vs. proposed

| | Contents | Basis |
|---|---|---|
| **PROVEN** | The failure mechanism (starvation + dead band); the client re-persisting the echoed cursor; the wallet's inbound path being unary-`FetchNotes`-only; the rescan API being shipped-but-uncalled; PR #97 healing starvation but not the dead band | Source (repo/tag/file:line) + reproduction against the real gRPC handler over a real SQLite store |
| **PROPOSED** | The layered fix (durable ordering, client rescan, epoch identity, delivery-aware retention) | Design judgment, adversarially reviewed |
| **NOT VERIFIED** | The deployed storage configuration at transport.miden.io; the specific field-incident numbers; that a recovered note becomes spendable | Stated as such wherever they appear |

## Failure mechanism (proven)

Verified at note-transport-service `origin/main` = `91ca4de` = tag **v0.4.1** (deployed); miden-client tag **v0.15.2**; `@miden-sdk/miden-sdk` **0.15.2**; wallet **v1.15.5 / v1.15.7**.

- `FetchNotes` returns rows `WHERE seq > cursor ORDER BY seq ASC LIMIT 500` (`crates/node/src/database/sqlite/mod.rs`); the handler computes `rcursor = max(cursor, returned seqs)` (`crates/node/src/node/grpc/mod.rs`). For any cursor ≤ `1e12` the echoed cursor can never move down, so a stale cursor is reflected back verbatim. (Cursors > `1e12` hit a separate legacy-reset branch that rewrites to 0 — not relevant to seq-magnitude staleness.)
- **Silent starvation** persists while the server high-water is below the stale cursor: every fetch returns nothing, no error anywhere. It is not permanent in itself — liveness resumes once the sequence climbs past the cursor. What is permanent is the loss of notes written while `seq < cursor`.
- **Dead band:** once the sequence regrows past the stale cursor, later notes arrive normally while the skipped range is silently omitted forever. The failure self-conceals.
- **The client closes the loop unconditionally.** Wallet `syncState()` → `client.sync()` → SDK `sync_state` → `sync_note_transport`, which reads the persisted cursor, fetches, then persists whatever the server echoes **with no empty-response guard** (miden-client v0.15.2, `crates/rust-client/src/sync/mod.rs`). Wallet v1.15.5 and v1.15.7 both pin `@miden-sdk/miden-sdk: 0.15.2`. So the fixed point exists on both sides.

Reproduced against the real handler over a real SQLite store (both file-backed and `:memory:`): starvation (cursor 2000 > high-water → 0 notes, cursor echoed back, six consecutive polls) and the dead band (C=620/hw=1220 and C=2000/hw=2503 — the low-seq range permanently invisible; six unaided recovery routes all failed; a control fetch at cursor 0 recovered them).

A note on scope: the dead band is a property of the **`FetchNotes` pull path**. `StreamNotes` discards the client-supplied cursor and replays a tag's full retained history to the first subscriber, which escapes it — but the wallet never subscribes, so wallet impact is unchanged. (That `StreamNotes` accepts a `cursor` field on the wire and silently ignores it is a separate defect worth filing: honour it or remove it.)

## A second trigger, needing no storage regression

The pull cursor is **global across tags** while `seq` interleaves across them. A client that begins tracking a new tag after its cursor has already advanced can never see that tag's older notes via `FetchNotes` — dead-band-shaped loss on a perfectly healthy, never-restarted server. This is why the fix must target the **class**, not one trigger, and why the client-side rescan (below) matters: it is the only remedy that covers every trigger.

## Why this is fund-relevant

Retention deletes purely by age — `DELETE FROM notes WHERE created_at < cutoff`, every 600 s, `retention_days` clap default `"30"` (the *deployed* value is unverified). There is no delivery concept in the schema, so an un-fetched, still-spendable payload is deleted on the same schedule as a delivered one. After deletion the recipient cannot spend the note: the chain holds only the commitment, and `NoteType::Encrypted` was removed (miden-protocol 0.14.4; 0.15.3 `note/note_type.rs` has only `Private`/`Public`), so there is no on-chain fallback.

**One honest caveat on severity.** Whether a recovered private note actually becomes *spendable* is **not verified**, and can no longer be tested on the original evidence: the account that held the stranded `Expected` notes has since been replaced (the tester recreated wallets several times), so those specific notes are on an account no longer in use and cannot be consume-tested. Across every account captured, no private note was observed reaching a consumed state — but given the account churn this is a weak signal, not a clean demonstration. The fixes below cover the delivery class regardless; the spendability of already-stranded notes is a separate, still-open question that would need a fresh controlled receive→consume test.

## Constraints that shape any fix (source-verified)

1. **Upload is decoupled from on-chain commitment.** A note can sit on NTS that never commits. `after_block_num` is a sender-set optional hint, stored verbatim, never validated/backfilled, nullable and unindexed. ⇒ a chain-height-anchored cursor is not viable.
2. **Recipient privacy.** Tags reveal only the top 14 bits of the target account (`miden-protocol note/note_tag.rs`), intentionally ambiguous — many accounts share a tag. Any per-recipient delivery tracking would force this service to learn a recipient↔note linkage it deliberately lacks.
3. **Payloads are plaintext and the service is unauthenticated** (no auth interceptor; CORS fully open). This rules out any "request a note by id" recovery endpoint.

## Proposal

Root principle: a client's resumption token must stop being a raw physical storage address whose validity depends on server storage lifetime. After these changes, cursor corruption should cost at most **bandwidth**, never funds — via two independent guarantees (server-side durable ordering + client-side rescan) so neither is a single point of failure.

### 1. Durable ordering (this service)

- Move the production store to **file-backed storage** and make `:memory:` **unreachable in a production configuration** — fail to start, not silently run non-durable. Remove any implicit `File::create`/fail-open path.
- This **closes the restart-class regression, the most frequent trigger.** It does **not** make the failure "structurally impossible": a table-rebuild migration that copies a *subset* of rows drops the AUTOINCREMENT high-water (reproduced: 100 → 40), and backup restore, rollback, and replica fork each regress or fork the sequence while remaining fully durable. Those need items 3 and 4.
- **Do not** move the high-water into "its own row independent of the notes table." SQLite already updates `sqlite_sequence` inside the same transaction as the INSERT (a rolled-back INSERT leaves no row), so insert/high-water atomicity is free today; an independent post-insert counter admits **`seq` reuse** (two notes, one sequence number) — silent, unrecoverable, strictly worse than the bug. Acceptable implementations: keep `AUTOINCREMENT` (same transaction), or a durable reservation/lease that burns a block of values and fsyncs *before* allocating. Read the high-water back as `max(stored, allocated)`, never `SELECT MAX(seq) FROM notes` (which regresses the moment retention empties the tail).
- **Guard rebuilds with a CI test** asserting `sqlite_sequence.seq` is non-decreasing across the full embedded migration chain. The 20260422 migration already uses the rebuild pattern, so the next one regresses the counter unless it explicitly restores the sequence. This is one test and closes one vector completely.
- **Single-writer enforcement:** an advisory lock on the DB file; refuse to start if held. This makes an accidental `replicas: 2` fail loudly instead of silently halving deliverability.

### 2. Bridge for already-poisoned clients — merge AND deploy PR #97

PR #97 heals starvation for deployed clients with no client update. It has been reproduced: it converges in two polls with no re-download storm, and it provably does **not** fix the dead band (the reset is gated on `cursor > high_water`). Notes:
- **Merging is not deploying.** transport.miden.io runs v0.4.1, which predates the PR; deploy is a separate manual step (the PR has been open ~19 days).
- **New risk to disclose:** if a migration or rebuild lowers the high-water below a *valid* caught-up client's cursor, #97 falsely resets that client to 0 and forces a full re-download (reproduced via the 100 → 40 drop). The migration-safety test in item 1 removes this; the residual is acceptable but should be known. The PR's inline comment calling the residual dead band "one-insert-wide" is inaccurate and should be corrected.

### 3. Client full-rescan recovery (rust-sdk / wallet) — promote to NOW

The rescan is **already shipped as a public, documented JS API**: `notes.fetchPrivate({ mode: 'all' })` → `fetchAllPrivateNotes()` (`@miden-sdk/miden-sdk` 0.15.2, `dist/mt/index.js:1266-1273`; typed in `api-types.d.ts`). Wallet v1.15.5/v1.15.7 pin that exact SDK version yet contain **zero** call sites (`git grep fetchPrivate -- src/**` is empty, including tests). Wiring it up is **one line of wallet JavaScript** — no SDK, WASM, proto, or server change. Because a cursor-0 fetch always returns everything currently held, this is the only remedy that covers **every** trigger, including the global-cursor tag-add case.

Two required conditions:
- It **must be bounded.** `send_note` is unauthenticated and unrate-limited with a 512 KB note cap; an unbounded rescan (1000-iteration cap) is a DoS amplifier against a flooded tag. Trigger it on **user-initiated refresh** (and, once item 4 lands, on server-epoch mismatch) — **not** on an unconditional periodic timer.
- It **must reset or bypass the stored cursor**, because `fetch_all_private_notes` persists `max(cursor, stored_cursor)` and otherwise leaves the poison in place.
- Prerequisite bug to fix first: the SDK's unchanged-record path reports already-known notes as `new_private_notes`, so the first rescan of a wallet with a backlog would fire every badge/notification/auto-consume keyed on that field. Fix it to report `None` for unchanged notes before shipping the rescan.

### 4. Storage-epoch identity (this service + SDK) — reinstated as required, not rejected

An earlier draft rejected an epoch-qualified cursor as "circular." The circularity holds **only** under `:memory:` and dissolves once item 1 makes storage durable. With durable storage, a storage epoch is the **only** mechanism that detects backup restore, rollback, and replica fork — failure modes durability alone cannot touch, because a restored database's high-water restores in lockstep with the data it was meant to police. Implementation note: it requires a **new proto field** (`FetchNotesRequest.epoch` / `FetchNotesResponse.epoch`); packing an epoch into the existing `fixed64` is not viable, because a 40-bit seq puts `epoch=1, seq=0` above the `1e12` legacy threshold and it is misclassified as a legacy cursor. Client resets on epoch mismatch.

### 5. Delivery-aware retention (this service)

Retention must not delete an undelivered but still-spendable payload; this requires new schema state. This overlaps issue #86 ("check whether a sent note has been fetched"). Reconcile both with a **per-tag fetch watermark** — never per-recipient or per-note — which preserves the 14-bit tag ambiguity (constraint 2) while giving retention the signal it needs. Needs a hard ceiling to bound growth for a tag that is never fetched.

### 6. Guardrails — do not regress

Keep tag-based, unauthenticated `FetchNotes`; do **not** add per-recipient tracking, ACKs, or id-addressed re-delivery. Separately (flag, don't block): payloads are plaintext on an open service — content e2ee would make durability far less dangerous and is worth a protocol conversation given `NoteType::Encrypted`'s removal.

## Rejected designs — do not re-propose

| Design | Why rejected |
|---|---|
| Sender-retained outbox + recipient re-delivery | Relocates custody to the least-available party; re-delivery either serves any payload by public id to anyone (confidentiality break, payloads are plaintext) or authenticates the recipient (the forbidden linkage); unbounded sender GC. |
| Chain-height-anchored cursor | Uncommitted notes have no height; `after_block_num` is nullable/unvalidated/non-unique/unindexed; a 32-byte note id doesn't fit the `fixed64` field. |
| Per-recipient ACKs / receipts | Privacy veto (constraint 2). Superseded by the per-tag watermark. |
| Naive client "never regress the cursor" clamp | Backfires — rejects #97's downward heal and keeps the client starved. Any guard must be epoch/instance-aware. |

## Suggested sequencing

| Priority | Action | Owner |
|---|---|---|
| Now | Migration-safety CI test + single-writer advisory lock (both trivial, each closes a silent failure) | this service |
| Now | Merge **and deploy** PR #97; correct its inline comment | this service + ops |
| Now | File-backed prod storage; `:memory:` fails to start; remove fail-open paths | this service |
| Now | Wire the one-line `notes.fetchPrivate({mode:'all'})` rescan into the wallet UI (bounded; resets cursor; after fixing unchanged-record reporting) | wallet + SDK |
| Next | Storage-epoch identity (new proto field; coordinated rollout) | this service + SDK |
| Then | Delivery-aware retention via per-tag watermark, reconciled with #86 | this service |

## Verification

- **Durability:** chaos test — send private notes continuously while killing/recreating the store mid-run; assert no client cursor exceeds high-water, zero notes leapfrogged, and prod `:memory:` refuses to start.
- **PR #97:** cursor above high-water returns the full set from 0 and echoes a healed cursor; `cursor == high_water` is not reset; a pruning migration does not falsely reset a caught-up client (with the migration guard in place).
- **Client rescan:** send while recipient offline; recreate the store; bring the wallet back; assert the note is received via rescan with no duplicate records, and that a **recovered note becomes consumable** (this last step is currently unverified and gates the fund-loss severity).

We run a blockchain e2e suite plus a stress suite with conservation checks and can contribute the chaos and e2e tests.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.