Repository navigation
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
HwRetainedSendServiceholds 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:
Failed to connect to Electrum:, raised before anything is sentBroadcast failed: ..., anything elseA refusal on a retry is kept because
bad-txns-inputs-missingorspentmay 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:
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
bitkit-corechange.Bitkit/Componentsis unchanged here, but it was not run onmaster.Known limits of this change:
TrezorManager.forgetDevicedoes 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-dockerstack with the Trezor emulator on a test seed holding several UTXOs, perjourneys/hardware-wallet/README.md, plus a local Electrum JSON-RPC proxy set as the custom Electrum server that injects faults onblockchain.transaction.broadcast. No proxy exists in this repo. The full recipe isbroadcast-unknown-retry.md.Journeys
N/A — not drivable; see Manual Tests. Broadcast faults are not in the journeys Capabilities table.
Manual Tests
broadcast-unknown-retry.mdon the Bridge emulator — Electrum fault injection not in CapabilitiesAutomated Checks
HwRetainedSendServiceTests.swift— record round trip, per-wallet isolation, no overwrite with different bytes, observed marking, corrupt entry behaviour, full wipeHwFundingSignerTests.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 unchangedHwWalletManagerTests.swift— watcher marks and does not delete; removal releasesBroadcastConnectivityTests.swift— connect-failure classificationHwTransferMocks.swift,SendConfirmationViewTests.swift,SendConfirmationLifecycleTests.swift,PaykitPaymentRequestServiceTests.swift— support for the aboveEvidence
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 threeTrezorBridgeDashboardUITests.node scripts/validate-translations.jspassed. Thelintworkflow 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:
min relay fee not met: released, next attempt signs anew with no warning. Retrybad-txns-inputs-missingorspent: 0 prompts, kept, reopen warns.Failed to connect to Electrum: Connection refused (os error 61), connectivity toast, no record, Retry on the sign screen with 0 prompts.getreceivedbyaddress: none was paid more than once without an explicit Yes, Send.Gaps
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.session not founderror came from another Bridge client taking the session.E2E_BUILDand a proxy that injects faults, so real-world error strings can differ.mastermerge 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.