Skip to content

fix: keep signed hardware sends on retry - #1208

Open
Jasonvdb wants to merge 28 commits into
masterfrom
fix/hw-send-keep-signed-tx
Open

Jasonvdb wants to merge 28 commits into
masterfrom
fix/hw-send-keep-signed-tx

Conversation

@Jasonvdb

@Jasonvdb Jasonvdb commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1218

Important

I don't have a hardware wallet, so this has not been tested on a physical Trezor or Jade. It was tested end to end with the Trezor Bridge emulator on local regtest, on the iPhone 17 simulator, iOS 26.5. USB, BLE, THP pairing and Jade have no live coverage.

Twin: synonymdev/bitkit-android#1770

This PR stops an ordinary hardware wallet send from asking for a second signature after a broadcast whose outcome is unknown. Today the signed transaction can be discarded in that case, so if the first broadcast was actually accepted, sending again pays the recipient twice.

Description

  • Rebroadcasting the same signed transaction is safe because it has the same txid. A new signature can create a second, separate payment. So ordinary sends now keep the signed transaction, written to the Keychain before the broadcast, so it survives leaving the sheet and restarting the app. HwRetainedSendService holds one device-only Keychain entry per hardware wallet, covered by the full wipe and excluded from backups.

  • After a broadcast attempt the outcome decides what happens:

    Outcome First attempt Retry
    Success, including already known Release Release
    Wallet watcher has already seen the transaction, any failure Success, release Success, release
    Recognised refusal, not yet seen Release, next attempt signs anew Keep and mark refused
    Failed to connect to Electrum:, raised before anything is sent Release, Retry still available on the sign screen Keep
    Timeout, unrecognised Broadcast failed: ..., anything else Keep Keep

    A refusal on a retry is kept because bad-txns-inputs-missingorspent may describe our own earlier accepted transaction.

  • When the wallet watcher sees the transaction, the record is marked as observed and kept. An earlier version of this branch deleted it at that point, and live testing showed a real double payment: the button still read Retry, and tapping it signed a second transaction. Observation is now handled like this:

    • Retry in the open sheet rebroadcasts the same bytes and ends in success.
    • Reopening the same payment shows the warning first.
    • A different address or amount proceeds normally with no warning.
  • Retrying an unobserved send with the same wallet, address and amount rebroadcasts the original bytes with no device prompt. Fee and tags are not part of the match, and the review screen shows the original fee and locks it. Amount checks accept the kept amount, so a near-max payment can still reach Retry after fees rise.

  • A different address or amount over an unobserved record, a refused retry, or an unreadable record shows a warning before any device contact, with Cancel and Yes, Send. Yes, Send gives consent for that one payment and continues to normal signing, so a wallet can never be stuck and a genuine repeat payment still works. Consent lasts until the new payment is signed and saved, so a device rejection or timeout keeps it and the next tap asks the device again. It's also given back if the new payment is refused on its first attempt. Leaving the sign flow ends it, and the warning comes back. This is the only new UI behaviour.

  • The old record is never dropped before its replacement is signed. A consented or different payment replaces it in one write once signed, so backing out, a device rejection or a signing failure leaves the original protected. If the new payment is refused or can't connect on its first attempt, the original is put back. A signature identical to an attempted record counts as a retry.

  • Records are released on success and on wallet removal, which also clears a corrupt entry.

  • Leaving is not locked after a failed ordinary send, so nobody gets trapped on the sign screen while offline.

  • Unknown outcomes and refused retries show the existing "may have been sent" message instead of raw error text.

Out of Scope

  • Connect-failure check: it matches core's error text. A typed core error would be sturdier and needs a bitkit-core change.
  • Shop payments: unchanged, they keep their existing durable path.
  • Sign screen: going back to review leaves the swipe stuck on a spinner, so you have to go back to Amount and continue. A control run with no failure reproduced it, and Bitkit/Components is unchanged here, but it was not run on master.
  • Reused alert copy says "funding transaction", which is slightly off for a plain send. Rewording it would touch every localization.

Known limits of this change:

  • Electrum error text is unauthenticated. A faulty or hostile server can accept the transaction and return a recognised refusal on the first attempt, before the watcher sees it, which releases the record. Signing again still needs approval on the device.
  • Yes, Send allows a second signature after an explicit warning. Once the replacement is signed and sent, the original payment's record is gone.
  • Removing a wallet deletes its record.
  • A restore on a new device has no record, because records are excluded from backups.
  • TrezorManager.forgetDevice does not release a record, so a re-paired wallet gets the warning instead.

Design

N/A — no design available. Existing alert and toast components are reused.

Preview

N/A — the visible changes are the reused warning, the Retry label in a reopened sheet and the locked fee.

QA Notes

Setup

Regtest only: the synonymdev/bitkit-docker stack with the Trezor emulator on a test seed holding several UTXOs, per journeys/hardware-wallet/README.md, plus a local Electrum JSON-RPC proxy set as the custom Electrum server that injects faults on blockchain.transaction.broadcast. No proxy exists in this repo. The full recipe is broadcast-unknown-retry.md.

Journeys

N/A — not drivable; see Manual Tests. Broadcast faults are not in the journeys Capabilities table.

Manual Tests

  • 1. Follow broadcast-unknown-retry.md on the Bridge emulator — Electrum fault injection not in Capabilities
  • 2. On a physical Trezor and Jade, an ordinary send still succeeds end to end — physical hardware wallet not available to the author

Automated Checks

  • added HwRetainedSendServiceTests.swift — record round trip, per-wallet isolation, no overwrite with different bytes, observed marking, corrupt entry behaviour, full wipe
  • updated HwFundingSignerTests.swift — one signature with identical bytes across unknown errors, timeout then leave, a recreated coordinator and a changed fee; the outcome table; observed outcomes; warning with zero device contact; Shop unchanged
  • updated HwWalletManagerTests.swift — watcher marks and does not delete; removal releases
  • updated BroadcastConnectivityTests.swift — connect-failure classification
  • updated HwTransferMocks.swift, SendConfirmationViewTests.swift, SendConfirmationLifecycleTests.swift, PaykitPaymentRequestServiceTests.swift — support for the above

Evidence

Unit suite at 81ff846, iPhone 17 simulator, iOS 26.5, Xcode 26.6, with the same selection as unit-tests.yml: 3081 passed, 0 failed, 5 skipped. The skips are outside the suites this branch touches and include the three TrezorBridgeDashboardUITests. node scripts/validate-translations.js passed. The lint workflow ran the pinned SwiftFormat.

Live on local regtest at afd9082 with the Trezor Bridge emulator and a fault proxy, run by a verifier who did not write the fix. A normal signing shows 4 device prompts:

  • Broadcast reaches the network then errors, watcher sees it, Retry 12 s or more later: 0 prompts, identical bytes, one transaction, recipient paid once. Three runs, one with the error still injected on the Retry.
  • Reopen the same payment after that: warning before any prompt, Cancel stays. Yes, Send signs anew as the consented repeat.
  • Different amount or address after that: no warning, 4 prompts, new transaction.
  • Force quit with a delivered transaction, unmined and mined: warning before any prompt on relaunch.
  • Swallowed broadcast, leave or force quit, reopen, including after changing the default fee: Retry with the fee locked, identical bytes, 0 prompts.
  • Different amount over an unobserved record: warning; Cancel stays; Yes, Send signs anew.
  • First-attempt min relay fee not met: released, next attempt signs anew with no warning. Retry bad-txns-inputs-missingorspent: 0 prompts, kept, reopen warns.
  • Connection refused: real string Failed to connect to Electrum: Connection refused (os error 61), connectivity toast, no record, Retry on the sign screen with 0 prompts.
  • Silent socket: the 120 s timeout fires. Seen by the watcher, the send ends in success with no toast. Not seen, it shows the toast and Retry, with identical bytes and 0 prompts.
  • Near-max payment after the default fee rose: Continue works, fee stays locked, identical bytes, 0 prompts.
  • Every recipient checked with getreceivedbyaddress: none was paid more than once without an explicit Yes, Send.

Gaps

  • The live regtest run above predates the review fixes on this PR. The merge from master, the guard-until-signed change, the consent changes and the fee changes have unit tests and CI only (unit suite at 42abdf5: 3315 passed, 0 failed, 5 skipped). The live scenarios still need re-running at the current head.
  • jvsena42 ran the recipe at 42abdf5 on the iOS simulator with the Trezor emulator through Bridge and staging Electrum behind a fault proxy: checks 1 to 8 and passphrase cases P1 to P5 passed. See their comment on this PR. That run is still an emulator, not a physical device. The earlier Bridge session not found error came from another Bridge client taking the session.
  • No physical hardware wallet. USB, BLE, THP pairing and Jade have no live coverage; Jade is unit tests with mocks only.
  • The live run used a Debug build with E2E_BUILD and a proxy that injects faults, so real-world error strings can differ.
  • The record's observed flag was inferred from Keychain log timing and behaviour, not read directly.
  • Commit 81ff846 after the live run only changes the recipe text.
  • The master merge at 33fab66 brought in the hardware-wallet transfer to Spending (Spark). f9e159c adds the same Cancel / Yes, Send warning there before any device contact, using the send sheet's existing strings. It has unit tests only, with no emulator run yet. 532d0ab keeps a reopened kept transfer on Retry after a restart: the amount screen accepts its amount even above today's maximum, both screens price it from the signed bytes at the original fee, and the button reads Retry. Unit tests only, with no emulator run yet.

Loading
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.

bug: hardware send asks to sign again on retry

5 participants