Repository navigation
feat(platform-wallet): send, receive and accept DashPay private contact requests - #5810
QuantumExplorer wants to merge 134 commits into
Conversation
The prepared-keys test also opens an entry with the first key of the set, so a scan skipping earlier keys would fail it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The SDK can now append entries to a private document type's store, paid from the shielded pool (type 29), and find a wallet's entries in it: - `PrivateDocumentCreateFromShieldedPool` builds, structure-checks and broadcasts the transition from a proven bundle, and waits for its nullifiers to be proven spent. - `PrivateDocumentScanKeys` and `Sdk::scan_private_document_page` sort a proved page of a store into what the wallet's identities received (their keys open it), what they sent (their outgoing viewing keys recover it) and what opened but is refused, with the cursor to scan from next. - DashPay: `PrivateContactRequest` with the schema's bounds, `seal_private_contact_request` (the sender's encryption key, a deniable MAC, and the outgoing viewing key derived from that key), and `Sdk::scan_private_contact_requests`, which decodes each request, checks its sender against the identity key it names and keeps the first request per sender, recipient and account reference. platform-encryption: prepared recipient keys check a sender with the key that opened the entry, so a scan never copies secret keys out. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A wallet restoring its sent requests needs the encryption key each was sealed with to rebuild its local record of the request. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…requests The wallet sends, reads and accepts DashPay private contact requests (protocol version 15), paying for each from the shielded pool: - `PlatformWallet::send_private_contact_request` builds the request a `contactRequest` document would carry (receiving xpub, masked account reference, label, auto-accept proof) plus a shielded address, has the crypto provider seal it, and appends it with a type 29 transition paid from an account's notes (new `ShieldedFeeKind::PrivateDocumentCreate`, reserved for the fee alone). - `DashPayView::sync_private_contact_requests` reads DashPay's private store from each identity's cursor: received requests with their senders checked, and sent ones recovered from the store alone. - `accept_private_contact_request` replies privately. The public accept refuses a request that came privately (`PrivateContactRequestNeedsPrivateReply`), as a document reply would publish the relationship. A private request is recorded as a `ContactRequest`, its xpub and label encrypted under the pair's DIP-15 shared key with IVs from the entry's commitment, so contacts establish, rotate and get their accounts as they do from documents, and nothing persisted changes shape. Whether a request came privately and the contact's shielded address are kept in memory (`PrivateContact`), with the store cursor, and rebuilt by the scan a cold start runs. Requests that came privately are not auto-accepted. `ContactCryptoProvider` gains `scan_private_document_page`, `read_private_contact_requests` and `seal_private_contact_request`, all defaulted to unsupported, so the keys stay in the signer. The document send and accept share their preparation and accept steps with the private ones, and the document sync's stale-account teardown moved into `tear_down_stale_external_accounts`, which both syncs call. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A private contact request carried a deniable MAC keyed by the static ECDH of the sender's encryption key and the recipient's key. Anyone holding the recipient's key could compute it, so a stolen recipient key forged requests from any sender, including rotations that redirect the recipient's payments. The request is now signed by one of the sender's signing keys, as its public request's transition is: an enabled AUTHENTICATION key at a level DashPay's documents are signed at (CRITICAL or HIGH), ECDSA_SECP256K1 or ECDSA_HASH160. The entry still names the encryption key, which the pair's shared key and the outgoing viewing key come from; the reader recovers the signing key from the signature. Entries with a MAC are refused. Envelope (signature mode, nothing deployed): the signature now signs the double SHA-256 of the signing data, as identity signers sign data, so a wallet signs with the signer it signs transitions with. The caller passes the signature; `private_document_signing_data`, `sign_private_document_signing_data`, `recover_private_document_signer` and `OpenedPrivateDocumentEntry::recover_signer` are new. Prepared recipient keys are wiped on drop. Reading also checks, at the entry's block time, the recipient key it is encrypted to, and that a sent entry names the identity and key its outgoing viewing key comes from and is signed by that identity. The sender is checked before the payload is decoded. `PrivateDocumentScanKeys::verify_sender` tries every key set of the identity. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tform-wallet-private-contact-requests
…vate across restarts - Private requests are signed by the identity signer, with a signing key of the sender at a level DashPay's documents are signed at, as public requests are. `send_private_contact_request` and `accept_private_contact_request` take the signer. The crypto provider seals with the signature and keeps the encryption key; `read_private_contact_requests` is gone, as checking an entry needs no secret key. - A private request is recorded at core height 0, which no contactRequest document has (DashPay requires `$createdAtCoreBlockHeight`). `ContactRequest::came_privately` and `DashPayState::private_contact` derive from the records, so the privacy of a contact holds after a restart; only the shielded addresses stay in memory, noted from the tracked request. - A public send to a contact whose requests came or went privately is refused, as is a public accept that would broadcast a reply. - Ingest passes over a request older than the tracked one (by its reference's version, then its time) on both sides, so a cold start's rescan of the private store, or a replayed entry, does not roll a contact back to a retired xpub. - Sending computes the shared key before paying, and nothing after paying fails the send; a record that does not reach disk is recovered by the next sync. Sent private records note core height 0 for the receiving account's scan checkpoint. - A sync whose ingest did not reach disk for an identity is not complete. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
Walkthrough
Pre-merge checks:
|
|
⛔ Final review complete — 3 blocking finding(s) (commit 8ede791) · triage: critical |
Review fixes: - The envelope's authenticated data names the recipient key `ser(R)`, so a body re-encrypted to another key of the same identity (an old key's holder re-targeting a signed request) authenticates no sender. `private_document_signing_data` takes the recipient. The MAC and signature vectors are re-pinned; nothing is deployed. - A signing key bound to a contract signs only inside its bounds: DashPay, or its `privateContactRequest` type. A contract group bound signs none, as a client cannot read the group's members as they were. The seal and the reader apply the same rule; `private_contact_request_signing_keys` lists the keys that qualify. - `PrivateContactRequest::from_properties` drops a null only under an optional property's name, so an unknown property set to null is refused. - Recovered sent requests are deduplicated per sender, recipient and account reference, apart from the received ones. - Senders added with the same outgoing viewing key: an entry recovers as the one it names. - Docs: the transition's proof shows the nullifiers spent, not that its entries were appended; fetching a sender right after its page tells the serving node which identity the wallet looked up. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tform-wallet-private-contact-requests
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tform-wallet-private-contact-requests
…lows The private send picks the first signing key the signer holds among `private_contact_request_signing_keys`, which adds the contract bounds rule of #5809 to the level and type rules: a key bound to another contract or document type, or to a contract group, is never chosen, as the seal would refuse it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
`accept_contact_request_replying_in_kind` (on `DashPayView`, and on `PlatformWallet` with the `shielded` feature) is the one accept a host calls: a request that came privately, with no reply sent yet, is accepted with a private reply paid from the shielded pool; any other is accepted with a document, or by adopting the reply already sent. The choice was the hosts' (catch code 68, then call the private accept); it is the wallet's now. `PlatformWallet` derives the spend authority only for a private reply. The explicitly public and private accepts are unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…y heights Review fixes: - Requests are checked against their type's schema with dpp's document validation (`DataContract::validate_document_properties`), before encoding when sealing and after decoding when reading, as the payload codec asks. The SDK's own copies of the label and proof bounds are gone; `to_properties` is a typed conversion. The SDK enables dpp's `validation` feature. `seal_private_contact_request` and `private_contact_request_signing_data` take `platform_version` last. - The reader takes each identity with the height its proof is of (`ProvedIdentity`). `private_contact_request_identities` lists, per identity, the latest block whose entry it checks; a snapshot proved before it, which a node may serve within the SDK's height tolerance, or one not given, fails the read instead of judging the entry against keys as they were before it. Only signed entries need identities. - `Sdk::fetch_private_contact_request_identities` fetches them four at a time with their metadata and fetches a stale one once more. - Both SDK helpers establish protocol version 15 before resolving DashPay, so an unpinned SDK starting below it does not read a DashPay without the private type. - `PrivateDocumentScanKeys::derive` builds scan keys from key paths with a derivation callback, wiping what it derives, for signers that keep keys. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tform-wallet-private-contact-requests
Review fixes (with the SDK changes of #5809 merged in): - A private request whose store fails after it was paid for is kept in memory (`add_paid_sent_contact_request`): the contact it answers establishes, so a retried accept adopts it instead of paying for another reply, and the next add of the same request (the private sync recovers it from the store) stores it. Before, the failed store left no request and no contact, so the accept failed and a retry paid again. - `PlatformWallet::accept_contact_request_replying_in_kind` takes what a private reply is paid with as `payment`, a closure called only for a private reply (`PrivateReplyPayment`: coordinator, seed, account, prover). A public reply or an adopted one needs neither shielded support nor the seed. - The private sync checks each page against identities proved at or after its entries (`ProvedIdentity`), fetching a few at a time only those it lacks or holds from an older proof. - Scan keys are built with the SDK's `PrivateDocumentScanKeys::derive`; the crypto provider's seal takes `platform_version`, which the SDK's schema validation needs. Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of head 7bd302a confirms five blocking issues involving publication confirmation, duplicate private-reply payments, rotation selection, cursor rewinds, and stale recipient identities. Additional suggestions address persistence recovery, failure isolation, entropy errors, and the public-send privacy precondition. Current CI shows Kotlin build/tests and policy checks passing, with the automated review pending and no targeted Rust validation check visible; no builds or tests were run.
🔴 5 blocking | 🟡 4 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate change directly modifies shielded funds movement and note selection in shielded/operations.rs and shielded/note_selection.rs, as well as signing-key selection and encryption-key handling in private_contact_requests.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1678-1686: Verify entry inclusion before recording a private request as sent
This branch accepts an AffectedState result and returns success to send_private_contact_request, which records the outgoing request and can establish the contact. However, the SDK explicitly documents that type 29 proves only spent nullifiers, and Drive verifies only those nullifiers and classifies the result as non-execution-proving. With two real input notes, a competing spend by another device can consume the same nullifiers without appending this entry; an untrusted DAPI can then supply a genuine spent-nullifier snapshot for a request that was never published. Subsequent accepts adopt the nonexistent reciprocal instead of sending one. Keep consumed-note reconciliation separate from publication confirmation, and require a proof-verified private-store read locating the exact sealed entry before recording the request as delivered. If inclusion remains unknown, retain a typed unresolved outcome rather than returning publication success.
In `packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:315-320: Retain and serialize pending private replies before paying
When pay returns ShieldedSpendUnconfirmed, this function exits without associating an outgoing request with the contact, while the shielded operation retains reserved notes and a byte-identical redrive that may still execute. An accept retry before private sync therefore sees no reciprocal, seals another entry, and can pay again from other available notes. Concurrent accepts have the same gap because accept_contact_request_replying_with releases its state lock before invoking the reply closure. The private store is an opaque append-only log with no cross-transition uniqueness constraint on the contact pair or account reference, so both payments can execute. The existing unstored_sent handling protects only failures after payment has returned success; later sync recovery does not prevent these duplicate fees. Reserve a contact-scoped pending reply before payment, serialize the decision for that pair, and associate the sealed entry with its shielded operation so retries resume or report the existing attempt instead of funding another one.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:728-740: Apply version-first ordering before collapsing scan candidates
Both collapse helpers select by (created_at, account_reference), but the new predates guard selects freshness by (account_reference_version, created_at). The guard cannot recover a rotation already discarded during collapse. For example, after an earlier page established version 0, a later page can contain a legitimate version 1 rotation followed by a replay of the version 0 ciphertext. Both survive SDK filtering because their account references differ, and the replay receives the later append timestamp. Collapse keeps version 0, ingest skips its already-tracked reference, and the cursor advances past version 1. This affects both received requests and recovered sent requests, leaving retired payment material or an obsolete outgoing rotation version tracked. Use one version-first ordering for collapse and supersession, and test an earlier-page original followed by a same-page rotation and replay in both directions.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:747-752: Preserve a concurrent cursor rewind across the entire scan pass
The compare-and-advance helper correctly refuses to overwrite a concurrent unignore rewind, but the assignment immediately afterward adopts the rewound cursor as this pass's new baseline. If a pass starts at position 100 and unignore_sender resets the live cursor to 0 while the first page is fetched, the first advance is refused and identity.position becomes 0. The next page is still fetched beyond 100 using read.next_position, yet its advancement now compares successfully against 0 and overwrites the rewind. The earlier ignored entries were never reread, so the newly unignored sender can remain missing for the session. Once a cursor mismatch is detected, stop advancing that identity for the rest of the pass or restart its scan at the rewind. Add a two-page regression with unignore occurring during the first fetch.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:599-602: Height-pin recipient identities used for outgoing recovery
Unlike proved_identities, this recipient cache discards proof metadata and never refreshes an identity during the pass. A later page can contain a request sealed to a key added after the cached recipient snapshot; even the initial fetch can return a proof older than sent.block_height within the SDK's tolerance. The lookup below then treats the stale snapshot as evidence that the recipient does not hold the key, skips the valid outgoing request, and advances the sender's cursor past it. A restored request or rotation is consequently lost for the running session. Fetch these recipients with the existing height-aware SDK mechanism, requiring proof height at least sent.block_height and refreshing when later pages require it. Failure to obtain a sufficiently recent snapshot must preserve the affected sender's cursor rather than become a permanent skip.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:599-602: Isolate recipient-fetch failures without advancing affected senders
An error fetching any recovered outgoing recipient returns from the whole sync before the current page's unrelated incoming or outgoing requests can be ingested. Earlier pages remain committed, but one failing recovery lookup prevents progress for every identity on this page. The public sweep already supports per-identity degradation. Apply equivalent isolation here: report the failure, hold back cursors for identities whose outgoing records could not be reconstructed, and continue ingesting unaffected identities. Do not simply convert fetch errors to None and advance normally, because that would permanently skip recoverable outgoing requests for the session.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:286-287: Return entropy failures through the send result
StdRng::from_os_rng panics when the operating system cannot supply entropy; rand_core explicitly provides try_from_os_rng for callers that must handle this failure. This introduces a panic into a fallible wallet-library send operation and can terminate an embedding application rather than returning PlatformWalletError. Seed through try_from_os_rng and map the error before filling esk_seed, retaining the existing zeroization after sealing.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1305-1311: Let same-reference recovery retry an unstored paid request
A freshly paid request uses the confirmation proof's metadata.time_ms, whereas scan recovery uses the entry's block_time_ms. GetProofs proves current state, so confirmation can occur in a later block than the append. If initial persistence failed, add_paid_sent_contact_request keeps that later-timestamped request in memory and marks it unstored. This guard then rejects recovery of the identical account reference as older, preventing add_sent_contact_request from reaching its same-reference persistence retry. Sync can advance and report completion while the request remains unstored until a cold rescan. Exempt the same account reference from the freshness rejection so the existing retry persists the tracked record without replacing metadata. Cover a later confirmation timestamp and a failing-then-successful persister.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:583-594: Document or enforce the privacy gate's synchronization precondition
This gate checks only locally tracked requests. A restored or second device that has not completed private sync has no private records for the pair, so it can publish a public contactRequest even though the relationship was previously private. There is no private-sync readiness check in this send path, and its API documentation does not state that the privacy guarantee depends on synchronization order. Document that limitation and the required completed scan before public sends on restored state, or enforce an explicit readiness policy in Rust. The local gate alone does not establish the PR's unconditional claim that contacts exchanged privately are never sent public requests.
…he private type Review fixes: - A signing key past its `expires_at` signs nothing: the reader checks it at the entry's block time with dpp's `is_expired_at` (the expiry instant is expired), and the key list and the seal check it at this device's clock, so an expired key is not offered. - `private_document_create_from_shielded_pool` establishes protocol version 15 before building the transition, which an older SDK's limits would refuse for any entries. - A DashPay the context provider holds from before the private type is refetched from the network and registered with the provider, which the page's proof is verified against. - The reader refuses an identity given under another identity's id, and a recipient key id that is not the key the entry is encrypted to. - The identity fetch iterates owned pairs, so its future is `Send` (rust-lang/rust#100013), and its retry logic is a function tests drive with scripted proofs. - An OS RNG failure is an error, not a panic. - The wasm-sdk contract fixture gives its indexed `rarity` string the `maxLength` dpp's now enabled validation requires of an indexed string. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tform-wallet-private-contact-requests
…ne at a time Review fixes (with the SDK changes of #5809 merged in): - A paid private request is recorded only as the store proves it: the type 29 proof shows the notes spent, not the entry appended, so a send reads the store back with a short private sync and returns the request recorded from it. Not read back yet, it returns the new `PrivateContactRequestPending`. - One private request to a recipient is in flight at a time: reserved before sealing, it keeps a retried or concurrent send or accept from paying again (`PrivateContactRequestPending`) while it is being paid for, while its payment is unconfirmed, and until a sync reads its entry back, which settles it. A payment that definitely failed releases it. Pending requests are kept in memory. - The keep-in-memory recording of a paid request whose store failed is gone: nothing is recorded outside the sync, whose failed ingest holds the identity's cursor while the request stays pending. - Collapsing a sweep's requests orders by the reference's version first, as passing over older requests does (`freshness`), so a rotation and a later replay of the request it rotated past collapse to the rotation. - A cursor rewound under a pass (unignoring a sender) is not advanced again that pass. - The recipients of recovered sent requests are proved at or after their entries, each fetched on its own: one that cannot be read holds back its senders' cursors rather than failing the pass or skipping the request. - An OS RNG failure is an error, not a panic. - The public send documents that its privacy refusal needs the private sync to have read the store on a restored device. Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the exact head d0db59f against the source: eight prior findings are fixed, but pending-operation lifecycle gaps still permit duplicate payments or stranded reservations. The public-send gate also overlooks unresolved private attempts, and exhausted rotation versions can consume fees without producing a usable request. This was a static review with no local builds or tests; current head-specific CI shows the Kotlin build/tests passing and no failed code checks, with the review check still pending.
🔴 3 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Reject exhausted rotation versions before funding a private send
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:926-935
This helper now prepares private sends as well as public documents, but saturation still relies on the public document's unique index to reject a repeated reference. With a tracked outgoing version of 15 and unchanged encryption key and receiving xpub, another private send derives the same account reference and pays for a fresh sealed entry. The append-only private store accepts it, while received ingest and add_sent_contact_request treat the reference as already tracked and ignore the new request. The sender's readback check then fails because the fresh commitment produces different encrypted-xpub bytes, so the operation spends its fee but returns pending without applying the request. Sync can clear the reservation, allowing further charged retries. Reject version exhaustion before sealing/payment, or implement a supported supersession scheme that does not depend on the public index.
source: gpt-6.1-sol (phase2-reviewer: general)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate change directly modifies shielded funds movement and note selection in shielded/operations.rs and shielded/note_selection.rs, and signing and encryption-key handling in network/private_contact_requests.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:591-601: Include unresolved private sends in the public-send privacy gate
This gate checks only recorded incoming/outgoing requests through `private_contact`; it does not check `private_reply_pending`. On a device whose initial private sync is already complete, a first private send can return `ShieldedSpendUnconfirmed` or `PrivateContactRequestPending` while retaining its commitment but creating no outgoing record. A subsequent public send passes this gate and publishes both identities even though the private transition may already have executed or may still be published by redrive. This is separate from the documented restored-device synchronization precondition: scanning cannot recover an entry that has not executed yet, and this wallet already knows about the attempt. Make public-channel eligibility include unresolved private operations for the pair, retaining the refusal until definitive failure or reconciliation.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:926-935: Reject exhausted rotation versions before funding a private send
This helper now prepares private sends as well as public documents, but saturation still relies on the public document's unique index to reject a repeated reference. With a tracked outgoing version of 15 and unchanged encryption key and receiving xpub, another private send derives the same account reference and pays for a fresh sealed entry. The append-only private store accepts it, while received ingest and `add_sent_contact_request` treat the reference as already tracked and ignore the new request. The sender's readback check then fails because the fresh commitment produces different encrypted-xpub bytes, so the operation spends its fee but returns pending without applying the request. Sync can clear the reservation, allowing further charged retries. Reject version exhaustion before sealing/payment, or implement a supported supersession scheme that does not depend on the public index.
In `packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:888-889: Report committed requests rather than pre-ingest candidates
`PrivateContactSyncReport::requests` is documented as requests ingested, but these extensions run before collapsing candidates, filtering ignored senders, rejecting older requests, or attempting persistence. An ignored request or a request whose persistence failed is therefore returned as ingested even though it never became wallet state. The received helper already returns successful ingestions through `ingested`, but that vector is discarded here. Populate the report from successful ingestions and add equivalent sent-side tracking; cover ignored senders and a failing persister so callers can distinguish fetched candidates from committed state.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:260-264: Retain and serialize pending private replies before paying
(existing thread: https://github.com/dashpay/platform/pull/5810#discussion_r4506856812)
The pre-seal reservation closes the straightforward overlapping-call case, but it is not owned and reconciled with the payment throughout its lifecycle. Four verified paths remain:
- Restart loses `pending_private_replies`, while `FileBackedShieldedStore::rehydrate_pending_spends` restores the original signed transition and its note reservations. If the original request has not landed, even a completed private scan finds no reciprocal. Retrying accept can fund another entry from other notes while the original remains executable or is redriven. GroveDB's private store appends opaque entries without contact-pair/reference uniqueness, so both fees can execute.
- Lines 912–920 settle every matching item in `read.sent`, including entries omitted from `records` because the recipient fetch failed at lines 799–801. Both ingest helpers can succeed on empty inputs, removing the reservation without storing an outgoing request. The send returns pending, but an accept retry can pay again.
- After sync settles attempt A, it awaits account building, and A subsequently awaits account registration. Attempt B can reserve the same pair during that interval. A's recipient-only cleanup at lines 288–292 then removes B's reservation, admitting another payment while B is unresolved.
- Dropping the send future during preparation bypasses cleanup. Cancellation before `note_private_reply_commitment` leaves a `None` reservation that no store scan can settle, blocking further sends and accepts for that pair until the wallet state is recreated.
Use an attempt-owned lifecycle in the Rust wallet: retain the pair-to-attempt association durably before submission can occur, compare attempt identities when updating or releasing reservations, release abandoned pre-payment preparation safely, and settle only after the corresponding request has been durably reconciled or the payment is proven unable to execute. Add regressions for reopening with an unresolved redrive, recipient-fetch failure after payment, A-settles/B-reserves/A-cleans-up, and cancellation during preparation.
… the refreshed DashPay A private contact request's encryption keys, the sender's and the recipient's, are now used only inside their contract bounds: unbound, bound to DashPay, or bound to DashPay's contactRequest or privateContactRequest. contactRequest is allowed because DashPay requires bound keys for it and apps register their DashPay keys bound to it. Keys bound to another contract, another DashPay type or a contract group are refused by the seal and the reader. dpp decides the document-type scope once, in ContractBounds::check_document_type (Allowed, Denied, or needing the group's members, which a client refuses). The signing key check and encrypted_for's key check use it too. After registering a refreshed DashPay, the helper reads it back from the context provider and fails if the provider still lacks the private type, as every page's proof would. The reader's docs no longer claim a key was the identity's when the entry was appended. Tests: encryption key bounds for both roles in the seal and the reader; the DashPay refresh on an unpinned SDK, the already-current shortcut, and a refresh the provider does not take; a send on an unpinned SDK learns the network's version before it builds. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tform-wallet-private-contact-requests
…pending, and report only recorded requests A public `contactRequest` to a recipient a private request is still pending to is refused with `PrivateContactRequestPending`: that private request may already be on chain, and the document would publish the pair. `PrivateContactSyncReport::requests` now lists the requests the pass recorded, received and sent, instead of every candidate the page opened: `ingest_sent_requests` reports what it recorded, as the received ingest already did, so an ignored sender's request, a replay older than the tracked one, one already tracked, or one whose store failed is not reported. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…, and restore it after a restart The reservation that keeps a second private request to a recipient from being paid for, and a public one from publishing the pair, had four gaps. - Attempt tokens: a reservation is a `PendingPrivateReply` owned by the attempt that made it. Only that attempt notes its commitment or releases it, so an attempt whose request was settled can no longer release the reservation of the next attempt to the same recipient. - Cancellation: the send holds the reservation in a guard that releases it when the send is dropped before it pays (synchronously when the wallet lock is free, otherwise on a spawned task). It is kept once payment may have started. - Settle only what was reconciled: the private sync settles a pending request only for a sent entry whose record it built and that the tracked request to the recipient now matches (by account reference, or by being newer), after both ingests succeeded. An entry whose recipient could not be read no longer settles it with nothing recorded. - Restart: reservations are in memory, but an unconfirmed payment survives a restart as the shielded store's redrive record. `PlatformWallet::restore_pending_private_contact_requests` reads the type-29 transitions to DashPay's store from those records and reserves each recipient again under its entry's commitment, so a sync settles it. The private payment runs the same check right before it spends, and refuses with `PrivateContactRequestPending` when an unresolved request between the same pair is found, leaving the recipient reserved for that one. A rotation still in flight is not restored. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tract in a proof plan test #5774 added proof_plan_equivalence_tests.rs calling add_document_for_contract with the argument #5798 removed, so drive's tests did not compile on v5.1-dev. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
…lded store An identical rebind returns without waiting for a store a scan holds: the wallet is held until the restoration it defers runs once the store frees, and the private payment's pair stays held throughout. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification at head 1b3a37b confirms four blocking issues: unauthenticated rejection evidence can discharge privacy recovery, public sends can outlive wallet replacement, stale restoration can resurrect discharged reservations, and provably expired payments retain permanent contact holds. Most prior durability, cancellation, recovery-corruption, and scanning findings are fixed; one additional suggestion concerns duplicated transport retry policy. Current CI reports successful Rust workspace, Swift, and Kotlin checks, a skipped dedicated Rust wallet job, and pending review/hygiene checks; no local builds or tests were run.
🔴 4 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Release private reservations after terminal shielded failures
packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1815-1819
Keeping a hold after a relay refusal is correct, but this proof-backed terminal cleanup also leaves the contact reserved forever. For an unconfirmed private payment that never executes, the pre-scan anchor query can prove its anchor pruned, and the completed note reconciliation can leave an input unspent. This branch then releases the notes and marks the activity Failed. However, clear_pending deliberately preserves the private-payment journal, and no corresponding path discharges that journal or releases the commitment-owned contact reservation. The only discharge paths are entry read-back and an initial never-delivered outcome; this expired payment can never append an entry to read back. Subsequent sends remain pending, and every restart restores the same hold. The journal also omits the anchor, while cleanup deletes the redrive containing it, losing evidence needed for later settlement. Complete terminal reconciliation in the shared Rust shielded owner: retain the required anchor/finality evidence, durably discharge a payment when verified pruning plus subsequent unspent-input reconciliation establishes non-execution, and release its matching contact reservation without treating a relay refusal or missing redrive as proof. Cover expiry without publication followed by a successful retry, including reopening the store.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate change directly modifies shielded funds spending and note selection in packages/rs-platform-wallet/src/wallet/shielded/operations.rs and note_selection.rs, as well as private-request signatures and key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/broadcast_outcome.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/broadcast_outcome.rs:197-201: Do not discharge privacy recovery on an unauthenticated DAPI verdict
A consensus-error-shaped response does not prove non-delivery. In rs-sdk/src/error.rs, From<DapiClientError> deserializes endpoint-supplied dash-serialized-consensus-error-bin metadata without verifying an execution proof or quorum signature. A malicious DAPI endpoint can retain or forward the signed type-29 transition and return valid serialized consensus-error metadata on the first dispatch. This branch then returns NeverDelivered. settle_private_document_payment consequently discharges the journal, cancels note recovery, and marks the payment safe for the outer send to release its contact reservation and unresolved marker. A subsequent public request can expose the identity pair, or a private retry can fund another request, while the endpoint still holds an executable original transition. Restrict never-delivered classification to demonstrable pre-dispatch failures; remote rejection metadata must retain uncertainty until proof-verified reconciliation establishes publication or non-execution. Add a regression in which an endpoint retains the submitted bytes but responds with serialized consensus-error metadata, and verify that public disclosure and replacement funding remain blocked.
- [SUGGESTION] packages/rs-platform-wallet/src/broadcast_outcome.rs:159-166: Preserve SDK retry policy when tracking cumulative delivery
This disables the SDK's retry loop and replaces it with a fixed six-dispatch wallet loop. The SDK already owns configured overall retry budgets and address/error aggregation in platform/transition/broadcast.rs and sync.rs, so a client configured with zero retries can now make additional wallet-level submissions after a transport error. The tracker is transport-generic rather than wallet-specific, and direct SDK consumers still lack its cumulative evidence. Centralization is already tracked by dashpay/platform#5556; use cumulative delivery evidence from that owner for wallet reservation decisions. Until that integration lands, preserve the configured overall retry budget instead of imposing six dispatches here. Journal, contact, and note-settlement policy should remain in platform-wallet.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:2973-2982: Release private reservations after terminal shielded failures
Journal restoration no longer has the completed-commitment check that protected the restore race described in your follow-up. It snapshots payment R under the store lock, releases that lock, and then awaits the wallet lock. Restoration and payment hold shared lifecycle read guards, so an initial never-delivered payment can discharge R and finish releasing its owning contact reservation before this stale restoration resumes. restore_journaled_private_payments then inserts R again without checking whether it was discharged. No published entry exists to settle that reservation, and a later restoration with an empty journal does not remove existing pending_private_replies, leaving the pair blocked for the session. Preserve a discharge/completion generation or tombstone checked when applying restoration, or serialize journal snapshot/application against discharge and reservation release. Add a regression that pauses restoration after its journal snapshot, discharges and releases the payment, then resumes restoration and successfully retries the pair.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1815-1819: Release private reservations after terminal shielded failures
Keeping a hold after a relay refusal is correct, but this proof-backed terminal cleanup also leaves the contact reserved forever. For an unconfirmed private payment that never executes, the pre-scan anchor query can prove its anchor pruned, and the completed note reconciliation can leave an input unspent. This branch then releases the notes and marks the activity Failed. However, clear_pending deliberately preserves the private-payment journal, and no corresponding path discharges that journal or releases the commitment-owned contact reservation. The only discharge paths are entry read-back and an initial never-delivered outcome; this expired payment can never append an entry to read back. Subsequent sends remain pending, and every restart restores the same hold. The journal also omits the anchor, while cleanup deletes the redrive containing it, losing evidence needed for later settlement. Complete terminal reconciliation in the shared Rust shielded owner: retain the required anchor/finality evidence, durably discharge a payment when verified pruning plus subsequent unspent-input reconciliation establishes non-execution, and release its matching contact reservation without treating a relay refusal or missing redrive as proof. Cover expiry without publication followed by a successful retry, including reopening the store.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:773-782: Serialize public sends with private channel eligibility
(existing thread: https://github.com/dashpay/platform/pull/5810#discussion_r4507940815)
Attempt-owned cleanup protects a replacement claim, but it does not prevent the old public operation from submitting after its claim's wallet disappears. Pause this future during preparation, ECDH, or signing after it claims the pair, remove the wallet, and recreate it under the same seed-derived ID. Removal discards the managed identity containing the claim, allowing the replacement wallet to reserve the pair and send privately. Resuming the old future still reaches sdk_writer.send_contact_request: neither this path nor SdkWriter checks the original WalletGeneration or holds its lifecycle exclusion. The subsequent ID-keyed bookkeeping can also modify the replacement wallet. The replacement regression exercises dropping the old claim, not resuming its submission. Bind public submission to its original generation, acquire the existing native lifecycle guard before channel eligibility, validate generation liveness, and retain the guard through submission and bookkeeping. Add a regression that resumes an old public send after removal/recreation and proves that it cannot publish or mutate the replacement.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
…pend never left the wallet The SDK decodes a consensus error from an endpoint's dash-serialized-consensus-error-bin metadata with no proof or quorum signature behind it. A malicious endpoint could keep or forward a signed private contact request payment and answer with a valid-looking verdict: the first dispatch then counted as never delivered, which discharged the journal record, released the notes and freed the pair. - A broadcast is never delivered only when it was refused before anything was sent (no address, or the SDK's own refusal). Any endpoint answer, a consensus verdict included, keeps it maybe delivered: the spend waits for its result and stays unconfirmed, notes and journal record kept, until a read-back or a proven expiry resolves it. The same holds for a verdict on the result wait of every note spend, and of an identity create from the pool, which now falls through to its fetch-by-id fallback. - The tracker keeps the SDK's configured retry budget (one dispatch and its effective retries) instead of six dispatches, so a client configured with no retries dispatches once. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
…arged private payment again A restoration snapshots the private payment journal under the store lock, then waits for the wallet lock. A payment whose record was discharged and whose reservation was released in between (a never-delivered payment) was reserved again when the restoration applied, with no entry to ever settle it, blocking the pair for the session. The persister now keeps, for the session, the commitments whose journal records were discharged (never delivered, read back, and later proven expired). The restoration skips them under the wallet lock. The ones whose reservation is still held are released by the private send, the public claim and the private sync, which store the identity's marker again when it changed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
…eration A public send could pause after claiming its pair (preparation, ECDH or signing) while its wallet was removed and re-created under the same seed-derived id; resumed, it still reached the SDK writer, and its id-keyed bookkeeping could change the replacement wallet. The identity wallet now keeps the generation it belongs to. A public send takes that generation's lifecycle gate before deciding its channel, checks that the generation is still the live one, and holds the gate through the submission and its bookkeeping: a removal waits for it, and a send through a handle whose wallet was removed and re-created is refused with WalletRemovedBeforeSend before it decides anything. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
…free its pair A private payment that never executes kept its journal record, and so its pair held, forever: the stranded-release pass proved such a spend dead and freed its notes, but clear_pending keeps the journal, and the journal did not even keep the anchor the proof needs once the redrive was deleted. - The journal record now keeps the spend's anchor (format version 2; version 1 records, written by unreleased builds only, do not read back and keep their wallet held). - After the pass's spent-note reconcile, the coordinator discharges a record, durably, when its anchor is absent from the recorded set fetched before the pass, one of its input notes is still unspent, and its entry is absent from DashPay's private contact request store read raw after that. The anchor set is fetched whenever the journal holds records, not only while a reservation does. The discharge is noted on the wallet's persister, so the next private send, public claim or private sync releases the pair and stores its marker. A relay refusal or a missing redrive record is never taken as proof. - A never-landing payment's pair is therefore freed once its anchor ages out (shielded_anchor_retention_blocks 1000, pruned every shielded_anchor_pruning_interval 100). README and docs updated. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification at 8ede791 confirms three blocking recovery defects: expiry reconciliation can discharge executable or already-executed payments, and legacy restoration can recreate a discharged reservation. Of the 43 prior findings, 41 are fixed, one remains valid, and one retains its tracked host-mapping deferral; the expiry scan also has an avoidable network-wide memory cost. Current CI reports Rust workspace tests and Kotlin build/tests passing, with Swift build/tests still pending; no local builds or tests were run.
🔴 3 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in wallet/shielded/operations.rs and wallet/identity/network/private_contact_requests.rs directly alter shielded-note reservation and spending, payment recovery, request signing, and encryption-key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1910-1916: Snapshot expiry candidates before fetching their anchor evidence
This enumerates the live journal after the note scan but compares every record against the anchor set fetched before that scan. During a pass with an existing pending payment, the scan can add a newer recorded root to the local tree. A concurrent private send can then select that root using its own fresh anchor query and journal its payment before this helper acquires the store lock. The new anchor is absent from the older set because it is new, not pruned. While the payment awaits inclusion, its input remains unspent and the private store legitimately lacks its entry, so the remaining checks pass and durably discharge an executable payment. Consuming the discharge notice then permits replacement funding or a public request while the original transition can still execute. Snapshot eligible journal records before fetching the anchor set, as the stranded-reservation path does, ensure the anchor evidence covers those candidates, and reconcile only unchanged snapshot records. Add a barrier regression that journals a payment against a root discovered during the scan.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1935-1952: Height-pin the evidence used to discharge expired payments
Later wall-clock reads do not prove state at or after the anchor snapshot. The prefetch discards the anchor response height, the note scan has no floor tied to it, and private_document_pages uses ordinary Fetch and exposes no response metadata. The SDK defaults to accepting responses one block behind its highest observed height. A payment can execute in block H before run_block_proposal prunes its anchor at the end of that same block. An anchor proof from H combined with valid note and private-store snapshots from H-1 therefore shows an absent anchor, an apparently unspent input, and no entry. This code then deletes the journal and announces discharge even though the private relationship was published. If private-store readiness was already established, a subsequent public send can disclose the pair without discovering that entry. Preserve the authenticated anchor height and require the spent-note reconciliation and complete store-absence evidence to cover at least that height, failing closed otherwise. Keep height-aware pagination in the SDK and the discharge decision in the wallet. Cover execution in the pruning block with otherwise valid lagging responses.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1987-1995: Bound expiry-scan memory to candidate payments
The expiry scan retains the commitment of every entry in the global append-only private store, although the caller only needs membership results for its local expiry candidates. Expiring one local payment therefore allocates memory proportional to unrelated network activity instead of retaining the page stream's bounded-memory behavior. Pass the candidate commitment set into this helper and retain only matches while still completing the scan needed to establish absence. Add coverage with many irrelevant entries and a small candidate set.
In `packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:2219-2224: Release private reservations after terminal shielded failures
(existing thread: https://github.com/dashpay/platform/pull/5810#discussion_r4508317355)
Your retained session-discharge set protects restore_journaled_private_payments, but the second restoration path described in your reply no longer checks it. PlatformWallet::restore_pending_private_contact_requests snapshots redrive entries and then awaits crypto scanning through this helper. While scanning is suspended, the payment can be proven never delivered or expired, its journal discharged, and its reservation released. If another send or sync consumes the discharge notification before scanning resumes, this branch recreates the old reservation despite the session tombstone. Subsequent reconciliation has no notification left to apply, and no unpublished entry can settle the reservation, so public and private requests to the pair remain blocked for the session. Apply the same discharge check under the wallet lock before either restoring or retargeting here, preferably through a shared generation-fenced restoration path. Extend the stale-restoration regression through the public legacy API, consuming the discharge notification before resuming its scan.
| let Ok(records) = store.private_payment_journal(*wallet_id) else { | ||
| continue; | ||
| }; | ||
| for record in records { | ||
| if recorded.contains(&record.anchor) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Snapshot expiry candidates before fetching their anchor evidence
This enumerates the live journal after the note scan but compares every record against the anchor set fetched before that scan. During a pass with an existing pending payment, the scan can add a newer recorded root to the local tree. A concurrent private send can then select that root using its own fresh anchor query and journal its payment before this helper acquires the store lock. The new anchor is absent from the older set because it is new, not pruned. While the payment awaits inclusion, its input remains unspent and the private store legitimately lacks its entry, so the remaining checks pass and durably discharge an executable payment. Consuming the discharge notice then permits replacement funding or a public request while the original transition can still execute. Snapshot eligible journal records before fetching the anchor set, as the stranded-reservation path does, ensure the anchor evidence covers those candidates, and reconcile only unchanged snapshot records. Add a barrier regression that journals a payment against a root discovered during the scan.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
| let appended = match self.private_contact_request_commitments().await { | ||
| Ok(appended) => appended, | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| %error, | ||
| "private payment expiry: the private contact request store was not read; \ | ||
| retried next pass" | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
| let persisters = self.persisters.read().await; | ||
| let mut store = self.store.write().await; | ||
| for (wallet_id, record) in candidates { | ||
| if appended.contains(&record.commitment) { | ||
| continue; | ||
| } | ||
| match store.discharge_private_payment(wallet_id, &record.commitment) { |
There was a problem hiding this comment.
🔴 Blocking: Height-pin the evidence used to discharge expired payments
Later wall-clock reads do not prove state at or after the anchor snapshot. The prefetch discards the anchor response height, the note scan has no floor tied to it, and private_document_pages uses ordinary Fetch and exposes no response metadata. The SDK defaults to accepting responses one block behind its highest observed height. A payment can execute in block H before run_block_proposal prunes its anchor at the end of that same block. An anchor proof from H combined with valid note and private-store snapshots from H-1 therefore shows an absent anchor, an apparently unspent input, and no entry. This code then deletes the journal and announces discharge even though the private relationship was published. If private-store readiness was already established, a subsequent public send can disclose the pair without discovering that entry. Preserve the authenticated anchor height and require the spent-note reconciliation and complete store-absence evidence to cover at least that height, failing closed otherwise. Keep height-aware pagination in the SDK and the discharge decision in the wallet. Cover execution in the pruning block with otherwise valid lagging responses.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
| let mut commitments = HashSet::new(); | ||
| while let Some(page) = pages.next().await { | ||
| for entry in page?.entries.entries { | ||
| if let Some(commitment) = entry | ||
| .data | ||
| .get(..32) | ||
| .and_then(|commitment| <[u8; 32]>::try_from(commitment).ok()) | ||
| { | ||
| commitments.insert(commitment); |
There was a problem hiding this comment.
🟡 Suggestion: Bound expiry-scan memory to candidate payments
The expiry scan retains the commitment of every entry in the global append-only private store, although the caller only needs membership results for its local expiry candidates. Expiring one local payment therefore allocates memory proportional to unrelated network activity instead of retaining the page stream's bounded-memory behavior. Pass the candidate commitment set into this helper and retain only matches while still completing the scan needed to establish absence. Add coverage with many irrelevant entries and a small candidate set.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
Basic explanation
What this does: the wallet can now send, receive and accept DashPay contact requests that nobody else can see.
Value: private requests make DashPay contacts without publishing who knows whom. Once received, a private request establishes a contact and gets its payment accounts exactly as a public one does, so payments, rotation and restore keep working.
Risks: low to medium.
Issue being fixed or feature implemented
Part of anonymous DashPay contact requests for 5.1. It builds on the SDK calls in #5809 (send, scan and read private contact requests). Sam's decisions applied here:
What was done?
Sending:
PlatformWallet::send_private_contact_request(coordinator, seed, account, sender, recipient, label, auto_accept_proof, shielded_address, signer, crypto, prover)DashPayView::prepare_contact_request, shared by both sends. A later request to the same recipient, either way, rotates past this one.signersigns the request with the first of the sender's keys it holds amongprivate_contact_request_signing_keys: enabledAUTHENTICATIONkeys at a level DashPay's documents are signed at (HIGHorCRITICAL), bound to no other contract or document type.private_document_create_from_pool_with_metadataappends the entry with a type 29 transition paid from the account's notes:ShieldedFeeKind::PrivateDocumentCreate { entry_count, entry_size };Receiving:
DashPayView::sync_private_contact_requests(crypto, max_pages)private_contact_requests_from_scan). That needs no secret key.How a private request is recorded: as a
ContactRequest.contactRequestdocument carries them, with IVs taken from the entry's commitment. So:contactRequestdocument to carry its Platform-assigned$createdAtCoreBlockHeight, and no Core chain puts that at 0. SoContactRequest::came_privately()reads the channel from the record itself, and it holds across restarts. Our receiving account for the contact is scanned from the wallet's birth.DashPayState::private_contact(contact)derivesreceived_privatelyandsent_privatelyfrom the tracked requests. Only the contact's shielded address stays in memory, noted from the request tracked from that contact and rebuilt by the scan a cold start runs.Accepting:
PlatformWallet::accept_private_contact_request(coordinator, seed, account, request, shielded_address, signer, crypto, prover)accept_contact_request_replying_with, shared by both accepts.Older requests are no rotation.
ingest_received_requestsandingest_sent_requestspass over a request older than the one tracked with the same contact. Older is compared by the account reference's version, which a rotation bumps, then by creation time. A cold start reads the private store from its start, and could otherwise replay a request that a public one already superseded. The same goes for an old request appended to the store again.Auto-accept. A request that came privately carries no auto-accept proof into the wallet. Accepting it pays from the shielded pool, which the unattended auto-accept drain cannot do, so it is accepted by hand.
Crypto provider.
ContactCryptoProvidergains two methods:scan_private_document_pageseal_private_contact_request, which takes the signature from the identity signerThey take Rust-built key paths (
PrivateDocumentKeyPaths), so the scalars never leave the signer. Both default to an "unsupported" error, so existing providers compile unchanged. The testSeedCryptoProviderimplements them. The Keychain-backed provider comes with the FFI PR.Unignoring a sender rewinds the private cursor, as it rewinds the document cursor, so the skipped requests are read again.
Refactor. The document sync's teardown of stale external accounts moved into
tear_down_stale_external_accounts, which both syncs call. The cursor advance moved ahead of it. That is behaviour-neutral, as all of it runs under one lock.Changes from review
accept_contact_request_replying_in_kind(onDashPayView, and onPlatformWalletwithshielded) replies the way a request came. It replies privately when the request came privately and no reply exists yet, and otherwise with a document or by adopting the reply already sent. Hosts no longer catch code 68 and choose.PlatformWallet's version takes what a private reply is paid with as a closure (PrivateReplyPayment), called only for a private reply, so a public reply needs neither shielded support nor the seed.PrivateContactRequestPending.PrivateContactRequestPendinginstead of paying again, while the payment is in flight or unconfirmed, and until a sync reads the entry back. A payment that definitely failed releases it.PlatformWallet::restore_pending_private_contact_requestsand right before every private payment, so no second request to that pair is paid for.PrivateContactRequestPending, since the private one may already be on chain.PrivateContactSyncReport::requestsholds what the ingests recorded. Not included: a request already tracked, an ignored sender's, a replay, or one whose store failed.How Has This Been Tested?
cargo test -p platform-wallet --features shielded --lib -- private_contact private_document_create contact dashpay older_request: 233 passed.should_send_read_and_accept_a_private_contact_request. Two wallets on mock SDKs:should_recover_a_sent_private_contact_request_from_the_store: a wallet restored from seed rebuilds its sent record byte for byte from the store alone.should_record_a_private_request_the_same_way_every_timean_older_request_than_the_tracked_one_is_no_rotation: replays and earlier requests of the same version are passed over on both sides, and a newer rotation is still taken.test_select_for_a_private_document_create_covers_its_fee_alone: the fee kind equals the consensus fee, 141,155,400 credits for one DashPay entry.cargo clippy -p platform-wallet --features shielded --all-targets -- -D warnings: clean.cargo check -p platform-wallet-ffi, with and without--features shielded.Breaking Changes
None for consensus. Two host-visible behaviour changes, both
PlatformWalletError::PrivateContactRequestNeedsPrivateReply: the public accept refuses a request that came privately, and the public send refuses a contact whose requests came or went privately. A host routes those to the private accept and send.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
8ede791/self-reviewedrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md,packages/rs-platform-wallet-ffi/src/contact_request.rs,packages/rs-platform-wallet-ffi/src/dashpay.rsand 2 more) — HashEngineering or ZocoLini or llbartekll or romchornyirs-platform-wallet(packages/rs-platform-wallet/Cargo.toml,packages/rs-platform-wallet/README.md,packages/rs-platform-wallet/src/broadcast_outcome.rsand 31 more) — HashEngineering or ZocoLini or llbartekll or romchornyirust-sdk-ffi(packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs) — lklimek or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit
New Features
Documentation