fix(platform-wallet)!: DashPay sync correctness, consensus entropy fix, and DIP-15 mobile interop (M1)#3841
Open
shumkov wants to merge 3 commits into
Open
fix(platform-wallet)!: DashPay sync correctness, consensus entropy fix, and DIP-15 mobile interop (M1)#3841shumkov wants to merge 3 commits into
shumkov wants to merge 3 commits into
Conversation
…earch Seven-agent reviewed spec for completing the full DashPay flow (sync, contact requests, payments, profiles) in the platform wallet + SwiftExampleApp: protocol reference (DIP-9/11/13/14/15), per-layer implementation inventory, 15 prioritized gaps (G1-G15), 5-milestone work plan, Swift UI design with normative interaction states, and a two-tier test plan aligned with the unmerged e2e framework (PR #3549). Backed by 6 source-cited research files, including the cross-client interop desk-check and an on-chain census of all 368 testnet contactRequest documents. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ompact xpub, key-purpose interop Three fixes to the rs-sdk/platform-encryption contact-request layer, each pinned red-to-green: 1. Entropy mismatch (consensus rejection). send_contact_request generated fresh entropy for broadcast while the document id was derived from the creation entropy; drive-abci recomputes the id from the broadcast entropy and rejected EVERY send with InvalidDocumentTransitionIdError. ContactRequestResult now carries the creation entropy and send reuses it. Test: contact_request_result_entropy_derives_returned_id (red: field inexpressible pre-fix; green after). 2. DIP-15 69-byte compact xpub wire format. We encrypted the 107-byte DIP-14 ExtendedPubKey::encode() form (failing our own 96-byte ciphertext check); DIP-15 and both reference mobile clients use fingerprint||chaincode||pubkey = 69 bytes. New compact_xpub_bytes/parse_compact_xpub codec in platform-encryption; the get_extended_public_key callback contract is now the 69-byte compact, validated before encryption. Test: test_encrypt_compact_xpub_is_exactly_96_bytes (+ round-trip and wrong-length rejection). 3. Key-purpose alignment with on-chain reality. Verified against all 368 testnet contactRequests: the dominant mobile cohort references an ENCRYPTION key for BOTH indices (mobile identities carry no DECRYPTION key). The recipient-key assertion now accepts DECRYPTION or ENCRYPTION. Test: recipient_key_purpose_accepts_decryption_and_encryption (red on DECRYPTION-only predicate; green after). BREAKING: the SDK-side get_extended_public_key callback must now return the 69-byte DIP-15 compact form (rs-sdk-ffi C ABI unchanged; caller doc contract tightened). Also enables dashcore/rand in platform-encryption dev-deps — the crate's tests previously failed to compile at all. dash-sdk: 139 lib tests green (mocks,offline-testing); platform-encryption 7/7; rs-sdk-ffi check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…blish/reconcile, account rebuild Milestone 1 of docs/dashpay/SPEC.md. Makes DashPay sync actually converge to a payable state, recurring, and restore-safe. Each behavior pinned red-to-green (see SPEC.md Part 5 M1 DONE notes for the full test list). - Recurring sync (G12): new DashPaySyncManager (modeled on PlatformAddressSyncManager) drives dashpay_sync() per wallet on the shared cadence/cancel/quiesce machinery — iterating the wallets map, NOT the token registry (which skips zero-token identities). Per-identity log-and-continue pushed into sync_contact_requests. Test: recurring_pass_syncs_every_wallet_including_zero_token_identities. - Establish via sync (G1a): the ingest guard dropped reciprocal requests whose sender we had already sent to — the offline-accept scenario could never establish. Guard relaxed; reciprocals now flow into auto-establish. - Sent-side reconcile (G13): sync now ingests our own on-platform sent requests (idempotent, metadata-preserving merge — naive re-establish wiped alias/note every sweep), and Accept adopts an existing reciprocal instead of re-broadcasting into the unique-index rejection that permanently bricked Accept after restore-from-seed. - Account rebuild sweep (G1b): every established contact missing accounts gets validate-key-indices -> decrypt -> register external account, plus the DashpayReceivingFunds account (previously only created on fresh send, so restore-from-seed left incoming payments invisible). Candidates collected under the write guard, registered after guard drop (tokio RwLock is non-reentrant). - Failure policy (G1c): transient failures retry next sweep; permanent decrypt/parse failures set the new EstablishedContact.payment_channel_broken flag (persisted; FFI accessor added) and stop retrying. Purpose-validation mismatches only log-and-skip. - Reject tombstone (G5 stage 1): rejected requests are tombstoned by (owner, sender, accountReference) — never bare sender, so a rotated request with a bumped accountReference still gets through. New rejected_contact_requests table + ContactChangeSet.rejected. - Receive-side compact xpub (G14): register_external_contact_account parses the 69-byte DIP-15 compact and reconstructs the contact xpub (address-equality pinned by reconstructed_xpub_derives_identical_addresses); legacy 78/107 fallback kept. - Key-purpose envelope (G15, verified on-chain): send prefers the recipient's DECRYPTION key and falls back to ENCRYPTION (mobile identities have no DECRYPTION key); validate_contact_request gains a recipient purpose gate (AUTHENTICATION was silently accepted before) and a purpose_mismatch classification. - Testability seam (G11): DashPaySdkWriter object-safe trait over the SDK write paths; fetch paths use the SDK's built-in mock. platform-wallet: 196 lib + 8 integration tests green (was 170); storage + FFI checks clean; FFI ABI extended by one accessor (established_contact_is_payment_channel_broken). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR delivers comprehensive DashPay correctness checkpoint work spanning protocol compliance, state management, background coordination, and lifecycle refactoring. It implements DIP-15 compact extended public key wire-format, rejected contact-request tombstones with broken payment-channel tracking, recurring sync coordination, and refactored contact request send/sync/accept/reject flows with stricter validation and testable seams. ChangesCryptography layer: Compact xpub and key-purpose validation
State model and storage: Rejected requests and broken channels
Recurring DashPay sync coordination
Contact request send/sync/accept/reject refactoring
DashPay specification and research documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Collaborator
|
✅ Review complete (commit 9f770b8) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (1)
806-875:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe accept-adopt check is only local, not platform-aware.
already_reciprocatedis derived from localsent_contact_requests/established_contacts, but the sync code above explicitly allows "received loaded, sent fetch failed" by logging and continuing. In that state the reciprocal already exists on Platform whilealready_reciprocatedis still false here, so this path retries the same(ownerId, toUserId, accountReference)write and gets the unique-index rejection instead of adopting. This needs a platform check here, or a duplicate-send fallback that switches to the adopt path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs` around lines 806 - 875, The local-only already_reciprocated check (variable already_reciprocated) can be stale; change the flow so before attempting send_contact_request_with_external_signer you either (A) perform a platform check for an existing reciprocal contact request/relationship (use whatever network client/query you have for checking platform contact requests for (ownerId,toUserId,accountReference)) and set already_reciprocated accordingly, or (B) keep the existing local check but add a duplicate-send fallback: catch the unique-index conflict/error returned by send_contact_request_with_external_signer and, on that specific error, log that the reciprocal exists on Platform and run the adopt path (call register_contact_account(&our_identity_id, &sender_id, 0) and treat as success). Reference already_reciprocated, send_contact_request_with_external_signer, and register_contact_account when implementing either fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dashpay/research/01-dip-spec.md`:
- Line 131: Several fenced code blocks use plain ``` without a language tag;
update each triple-backtick fence in the document (e.g., the blocks currently
shown as ``` at the indicated locations) to include an explicit language token
(for non-code or prose use `text`, or a specific language like `json`, `bash`,
`markdown` where applicable) so the markdown linter passes; search for all
occurrences of ``` (including the ones noted around 131, 194, 245, 289, 418,
455) and replace them with ```text or the appropriate language identifier.
In `@docs/dashpay/research/02-rust-dashcore-keywallet.md`:
- Line 232: The markdown contains fenced code blocks without language tags;
update the offending triple-backtick fences to include the appropriate language
identifier (e.g., ```rust, ```bash, or ```text) for the code snippets so
markdownlint passes and syntax highlighting works—locate the plain ``` fences in
the document (the blocks referenced in the review) and replace them with
language-tagged fences.
In `@docs/dashpay/research/05-swift-app.md`:
- Line 47: The fenced code block currently uses a bare triple-backtick fence
(```); add a language tag (e.g., ```swift or ```text) immediately after the
opening backticks to satisfy markdownlint and enable proper syntax highlighting
for that block.
In `@docs/dashpay/research/06-interop-desk-check.md`:
- Line 366: The fenced code block uses plain ``` without a language tag; update
the opening fence to include an appropriate language identifier (for example
`http`, `text`, or `bash`) so markdownlint is satisfied and readability
improves—locate the triple-backtick fenced block in the document and add the
language tag immediately after the opening ``` fence.
- Line 24: The table row contains an extra leading column ("2") so it has four
columns while the table header defines three; remove the extra column in the row
that contains "2" (the row with "ECDH shared-key derivation" and the
libsecp256k1 SHA256 expression) so the row matches the 3-column header layout,
keeping the description "ECDH shared-key derivation" and the verdict "**PASS** —
all three stacks compute libsecp256k1-style `SHA256((y[31]&0x1|0x2) ‖ x)`" as
the remaining columns.
In `@docs/dashpay/SPEC.md`:
- Line 111: Several fenced code blocks in SPEC.md are missing language
identifiers; update each triple-backtick fence (``` ) at the noted examples so
they include an appropriate language tag (e.g., change ``` to ```text, ```rust,
or ```swift as appropriate) to satisfy markdownlint and enable correct syntax
highlighting; search for the bare ``` occurrences (including the ones referenced
near the examples) and replace them with language-tagged fences, ensuring
opening and closing fences remain paired.
In `@packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- Around line 221-223: The background loop cleanup currently unconditionally
sets this.background_cancel to None (in the block near start()), which can
overwrite a newer token if stop() and start() race; change the logic so the
background thread only clears background_cancel if the stored cancel token it
captured at spawn time still matches the current token in this.background_cancel
(i.e., capture the Arc/ID of the cancel handle when spawning and
compare-before-clearing); apply the same compare-and-clear pattern in the stop()
/ thread-exit cleanup (references: this.background_cancel, start(), stop()) so a
late-exiting old loop cannot null out a replacement token.
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 103-118: The sender/recipient key selection currently using
sender_identity.public_keys().iter().find(...) (checking Purpose::ENCRYPTION and
KeyType::ECDSA_SECP256K1) can pick a disabled/rotated key; update the logic to
only consider active/enabled keys (e.g., filter by .enabled() or reuse the
existing enabled-key selection utility used for signing) so
sender_encryption_key and recipient_key_index (the call to
select_recipient_key_index should be updated similarly or replaced) always
reference the current active ENCRYPTION/DECRYPTION ECDSA_SECP256K1 key; ensure
you still call .map(...).ok_or_else(...) and preserve error type
PlatformWalletError::InvalidIdentityData when no active key is found.
- Around line 516-556: collect_account_build_candidates currently skips contacts
when info.core_wallet.accounts.dashpay_external_accounts.contains_key(&key) is
true, which prevents retries if register_contact_account previously failed after
inserting an external entry; remove that gating so contacts with an
incoming_request (incoming.encrypted_public_key and key indices) are always
returned as AccountBuildCandidate (unless payment_channel_broken) to allow
build_contact_accounts -> register_contact_account to retry; specifically, in
collect_account_build_candidates remove or change the has_external
check/continue and rely on contact.incoming_request and payment_channel_broken
to decide inclusion (keep AccountBuildCandidate fields: contact_id,
encrypted_public_key, our_decryption_key_index, contact_encryption_key_index).
- Around line 452-509: parse_contact_request_doc currently only extracts
required fields and drops optional fields encryptedAccountLabel and
autoAcceptProof, causing restores to lose these values; update
parse_contact_request_doc (and thus parse_sent_contact_request_doc which calls
it) to also read props.get("encryptedAccountLabel").and_then(|v: &Value|
v.as_str()).map(|s| s.to_owned()) and props.get("autoAcceptProof").and_then(|v:
&Value| v.as_bytes()).cloned() (or appropriate conversions) and pass them into
ContactRequest::new (or the appropriate constructor/factory) so the
ContactRequest created preserves encryptedAccountLabel and autoAcceptProof
during ingest/reconcile. Ensure the match arm pattern includes these Option
values and the fallback logging remains unchanged.
In
`@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- Around line 116-130: The code removes an incoming request from
self.incoming_contact_requests but the returned ContactChangeSet only records
cs.rejected, so on replay the incoming entry isn't removed; update the change
set returned by the function to also include the incoming-removal for (owner_id,
*sender_id, account_reference) (i.e., add the corresponding removal entry to the
ContactChangeSet alongside cs.rejected) so that replay will delete the
incoming_contact_requests entry when applying the rejection tombstone.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 806-875: The local-only already_reciprocated check (variable
already_reciprocated) can be stale; change the flow so before attempting
send_contact_request_with_external_signer you either (A) perform a platform
check for an existing reciprocal contact request/relationship (use whatever
network client/query you have for checking platform contact requests for
(ownerId,toUserId,accountReference)) and set already_reciprocated accordingly,
or (B) keep the existing local check but add a duplicate-send fallback: catch
the unique-index conflict/error returned by
send_contact_request_with_external_signer and, on that specific error, log that
the reciprocal exists on Platform and run the adopt path (call
register_contact_account(&our_identity_id, &sender_id, 0) and treat as success).
Reference already_reciprocated, send_contact_request_with_external_signer, and
register_contact_account when implementing either fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 596c3a94-3c49-4cc0-869e-b392a37c181e
📒 Files selected for processing (38)
docs/dashpay/SPEC.mddocs/dashpay/research/01-dip-spec.mddocs/dashpay/research/02-rust-dashcore-keywallet.mddocs/dashpay/research/03-rs-platform-wallet.mddocs/dashpay/research/04-sdk-and-contract.mddocs/dashpay/research/05-swift-app.mddocs/dashpay/research/06-interop-desk-check.mdpackages/rs-platform-encryption/Cargo.tomlpackages/rs-platform-encryption/src/lib.rspackages/rs-platform-wallet-ffi/src/established_contact.rspackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identities.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rspackages/rs-platform-wallet/src/wallet/identity/crypto/validation.rspackages/rs-platform-wallet/src/wallet/identity/network/account_labels.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/contacts.rspackages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/profile.rspackages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-sdk-ffi/src/dashpay/contact_request.rspackages/rs-sdk/src/platform/dashpay/contact_request.rs
| `autoAcceptProof` (optional, 38–102 bytes) lets a recipient pre-authorize automatic acceptance. | ||
| DIP-0015 defines a **separate** derivation path for the auto-accept proof keys: | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Specify code-fence languages for markdown lint compliance.
Please add explicit language tags to these fenced blocks (at least text where no specific language applies).
Also applies to: 194-194, 245-245, 289-289, 418-418, 455-455
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/research/01-dip-spec.md` at line 131, Several fenced code blocks
use plain ``` without a language tag; update each triple-backtick fence in the
document (e.g., the blocks currently shown as ``` at the indicated locations) to
include an explicit language token (for non-code or prose use `text`, or a
specific language like `json`, `bash`, `markdown` where applicable) so the
markdown linter passes; search for all occurrences of ``` (including the ones
noted around 131, 194, 245, 289, 418, 455) and replace them with ```text or the
appropriate language identifier.
Source: Linters/SAST tools
|
|
||
| `test_dashpay_vector_1..4` derive against real DashPay-shaped paths, e.g.: | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Missing language tags on fenced code blocks.
Add language identifiers to keep markdownlint clean and preserve proper syntax highlighting.
Also applies to: 394-394
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 232-232: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/research/02-rust-dashcore-keywallet.md` at line 232, The
markdown contains fenced code blocks without language tags; update the offending
triple-backtick fences to include the appropriate language identifier (e.g.,
```rust, ```bash, or ```text) for the code snippets so markdownlint passes and
syntax highlighting works—locate the plain ``` fences in the document (the
blocks referenced in the review) and replace them with language-tagged fences.
Source: Linters/SAST tools
|
|
||
| `ContentView.swift` is the root. A 5-tab `TabView` (enum `RootTab`): | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language tag to the fenced block.
This block should declare a language (or text) to satisfy markdownlint.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/research/05-swift-app.md` at line 47, The fenced code block
currently uses a bare triple-backtick fence (```); add a language tag (e.g.,
```swift or ```text) immediately after the opening backticks to satisfy
markdownlint and enable proper syntax highlighting for that block.
Source: Linters/SAST tools
| | # | Item | Verdict | | ||
| |---|------|---------| | ||
| | 1 | encryptedPublicKey plaintext layout | **FAIL** — ours is a 107-byte DIP-14 serialization; spec + both reference clients use the 69-byte compact (`fingerprint(4) ‖ chainCode(32) ‖ pubKey(33)`). Our current send path cannot even produce a valid document (128-byte ciphertext vs the contract's hard 96). Our receive path rejects reference-client payloads. | | ||
| | 2 | ECDH shared-key derivation | **PASS** — all three stacks compute libsecp256k1-style `SHA256((y[31]&0x1|0x2) ‖ x)`. | |
Contributor
There was a problem hiding this comment.
Fix table column mismatch in the verdict summary.
The row includes an extra column relative to the 3-column header, which can drop content in renderers/linters. Align all rows to the header shape.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/research/06-interop-desk-check.md` at line 24, The table row
contains an extra leading column ("2") so it has four columns while the table
header defines three; remove the extra column in the row that contains "2" (the
row with "ECDH shared-key derivation" and the libsecp256k1 SHA256 expression) so
the row matches the 3-column header layout, keeping the description "ECDH
shared-key derivation" and the verdict "**PASS** — all three stacks compute
libsecp256k1-style `SHA256((y[31]&0x1|0x2) ‖ x)`" as the remaining columns.
Source: Linters/SAST tools
| bundle: **`https://testnet.platform-explorer.pshenmic.dev`** (routes in | ||
| `pshenmic/platform-explorer` `packages/api/src/routes.js`). Endpoints used: | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Specify the language for the fenced block.
Add a language tag (http, text, etc.) to satisfy markdownlint and improve readability.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 366-366: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/research/06-interop-desk-check.md` at line 366, The fenced code
block uses plain ``` without a language tag; update the opening fence to include
an appropriate language identifier (for example `http`, `text`, or `bash`) so
markdownlint is satisfied and readability improves—locate the triple-backtick
fenced block in the document and add the language tag immediately after the
opening ``` fence.
Source: Linters/SAST tools
Comment on lines
+221
to
+223
| if let Ok(mut guard) = this.background_cancel.lock() { | ||
| *guard = None; | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent old-loop cleanup from clearing a newer loop’s cancel token
Line 221 clears background_cancel unconditionally. If stop() (Line 238) is followed by a quick start(), the old thread can exit later and overwrite the new token with None, leaving the new loop untracked and potentially un-stoppable.
Suggested fix
pub struct DashPaySyncManager {
wallets: Arc<RwLock<BTreeMap<WalletId, Arc<PlatformWallet>>>>,
- background_cancel: StdMutex<Option<CancellationToken>>,
+ background_cancel: StdMutex<Option<(u64, CancellationToken)>>,
+ run_id: AtomicU64,
interval_secs: AtomicU64,
is_syncing: AtomicBool,
quiescing: AtomicBool,
last_sync_unix: AtomicU64,
}
impl DashPaySyncManager {
pub fn new(wallets: Arc<RwLock<BTreeMap<WalletId, Arc<PlatformWallet>>>>) -> Self {
Self {
wallets,
background_cancel: StdMutex::new(None),
+ run_id: AtomicU64::new(1),
interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS),
is_syncing: AtomicBool::new(false),
quiescing: AtomicBool::new(false),
last_sync_unix: AtomicU64::new(0),
}
}
pub fn start(self: Arc<Self>) {
let mut guard = self.background_cancel.lock().expect("bg_cancel poisoned");
if guard.is_some() {
return;
}
let cancel = CancellationToken::new();
- *guard = Some(cancel.clone());
+ let run_id = self.run_id.fetch_add(1, Ordering::Relaxed);
+ *guard = Some((run_id, cancel.clone()));
drop(guard);
let handle = tokio::runtime::Handle::current();
let this = self;
std::thread::Builder::new()
.name("dashpay-sync".into())
.spawn(move || {
handle.block_on(async move {
@@
- if let Ok(mut guard) = this.background_cancel.lock() {
- *guard = None;
+ if let Ok(mut guard) = this.background_cancel.lock() {
+ if guard.as_ref().map(|(id, _)| *id) == Some(run_id) {
+ *guard = None;
+ }
}
});
})
.expect("failed to spawn dashpay-sync thread");
}
pub fn stop(&self) {
- if let Some(token) = self
+ if let Some((_, token)) = self
.background_cancel
.lock()
.expect("bg_cancel poisoned")
.take()
{
token.cancel();
}
}
}Also applies to: 238-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/manager/dashpay_sync.rs` around lines 221 -
223, The background loop cleanup currently unconditionally sets
this.background_cancel to None (in the block near start()), which can overwrite
a newer token if stop() and start() race; change the logic so the background
thread only clears background_cancel if the stored cancel token it captured at
spawn time still matches the current token in this.background_cancel (i.e.,
capture the Arc/ID of the cancel handle when spawning and
compare-before-clearing); apply the same compare-and-clear pattern in the stop()
/ thread-exit cleanup (references: this.background_cancel, start(), stop()) so a
late-exiting old loop cannot null out a replacement token.
Comment on lines
103
to
+118
| let sender_encryption_key = sender_identity | ||
| .public_keys() | ||
| .iter() | ||
| .find(|(_, k)| k.purpose() == Purpose::ENCRYPTION) | ||
| .find(|(_, k)| { | ||
| k.purpose() == Purpose::ENCRYPTION | ||
| && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) | ||
| .map(|(_, k)| k.clone()) | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData( | ||
| "Sender identity has no encryption key".to_string(), | ||
| "Sender identity has no ECDSA_SECP256K1 encryption key".to_string(), | ||
| ) | ||
| })?; | ||
| let sender_key_index = sender_encryption_key.id(); | ||
|
|
||
| let recipient_key_index = recipient_identity | ||
| .public_keys() | ||
| .iter() | ||
| .find(|(_, k)| k.purpose() == Purpose::DECRYPTION) | ||
| .map(|(id, _)| *id) | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData( | ||
| "Recipient identity has no decryption key".to_string(), | ||
| ) | ||
| })?; | ||
| let recipient_key_index = select_recipient_key_index(&recipient_identity)?; |
Contributor
There was a problem hiding this comment.
Skip disabled keys when choosing sender/recipient key IDs.
These manual .find(...) calls only check purpose and type, so a rotated identity can still select an older disabled ENCRYPTION/DECRYPTION key if it appears first in the map. That makes the new request reference a key the other side should no longer use, and on the sender side can derive ECDH from a stale slot. Filter to active keys only, or route this through the same enabled-key selection logic you already use for signing keys.
Also applies to: 245-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`
around lines 103 - 118, The sender/recipient key selection currently using
sender_identity.public_keys().iter().find(...) (checking Purpose::ENCRYPTION and
KeyType::ECDSA_SECP256K1) can pick a disabled/rotated key; update the logic to
only consider active/enabled keys (e.g., filter by .enabled() or reuse the
existing enabled-key selection utility used for signing) so
sender_encryption_key and recipient_key_index (the call to
select_recipient_key_index should be updated similarly or replaced) always
reference the current active ENCRYPTION/DECRYPTION ECDSA_SECP256K1 key; ensure
you still call .map(...).ok_or_else(...) and preserve error type
PlatformWalletError::InvalidIdentityData when no active key is found.
Comment on lines
+452
to
+509
| fn parse_contact_request_doc( | ||
| doc: &dpp::document::Document, | ||
| sender_id: Identifier, | ||
| recipient_id: Identifier, | ||
| ) -> Option<ContactRequest> { | ||
| let props = doc.properties(); | ||
| let sender_key_index = props | ||
| .get("senderKeyIndex") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()); | ||
| let recipient_key_index = props | ||
| .get("recipientKeyIndex") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()); | ||
| let account_reference = props | ||
| .get("accountReference") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()); | ||
| let encrypted_public_key = props | ||
| .get("encryptedPublicKey") | ||
| .and_then(|v: &Value| v.as_bytes()) | ||
| .cloned(); | ||
|
|
||
| match ( | ||
| sender_key_index, | ||
| recipient_key_index, | ||
| account_reference, | ||
| encrypted_public_key, | ||
| ) { | ||
| (Some(ski), Some(rki), Some(ar), Some(epk)) => Some(ContactRequest::new( | ||
| sender_id, | ||
| recipient_id, | ||
| ski, | ||
| rki, | ||
| ar, | ||
| epk, | ||
| doc.created_at_core_block_height().unwrap_or(0), | ||
| doc.created_at().unwrap_or(0), | ||
| )), | ||
| _ => { | ||
| tracing::warn!( | ||
| sender = %sender_id, | ||
| recipient = %recipient_id, | ||
| "Skipping contact request document: missing required field" | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Parse our own sent `contactRequest` document into a [`ContactRequest`] | ||
| /// (owner is us, recipient is `toUserId`). | ||
| fn parse_sent_contact_request_doc( | ||
| doc: &dpp::document::Document, | ||
| owner_id: Identifier, | ||
| recipient_id: Identifier, | ||
| ) -> Option<ContactRequest> { | ||
| // Same field set as the received side; the only difference is which | ||
| // identity is owner vs recipient. | ||
| Self::parse_contact_request_doc(doc, owner_id, recipient_id) | ||
| } |
Contributor
There was a problem hiding this comment.
Sync ingest is dropping optional contact-request payloads.
parse_contact_request_doc() only preserves the required fields, so both the received-side ingest and the sent-side reconcile path lose encryptedAccountLabel and autoAcceptProof even though ContactRequest carries them and sent_contact_requests() parses them later in this file. After a restore/sync sweep those values become None, which breaks metadata round-tripping and any auto-accept behavior that depends on the proof.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`
around lines 452 - 509, parse_contact_request_doc currently only extracts
required fields and drops optional fields encryptedAccountLabel and
autoAcceptProof, causing restores to lose these values; update
parse_contact_request_doc (and thus parse_sent_contact_request_doc which calls
it) to also read props.get("encryptedAccountLabel").and_then(|v: &Value|
v.as_str()).map(|s| s.to_owned()) and props.get("autoAcceptProof").and_then(|v:
&Value| v.as_bytes()).cloned() (or appropriate conversions) and pass them into
ContactRequest::new (or the appropriate constructor/factory) so the
ContactRequest created preserves encryptedAccountLabel and autoAcceptProof
during ingest/reconcile. Ensure the match arm pattern includes these Option
values and the fallback logging remains unchanged.
Comment on lines
+516
to
+556
| fn collect_account_build_candidates( | ||
| info: &crate::wallet::platform_wallet::PlatformWalletInfo, | ||
| identity_id: &Identifier, | ||
| ) -> Vec<AccountBuildCandidate> { | ||
| use key_wallet::account::account_collection::DashpayAccountKey; | ||
|
|
||
| let Some(managed) = info.identity_manager.managed_identity(identity_id) else { | ||
| return Vec::new(); | ||
| }; | ||
|
|
||
| let mut out = Vec::new(); | ||
| for (contact_id, contact) in &managed.established_contacts { | ||
| // G1c: never retry a permanently-broken channel — wait for a | ||
| // superseding request (which clears the flag on re-establish). | ||
| if contact.payment_channel_broken { | ||
| continue; | ||
| } | ||
| let key = DashpayAccountKey { | ||
| index: 0, | ||
| user_identity_id: identity_id.to_buffer(), | ||
| friend_identity_id: contact_id.to_buffer(), | ||
| }; | ||
| let has_external = info | ||
| .core_wallet | ||
| .accounts | ||
| .dashpay_external_accounts | ||
| .contains_key(&key); | ||
| if has_external { | ||
| continue; | ||
| } | ||
| // The incoming request carries the counterparty's encrypted | ||
| // xpub + the key indices needed for ECDH. | ||
| let incoming = &contact.incoming_request; | ||
| out.push(AccountBuildCandidate { | ||
| contact_id: *contact_id, | ||
| encrypted_public_key: incoming.encrypted_public_key.clone(), | ||
| our_decryption_key_index: incoming.recipient_key_index, | ||
| contact_encryption_key_index: incoming.sender_key_index, | ||
| }); | ||
| } | ||
| out |
Contributor
There was a problem hiding this comment.
Don't gate rebuild retries on the external account alone.
build_contact_accounts() always retries the receiving-account registration first, but this candidate collector only schedules contacts when dashpay_external_accounts is missing. If register_contact_account() fails once and the external account still gets inserted, later sweeps never revisit that contact, so incoming-payment visibility stays broken permanently despite the "will retry next sweep" path below.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`
around lines 516 - 556, collect_account_build_candidates currently skips
contacts when
info.core_wallet.accounts.dashpay_external_accounts.contains_key(&key) is true,
which prevents retries if register_contact_account previously failed after
inserting an external entry; remove that gating so contacts with an
incoming_request (incoming.encrypted_public_key and key indices) are always
returned as AccountBuildCandidate (unless payment_channel_broken) to allow
build_contact_accounts -> register_contact_account to retry; specifically, in
collect_account_build_candidates remove or change the has_external
check/continue and rely on contact.incoming_request and payment_channel_broken
to decide inclusion (keep AccountBuildCandidate fields: contact_id,
encrypted_public_key, our_decryption_key_index, contact_encryption_key_index).
Comment on lines
+116
to
+130
| self.incoming_contact_requests.remove(sender_id); | ||
|
|
||
| let tombstone = RejectedContactRequest { | ||
| owner_id, | ||
| sender_id: *sender_id, | ||
| account_reference, | ||
| document_id, | ||
| }; | ||
| self.rejected_contact_requests | ||
| .insert((*sender_id, account_reference), tombstone.clone()); | ||
|
|
||
| let mut cs = ContactChangeSet::default(); | ||
| cs.rejected | ||
| .insert((owner_id, *sender_id, account_reference), tombstone); | ||
| cs |
Contributor
There was a problem hiding this comment.
Persist the incoming-removal when recording a rejection tombstone.
At Line 116, the method removes the incoming request from in-memory state, but the returned changeset (Line 127-Line 130) only contains rejected. On replay, rejected does not remove incoming entries, so rejected requests can reappear after restore.
Proposed fix
pub fn record_rejected_contact_request(
&mut self,
sender_id: &Identifier,
account_reference: u32,
document_id: Option<Identifier>,
) -> ContactChangeSet {
let owner_id = self.id();
- self.incoming_contact_requests.remove(sender_id);
+ let removed_incoming = self.incoming_contact_requests.remove(sender_id).is_some();
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
+ if removed_incoming {
+ cs.removed_incoming.insert(ReceivedContactRequestKey {
+ owner_id,
+ sender_id: *sender_id,
+ });
+ }
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`
around lines 116 - 130, The code removes an incoming request from
self.incoming_contact_requests but the returned ContactChangeSet only records
cs.rejected, so on replay the incoming entry isn't removed; update the change
set returned by the function to also include the incoming-removal for (owner_id,
*sender_id, account_reference) (i.e., add the corresponding removal entry to the
ContactChangeSet alongside cs.rejected) so that replay will delete the
incoming_contact_requests entry when applying the rejection tombstone.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3841 +/- ##
============================================
+ Coverage 87.04% 87.07% +0.02%
============================================
Files 2677 2641 -36
Lines 329918 327345 -2573
============================================
- Hits 287182 285036 -2146
+ Misses 42736 42309 -427
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
Jun 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
M1 of the DashPay completion plan: the SDK entropy / DIP-15 compact xpub / key-purpose interop fixes are correct, but six in-scope correctness issues block merge. The most concerning is editing V001 in-place (violates the documented append-only migration policy and bricks DB rehydration for the v4.0.0-beta.4 cohort). Additional blockers: the reject path emits an incomplete ChangeSet (no removed_incoming); the new rejected_contact_requests table is written but never read; transient identity fetches in register_external_contact_account are misclassified as permanent and brick the channel; validation.purpose_mismatch is set even when a hard error is also present, masking permanent failures as retryable; and the sync sweep skips superseding requests from established contacts, making the documented payment_channel_broken recovery path unreachable.
🔴 6 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 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-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-213: V001 migration edited in-place violates append-only policy and breaks upgrade from v4.0.0-beta.4
This PR adds `contacts.payment_channel_broken` and a new `rejected_contact_requests` table by editing V001 directly. V001 (without these additions) was already shipped in `v4.0.0-beta.4` (commit da9d3fe84e / schema confirmed via `git show`), and `packages/rs-platform-wallet-storage/README.md:106` explicitly states migrations are append-only and applied by refinery on every `open`. refinery checksums each migration in `refinery_schema_history`; against an existing v4.0.0-beta.4 DB it will either abort with a divergent-checksum error or silently skip V001 (already applied) — in which case neither the new table nor the new column is ever created, and the first runtime write in `contacts.rs:240` (`INSERT INTO rejected_contact_requests …`) or `contacts.rs:194-212` (`payment_channel_broken` column) fails at the SQLite layer. `tc029_migration_fingerprint_stable` does not catch this because it only checks self-stability, not a pinned hash. Add `V002__dashpay_reject_and_broken_channel.rs` doing `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` and `CREATE TABLE rejected_contact_requests (…)`; the loader at `contacts.rs::load_state` already tolerates NULL `payment_channel_broken`, so a default-less ALTER is compatible.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-131: `record_rejected_contact_request` removes incoming in memory but does not emit `removed_incoming`
The function calls `self.incoming_contact_requests.remove(sender_id)` and returns a `ContactChangeSet` populated only with `cs.rejected`. The unified-`contacts`-table writer at `rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193` only `DELETE`s when `cs.removed_incoming` is non-empty, so the previously persisted state='received' row (with the `incoming_request` blob) stays in SQLite. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the unified contacts reader rebuckets that row as an incoming request, `apply_changeset` re-inserts it into `incoming_contact_requests`, and the FFI surfaces the explicitly-rejected request back to the UI. The persisted delta is also internally inconsistent with the in-memory mutation — a delta-persistence invariant violation. The in-memory `rejected_tombstone_round_trips_and_respects_account_reference` test does not catch this because it round-trips via `apply_changeset`, not the SQLite reader. Fix by also inserting a matching `ReceivedContactRequestKey { owner_id, sender_id: *sender_id }` into `cs.removed_incoming`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: `rejected_contact_requests` is written but never read — tombstones lost across restart
The PR adds a writer (`contacts.rs:240`), a migration row (`V001__initial.rs:203`), and an `apply_changeset` branch that restores `ManagedIdentity.rejected_contact_requests` from `cs.rejected`. But `managed_identity_from_entry` hard-codes `rejected_contact_requests: Default::default()` (line 214), and grep confirms no `load_state` reader for the new table. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the in-memory tombstone map is always empty after restart even though SQLite holds the rows. `is_request_rejected` then returns `false`, the sweep's tombstone-skip in `network/contact_requests.rs:396-404` does not fire, and the recurring DashPay loop (G12) resurrects every rejected request on the first sweep — exactly the M1 failure mode the SPEC.md cites as the reason G5 must land with G12. Add a per-wallet `load_state` for `rejected_contact_requests` and route its output into the `ContactChangeSet.rejected` synthesized during rehydration.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:711-728: Transient identity fetch failures inside account-build are marked as permanent
`build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. But `register_external_contact_account` (`network/contacts.rs:400-407`) performs another `Identity::fetch` for the same contact and wraps the DAPI/network error as `PlatformWalletError::InvalidIdentityData`. A transient DAPI hiccup after validation therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter at line 530, and recovery only fires if a superseding contactRequest happens to arrive — contradicting the policy in the docstring at lines 573-578 ("Transient (identity fetch / network): logged, left for the next sweep to retry. The broken flag stays clear."). The fix is to perform the contact-identity fetch and treat its failure as transient *before* calling `register_external_contact_account` (mirroring the existing fetch at lines 631-655) and to scope the permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:383-392: Superseding requests from established contacts are skipped — `payment_channel_broken` recovery is unreachable
The received-side ingest drops every doc whose sender is already in `established_contacts` before consulting `accountReference`. `EstablishedContact::reestablish_preserving_metadata` exists precisely to clear `payment_channel_broken = false` when a fresh request flows in (see `types/dashpay/established_contact.rs:84-104`), and `collect_account_build_candidates` documents the recovery contract at lines 528-529 ("never retry a permanently-broken channel — wait for a superseding request (which clears the flag on re-establish)"). But there is no path that reaches `reestablish_preserving_metadata` for an already-established sender from the sync sweep — `add_incoming_contact_request` is only called for new senders here, and the send-side guard at `state/managed_identity/contact_requests.rs:46-48` similarly returns early for established contacts. Net effect: once `payment_channel_broken` is set, it stays set forever. Either (a) detect a superseding incoming request (new `accountReference` for the same sender) and route it through the reestablish path, or (b) change the broken-channel policy so the next sweep can retry under controlled conditions.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:5733-5749: `select_recipient_key_index` returns disabled keys
The send-side recipient-key selector iterates `recipient_identity.public_keys()` and returns the first key whose purpose is DECRYPTION (then ENCRYPTION) and whose type is ECDSA_SECP256K1, with no `disabled_at` check. `validate_contact_request` in `crypto/validation.rs` does gate on disabled keys, so if a preferred DECRYPTION key has been rotated/disabled this selector returns it anyway and the broadcast fails downstream with an opaque error instead of falling through to a usable ENCRYPTION key on the same identity. Add `&& k.disabled_at().is_none()` to both branches so selection is consistent with validation.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:50-62: `purpose_mismatch` is set even when a non-purpose hard error is also present
The docstring at lines 19-29 contracts `purpose_mismatch` as `true` *only* when the sole reason for invalidity is a key-purpose mismatch — it is what tells `build_contact_accounts` at `network/contact_requests.rs:689` to treat the failure as a non-permanent skip instead of marking the channel permanently broken. The implementation does not preserve that invariant: `add_purpose_error` unconditionally sets `purpose_mismatch = true`, and `add_error` never clears it. A request whose key has both a wrong key type (hard, permanent error) and a wrong purpose ends up with `is_valid = false, purpose_mismatch = true`, and the caller skips + retries forever instead of marking broken. The mobile testnet census makes this rare in practice today, but the classifier is the load-bearing primitive the recovery policy is built on — fix `add_error` to clear the flag, and fix `add_purpose_error` to only set it if no prior hard error was recorded.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:192-227: `stop()` followed quickly by `start()` can let the old thread null out the new cancel token
`stop()` takes the current `background_cancel`, sets it to `None`, then cancels the old token. The spawned thread exits its loop on cancellation and at lines 221-223 re-acquires the guard and writes `*guard = None`. If `start()` runs between `stop()` and the old thread's cleanup block, the old thread's final clear will overwrite the new token just installed by `start()` — leaving `background_cancel` empty while a fresh sync thread is still running, so a subsequent `stop()`/`quiesce()` will be a no-op against that running thread. The normal shutdown path (`quiesce` waits for in-flight passes) does not hit this, but bare `stop()`/`start()` races can. Fix by capturing the token at spawn time and only clearing the guard if it still holds that same token (`if matches!(*guard, Some(t) if Arc::ptr_eq(...)) { *guard = None; }`).
Comment on lines
186
to
+213
| note TEXT, | ||
| is_hidden INTEGER, | ||
| accepted_accounts BLOB, | ||
| payment_channel_broken INTEGER, | ||
| updated_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, contact_id), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- Rejected-request tombstone (G5 stage 1). Keyed by | ||
| -- `(wallet_id, owner_id, sender_id, account_reference)` — NOT bare | ||
| -- sender id — so a once-rejected sender can still re-request via a | ||
| -- bumped accountReference (the DIP-15 rotation mechanism), while a | ||
| -- replay of the exact same immutable request stays suppressed. The | ||
| -- optional `document_id` records the rejected document's id for audit / | ||
| -- exact-match suppression. The sync ingest path consults this table | ||
| -- before re-ingesting a received contactRequest. | ||
| CREATE TABLE rejected_contact_requests ( | ||
| wallet_id BLOB NOT NULL, | ||
| owner_id BLOB NOT NULL, | ||
| sender_id BLOB NOT NULL, | ||
| account_reference INTEGER NOT NULL, | ||
| document_id BLOB, | ||
| rejected_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, sender_id, account_reference), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); | ||
|
|
Collaborator
There was a problem hiding this comment.
🔴 Blocking: V001 migration edited in-place violates append-only policy and breaks upgrade from v4.0.0-beta.4
This PR adds contacts.payment_channel_broken and a new rejected_contact_requests table by editing V001 directly. V001 (without these additions) was already shipped in v4.0.0-beta.4 (commit da9d3fe / schema confirmed via git show), and packages/rs-platform-wallet-storage/README.md:106 explicitly states migrations are append-only and applied by refinery on every open. refinery checksums each migration in refinery_schema_history; against an existing v4.0.0-beta.4 DB it will either abort with a divergent-checksum error or silently skip V001 (already applied) — in which case neither the new table nor the new column is ever created, and the first runtime write in contacts.rs:240 (INSERT INTO rejected_contact_requests …) or contacts.rs:194-212 (payment_channel_broken column) fails at the SQLite layer. tc029_migration_fingerprint_stable does not catch this because it only checks self-stability, not a pinned hash. Add V002__dashpay_reject_and_broken_channel.rs doing ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER and CREATE TABLE rejected_contact_requests (…); the loader at contacts.rs::load_state already tolerates NULL payment_channel_broken, so a default-less ALTER is compatible.
source: ['claude']
Comment on lines
+109
to
+131
| pub fn record_rejected_contact_request( | ||
| &mut self, | ||
| sender_id: &Identifier, | ||
| account_reference: u32, | ||
| document_id: Option<Identifier>, | ||
| ) -> ContactChangeSet { | ||
| let owner_id = self.id(); | ||
| self.incoming_contact_requests.remove(sender_id); | ||
|
|
||
| let tombstone = RejectedContactRequest { | ||
| owner_id, | ||
| sender_id: *sender_id, | ||
| account_reference, | ||
| document_id, | ||
| }; | ||
| self.rejected_contact_requests | ||
| .insert((*sender_id, account_reference), tombstone.clone()); | ||
|
|
||
| let mut cs = ContactChangeSet::default(); | ||
| cs.rejected | ||
| .insert((owner_id, *sender_id, account_reference), tombstone); | ||
| cs | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: record_rejected_contact_request removes incoming in memory but does not emit removed_incoming
The function calls self.incoming_contact_requests.remove(sender_id) and returns a ContactChangeSet populated only with cs.rejected. The unified-contacts-table writer at rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193 only DELETEs when cs.removed_incoming is non-empty, so the previously persisted state='received' row (with the incoming_request blob) stays in SQLite. Once persister.load() (TODO at sqlite/persister.rs:909) is wired up, the unified contacts reader rebuckets that row as an incoming request, apply_changeset re-inserts it into incoming_contact_requests, and the FFI surfaces the explicitly-rejected request back to the UI. The persisted delta is also internally inconsistent with the in-memory mutation — a delta-persistence invariant violation. The in-memory rejected_tombstone_round_trips_and_respects_account_reference test does not catch this because it round-trips via apply_changeset, not the SQLite reader. Fix by also inserting a matching ReceivedContactRequestKey { owner_id, sender_id: *sender_id } into cs.removed_incoming.
Suggested change
| pub fn record_rejected_contact_request( | |
| &mut self, | |
| sender_id: &Identifier, | |
| account_reference: u32, | |
| document_id: Option<Identifier>, | |
| ) -> ContactChangeSet { | |
| let owner_id = self.id(); | |
| self.incoming_contact_requests.remove(sender_id); | |
| let tombstone = RejectedContactRequest { | |
| owner_id, | |
| sender_id: *sender_id, | |
| account_reference, | |
| document_id, | |
| }; | |
| self.rejected_contact_requests | |
| .insert((*sender_id, account_reference), tombstone.clone()); | |
| let mut cs = ContactChangeSet::default(); | |
| cs.rejected | |
| .insert((owner_id, *sender_id, account_reference), tombstone); | |
| cs | |
| } | |
| let owner_id = self.id(); | |
| self.incoming_contact_requests.remove(sender_id); | |
| let tombstone = RejectedContactRequest { | |
| owner_id, | |
| sender_id: *sender_id, | |
| account_reference, | |
| document_id, | |
| }; | |
| self.rejected_contact_requests | |
| .insert((*sender_id, account_reference), tombstone.clone()); | |
| let mut cs = ContactChangeSet::default(); | |
| cs.removed_incoming.insert(ReceivedContactRequestKey { | |
| owner_id, | |
| sender_id: *sender_id, | |
| }); | |
| cs.rejected | |
| .insert((owner_id, *sender_id, account_reference), tombstone); | |
| cs |
source: ['claude', 'codex']
Comment on lines
+711
to
+728
| if let Err(e) = self | ||
| .register_external_contact_account( | ||
| identity_id, | ||
| &contact_id, | ||
| &candidate.encrypted_public_key, | ||
| candidate.our_decryption_key_index, | ||
| candidate.contact_encryption_key_index, | ||
| ) | ||
| .await | ||
| { | ||
| tracing::warn!( | ||
| identity = %identity_id, | ||
| contact = %contact_id, | ||
| error = %e, | ||
| "Failed to register DashPay external account; marking payment channel broken (permanent)" | ||
| ); | ||
| self.mark_contact_channel_broken(identity_id, &contact_id) | ||
| .await; |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Transient identity fetch failures inside account-build are marked as permanent
build_contact_accounts treats any error from register_external_contact_account as permanent and calls mark_contact_channel_broken. But register_external_contact_account (network/contacts.rs:400-407) performs another Identity::fetch for the same contact and wraps the DAPI/network error as PlatformWalletError::InvalidIdentityData. A transient DAPI hiccup after validation therefore permanently disables the payment channel; subsequent sweeps skip the contact via the payment_channel_broken filter at line 530, and recovery only fires if a superseding contactRequest happens to arrive — contradicting the policy in the docstring at lines 573-578 ("Transient (identity fetch / network): logged, left for the next sweep to retry. The broken flag stays clear."). The fix is to perform the contact-identity fetch and treat its failure as transient before calling register_external_contact_account (mirroring the existing fetch at lines 631-655) and to scope the permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
source: ['codex']
Comment on lines
52
to
+62
| self.is_valid = false; | ||
| } | ||
|
|
||
| /// Add a key-PURPOSE error: sets `is_valid = false` AND flags | ||
| /// `purpose_mismatch` so callers can downgrade this to a non-permanent | ||
| /// skip rather than a permanent broken-channel mark (G15). | ||
| pub fn add_purpose_error(&mut self, error: String) { | ||
| self.errors.push(error); | ||
| self.is_valid = false; | ||
| self.purpose_mismatch = true; | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: purpose_mismatch is set even when a non-purpose hard error is also present
The docstring at lines 19-29 contracts purpose_mismatch as true only when the sole reason for invalidity is a key-purpose mismatch — it is what tells build_contact_accounts at network/contact_requests.rs:689 to treat the failure as a non-permanent skip instead of marking the channel permanently broken. The implementation does not preserve that invariant: add_purpose_error unconditionally sets purpose_mismatch = true, and add_error never clears it. A request whose key has both a wrong key type (hard, permanent error) and a wrong purpose ends up with is_valid = false, purpose_mismatch = true, and the caller skips + retries forever instead of marking broken. The mobile testnet census makes this rare in practice today, but the classifier is the load-bearing primitive the recovery policy is built on — fix add_error to clear the flag, and fix add_purpose_error to only set it if no prior hard error was recorded.
Suggested change
| self.is_valid = false; | |
| } | |
| /// Add a key-PURPOSE error: sets `is_valid = false` AND flags | |
| /// `purpose_mismatch` so callers can downgrade this to a non-permanent | |
| /// skip rather than a permanent broken-channel mark (G15). | |
| pub fn add_purpose_error(&mut self, error: String) { | |
| self.errors.push(error); | |
| self.is_valid = false; | |
| self.purpose_mismatch = true; | |
| } | |
| pub fn add_error(&mut self, error: String) { | |
| self.errors.push(error); | |
| self.is_valid = false; | |
| self.purpose_mismatch = false; | |
| } | |
| /// Add a key-PURPOSE error: sets `is_valid = false` AND flags | |
| /// `purpose_mismatch` so callers can downgrade this to a non-permanent | |
| /// skip rather than a permanent broken-channel mark (G15). | |
| pub fn add_purpose_error(&mut self, error: String) { | |
| let already_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch; | |
| self.errors.push(error); | |
| self.is_valid = false; | |
| self.purpose_mismatch = !already_has_hard_error; | |
| } |
source: ['codex']
Comment on lines
+383
to
392
| // G1a: do NOT skip just because the sender is in | ||
| // `sent_contact_requests` — that is the reciprocal we | ||
| // need to let through to auto-establish. Skip only when | ||
| // already tracked as incoming or established (true | ||
| // dedup), or suppressed by a rejected-request tombstone. | ||
| if managed.incoming_contact_requests.contains_key(&sender_id) | ||
| || managed.established_contacts.contains_key(&sender_id) | ||
| { | ||
| continue; | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Superseding requests from established contacts are skipped — payment_channel_broken recovery is unreachable
The received-side ingest drops every doc whose sender is already in established_contacts before consulting accountReference. EstablishedContact::reestablish_preserving_metadata exists precisely to clear payment_channel_broken = false when a fresh request flows in (see types/dashpay/established_contact.rs:84-104), and collect_account_build_candidates documents the recovery contract at lines 528-529 ("never retry a permanently-broken channel — wait for a superseding request (which clears the flag on re-establish)"). But there is no path that reaches reestablish_preserving_metadata for an already-established sender from the sync sweep — add_incoming_contact_request is only called for new senders here, and the send-side guard at state/managed_identity/contact_requests.rs:46-48 similarly returns early for established contacts. Net effect: once payment_channel_broken is set, it stays set forever. Either (a) detect a superseding incoming request (new accountReference for the same sender) and route it through the reestablish path, or (b) change the broken-channel policy so the next sweep can retry under controlled conditions.
source: ['codex']
Comment on lines
+192
to
+227
| pub fn start(self: Arc<Self>) { | ||
| let mut guard = self.background_cancel.lock().expect("bg_cancel poisoned"); | ||
| if guard.is_some() { | ||
| return; | ||
| } | ||
| let cancel = CancellationToken::new(); | ||
| *guard = Some(cancel.clone()); | ||
| drop(guard); | ||
|
|
||
| let handle = tokio::runtime::Handle::current(); | ||
| let this = self; | ||
| std::thread::Builder::new() | ||
| .name("dashpay-sync".into()) | ||
| .spawn(move || { | ||
| handle.block_on(async move { | ||
| loop { | ||
| if cancel.is_cancelled() { | ||
| break; | ||
| } | ||
|
|
||
| this.sync_now().await; | ||
|
|
||
| let interval = this.interval(); | ||
| tokio::select! { | ||
| _ = tokio::time::sleep(interval) => {} | ||
| _ = cancel.cancelled() => break, | ||
| } | ||
| } | ||
|
|
||
| if let Ok(mut guard) = this.background_cancel.lock() { | ||
| *guard = None; | ||
| } | ||
| }); | ||
| }) | ||
| .expect("failed to spawn dashpay-sync thread"); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: stop() followed quickly by start() can let the old thread null out the new cancel token
stop() takes the current background_cancel, sets it to None, then cancels the old token. The spawned thread exits its loop on cancellation and at lines 221-223 re-acquires the guard and writes *guard = None. If start() runs between stop() and the old thread's cleanup block, the old thread's final clear will overwrite the new token just installed by start() — leaving background_cancel empty while a fresh sync thread is still running, so a subsequent stop()/quiesce() will be a no-op against that running thread. The normal shutdown path (quiesce waits for in-flight passes) does not hit this, but bare stop()/start() races can. Fix by capturing the token at spawn time and only clearing the guard if it still holds that same token (if matches!(*guard, Some(t) if Arc::ptr_eq(...)) { *guard = None; }).
source: ['claude']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Milestone 1 of the DashPay completion plan (
docs/dashpay/SPEC.md, included in this PR with its research base). DashPay's contact-request flow was broken in four independent, previously-unknown ways:send_contact_requestwas rejected by consensus — the broadcast carried a document id derived from the creation entropy but fresh entropy in the transition; drive-abci recomputes the id and rejects withInvalidDocumentTransitionIdError.ExtendedPubKey::encode()form; DIP-15 and both reference mobile clients (iOS dash-shared-core, Android dashj) use the 69-byte compactfingerprint‖chaincode‖pubkey. Our send failed its own 96-byte ciphertext check; our receive couldn't parse mobile payloads.What was done?
Three logical commits:
docs(dashpay)— the 7-agent-reviewed implementation spec (protocol reference, per-layer inventory, gaps G1–G15, 5-milestone plan, Swift UI design, test plan) + 6 research files including the cross-client interop desk-check and the testnet key-purpose census.fix(sdk)!— entropy threading (ContactRequestResult.entropyreused at broadcast), the DIP-15 69-byte compact-xpub codec inplatform-encryption+ the SDK callback contract switched to it, and the recipient key-purpose assertion relaxed to DECRYPTION-or-ENCRYPTION.fix(platform-wallet)— new recurringDashPaySyncManager(iterates the wallets map, not the token registry; per-identity log-and-continue); ingest-guard relaxation + sent-side reconcile with idempotent, metadata-preserving merge; Accept adopts an existing on-platform reciprocal instead of re-broadcasting; per-sweep account rebuild (external and receiving accounts) with validate-before-ECDH, guard-drop lock ordering, and a transient/permanent failure policy (payment_channel_brokenflag, persisted + FFI accessor); rejected-request tombstone keyed(owner, sender, accountReference)so rotated requests still surface; 69-byte compact parsing on receive with address-equality pinned; key-purpose envelope aligned with on-chain reality;DashPaySdkWriterseam making the write paths testable.How Has This Been Tested?
TDD throughout — every behavioral fix has a test that was red against the unfixed code and green after (red→green evidence recorded in the SPEC.md M1 DONE notes and the three commit messages):
platform-wallet: 196 lib + 8 integration tests green (was 170 before this branch; +34 new)dash-sdk(--features mocks,offline-testing): 139 lib tests green (incl. the entropy-id and 69→96-byte pins)platform-encryption: 7/7 (the crate's test target previously failed to compile — fixed dev-deps)cargo checkclean onrs-sdk-ffi,platform-wallet-ffi,platform-wallet-storage; clippy clean on touched cratesdp_001..dp_006) is specced to ride the e2e framework in test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549 and is explicitly not gated on this PR (SPEC.md Part 7.4)Breaking Changes
get_extended_public_keycallback contract forcreate_contact_request/send_contact_requestis now "return the 69-byte DIP-15 compact form" (was an encodedExtendedPubKey); validated before encryption.ContactRequestResultgains a publicentropy: Bytes32field. Thers-sdk-ffiC ABI is unchanged (caller doc contract tightened).contacts.payment_channel_brokencolumn,rejected_contact_requeststable) in the initial migration;ContactChangeSetgains arejectedfield.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation