fix(platform-wallet): accept legacy dashj key purposes on inbound contact requests - #4372
Conversation
…tact requests Contacts established through the legacy Android/dashj client could never be paid from iOS. `send_payment` failed with "No DashpayExternalAccount found for contact ... — call register_external_contact_account first" on every attempt, while contacts created on iOS worked fine. Those contacts are only ever built by the deferred path: the signerless sweep enqueues a `RegisterExternal` op and the signer-backed drain completes it. `validate_contact_request` rejected every legacy document there, because the `recipientKeyIndex` on an inbound dashj request points at the recipient's AUTHENTICATION (key ids 0-2) or TRANSFER (key id 3) key rather than ENCRYPTION/DECRYPTION. The drain classified that as a purpose-only mismatch — correctly refusing to mark the channel broken — so it retried forever and never succeeded, and `send_payment` kept finding no external account. Mainnet device logs show 27 of one wallet's 29 contacts in this state, the 2 survivors being the ones established on iOS. Purpose is not a security boundary for this ECDH: it is defined over the secp256k1 keypair, and DIP-9 indexes the identity-key tree by key type and id, never by purpose, so the same derivation reaches all of them. The gates that do carry weight — the ECDSA key-type gate and the disabled-key check — are untouched. The previous policy was calibrated on a 368-document testnet census that contains no dashj-era cohort. Split the policy in two rather than widening the existing predicate: `recipient_key_purpose_is_valid` still governs the requests we mint (and so `select_recipient_key_index` still practices key separation), while the new `*_key_purpose_is_acceptable_on_receive` govern what we accept from immutable history. A `contactRequest` cannot be re-minted to fit a rule we invent later, so rejecting one is a permanent sentence on a relationship the user has no way to appeal. The node-operational purposes (SYSTEM, VOTING, OWNER) stay rejected, and stay a non-permanent purpose mismatch, so a later evidence-driven widening can still recover those contacts instead of finding their channels broken.
📝 WalkthroughWalkthroughThe SDK now exposes separate receive-side key-purpose predicates. Wallet validation uses them to accept legacy DashPay key purposes while outgoing request selection remains strict. Registration handling preserves queued legacy requests after permanent failures. Tests cover these flows. ChangesDashPay receive compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ContactRequest
participant WalletValidation
participant ReceivePurposePredicates
participant ExternalAccountRegistration
participant PaymentChannel
ContactRequest->>WalletValidation: validate sender and recipient keys
WalletValidation->>ReceivePurposePredicates: check receive purposes
ReceivePurposePredicates-->>WalletValidation: return acceptance or mismatch
ContactRequest->>ExternalAccountRegistration: register external account
ExternalAccountRegistration-->>ContactRequest: return registration result
ContactRequest->>PaymentChannel: retain legacy failure or mark standard failure broken
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🔍 Review in progress — actively reviewing now (commit ee984ad) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/dashpay/contact_request.rs (1)
160-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
select_recipient_key_indexuserecipient_key_purpose_is_valid.The documentation says that the selector defers to the SDK mint predicate. The selector currently duplicates direct
Purpose::DECRYPTIONandPurpose::ENCRYPTIONchecks. A future mint-policy update can make SDK request creation and wallet key selection diverge.
packages/rs-sdk/src/platform/dashpay/contact_request.rs#L160-L162: retain this statement only after the selector invokes the predicate.packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs#L989-L991: userecipient_key_purpose_is_validfor membership while retaining DECRYPTION-first and ENCRYPTION-second preference.🤖 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-sdk/src/platform/dashpay/contact_request.rs` around lines 160 - 162, The selector must use recipient_key_purpose_is_valid as the single membership predicate instead of duplicating direct Purpose checks. In packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs lines 989-991, update select_recipient_key_index to filter with that predicate while preserving DECRYPTION-first and ENCRYPTION-second preference. In packages/rs-sdk/src/platform/dashpay/contact_request.rs lines 160-162, retain the documentation because it becomes accurate after the selector change.
🤖 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.
Nitpick comments:
In `@packages/rs-sdk/src/platform/dashpay/contact_request.rs`:
- Around line 160-162: The selector must use recipient_key_purpose_is_valid as
the single membership predicate instead of duplicating direct Purpose checks. In
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
lines 989-991, update select_recipient_key_index to filter with that predicate
while preserving DECRYPTION-first and ENCRYPTION-second preference. In
packages/rs-sdk/src/platform/dashpay/contact_request.rs lines 160-162, retain
the documentation because it becomes accurate after the selector change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dc90e7c-01d2-4ca0-895d-d642098d29e0
📒 Files selected for processing (4)
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-sdk/src/platform/dashpay/contact_request.rspackages/rs-sdk/src/platform/dashpay/mod.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4372 +/- ##
============================================
- Coverage 87.80% 87.63% -0.18%
============================================
Files 2641 2670 +29
Lines 336510 339447 +2937
============================================
+ Hits 295467 297464 +1997
- Misses 41043 41983 +940
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The receive-side policy is correctly separated from the stricter mint-side policy, and the existing key-type and disabled-key checks remain intact. One in-scope test gap remains: the regression tests stop at purpose validation and do not prove that a DashJ-produced legacy payload succeeds through key derivation, ECDH, decryption, and compact-xpub parsing before the permanent-failure path can mark the channel broken. Source: reviewer backends — general: gpt-5.6-sol; security-auditor: gpt-5.6-sol; rust-quality: gpt-5.6-sol; final verifier backend — gpt-5.6-sol. Orchestration only (not reviewer evidence): openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 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/crypto/validation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:539-575: Add a DashJ interoperability fixture for the newly accepted path
The new tests use synthetic identities and stop after `validate_contact_request`, so they only prove that the widened purpose predicates accept AUTHENTICATION and TRANSFER. This PR now allows those requests to continue through the deferred `RegisterExternal` path, where the wallet derives the recipient key at the legacy key ID, performs ECDH, decrypts `encryptedPublicKey`, and parses the compact xpub. There is no DashJ-produced known-answer fixture covering those steps, so these tests would remain green if the actual legacy payload still failed because of a derivation-path or ECDH/AES convention mismatch. Such a failure is classified as permanent by `register_external_contact_account`; the drain then marks `payment_channel_broken` and removes the queued operation. Add a sanitized fixed seed and DashJ-generated payload—covering at least an AUTHENTICATION recipient and preferably TRANSFER key ID 3—and run it through the production derivation/decryption path, asserting that the expected compact xpub or external account is produced without marking the channel broken.
| fn legacy_dashj_recipient_key_purposes_are_accepted() { | ||
| for purpose in [Purpose::AUTHENTICATION, Purpose::TRANSFER] { | ||
| let sender = make_identity(vec![make_key( | ||
| 0, | ||
| KeyType::ECDSA_SECP256K1, | ||
| Purpose::ENCRYPTION, | ||
| )]); | ||
| let recipient = make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, purpose)]); | ||
|
|
||
| let result = validate_contact_request(&sender, 0, &recipient, 0); | ||
| assert!( | ||
| result.is_valid, | ||
| "a {purpose:?} recipient key must be accepted from an immutable on-chain \ | ||
| document, errors: {:?}", | ||
| result.errors | ||
| ); | ||
| assert!(!result.purpose_mismatch); | ||
| } | ||
| } | ||
|
|
||
| /// The whole legacy pair — AUTHENTICATION sender against AUTHENTICATION | ||
| /// recipient — is the exact shape 15 of the logged mainnet failures took. | ||
| #[test] | ||
| fn recipient_authentication_key_is_rejected_as_purpose_mismatch() { | ||
| fn legacy_dashj_authentication_pair_is_accepted() { | ||
| let sender = make_identity(vec![make_key( | ||
| 0, | ||
| 1, | ||
| KeyType::ECDSA_SECP256K1, | ||
| Purpose::ENCRYPTION, | ||
| Purpose::AUTHENTICATION, | ||
| )]); | ||
| let recipient = make_identity(vec![make_key( | ||
| 0, | ||
| 1, | ||
| KeyType::ECDSA_SECP256K1, | ||
| Purpose::AUTHENTICATION, | ||
| )]); | ||
|
|
||
| let result = validate_contact_request(&sender, 0, &recipient, 0); | ||
| assert!( | ||
| !result.is_valid, | ||
| "an AUTHENTICATION recipient key must be rejected" | ||
| ); | ||
| assert!( | ||
| result.purpose_mismatch, | ||
| "an AUTHENTICATION recipient is a PURPOSE mismatch (non-permanent skip), not a hard/permanent failure" | ||
| ); | ||
| assert!(result.errors.iter().any(|e| e.contains("ENCRYPTION") | ||
| || e.contains("DECRYPTION") | ||
| || e.contains("purpose"))); | ||
| let result = validate_contact_request(&sender, 1, &recipient, 1); | ||
| assert!(result.is_valid, "errors: {:?}", result.errors); |
There was a problem hiding this comment.
🟡 Suggestion: Add a DashJ interoperability fixture for the newly accepted path
The new tests use synthetic identities and stop after validate_contact_request, so they only prove that the widened purpose predicates accept AUTHENTICATION and TRANSFER. This PR now allows those requests to continue through the deferred RegisterExternal path, where the wallet derives the recipient key at the legacy key ID, performs ECDH, decrypts encryptedPublicKey, and parses the compact xpub. There is no DashJ-produced known-answer fixture covering those steps, so these tests would remain green if the actual legacy payload still failed because of a derivation-path or ECDH/AES convention mismatch. Such a failure is classified as permanent by register_external_contact_account; the drain then marks payment_channel_broken and removes the queued operation. Add a sanitized fixed seed and DashJ-generated payload—covering at least an AUTHENTICATION recipient and preferably TRANSFER key ID 3—and run it through the production derivation/decryption path, asserting that the expected compact xpub or external account is produced without marking the channel broken.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Add a DashJ interoperability fixture for the newly accepted path no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…hared mint predicate select_recipient_key_index documented that it defers to recipient_key_purpose_is_valid but repeated the DECRYPTION/ENCRYPTION list inline, so a mint-policy change would silently desync the SDK's request-creation gate from the wallet's key selection. Filter through the predicate and keep only the preference order (DECRYPTION first, then lowest key id) local to the selector. Raised by CodeRabbit on #4372.
… the document Review on #4372 pointed out that the widening lets legacy requests reach the `RegisterExternal` path — derivation at the legacy key id, ECDH, AES decrypt, compact-xpub parse — and that a failure there is classified permanent, so the drain marks `payment_channel_broken`. If our ECDH/AES conventions turn out to differ from dashj's, that would break every legacy channel at once, and a broken channel only heals when the CONTACT sends a fresh request — an appeal the user cannot file. Decrypt and compact-xpub parse are the only gates on the plaintext, so a convention gap is indistinguishable from a corrupt document at that point. When a request was accepted only by the widened receive-side policy (it names a purpose we would never mint), a permanent register fault now leaves the entry queued instead of breaking the channel. The cost is a retry; the alternative costs the user a relationship they cannot repair. Adds `legacy_key_id_and_purpose_survive_the_whole_external_build`: key id 3 (the TRANSFER slot dashj references) through the production provider's ECDH at the real DIP-9 auth path, with the sender's side derived independently from our public key at that same path, then encrypt → decrypt → parse → register. It pins that nothing downstream of the predicate is purpose- or id-sensitive. It deliberately does not claim to prove dashj byte compatibility — that needs a dashj-generated known answer this repo has no fixture for, which is exactly why the classification change above is the safety net rather than the test.
|
Addressed in afec36d — thanks, the harm you named is the right one to design around. The test gap is real and I cannot fully close it. A dashj known-answer fixture needs a dashj-generated payload plus the matching seed, and neither exists in this repo. Rather than write a test that looks like it covers that and does not, I split the response: 1. What I could pin, I pinned. 2. What I could not pin, I made non-destructive. Your finding's sharpest point is the consequence, not the gap: decrypt and compact-xpub parse are the only gates on the plaintext, so a convention mismatch is indistinguishable from a corrupt document, and the permanent classification then breaks every legacy channel at once — healable only by the contact sending a fresh request, which the user cannot make them do. So a permanent register fault on a request accepted only by the widened policy now leaves the entry queued instead of marking the channel broken. Requests that would pass the mint-side policy keep the original permanent classification. Cost is a retry; the alternative costs the user a relationship they cannot repair. Worth noting the failure mode is skewed the safe way regardless: Still open, and I think it should gate the merge: decrypting 2–3 of the real logged mainnet documents against the affected user's seed. That is the only thing that actually settles dashj interop, and it needs material only the user has. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The receive-side purpose widening remains correctly separated from the stricter mint-side policy, and the current head adds useful Rust end-to-end coverage plus non-destructive handling for recipient-purpose legacy failures. Actual DashJ interoperability is still unverified, and the new failure classification does not protect requests admitted solely by the widened sender-purpose policy.
Source: reviewers gpt-5.6-sol (general), gpt-5.6-sol (security-auditor), and gpt-5.6-sol (rust-quality); final verifier gpt-5.6-sol. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
🤖 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/payments.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:5041-5045: Add a DashJ interoperability fixture for the newly accepted path
The new end-to-end test verifies key ID 3 derivation, ECDH, AES decryption, compact-xpub parsing, and registration, but both sides still use this repository's Rust implementations. As the test comment acknowledges, two internally consistent Rust paths cannot detect disagreement with the historical DashJ implementation over the derivation path, ECDH shared-secret representation, AES convention, or compact payload. The new queue-preserving branch prevents a recipient-purpose legacy request from being marked broken on such a failure, but the external account would still never be registered and payment attempts would continue failing indefinitely. Add a known-answer payload produced by the relevant DashJ version from synthetic fixed key material and pass it through the production derivation, decryption, parsing, and registration path, covering at least an AUTHENTICATION recipient and preferably TRANSFER key ID 3.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2115-2122: Include sender-only widening in the legacy failure classification
`accepted_by_legacy_widening` is described as identifying requests admitted only by the wider receive policy, but it checks only the recipient key. This PR also widens the sender policy from ENCRYPTION to AUTHENTICATION, so an AUTHENTICATION sender paired with a mint-valid ENCRYPTION or DECRYPTION recipient passes validation solely because of the receive-side widening while this flag remains false. If decryption or compact-xpub parsing then returns a permanent error, the ordinary arm marks the channel broken and clears the queued operation, preserving the destructive behavior that the new classification is intended to avoid for unverified legacy payloads. Derive the flag from both referenced keys and add a drain-level test for the sender-only widened shape.
| /// What this does NOT prove: that a payload produced by **dashj** decrypts | ||
| /// under our ECDH/AES conventions. That needs a dashj-generated known | ||
| /// answer, which no fixture in this repo has. It is why the drain treats a | ||
| /// permanent register fault on a legacy-cohort request as "leave queued" | ||
| /// rather than "break the channel". |
There was a problem hiding this comment.
🟡 Suggestion: Add a DashJ interoperability fixture for the newly accepted path
The new end-to-end test verifies key ID 3 derivation, ECDH, AES decryption, compact-xpub parsing, and registration, but both sides still use this repository's Rust implementations. As the test comment acknowledges, two internally consistent Rust paths cannot detect disagreement with the historical DashJ implementation over the derivation path, ECDH shared-secret representation, AES convention, or compact payload. The new queue-preserving branch prevents a recipient-purpose legacy request from being marked broken on such a failure, but the external account would still never be registered and payment attempts would continue failing indefinitely. Add a known-answer payload produced by the relevant DashJ version from synthetic fixed key material and pass it through the production derivation, decryption, parsing, and registration path, covering at least an AUTHENTICATION recipient and preferably TRANSFER key ID 3.
source: ['codex']
| let accepted_by_legacy_widening = our_identity | ||
| .get_public_key_by_id(*our_decryption_key_index) | ||
| .map(|k| { | ||
| !dash_sdk::platform::dashpay::recipient_key_purpose_is_valid( | ||
| k.purpose(), | ||
| ) | ||
| }) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
🟡 Suggestion: Include sender-only widening in the legacy failure classification
accepted_by_legacy_widening is described as identifying requests admitted only by the wider receive policy, but it checks only the recipient key. This PR also widens the sender policy from ENCRYPTION to AUTHENTICATION, so an AUTHENTICATION sender paired with a mint-valid ENCRYPTION or DECRYPTION recipient passes validation solely because of the receive-side widening while this flag remains false. If decryption or compact-xpub parsing then returns a permanent error, the ordinary arm marks the channel broken and clears the queued operation, preserving the destructive behavior that the new classification is intended to avoid for unverified legacy payloads. Derive the flag from both referenced keys and add a drain-level test for the sender-only widened shape.
| let accepted_by_legacy_widening = our_identity | |
| .get_public_key_by_id(*our_decryption_key_index) | |
| .map(|k| { | |
| !dash_sdk::platform::dashpay::recipient_key_purpose_is_valid( | |
| k.purpose(), | |
| ) | |
| }) | |
| .unwrap_or(false); | |
| let recipient_required_legacy_widening = our_identity | |
| .get_public_key_by_id(*our_decryption_key_index) | |
| .map(|key| { | |
| !dash_sdk::platform::dashpay::recipient_key_purpose_is_valid(key.purpose()) | |
| }) | |
| .unwrap_or(false); | |
| let sender_required_legacy_widening = contact_identity | |
| .get_public_key_by_id(*contact_encryption_key_index) | |
| .map(|key| key.purpose() != Purpose::ENCRYPTION) | |
| .unwrap_or(false); | |
| let accepted_by_legacy_widening = | |
| recipient_required_legacy_widening || sender_required_legacy_widening; |
source: ['codex']
There was a problem hiding this comment.
Resolved in 38e6c91 — Include sender-only widening in the legacy failure classification no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Review caught that `accepted_by_legacy_widening` inspected only the recipient key, while this PR widens the sender rule as well (ENCRYPTION-only to ENCRYPTION-or-AUTHENTICATION). An AUTHENTICATION sender paired with a mint-valid DECRYPTION/ENCRYPTION recipient therefore reached the decrypt purely because of the receive-side policy, yet the flag stayed false — so a decrypt or compact-xpub failure took the ordinary permanent arm and destroyed the channel, which is exactly what the classification exists to prevent for payloads whose dashj byte compatibility is unverified. The flag is now the OR of both referenced keys against their respective mint-side rules. `sender_only_legacy_shape_is_not_charged_for_a_decrypt_failure` pins the shape the reviewer named: AUTHENTICATION sender, DECRYPTION recipient, undecryptable ciphertext. Verified it catches the reported defect — with the sender term removed it fails with drained 1 vs 0 and the channel marked broken.
|
Both addressed in 38e6c91. Sender-only widening — you found a real hole in the classification, thank you. The flag inspected only the recipient key while the PR widens the sender rule too, so an AUTHENTICATION sender with a mint-valid recipient reached the decrypt purely on the receive-side policy and would still have had a convention gap charged to it. It is now the OR of both referenced keys against their respective mint-side rules.
DashJ fixture — I cannot close this one, and I would rather say so than simulate it. Producing a known answer needs a payload generated by the historical dashj build plus its key material; neither exists in this repo, and I have no way to generate one here. Writing a fixture from our own implementation would be worse than no fixture, because it would look like interop coverage while proving only self-consistency — which is precisely the limitation the test's own doc comment states. Your point that the queue-preserving branch does not fix a convention gap is correct, and I want to be plain about it: it bounds the damage to "still unpayable, recoverable later" instead of "unpayable forever, unrecoverable without the contact's action". Given the alternative is shipping nothing to 27 contacts that are already 100% broken today, I think bounded-and-recoverable is the right posture — but it is not a fix for that scenario and should not be read as one. What actually settles it is empirical, and the material exists: decrypting 2-3 of the real logged mainnet documents against the affected user's seed. I have flagged that in the description as a merge gate. If you would prefer this PR block until that runs, I have no objection — it needs the user's seed, so it is a coordination question, not an engineering one. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 5265-5288: Update the test after drain_pending_contact_crypto to
inspect the contact’s pending_contact_crypto queue and assert that the
RegisterExternal entry for contact is still present before checking
payment_channel_broken. Keep the existing drained == 0 assertion and use the
established wallet/identity access path to verify the deferred operation was not
removed.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c7b665f-783c-4fa2-be16-a7b003b0a248
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
…osing If the fix does not work on a real mainnet wallet, the current logs say a contact failed but not enough to say why — which is how this bug went undiagnosed in the first place. The open question (whether dashj-produced ciphertext decrypts under our ECDH/AES conventions) can only be answered from an exported log, so the log has to carry the answer. Three additions, all public metadata — never the shared secret, never the decrypted xpub, which is the contact's payment key and would leak into a log the user hands over: - Our identity's key inventory (id:purpose/type, disabled marker), once per drain that has external builds queued. The whole bug is a statement about this layout: an identity minted before DashPay encryption keys existed carries only AUTHENTICATION/TRANSFER slots, and nothing downstream reads correctly without it. - Per-attempt context before anything can fail: both key ids with their purposes and types, the HD identity index, the ECDH path, the ciphertext length, and whether the widened receive policy is what admitted the request. - A one-line pass verdict (entries / drained / still_queued), so "did the legacy contacts build?" is answerable without counting lines in a multi-megabyte export. The two failure messages now state what they imply, because the distinction is the whole diagnosis and is not obvious from the error text alone: - decrypt failure ⇒ the shared secret did not match (AES-CBC under a wrong key is pseudorandom and PKCS7 rejects it ~99.6% of the time), i.e. a key-derivation or ECDH-convention gap; - decrypt success + parse failure ⇒ the secret was right and only the plaintext layout differs. The decrypted length now leads that message, since it is the discriminator.
The reporter's mainnet TestFlight was 9.0.0 (28); 29 is the build carrying dashpay/platform#4372 + #4373 (legacy dashj key purposes accepted, plus the drain diagnostics) and the Pay gate, so the two runs are distinguishable in a crash report or an exported log.
|
Confirming this from the Android side: we hit the same failure class during kotlin-sdk cutover field testing on mainnet, from the same legacy-dashj cohort. A long-lived wallet (~150 contacts, dashj-era history) had 3 established contacts whose inbound requests the SDK rejected with the verbatim — contacts that work normally in the shipped dashj-based app, where both sides see identical transaction history. Our own interop analysis of v11.9-era Android and shipped iOS independently reached the same conclusion this PR implements: reaching pre-SDK mobile contacts requires key-purpose tolerance (the AUTHENTICATION fallback) on the receive path. The shape here looks right to us — liberal on receive, while the stricter |
The dashj interoperability gap is now closed — by real mainnet data, not a fixtureThe reporting user ran a build carrying this branch against their live mainnet wallet. Result: Before the fix that same wallet built 2 external accounts out of 29 and logged 396 This answers the finding I could not close with a test. Those 27 payloads are genuine dashj-produced I still think a committed fixture would be worth having for regression safety, but it is no longer the difference between merging and not: the interop claim now rests on 27 independent live samples rather than on reading the code. The queue-preserving branch for legacy-cohort permanent faults never fired, which is the outcome it was designed for — it stays as insurance for a cohort we have not seen yet. Note on scope: the same run surfaced a separate, pre-existing bug — after 8 contact payments the wallet reports |
Build 29 carried dashpay/platform#4372 + #4373 and proved the legacy-contact fix on mainnet (29/29 external accounts built, 8 payments). It then hit a second, separate bug: contact payments fund only from BIP44, so the ninth payment failed with 'Insufficient funds: available 41505' while the balance showed plenty. 30 adds dashpay/platform#4378, which pools BIP44 + BIP32 + DashPay receiving accounts on the contact-payment path — the funding set a plain send has used since #4329.
5 participants
Issue being fixed or feature implemented
A mainnet user could not pay any DashPay contact established before the iOS client existed — contacts brought over by importing an Android/dashj seed. Every attempt failed with:
Contacts created fresh on iOS worked normally, which is what made this look like a restore bug rather than a validation one.
Device logs (mainnet, one 29-contact wallet) show the split exactly:
drain: contact request key-purpose mismatchRegistered DashpayExternalAccountThe rejected purposes are all on our own key, as referenced by the inbound legacy document:
TRANSFERAUTHENTICATIONAUTHENTICATIONAUTHENTICATIONAUTHENTICATION)AUTHENTICATIONRoot cause.
validate_contact_requestrequiredENCRYPTION/DECRYPTIONforrecipientKeyIndex, but legacy dashjcontactRequestdocuments point at the recipient'sAUTHENTICATION(key ids 0-2) orTRANSFER(key id 3) key. The drain classified this as a purpose-only mismatch and — correctly — refused to mark the channel broken, so it retried forever and never succeeded.Two things kept it invisible:
RegisterExternal; the signer-backed drain completes it). Contacts established live build inline and surface real errors. That is the entire difference between the two cohorts.send_paymentcallsdrain_pending_contact_cryptoatpayments.rs:1099and discards its result, so every drain failure reaches the user as the same generic lookup error.The previous policy was calibrated on a 368-document testnet census that contains no dashj-era cohort.
What was done?
Split the purpose policy in two rather than widening the existing predicate — the existing one is also called from
create_contact_request(rs-sdk/src/platform/dashpay/contact_request.rs:243,265), so widening it would have silently relaxed the key we pick for our own outgoing requests too.recipient_key_purpose_is_valid— unchanged, governs what we mint.select_recipient_key_indexstill prefersDECRYPTIONthenENCRYPTION, so key separation on new documents is preserved.recipient_key_purpose_is_acceptable_on_receive/sender_key_purpose_is_acceptable_on_receive(new,rs-sdk/src/platform/dashpay/contact_request.rs) — govern what we accept from immutable history.Accepted on receive:
DECRYPTION,ENCRYPTION,AUTHENTICATION,TRANSFERENCRYPTION,AUTHENTICATIONvalidate_contact_request(rs-platform-wallet/src/wallet/identity/crypto/validation.rs) now uses the receive-side predicates. TheECDSA_SECP256K1key-type gate and the disabled-key check are untouched — those are the checks that actually protect the ECDH. Purpose is not a security boundary here: ECDH is defined over the secp256k1 keypair, and DIP-9 indexes the identity-key tree by key type and id, never by purpose, so the same derivation reaches all of them.SYSTEM/VOTING/OWNERstay rejected, and stay a non-permanent purpose mismatch, so a later evidence-driven widening can still recover those contacts instead of finding their channels broken.Doc comments on
validate_contact_requestandselect_recipient_key_indexwere updated to record the legacy cohort and why the mint-side rule deliberately stays stricter than the receive-side one.How Has This Been Tested?
cargo test -p platform-wallet -p dash-sdk— 664 + 203 tests pass.cargo clippy --all-targetsclean,cargo fmtapplied.New tests in
validation.rs:legacy_dashj_recipient_key_purposes_are_accepted—AUTHENTICATIONandTRANSFERrecipient keys validate. This is the regression guard for the 27 unpayable contacts.legacy_dashj_authentication_pair_is_accepted— the fullAUTHENTICATION/AUTHENTICATIONpair, the exact shape 15 of the logged failures took.recipient_node_operational_key_is_rejected_as_purpose_mismatch—SYSTEM/VOTING/OWNERstill rejected, and still non-permanent (purpose_mismatch && !hard_error).New test in
contact_request.rs:receive_side_accepts_the_legacy_dashj_cohort/receive_side_still_refuses_node_operational_purposes— pin the two new predicates.Rewritten because they asserted the behaviour being reversed:
mint_side_still_refuses_authentication_and_transfer(wasrecipient_key_purpose_rejects_authentication, now explicitly scoped to the mint side),unaccepted_sender_purpose_is_a_purpose_mismatchandtest_sender_wrong_purpose(both now use a purpose that is still rejected).Not yet verified, and worth gating the merge on: our ECDH convention (
rs-platform-encryption/src/ecdh.rs) has only a self-generated known-answer test — nothing pins it against real dashj output. If it diverges, this change does not risk funds (parse_compact_xpubrequires exactly 69 bytes, so a silently-accepted bogus xpub is ~2⁻⁵⁵), but a cleanPermanentdecrypt error would flip those 27 contacts from "queued" topayment_channel_broken, which only a superseding contact request can heal. Decrypting 2-3 of the real logged documents against the user's seed would settle it.Breaking Changes
None. The change only widens what is accepted from inbound documents; the documents we create are unaffected.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit