Skip to content

fix: retain exact fees for payment recovery - #915

Open
ben-kaufman wants to merge 2 commits into
masterfrom
codex/ios-prepared-recovery-fee-20261011
Open

ben-kaufman wants to merge 2 commits into
masterfrom
codex/ios-prepared-recovery-fee-20261011

Conversation

@ben-kaufman

Copy link
Copy Markdown
Contributor

This PR retains the exact prepared fee for each Bitcoin retry candidate so a positively accepted replacement can finish local recovery while wallet history catches up.

Description

  • Persists the prepared fee with its transaction before dispatch and uses only the proven winner's fee for local activity and follow-up.
  • Preserves unknown-outcome, original-input, wallet/identity and write-failure guards; canceled unsubmitted candidates lose only their own metadata.
  • Allows identical backup replay to preserve local fee evidence without importing fee metadata or accepting changes to the original operation, candidates or progress.

Ordinary iOS sends already retain their original fee. This fixes the narrower replacement-recovery gap; Android's matching original/replacement fix is synonymdev/bitkit-android#1459.

Out of Scope

  • Shared backup schema, SDK/native APIs, dependencies, Homeserver, Noise and safety waits: unchanged. Reset/cross-device restores without local evidence retain the existing wallet-history fallback.
  • Payment acceptance and remote proof delivery: unchanged. Knowing a fee never proves payment acceptance.
  • Performance PR fix: refresh Paykit views and local completion #914 and minimum-amount PR fix: validate minimum paykit request amounts #913 remain separate. This is not a confirmed cause of James's report, an Electrum fix or a measured latency improvement.

Design

N/A - no UI changes.

Preview

N/A - recovery bookkeeping only.

QA Notes

Journeys

  • updated onchain-original-payment-retry.xml - retains the original guarded operation through retry; its description includes exact-fee recovery checks requiring manual fault controls.

Manual Tests

  • In a disposable funded regtest fixture, delay wallet history after an accepted replacement and restart before local follow-up -> Sent details use that winner's prepared fee without another send; native history fault injection is not in Capabilities.
  • Exercise an unknown outcome, original-winner race, failed receipt save and identical backup replay -> unresolved payments remain guarded, only the winner's fee is used, persistence failure prevents dispatch and local fee evidence survives replay; controlled native outcomes and storage faults are not in Capabilities.

Automated Checks

  • added OnchainSendAttemptServiceTests.swift - accepted successor retains its exact fee through JSON restart without transaction history; conflicting fees for one transaction fail closed.
  • updated OnchainSendAttemptServiceTests.swift - unknown outcomes, original-winner selection, pre-dispatch persistence, absent-fee fallback and canceled candidate cleanup remain guarded.
  • updated PaykitPaymentStateBackupTests.swift - identical replay preserves local fees without importing new fee metadata; changed candidates/progress remain rejected.
  • updated TransferServiceActivityTests.swift - prepared original/replacement fixtures provide consistent exact fees for winner-specific activity details.

Final local validation on master 4e0c8ba5: 180 focused native tests and all 2,365 CI-selected unit tests passed, with the six unchanged repository integration-suite exclusions. Two tests added, none removed. Complete reports retain all 102 formatting findings and 1,898 translation warnings from base with no introduced findings. The focused rebuild's 49 compiler records map to existing baseline contexts; the final full-suite incremental build has no diagnostics. All three requested hardening skills and one fresh frozen combined correctness/quality review completed clean.

The controlled funded/native journey remains unrun; no new payment, funding, broadcast or protected-wallet interaction was performed. Readiness is for peer review, not a device-payment or merchant-unlock sign-off.

@greptile-apps

greptile-apps Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Critical impact] This PR appears safe to merge; no actionable issues were found.

Summary

This PR saves each prepared Bitcoin transaction’s exact fee before dispatch. Accepted recovery uses the winning transaction’s fee without waiting for wallet history.

  • Accepted retries keep their exact fee for Sent details.
  • Matching backup replays preserve local fee evidence.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Prepare transaction and exact fee] --> B[Save candidate and fee locally]
  B --> C[Authorize and dispatch]
  C --> D{Payment accepted?}
  D -->|No or unknown| E[Keep payment guarded]
  D -->|Yes| F[Select accepted transaction]
  F --> G{Local fee available?}
  G -->|Yes| H[Use winning prepared fee]
  G -->|No| I[Read fee from wallet history]
  H --> J[Save local activity]
  I --> J
  J --> K[Mark local follow-up complete]
Loading

Reviews (1) · Last reviewed commit: "chore: rename changelog fragment" · Reviewed by Greptile

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings. Stores each retry candidate's exact prepared fee in the recovery context before dispatch, prefers it over wallet history in winningFollowupAttempt, and ignores the local-only field when comparing a backup replay. Reviewed 6973637, full tier on the changed paths. Not built here: iOS does not build on this box, so this is reasoned from the code and from CI.

What I checked, and 5 candidates I ruled out

Read: the diff whole; in OnchainSendAttemptService.swift, the initial send receipt write, retrySamePayment, discardUnsubmittedRetryCandidate, restoreBackup, winningFollowupAttempt, OnchainSendRecoveryContext; PaykitPaymentStateBackup.ActiveOnchainAttempt.init.
Call sites traced: miningFeeSats producers (the prepareOnchainSend wrapper reads it from the same prepared tx as txid); every recoveryContext equality check.
CI: validate, detect-changes green; Run Tests, Run Integration Tests and build-local still pending at review time.

Ruled out

  • Original winner fee changes: for the original txid the retained fee is the same prepared.miningFeeSats already written to followupContext.feeSats, so the result matches the old branch.
  • Successor inheriting the original's fee: lookup is by the winner's lowercased txid; no fallback to another candidate.
  • retrySamePayment's attempt.recoveryContext == recovery guard: both sides are read from the same stored record, so the new field cannot make it differ.
  • Backup replay: the backup wire format has no fee field, restoreBackup nils it on both sides only for the comparison, and on match returns without saving, so local evidence survives; any other difference still throws.
  • Older records: the field is an optional Codable member, so existing keychain JSON decodes with nil and falls back to winningFee.

Merge confidence: 4/5, no findings, but the unit and integration test jobs had not finished and the change could not be built here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants