perf(platform-wallet): keep the contact fetch off the drain's repeating-rejection path - #4373
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.
…ng-rejection path A `RegisterExternal` entry the key-purpose policy turns away stays queued on purpose — the `contactRequest` is immutable, so what might change is our acceptance policy, and breaking the channel instead would need a superseding request from the contact to heal. The consequence is that the sweep re-offers the entry every pass, forever. The drain fetched the contact identity from Platform *before* it could validate, so each of those passes spent a round trip to reach a verdict it already had locally: the dominant rejection is on the `recipientKeyIndex` key, which is our own and always resident. Mainnet logs from one wallet show 27 contacts and 396 fetch-then-reject cycles in a single session, with a second wallet-lifetime of them still to come. Split `validate_contact_request` into `validate_sender_key` (needs the counterparty) and `validate_recipient_key` (needs only us); the composed function is unchanged, so every other caller keeps the identical contract. The drain now runs the recipient half first and only fetches when that passes. Both halves route their failure through one `apply_drain_validation_failure` helper so they classify identically — purpose-only leaves the entry queued, anything else marks the channel broken. Per-entry purpose rejections drop to DEBUG and are aggregated into a single end-of-drain WARN carrying the distinct reasons and their counts. That signal is how the legacy dashj cohort was found in an exported log, so it has to stay visible — just not 27 times a pass. `drain_decides_our_own_key_fault_without_fetching_the_contact` pins the ordering: a wallet-owned but keyless owner must have its channel marked broken with no contact fetch configured on the mock. Under the old ordering the fetch failed first and the verdict was never reached.
📝 WalkthroughWalkthroughThe wallet separates sender and recipient key validation and classifies missing keys as retryable. Deferred contact operations apply centralized failure handling. DashPay payments use pooled funding across account types and release reservations after rejected broadcasts. ChangesWallet identity and payment flows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PaymentSender
participant ContactDrain
participant ContactIdentity
participant FundingResolver
participant Broadcast
participant WalletAccounts
PaymentSender->>ContactDrain: drain deferred contact operations
ContactDrain->>ContactIdentity: fetch contact after local validation
ContactIdentity-->>ContactDrain: contact key data
ContactDrain-->>PaymentSender: validation outcome
PaymentSender->>FundingResolver: resolve pooled funding accounts
FundingResolver-->>PaymentSender: UTXOs and derivation paths
PaymentSender->>Broadcast: submit signed payment
Broadcast-->>PaymentSender: rejected broadcast
PaymentSender->>WalletAccounts: release reservations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 52fe0d9) |
…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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The recipient-first split removes the contact fetch from local rejection paths, but it changes the established classification of requests that combine a recipient-purpose mismatch with a hard sender-key fault. The main purpose-only performance path also lacks a direct regression test, and the internal validation halves unnecessarily expand the public API. Source: reviewer backends: codex general — gpt-5.6-sol; codex rust-quality — gpt-5.6-sol; final verifier backend: codex — 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— 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)
🟡 3 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/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2084-2095: Preserve hard-error precedence across the split validation
Applying the recipient result before fetching and validating the sender changes the failure policy for mixed faults. If the recipient key has only an unaccepted purpose but the sender key is missing, disabled, or the wrong type, this branch classifies the request as purpose-only and leaves it queued without examining the sender. The previous composed `validate_contact_request` merged both halves, propagated `hard_error`, and made `is_purpose_only()` false, so the channel was marked broken and the queue entry was cleared. This contradicts the documented mixed-failure invariant in `ContactRequestValidation`, the merge regression tests, and the PR's statement that only validation order changes. Preserve aggregate classification for mixed failures, or explicitly redefine the policy and update its documentation and regression coverage.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:5029-5043: Exercise the purpose-only no-fetch path directly
This test uses a missing recipient key, which is a hard failure. It proves that at least one local fault is checked before `Identity::fetch`, but it does not pin the PR's principal performance behavior: an unaccepted recipient purpose must remain queued while causing zero contact fetches across repeated drains. An implementation that short-circuits hard recipient faults but still fetches for purpose mismatches would pass this test and reintroduce the reported repeating network cost. Add a test with a valid ECDSA recipient key using an unaccepted purpose, drain more than once, and assert that no identity fetch occurs, the entry remains queued, and the channel remains unbroken.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:182-244: Keep the drain-specific validation halves crate-private
Both `wallet::identity::crypto` and its `validation` module are public, so declaring `validate_sender_key` and `validate_recipient_key` as `pub` exposes the new decomposition helpers to downstream crates. Their documentation identifies the internal deferred-crypto drain as the reason for the split, and the only callers are inside this crate. Use `pub(crate)` for these helpers so external callers continue through the complete `validate_contact_request` contract and the crate does not acquire an unnecessary public API commitment.
| if !recipient_validation.is_valid { | ||
| if self | ||
| .apply_drain_validation_failure( | ||
| entry, | ||
| &recipient_validation, | ||
| &mut policy_blocked, | ||
| ) | ||
| .await | ||
| { | ||
| cleared.push(entry.key()); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
🟡 Suggestion: Preserve hard-error precedence across the split validation
Applying the recipient result before fetching and validating the sender changes the failure policy for mixed faults. If the recipient key has only an unaccepted purpose but the sender key is missing, disabled, or the wrong type, this branch classifies the request as purpose-only and leaves it queued without examining the sender. The previous composed validate_contact_request merged both halves, propagated hard_error, and made is_purpose_only() false, so the channel was marked broken and the queue entry was cleared. This contradicts the documented mixed-failure invariant in ContactRequestValidation, the merge regression tests, and the PR's statement that only validation order changes. Preserve aggregate classification for mixed failures, or explicitly redefine the policy and update its documentation and regression coverage.
source: ['codex']
There was a problem hiding this comment.
Resolved in d822704 — Preserve hard-error precedence across the split validation 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.
| /// A `RegisterExternal` entry whose fault lies in OUR OWN key is decided | ||
| /// without a Platform round trip. | ||
| /// | ||
| /// The owner here is wallet-owned (so the drain gets past the HD-index | ||
| /// bail) but carries no keys at all, so `recipientKeyIndex` 0 resolves to | ||
| /// nothing — a hard, permanent fault that must break the channel. The mock | ||
| /// SDK has NO contact-identity fetch configured, so this can only pass if | ||
| /// the recipient half of the validation ran *before* the fetch: the old | ||
| /// ordering fetched first, failed transiently, and left the channel intact. | ||
| /// | ||
| /// That ordering is what keeps a purpose-rejected entry — which stays | ||
| /// queued by design, and so is retried on every sweep forever — from | ||
| /// spending a network round trip each time. | ||
| #[tokio::test] | ||
| async fn drain_decides_our_own_key_fault_without_fetching_the_contact() { |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the purpose-only no-fetch path directly
This test uses a missing recipient key, which is a hard failure. It proves that at least one local fault is checked before Identity::fetch, but it does not pin the PR's principal performance behavior: an unaccepted recipient purpose must remain queued while causing zero contact fetches across repeated drains. An implementation that short-circuits hard recipient faults but still fetches for purpose mismatches would pass this test and reintroduce the reported repeating network cost. Add a test with a valid ECDSA recipient key using an unaccepted purpose, drain more than once, and assert that no identity fetch occurs, the entry remains queued, and the channel remains unbroken.
source: ['codex']
There was a problem hiding this comment.
Resolved in d822704 — Exercise the purpose-only no-fetch path directly 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.
| @@ -213,9 +227,26 @@ pub fn validate_contact_request( | |||
| } | |||
| } | |||
|
|
|||
| // ----------------------------------------------------------------------- | |||
| // Recipient key validation | |||
| // ----------------------------------------------------------------------- | |||
| validation | |||
| } | |||
|
|
|||
| /// The recipient half of [`validate_contact_request`] — the checks that need | |||
| /// only **our own** identity, which is always already resident. | |||
| /// | |||
| /// Split out because the deferred-crypto drain would otherwise pay a Platform | |||
| /// round trip (`Identity::fetch` of the contact) before it could discover that | |||
| /// the request is unusable for a reason it could have known locally. A | |||
| /// purpose-rejected entry stays queued by design — the policy, not the | |||
| /// immutable document, is what might change — so that fetch was repeating on | |||
| /// every sweep, forever. Mainnet logs from one wallet show 27 contacts and 396 | |||
| /// such fetch-then-reject cycles in a single session. Running this half first | |||
| /// costs nothing and removes the network entirely from that loop. | |||
| pub fn validate_recipient_key( | |||
There was a problem hiding this comment.
🟡 Suggestion: Keep the drain-specific validation halves crate-private
Both wallet::identity::crypto and its validation module are public, so declaring validate_sender_key and validate_recipient_key as pub exposes the new decomposition helpers to downstream crates. Their documentation identifies the internal deferred-crypto drain as the reason for the split, and the only callers are inside this crate. Use pub(crate) for these helpers so external callers continue through the complete validate_contact_request contract and the crate does not acquire an unnecessary public API commitment.
source: ['codex']
There was a problem hiding this comment.
Resolved in 608dac5 — Keep the drain-specific validation halves crate-private 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.
… 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.
…retry-cost # Conflicts: # packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs # packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
…y explicit Review on #4373 caught that deciding on the recipient half alone silently changed the mixed-failure policy, contradicting this PR's own claim that only the ordering changed. It did: when our key is purpose-rejected AND the contact's key carries a hard fault, the composed validator merged both, saw `hard_error`, and marked the channel permanently broken; stopping at the recipient half classifies it purpose-only and leaves it queued. Keeping that behaviour, but as a stated decision rather than an accident. The `hard_error` precedence exists to stop a permanent fault from becoming a retry-forever loop — and that loop was expensive precisely because each retry fetched. With the fetch gone it costs a map lookup per sweep, while marking the channel broken is unappealable by the user: only a fresh request from the contact clears it. A sender-side hard fault still breaks the channel as soon as our own key stops being the blocker. Documented on `validate_recipient_key` and at the drain call site. `unaccepted_recipient_purpose_never_fetches_and_stays_recoverable` pins both halves: our key carries an unaccepted purpose, the contact identity IS configured on the mock with a hard sender-side fault, and the drain runs twice. Under the old ordering the fetch succeeds, the merge escalates to broken, and the entry is cleared (verified: the test fails with drained 1 vs 0 against the parent branch's file). Passing therefore proves no fetch was spent — a short-circuit that handled only hard recipient faults would still fail it. Also makes `validate_sender_key` / `validate_recipient_key` `pub(crate)`: they are drain-internal decomposition, and both the `crypto` and `validation` modules are public, so `pub` would have committed them as external API.
|
All three addressed in 608dac5 (plus d822704 merging the parent branch forward). 1. Hard-error precedence — you were right, and this was a real defect. I changed the mixed-failure policy and then wrote "only validation order changes" in the description. It was not just ordering: our key purpose-rejected + the contact's key hard-faulted went from broken to queued. I kept the new behaviour, but as a stated decision rather than an accident. The If you would rather have the original semantics preserved exactly, the only way is to fetch even when the recipient half rejects — which gives back the entire cost this PR removes. I think that trade is wrong, but it is your call to overrule. 2. Purpose-only no-fetch path — good catch, my test did not discriminate. Passing therefore proves no round trip was spent, and the "short-circuits hard faults but still fetches for purpose mismatches" implementation you described fails it. It covers finding 1 at the same time. 3. 667 tests pass, clippy clean. |
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.
…retry-cost # Conflicts: # packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current head explicitly documents and tests the intentional recipient-first mixed-failure policy, directly covers repeated purpose-only drains without fetching the sender identity, and keeps the split validation helpers crate-private. The remaining clone suggestion identifies a possible micro-optimization, but the same full-identity clone already occurred on every retry before this PR and is not required to achieve the PR's stated goal of eliminating the repeated Platform fetch, so it is outside this review's scope.
Source: Codex general, rust-quality, and security-auditor reviewers — gpt-5.6-sol; 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— rust-quality (completed),gpt-5.6-sol— security-auditor (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)
…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.
…retry-cost # Conflicts: # packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The recipient-first validation split correctly avoids fetching the counterparty for locally decidable failures, preserves the composed validator for existing callers, and directly tests the repeated purpose-only path. One in-scope performance suggestion remains: the new summary path clones every rejection reason on every sweep, including reasons already represented in the map.
Source: Codex general reviewer — gpt-5.6-sol; Codex rust-quality reviewer — gpt-5.6-sol; 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— 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/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2513-2516: Move validation errors into the summary instead of cloning them
`apply_drain_validation_failure` borrows the validation result, so every purpose-only reason is cloned before the map lookup. These entries intentionally remain queued and revisit this path on every sweep; consequently, an equivalent reason already present in `policy_blocked` still causes another temporary heap allocation. Neither caller uses its validation result afterward, so the helper can take `ContactRequestValidation` by value and move its strings after logging. Alternatively, retain the borrowed API but use `get_mut` first, cloning only when a distinct reason is inserted for the first time.
| if validation.is_purpose_only() { | ||
| for reason in &validation.errors { | ||
| *policy_blocked.entry(reason.clone()).or_default() += 1; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Move validation errors into the summary instead of cloning them
apply_drain_validation_failure borrows the validation result, so every purpose-only reason is cloned before the map lookup. These entries intentionally remain queued and revisit this path on every sweep; consequently, an equivalent reason already present in policy_blocked still causes another temporary heap allocation. Neither caller uses its validation result afterward, so the helper can take ContactRequestValidation by value and move its strings after logging. Alternatively, retain the borrowed API but use get_mut first, cloning only when a distinct reason is inserted for the first time.
| if validation.is_purpose_only() { | |
| for reason in &validation.errors { | |
| *policy_blocked.entry(reason.clone()).or_default() += 1; | |
| } | |
| if validation.is_purpose_only() { | |
| for reason in &validation.errors { | |
| if let Some(count) = policy_blocked.get_mut(reason) { | |
| *count += 1; | |
| } else { | |
| policy_blocked.insert(reason.clone(), 1); | |
| } | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in abe80c6 — Move validation errors into the summary instead of cloning them 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.
…ummary Review on #4373: the helper borrowed the validation result and cloned every reason before the map lookup. These entries stay queued by design and revisit the path on every sweep, so that is an allocation per contact per pass for the life of the wallet — 27 per pass on the mainnet wallet that motivated this work. Takes the validation by value and moves the strings instead. Neither caller uses the result afterwards, so nothing is lost, and the common case (a reason already counted) now allocates nothing at all.
![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-platform-wallet/src/wallet/identity/network/contact_requests.rs (1)
2374-2392: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the retry cost for legacy-widened permanent failures.
A legacy-widened entry that fails registration permanently is neither cleared nor marked broken. It therefore returns on every drain. Unlike the purpose-only case this PR made fetch-free, this path already spent a contact
Identity::fetch, a provider ECDH call, and a decrypt before reaching here, and it repeats all three on every sweep. It also emits one INFO line per entry per sweep at Line 2295, which is the same log-volume pattern the aggregated summary removed for purpose rejections.Consider recording an attempt count or a last-attempt timestamp on the queue entry and applying backoff, so a wallet with many legacy contacts does not pay a Platform round trip per contact per sweep indefinitely.
🤖 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 2374 - 2392, Bound retries for the legacy-widened permanent-failure branch in the contact-request drain, anchored by accepted_by_legacy_widening and the queued entry state. Record retry metadata such as an attempt count or last-attempt timestamp, apply backoff before re-fetching and reprocessing eligible entries, and avoid emitting the per-entry log on every sweep while preserving eventual retry behavior.
🤖 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-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 2374-2392: Bound retries for the legacy-widened permanent-failure
branch in the contact-request drain, anchored by accepted_by_legacy_widening and
the queued entry state. Record retry metadata such as an attempt count or
last-attempt timestamp, apply backoff before re-fetching and reprocessing
eligible entries, and avoid emitting the per-entry log on every sweep while
preserving eventual retry behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 775c45a5-2ec7-411e-a665-6b8e45b35658
📒 Files selected for processing (6)
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.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/payments.rspackages/rs-sdk/src/platform/dashpay/contact_request.rspackages/rs-sdk/src/platform/dashpay/mod.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The recipient-first validation split removes the repeated contact fetch while preserving the intended queue policy, and the prior reason-cloning finding is fixed at the current head. Two in-scope suggestions remain: avoid eagerly constructing INFO-only key inventories, and qualify the unauthenticated AES-CBC diagnostics instead of presenting PKCS7 results as conclusive evidence about the shared secret.
Source: Codex general reviewer — gpt-5.6-sol; Codex rust-quality reviewer — gpt-5.6-sol; 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— 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/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1943: Skip key-inventory construction when INFO logging is disabled
The key-inventory block runs before `tracing::info!` can apply its level filter. Every drain containing a `RegisterExternal` entry therefore builds a `BTreeSet`, acquires the wallet-manager read lock, allocates one formatted string per identity key, and joins those strings even when INFO events are disabled. Purpose-rejected entries intentionally remain queued and revisit this path on every sweep, so this adds recurring preparation cost to the path this PR is optimizing. Guard the entire block with `tracing::enabled!(tracing::Level::INFO)`.
In `packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs:453-504: Do not infer shared-secret correctness from PKCS7 padding
`decrypt_extended_public_key` performs unauthenticated AES-CBC decryption and validates only PKCS7 padding. A padding failure can result from malformed or corrupted ciphertext as well as a mismatched shared secret, while an incorrect key has roughly a 1/256 chance of producing acceptable padding and reaching the parsing fallback. The exported errors nevertheless state conclusively that a failure means the shared secret did not match and that successful unpadding means it matched and only the plaintext layout differs. This can misdirect investigation of the legacy interoperability issue. Describe these outcomes as likely or possible causes, unless the ciphertext format is authenticated before making a definitive attribution.
| // reference those ids and nothing downstream makes sense without | ||
| // knowing that. Reading it back off an exported log beats asking the | ||
| // user to query Platform. On-chain public metadata only; no key data. | ||
| { |
There was a problem hiding this comment.
🟡 Suggestion: Skip key-inventory construction when INFO logging is disabled
The key-inventory block runs before tracing::info! can apply its level filter. Every drain containing a RegisterExternal entry therefore builds a BTreeSet, acquires the wallet-manager read lock, allocates one formatted string per identity key, and joins those strings even when INFO events are disabled. Purpose-rejected entries intentionally remain queued and revisit this path on every sweep, so this adds recurring preparation cost to the path this PR is optimizing. Guard the entire block with tracing::enabled!(tracing::Level::INFO).
| { | |
| if tracing::enabled!(tracing::Level::INFO) { |
source: ['codex']
There was a problem hiding this comment.
Resolved in d9afc99 — Skip key-inventory construction when INFO logging is disabled 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.
| @@ -479,9 +491,18 @@ impl<B: TransactionBroadcaster + ?Sized> DashPayView<'_, B> { | |||
| .map_err(Permanent)?, | |||
| Err(_) => { | |||
| key_wallet::bip32::ExtendedPubKey::decode(&decrypted_xpub_bytes).map_err(|e| { | |||
| // Reaching here means the DECRYPT succeeded — PKCS7 unpadded | |||
| // cleanly, so the shared secret was almost certainly right — | |||
| // and only the plaintext LAYOUT is unexpected. The decrypted | |||
| // length is the discriminator, so it leads the message. The | |||
| // bytes themselves are never logged: they are the contact's | |||
| // payment xpub, and this text reaches an exported log. | |||
| Permanent(PlatformWalletError::InvalidIdentityData(format!( | |||
| "Decrypted contact xpub is neither a 69-byte DIP-15 compact form \ | |||
| nor a 78/107-byte BIP32/DIP-14 serialization: {e}" | |||
| "Decrypted contact xpub is {} bytes — neither a 69-byte DIP-15 compact \ | |||
| form nor a 78/107-byte BIP32/DIP-14 serialization ({e}). The decrypt \ | |||
| itself SUCCEEDED, so the shared secret matched and it is the plaintext \ | |||
| layout that differs", | |||
There was a problem hiding this comment.
🟡 Suggestion: Do not infer shared-secret correctness from PKCS7 padding
decrypt_extended_public_key performs unauthenticated AES-CBC decryption and validates only PKCS7 padding. A padding failure can result from malformed or corrupted ciphertext as well as a mismatched shared secret, while an incorrect key has roughly a 1/256 chance of producing acceptable padding and reaching the parsing fallback. The exported errors nevertheless state conclusively that a failure means the shared secret did not match and that successful unpadding means it matched and only the plaintext layout differs. This can misdirect investigation of the legacy interoperability issue. Describe these outcomes as likely or possible causes, unless the ciphertext format is authenticated before making a definitive attribution.
source: ['codex']
There was a problem hiding this comment.
Resolved in d9afc99 — Do not infer shared-secret correctness from PKCS7 padding 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.
…-retry-cost # Conflicts: # packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs # packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
…ot verdicts Review on #4373, and the sharper of the two findings. The messages added for the legacy-interop investigation asserted their conclusions outright: a PKCS7 rejection "means the ECDH shared secret did not match", and a clean unpad "SUCCEEDED, so the shared secret matched". Neither follows. `decrypt_extended_public_key` is unauthenticated AES-CBC with padding as its only check, so a rejection is equally consistent with a corrupted ciphertext, and a wrong key clears PKCS7 roughly 1 in 256 times and lands in the length branch instead. A diagnostic whose whole purpose is to direct an investigation is the last place to overstate certainty, so both now name the likely cause and the alternative that produces the same symptom. Also gates the identity key-inventory block on `tracing::enabled!(INFO)`. It allocates a set, a string per key and a join, and takes the wallet-manager read lock; purpose-rejected entries revisit this path every sweep, so leaving that unconditional added recurring cost to the path this PR exists to make cheap.
|
Both addressed in d9afc99, plus the merge of Padding as evidence — you are right, and this was the more important of the two. The messages asserted their conclusions outright ("the ECDH shared secret did not match", "the decrypt itself SUCCEEDED, so the shared secret matched"). Neither follows from unauthenticated CBC with padding as the only check: a rejection is equally consistent with a corrupted ciphertext, and a wrong key clears PKCS7 roughly 1 in 256 times and lands in the length branch instead. That overclaim is worse than a normal wording slip, because these messages exist precisely to steer the legacy-interop investigation — a confident wrong pointer costs more than a vague right one. Both now name the likely cause and the alternative that produces the same symptom. Key inventory now gated on 668 tests, clippy clean. |
…ment as for a plain send (#4378) Co-authored-by: Roman <51091564+jeanpierreroma@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 1101: The payment build flow around send_payment must retain the
reservation token alongside tx instead of discarding it. Propagate that token
through the persister.store failure path and rejected-broadcast handling, and
pass it to reservation cleanup so both paths use owner-guarded release rather
than unconditional cleanup; mirror finalize_transaction’s token-retention
pattern.
- Around line 6008-6110: Update
contact_payment_funds_from_a_dashpay_receiving_account to execute send_payment
with the rejecting broadcaster provided by with_rejecting_broadcaster, then
assert the result is PlatformWalletError::TransactionBroadcast(_). Replace the
permissive error-message checks so derivation, signing, or funding failures
cannot satisfy the test; preserve the receiving-account funding setup and verify
the pooled transaction reaches broadcast.
🪄 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: 294c2ec7-3e8c-4ef9-9858-8e464da925c4
📒 Files selected for processing (3)
packages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rs
| self.drain_pending_contact_crypto(provider).await; | ||
|
|
||
| let (payment_address, used_flip_changeset, tx, fee) = { | ||
| let (payment_address, used_flip_changeset, tx, fee, funding_accounts) = { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep reservation cleanup owner-guarded on every pre-broadcast failure.
Line 1101 retains funding_accounts but drops the reservation token. If persister.store fails at Line 1330, send_payment returns while the signed inputs remain reserved. If broadcast later returns Rejected, Lines 1350-1353 pass None, so cleanup is unconditional.
A TTL sweep can release this build and a later build can reserve the same input before rejection handling runs. The unconditional release can then clear the later build's reservation.
Return the build reservation token with tx. Use owner-guarded cleanup for both the persistence-error path and the rejected-broadcast path. Mirror finalize_transaction, which retains the token for this race.
Also applies to: 1310-1316, 1338-1353
🤖 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/payments.rs` at line
1101, The payment build flow around send_payment must retain the reservation
token alongside tx instead of discarding it. Propagate that token through the
persister.store failure path and rejected-broadcast handling, and pass it to
reservation cleanup so both paths use owner-guarded release rather than
unconditional cleanup; mirror finalize_transaction’s token-retention pattern.
… contact channel Review pushed back that a contact request should never be rejected locally, since it cleared consensus. Not absolute — ECDH over a BLS or hash-only key is impossible whatever consensus says — but the principle lands on the part that matters: `payment_channel_broken` is permanent, heals only when the CONTACT sends a fresh request, and the user cannot appeal it. The decision now turns on whether the fault can ever resolve, not on how badly validation failed: - An **absent key id** becomes retryable. Consensus checks nothing about the keys a `contactRequest` names, and identities gain keys — that is what the DashPay enablement flow does, and what dashwallet-ios#981 exists to notice when it happened on another device. A document referencing a key we do not have *yet* was ending the relationship over a gap that may close on its own. Same on the contact's side: the drain's "contact encryption key missing" arm now leaves the entry queued instead of breaking the channel. - **Key type** and **disabled key** stay permanent: a key's type is fixed for its lifetime, and a key we revoked is one we will never use again. Both are facts about immutable or deliberate state, not about today's snapshot. `is_permanent()` replaces `is_purpose_only()` as the drain's predicate, so the question it answers is the one being asked. The co-occurrence guard survives unchanged — a purpose mismatch alongside a genuinely permanent fault is still permanent. This is also the failure mode this stack already lived through: the key-purpose policy was wrong for two years, and only its softer classification kept 27 mainnet contacts recoverable. Had it been "hard", they would have been permanently broken with no path back. Two ordering tests were pinned on an absent key being permanent, so they no longer discriminated; both re-armed on a BLS key at the referenced index, which is permanent for a reason that cannot change. `an_absent_key_is_not_a_permanent_fault` and `a_non_ecdh_key_type_is_a_permanent_fault` pin the new boundary — verified the first fails when the absent key is put back on the permanent classification.
…to fix/dashpay-drain-retry-cost
|
Addressed in 0eac01e — you are right about the part that matters, and it exposed a hole I had walked past. Where the principle is exactly right. The concrete hole. An absent key id was classified permanent. But consensus checks nothing about the keys a Where I would not go all the way. "Never reject locally" cannot be absolute: ECDH over a BLS or hash-only key is impossible whatever consensus says. So the predicate is no longer "how badly did validation fail" but "can this ever resolve":
On scope — this changes policy rather than ordering, so it arguably wanted its own PR, but it is a direct correction to the failure policy this PR already redefines, and splitting it would have left the wrong behaviour on the branch under review. Two ordering tests were pinned on an absent key being permanent and stopped discriminating; both re-armed on a BLS key at the referenced index. 672 tests, clippy clean. #4378 is merged into this branch, so it now carries the pooled-funding fix as well. |
There was a problem hiding this comment.
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)
2526-2535: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse retryable-validation labels for aggregated failures.
apply_drain_validation_failurenow adds absent-key failures topolicy_blocked. The current comments and logs describe every retained entry as a key-purpose rejection. This reports the wrong cause for identities that do not yet contain the referenced key.
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs#L2526-L2535: Change the debug message to describe retryable contact-request key validation.packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs#L1992-L2000: Update the aggregation comment to include absent-key failures.packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs#L2472-L2485: Change the summary warning to describe retryable validation failures while retaining the per-reason map.🤖 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 2526 - 2535, Update the retryable-validation messaging across packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2526-2535 in the non-permanent branch of apply_drain_validation_failure to describe retryable contact-request key validation; update the aggregation comment at packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1992-2000 to include absent-key failures; and update the summary warning at packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2472-2485 to describe retryable validation failures while preserving the per-reason policy_blocked map.
🤖 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.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 2526-2535: Update the retryable-validation messaging across
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2526-2535
in the non-permanent branch of apply_drain_validation_failure to describe
retryable contact-request key validation; update the aggregation comment at
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1992-2000
to include absent-key failures; and update the summary warning at
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2472-2485
to describe retryable validation failures while preserving the per-reason
policy_blocked map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 42ce50d0-1dab-41e9-aaa0-9fefc3d84739
📒 Files selected for processing (3)
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The recipient-first drain behavior and both previously reported diagnostics/performance issues are correct at the current head. Two in-scope suggestions remain: retain the pooled funding reservation token through all pre-broadcast exits, and align the public validation API with the new retryable absent-key policy.
Source: Codex general, rust-quality, and security-auditor reviewers — gpt-5.6-sol; 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— rust-quality (completed),gpt-5.6-sol— security-auditor (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:1277-1353: Retain the pooled reservation token through pre-broadcast exits
`build_signed` discards the `ReservationToken` stamped across the contributing accounts. If `persister.store` fails at line 1330, the function returns without releasing the signed transaction's reserved inputs. If broadcast is rejected, passing `None` performs an unconditional release by outpoint. A TTL sweep can release the original reservation and allow another build to reserve the same inputs while broadcast is awaiting completion; the old build's unconditional cleanup can then clear the newer reservation. Use `build_signed_reserved`, carry its token alongside `tx` and `funding_accounts`, release with that token before returning a persistence error, and pass it to `release_reservation_after_rejected_broadcast` after a definitive rejection.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:29-103: Align the public retryability gate with absent-key classification
`add_absent_key_error` intentionally leaves `hard_error` false because an absent key is retryable, but the public documentation still says missing keys are permanent and directs callers to `is_purpose_only()` for skip-versus-break decisions. That predicate returns false for a lone absent-key failure, while returning true for a purpose mismatch combined with an absent key even though purpose is not then the sole cause. The in-crate drain correctly uses `is_permanent()`, but downstream callers following the advertised public contract can retain the obsolete permanent treatment. Make `is_permanent()` the documented canonical disposition predicate and deprecate or explicitly restrict `is_purpose_only()` to diagnostics, or track absent-key failures separately so its stated sole-cause semantics remain exact. Add coverage for lone-absent and purpose-plus-absent results.
| @@ -1269,7 +1307,13 @@ impl<B: TransactionBroadcaster + ?Sized> DashPayView<'_, B> { | |||
| } | |||
| }; | |||
|
|
|||
| (payment_address, used_flip_changeset, tx, fee) | |||
| ( | |||
| payment_address, | |||
| used_flip_changeset, | |||
| tx, | |||
| fee, | |||
| offered_accounts, | |||
| ) | |||
| }; | |||
|
|
|||
| // Persist the payment-address used flip now that the wallet-manager | |||
| @@ -1291,16 +1335,28 @@ impl<B: TransactionBroadcaster + ?Sized> DashPayView<'_, B> { | |||
|
|
|||
| // --- 3. Broadcast the transaction, releasing the build's UTXO | |||
| // reservation if the broadcast is definitively rejected pre-send. --- | |||
| let txid = match crate::wallet::reservations::broadcast_releasing_on_rejection( | |||
| self.broadcaster.as_ref(), | |||
| &self.wallet_manager, | |||
| &self.wallet_id, | |||
| key_wallet::account::account_type::StandardAccountType::BIP44Account, | |||
| 0, | |||
| &tx, | |||
| ) | |||
| .await | |||
| { | |||
| // Release across EVERY account that offered inputs, not just BIP44: | |||
| // now that the build pools funding, a rejected broadcast whose inputs | |||
| // came from a BIP32 or contact-receiving account would otherwise leave | |||
| // those reserved until the TTL backstop, and an immediate retry would | |||
| // fail with a spurious insufficient-funds. | |||
| let broadcast_result = match self.broadcaster.broadcast(&tx).await { | |||
| Err(e) if matches!(e, crate::broadcaster::BroadcastError::Rejected { .. }) => { | |||
| crate::wallet::reservations::release_reservation_after_rejected_broadcast( | |||
| &self.wallet_manager, | |||
| &self.wallet_id, | |||
| &funding_accounts, | |||
| &tx, | |||
| // This path does not thread the build's reservation token | |||
| // either; keep the historical unconditional release. | |||
| None, | |||
| ) | |||
There was a problem hiding this comment.
🟡 Suggestion: Retain the pooled reservation token through pre-broadcast exits
build_signed discards the ReservationToken stamped across the contributing accounts. If persister.store fails at line 1330, the function returns without releasing the signed transaction's reserved inputs. If broadcast is rejected, passing None performs an unconditional release by outpoint. A TTL sweep can release the original reservation and allow another build to reserve the same inputs while broadcast is awaiting completion; the old build's unconditional cleanup can then clear the newer reservation. Use build_signed_reserved, carry its token alongside tx and funding_accounts, release with that token before returning a persistence error, and pass it to release_reservation_after_rejected_broadcast after a definitive rejection.
source: ['codex', 'coderabbit']
| /// Whether the *sole* cause of invalidity is a key-purpose mismatch — | ||
| /// the only case that may be downgraded to a non-permanent skip. | ||
| /// A purpose mismatch that co-occurs with a hard error (disabled / | ||
| /// missing / wrong-type key) is NOT purpose-only and must stay permanent. | ||
| /// wrong-type key) is NOT purpose-only and must stay permanent. | ||
| pub fn is_purpose_only(&self) -> bool { | ||
| self.purpose_mismatch && !self.hard_error | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Align the public retryability gate with absent-key classification
add_absent_key_error intentionally leaves hard_error false because an absent key is retryable, but the public documentation still says missing keys are permanent and directs callers to is_purpose_only() for skip-versus-break decisions. That predicate returns false for a lone absent-key failure, while returning true for a purpose mismatch combined with an absent key even though purpose is not then the sole cause. The in-crate drain correctly uses is_permanent(), but downstream callers following the advertised public contract can retain the obsolete permanent treatment. Make is_permanent() the documented canonical disposition predicate and deprecate or explicitly restrict is_purpose_only() to diagnostics, or track absent-key failures separately so its stated sole-cause semantics remain exact. Add coverage for lone-absent and purpose-plus-absent results.
source: ['codex']
4 participants
Issue being fixed or feature implemented
A
RegisterExternalentry that the key-purpose policy turns away stays queued on purpose: thecontactRequestdocument is immutable, so what might change is our acceptance policy, and marking the channel broken instead would need a superseding request from the contact to heal — an appeal the user cannot file. The consequence is that the sweep re-offers the same entry every pass, for the life of the wallet.The drain fetched the contact identity from Platform before it could validate, so every one of those passes spent a network round trip to reach a verdict it already had locally. The dominant rejection is on
recipientKeyIndex— our own key, always resident, no fetch required.Mainnet logs from a single wallet, one session:
#4372 makes those 27 contacts succeed, but the mechanism stays: any future cohort our policy does not yet accept lands in the same forever-loop.
What was done?
Ordering — get the network off the rejection path
validate_contact_requestintovalidate_sender_key(needs the counterparty identity) andvalidate_recipient_key(needs only our own). The composedvalidate_contact_requestis unchanged and still calls both via the existingContactRequestValidation::merge, so every other caller keeps the identical contract.drain_pending_contact_crypto_untilnow runs the recipient half first and only performsIdentity::fetchwhen it passes.One failure policy, applied by both halves
apply_drain_validation_failurehelper: purpose-only → count it and leave queued; anything else (missing key, wrong type, disabled) → mark the channel broken and clear the entry. Both halves route through it, so they cannot drift.validate_sender_key/validate_recipient_keyarepub(crate)— drain-internal decomposition, and both thecryptoandvalidationmodules are public, sopubwould have committed them as external API.Logging — keep the signal, drop the flood
debug!.warn!carries the entry count plus the distinct reasons and how many entries each blocked.This deliberately does not add a persisted "policy-blocked" marker on the contact. That would need a SQLite migration plus the FFI persistence bridge, and — more importantly — a frozen contact needs a correct un-freeze trigger; getting that wrong reproduces the bug #4372 just fixed. Removing the cost of the retry is the smaller and safer lever.
How Has This Been Tested?
cargo test -p platform-wallet— 667 pass.cargo clippy --all-targetsclean,cargo fmtapplied.New test
drain_decides_our_own_key_fault_without_fetching_the_contact(payments.rs): a wallet-owned but keyless owner meansrecipientKeyIndex0 cannot resolve — a hard fault that must break the channel. The mock SDK has no contact-identity fetch configured, so the verdict is only reachable if the recipient half ran before the fetch.Verified it is a real guard, not a passenger: checked out the parent branch's
contact_requests.rsand re-ran it against the old ordering —Under the old ordering the fetch failed first and the verdict was never reached.
unaccepted_recipient_purpose_never_fetches_and_stays_recoverablecovers the case that actually repeats — an unaccepted purpose, not a hard fault. The contact identity is configured on the mock, with a hard sender-side fault, and the drain runs twice. Under the old ordering the fetch succeeds, the merge escalates to broken, and the entry is cleared; verified against the parent branch's file —So passing proves no fetch was spent: an implementation that short-circuited only hard recipient faults, and still fetched for purpose mismatches, would fail it.
Breaking Changes
No API breakage. But there is a deliberate behaviour change, and an earlier revision of this description wrongly claimed there was none — review caught it:
Mixed failures now resolve in favour of leaving the entry queued. When our key is purpose-rejected and the contact's key carries a hard fault (missing / disabled / wrong type), the composed validator merged both halves, saw
hard_error, and marked the channel permanently broken. Deciding on the recipient half alone classifies it purpose-only and leaves it queued.Kept, as a stated decision: the
hard_errorprecedence exists to stop a permanent fault becoming a retry-forever loop, and that loop was expensive because each retry fetched. With the fetch gone it costs a map lookup per sweep, while marking the channel broken is unappealable by the user — only a fresh request from the contact clears it. A sender-side hard fault still breaks the channel as soon as our own key stops being the blocker. Documented onvalidate_recipient_keyand at the call site.Also worth a reviewer's eye: a hard recipient-side fault now marks the channel broken without fetching the contact. That is correct — a missing, wrong-type or disabled key on our own identity is permanent independent of the contact — but the broken mark can now be set where a Platform round trip previously had to succeed first.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit