Repository navigation
Add the receiver PIR restore client - #124
czarcas7ic wants to merge 41 commits into
Conversation
9f86d25 to
d812f4f
Compare
d812f4f to
add1c40
Compare
57f6736 to
3e798c4
Compare
069393b to
78ca6b5
Compare
78ca6b5 to
ec734eb
Compare
ec734eb to
5452654
Compare
5452654 to
469db7e
Compare
469db7e to
699a491
Compare
zakura-pir-receiver runs restore sweeps of dynamic keys. It accepts a receiver directory publication only at a block of the wallet's own chain, tests the wallet's receivers against the publication's labeled filter sets, which every wallet downloads alike, and looks up only those in the paid set over PIR, fetching their note data through Enhance PIR. Results go through the sweep steps of the backend's dynamic IVK traits, so the client works with any store that implements them; SQLite is only a test dependency. A provider's recent set keeps a receiver it was given lately scanning, and its seen set marks a receiver it ever had so issuance skips it. The wallet trusts a recent set only when it is current, its feed having read the provider within the last fifteen minutes, and its declared window covers the wallet's own 24-hour restore watch with the feed running throughout; otherwise every restored key keeps scanning for the watch. A wallet with nothing paid makes no PIR query. When the directory cannot be accepted, every due key backs off as if its own lookup had failed. The receiver crates come from wallet-pir's open receiver stack, pinned at 502d930e, until they land on its main. The crate has its own CI lane and stays out of default; the graph check builds it without the wallet feature. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The receiver stack has merged into wallet-pir's main at 0d6ad4aa, so pin receiver-directory, receiver-pir and receiver-pir-server there instead of the open stack's 502d930e. The lock moves only the wallet-pir packages those crates use. The merged directory refuses a payment at transaction index 0 as a coinbase, which it excludes, so the sweep fixture's mock block, which has no coinbase, now places its payment after an implicit one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
be0f8e5 to
06685a8
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06685a8428
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut recent_complete = false; | ||
| let mut recent_incomplete = false; |
There was a problem hiding this comment.
When a publication contains a complete recent set for one provider but only a seen set for another, these global flags consider the publication complete and allow receivers absent from the first provider's set to stop scanning. A receiver may have been quoted recently by the provider lacking recent; if it is paid after the publication anchor, the wallet neither scans that payment nor reruns the completed sweep, leaving the funds undiscovered. Track completeness by the provider prefix and conservatively keep keys scanning whenever any represented provider lacks a complete recent set.
Useful? React with 👍 / 👎.
| let session = self.session.as_mut().expect("opened above"); | ||
| // Enhance groups these positions into shared row requests itself. | ||
| let stream = session.query_batch(self.transport, positions)?; |
There was a problem hiding this comment.
When a sweep needs another note-data batch more than 30 seconds after this cached client was accepted, Client::query_batch rejects the work because refresh_due() is true (zakura/pir-enhance/src/transport.rs:183-184,254-259,580). The same happens permanently after a query observes HTTP 409/410, yet self.session remains Some, so this path never fetches and wallet-accepts fresh routing; a slow or long restore therefore defers all remaining payments, and reuse of the same EnhanceNotes can never recover. Refresh the pending manifest and acceptance before new work when due, and replace or rebind an expired session.
Useful? React with 👍 / 👎.
A sweep let receivers stop scanning once any provider's recent set was current and complete. A publication with a complete `a/recent` but only `b/seen` therefore ruled out receivers that provider `b` may have quoted recently, and a payment to one after the publication anchor would go unscanned. Track completeness per provider prefix: every provider with a `recent` or `seen` set needs a current, complete recent set, otherwise every key keeps scanning. Hit matching still combines the sets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`EnhanceNotes` accepted an Enhance manifest once and reused the client. Enhance rejects new work once `refresh_due()`, 30 seconds after acceptance or for good after a 409 or 410, so a long restore deferred every later payment and reusing the same `EnhanceNotes` never recovered. Before each nonempty batch, when no client exists or a refresh is due, fetch the pending manifest and wallet-accept it: the first time through `PendingClient::accept`, then through `Client::accept_routing`, which clears expiry, rejects a routing rollback and keeps compatible sessions. A failed batch is not retried; the next call refreshes. The regression test serves an all-zero Enhance database whose first query answers 409 or 410, and checks that the next call refetches, re-accepts and succeeds. It reuses pir-enhance's synthetic manifest, which needs `sha2`, `hex`, `serde_json` and `base64` as dev-deps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A candidate whose application waits for scanning, a witness checkpoint or spend history takes the next attempt's answer: a corrected position is recovered and a paid-filter miss drops it. An unchanged answer is looked up again but reuses the queued note data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A seen set that held every tested receiver moved the lookahead after each batch, and the run selected the new keys again, so one publication could keep a run deriving and writing keys until index exhaustion. Drop the refill loop: a run prepares at most one batch per account, maintains the lookahead once and leaves the new keys pending for a later run. Tests that expected one run to finish everything now run until nothing is pending. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`KeyId` holds only a purpose and index, so with several accounts a deferred `Receive/0` could belong to any of them. `Swept` now takes the account type and lists each deferred key as `(account, key, error)`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 693ef90a35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| e, | ||
| Error::Directory( | ||
| DirectoryError::Revision | DirectoryError::Transport(_) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
If connect_manifest or witness decoding returns a non-transport directory error such as Malformed, Directory::session has already consumed self.transport while leaving self.session empty. This predicate does not stop the batch, so the next paid key is leased and deterministically receives Error::Unavailable; because that variant is also excluded here, every subsequent paid key is unnecessarily leased and backed off, potentially delaying recovery for up to twelve hours after the service is repaired. Treat an unusable session (including Error::Unavailable) as run-fatal and cover failed session initialization with failure injection.
AGENTS.md reference: AGENTS.md:L43-L46
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opening the session moves the transport out of the directory, so when that fails with a non-transport error such as a malformed witness snapshot, every later paid key was still leased, failed with `Error::Unavailable` and backed off. The run now stops after the first failure whenever the directory is left with neither a session nor a transport, as it already did for revisions and transport errors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1e359c9c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// none, or if any provider with a set lacks a seen set with its feed's read times (see | ||
| /// [`seen_sets`]). | ||
| pub async fn fetch_seen<T: Transport>(origin: &str, transport: &T) -> Result<Seen, DirectoryError> { | ||
| let manifest = DirectoryClient::fetch_manifest(origin, transport).await?; |
There was a problem hiding this comment.
When origin is accidentally configured for another network, this accepts that network's well-formed seen filters without checking manifest.directory.genesis. A miss can therefore be passed to ProviderSeen as evidence that an address was never quoted; if its timestamps cover the restored wallet's quote interval, the wallet may reissue an address previously used on the intended network and link two swaps. Require the expected genesis/network when fetching seen sets and validate it before trusting misses.
AGENTS.md reference: AGENTS.md:L43-L46
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`fetch_seen` trusted any well-formed publication at `origin`, without checking `manifest.directory.genesis`. With `origin` pointed at another network, a miss in that network's seen sets could pass to `ProviderSeen` as evidence that an address was never quoted, and a restored wallet could reissue an address it had already used. It now takes the network's genesis, as `sweep` does, and refuses a publication committing to another one with the error `sweep`'s coverage check gives, `receiver_directory::Error::Coverage`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb62c90f63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| receiver-directory = { git = "https://github.com/valargroup/wallet-pir", rev = "0d6ad4aa94abbb2838d4182c869f06e1881e6c35" } | ||
| receiver-pir = { git = "https://github.com/valargroup/wallet-pir", rev = "0d6ad4aa94abbb2838d4182c869f06e1881e6c35" } |
There was a problem hiding this comment.
When receiver and transparent PIR are enabled together, these new 0d6ad4aa dependencies coexist with the 06a972db pins in zakura/pir-transparent/Cargo.toml:36-42; Cargo.lock consequently contains two source-distinct copies of pir-native 0.1.0, plus duplicate pir-control and pir-observability in all-target builds. The graph verifier misses this because it collapses packages by name and version rather than source, so combined consumers compile two implementations and updates can silently diverge. Move the wallet-pir family to one shared revision.
AGENTS.md reference: AGENTS.md:L43-L46
Useful? React with 👍 / 👎.
| if note_data.len() < requested { | ||
| return Err(Error::Unavailable( | ||
| "note data is missing for a directory payment", | ||
| )); |
There was a problem hiding this comment.
If a NoteSource returns a partial or mis-keyed batch together with unrelated cached entries—such as requesting position p but receiving one entry for q—queue_directory_lookup returns false, while note_data.len() < requested is also false. The loop then requests the same missing position indefinitely, hanging the sweep instead of returning the intended missing-data error. Check that every requested position is present, or otherwise detect that the iteration made no progress.
Useful? React with 👍 / 👎.
zakura-pir-transparent pinned wallet-pir at 06a972db while zakura-pir-receiver pins 0d6ad4aa, so a build with both carried two copies of pir-native, pir-control and pir-observability. Move every wallet-pir dependency of pir-transparent, normal and dev, to 0d6ad4aa, a descendant of 06a972db whose transparent crates are unchanged; the lockfile drops the second copies and changes nothing else. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The graph check groups packages by name and version, so two git revisions of one crate at the same version passed, as wallet-pir's pir-native did while the PIR clients pinned different revisions. Fail when any package name in the workspace graph resolves to more than one git source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sweep compared the size of the returned note data with the number of positions it asked for, so a batch keyed by other positions passed that check while the lookup stayed incomplete, and the loop asked for the same positions forever. Require an entry for each requested position, so such a batch returns the missing-data error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sweep asks a note source for every key it sweeps, with no positions for a key without payments, so the mis-keyed regression's bound on repeated requests tripped on the unpaid keys of the window. Answer those without counting them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dbda8cd37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .into_iter() | ||
| .take(NOTE_BATCH) | ||
| .collect(); | ||
| let note_data = notes.note_data(wallet, batch.clone()).await?; |
There was a problem hiding this comment.
When the paid filter misses, the lookup is empty, or all returned payments already have queued note data, batch is empty but this still invokes the caller-provided NoteSource. Only EnhanceNotes explicitly handles that case; another valid source may perform unnecessary I/O or reject an empty request, causing an otherwise complete unpaid sweep to be deferred indefinitely. Pass an empty map directly to queue_directory_lookup when batch.is_empty() instead of calling the source.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A paid-filter miss, an empty lookup, or payments whose note data was already queued left the sweep with no positions to fetch, but it still called the caller's `NoteSource`. Only `EnhanceNotes` returned early on an empty request; another source could do needless I/O or reject it, deferring an otherwise complete unpaid sweep on every run. The sweep now queues the lookup with no note data without calling the source, and `NoteSource` documents that positions are never empty. `EnhanceNotes` keeps its early return, since it is public and may be called directly. The test sources drop their now-unused empty cases, and the paymentless sweep test uses a source that fails every call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main moved transparent PIR to wallet-pir 9d2cbda0, which contains the merged receiver crates unchanged, so the receiver pins follow it and the workspace keeps one copy of each wallet-pir crate. Main's transparent PIR files are taken as they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 091c60fadf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let unavailable = directory.transport.is_none() | ||
| && directory.session.is_none() | ||
| || matches!( |
There was a problem hiding this comment.
When EnhanceNotes cannot accept its manifest because the anchor is not yet scanned or mismatches, the first paid key returns Error::Unavailable after the receiver-directory session has already opened. This condition therefore remains false, and the loop leases every remaining paid key only to repeat the same failure; repeated scheduled runs can advance the whole batch to the twelve-hour backoff just before scanning catches up. Treat shared note-source setup or acceptance failures as run-fatal before leasing the tail, with multi-key failure-injection coverage.
AGENTS.md reference: AGENTS.md:L43-L46
Useful? React with 👍 / 👎.
| for account in accounts { | ||
| lock.write("dynamic_sweep.release", || { | ||
| wallet.finish_dynamic_nullifier_recovery(*account, through) | ||
| }) |
There was a problem hiding this comment.
If the wallet's chain height has advanced beyond through, the guard above skips all sweep work but this loop still calls finish_dynamic_nullifier_recovery with the stale point. That method's trait contract requires through to be a canonical scanned tip, so a generic DynamicIvkWrite backend may legitimately release evidence based on that precondition without repeating SQLite's defensive tip check. Keep the release under the same tip guard, or revalidate the full chain point immediately before invoking it.
Useful? React with 👍 / 👎.
| for &account in accounts { | ||
| let batch = lock | ||
| .write("dynamic_sweep.due", || { | ||
| wallet.prepare_dynamic_sweeps(account, through, now, BATCH) |
There was a problem hiding this comment.
When accounts contains the same account more than once, each call to prepare_dynamic_sweeps occurs before any key is leased, so both calls return the same due batch. The processing loop then leases and queries every duplicated key twice; a deferred key also has its attempt counter incremented twice and appears twice in deferred, accelerating its retry backoff. Deduplicate account IDs before preparing work, or explicitly reject duplicate input.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When EnhanceNotes could not accept its manifest because the anchor was unscanned or on another chain, it returned Error::Unavailable, which a sweep cannot tell apart from one key's missing note data. The run then leased every remaining paid key only to repeat the same failure and back the whole batch off. Report that case as a new Error::NotesUnavailable, which the sweep treats like a failed directory session: the run stops before leasing more keys. A failure-injection test pays two keys and checks that the second is neither leased nor backed off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sweep skipped its work when the wallet had scanned past `through`, but still called finish_dynamic_nullifier_recovery with that stale point. The trait requires `through` to be a canonical scanned tip, so a backend without SQLite's defensive tip check could release evidence a later sweep still needs. Put the release under the same tip guard as the sweep work. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each account's due batch is prepared before any key is leased, so an account listed twice got the same batch twice: every key was leased and looked up twice, and a deferred key was backed off twice and listed twice. Deduplicate the accounts, keeping first-seen order, before preparing work. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b65a4b8f59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if current { | ||
| lock.write("dynamic_sweep.release", || { | ||
| wallet.finish_dynamic_nullifier_recovery(*account, through) | ||
| }) |
There was a problem hiding this comment.
When another wallet writer advances or reorgs the chain during the directory or Enhance awaits, current still reflects the value sampled before that asynchronous work, so this calls finish_dynamic_nullifier_recovery with a point that is no longer the canonical scanned tip. Unlike the previously reported stale-at-entry case, the fresh evidence is that WriteLock is released across those awaits, allowing another database handle to change the tip before this block runs. The trait explicitly permits backends to rely on the canonical-tip precondition, so a non-SQLite implementation can prematurely discard spend evidence; revalidate the full chain point inside the release lock and cover this writer interaction.
AGENTS.md reference: AGENTS.md:L43-L46
Useful? React with 👍 / 👎.
1 participant
zakura-pir-receiver runs restore sweeps of dynamic keys. It accepts a receiver directory publication only at a block of the wallet's own chain, tests the wallet's receivers against the publication's labeled filter sets, which every wallet downloads alike, and looks up only those in the paid set over PIR, fetching their note data through Enhance PIR. Results go through the sweep steps of the backend's dynamic IVK traits, so the client works with any store that implements them; SQLite is only a test dependency.
A provider's recent set keeps a receiver it was given lately scanning, and its seen set marks a receiver it ever had so issuance skips it. The wallet trusts a recent set only when it is current, its feed having read the provider within the last fifteen minutes, and its declared window covers the wallet's own 24-hour restore watch with the feed running throughout; otherwise every restored key keeps scanning for the watch. A wallet with nothing paid makes no PIR query. When the directory cannot be accepted, every due key backs off as if its own lookup had failed.
The receiver crates come from wallet-pir's main, pinned at 9d2cbda0, the revision transparent PIR also uses. The crate has its own CI lane and stays out of default; the graph check builds it without the wallet feature.
Tests: the library CI matrix (fmt, every
scripts/dev.pylane and the three verify scripts) passes at the top of the stack, and the lanes this PR affects pass at this PR.🤖 Generated with Claude Code