fix: prevent false on-chain send success

[ovitrif]

Closes #717
Twin: synonymdev/bitkit-android#1384
Refs: synonymdev/ldk-node#119

Description

The published rc68 archive and resolved framework match the canonical artifact. The current merge at c339e9e integrates master 1eabf70 and passed its simulator build and 263 affected tests. Funded fixed-amount and Manual Max device evidence below predates this merge. This PR remains draft pending approval of the LDK change.

• Uses explicit LDK broadcast outcomes for normal sends, send-all, transfers and Shop payments so a transaction ID alone never produces success or payment proof.
• Saves a bounded attempt guard before calling the node and blocks another send after refusal, unknown outcome, cancellation or unreadable state. Shop request protection also covers switching payment methods.
• Preserves accepted transaction IDs after local storage/activity/proof failures and resumes local follow-up without creating another payment. Reconciliation requires independent observation of the exact transaction ID.
• Restores original Shop and order follow-up after local failures, retains transfer accounting across wallet changes, keeps an earlier ordinary payment separate from a new unsent payment, and preserves explicit verified acceptance in Shop proof backups.
• Requires fresh observation of the exact outgoing hardware transaction in the original wallet before Shop proof, Sent activity or Success. Missing observation keeps the original payer/request/wallet pending; the pending screen retains that context across profile and wallet changes.
• Preserves the started hardware Shop guard after missing results or candidate-save failure; cancellation can only discard unstarted preparation.
• Releases only the exact attempt when its callback fails before native dispatch; a failed guard write remains unresolved.
• Serializes proof load/edit/save mutations so Lightning completion and pruning cannot overwrite an on-chain started marker.
• Restores delayed hardware Sent activity, original contact and retained tags before proof delivery, and resolves reopened Pending from exact original payment evidence after the listener consumes its event.
• Preserves current contact/payment authorization before software dispatch and each hardware retry while retaining the original payer/request/wallet context and unresolved-send protection.
• Pins published LDK 0.7.0-rc.68 at 773792d; the downloaded archive, SwiftPM checksum and extracted simulator framework match.

Out of Scope

• Transaction recovery: journals, raw transaction storage, automatic rebroadcast, replacement, RBF recovery and abandonment.

• Chain handling: reorg and event-delivery redesign.

• Coin selection: Autopilot Max failed before Confirm with CoinSelectionFailed in the regtest fixture; the accepted-send journey uses Manual selection. Coin-selection recovery is outside this change.

• Unresolved sends: no timeout/reset escape. A refused or unknown result can block further on-chain sends indefinitely when the transaction never becomes positively observed; backend acceptance does not guarantee confirmation.

• Legacy software proofs: queued-era transaction IDs without positive acceptance and original wallet provenance are not promoted or delivered. Opted-in released users can have these records; migration is excluded from this PR, with the documented limitation accepted in review.

Design

N/A — no design available for the new unresolved-send state.

Preview

Historical rc68 candidate before the current feedback batch: fixed 1,000-sat and Manual Max 198,745-sat regtest sends using the actual published and resolved LDK package. Both exact UI transaction IDs matched native successful-broadcast logs and independent backend lookup; Max spends the original fixed-send change output with no change remaining.

Fixed: Sent (historical)	Fixed: Details (historical)	Max: Sent (historical)	Max: Details (historical)

QA Notes

Journeys

• new shop-onchain-proof.xml — linked issuer and funded Bridge hardware wallet: exact original payer, request, transaction, proof and wallet Details, including delayed Sent/contact/tags and reopened Pending after listener consumption, with no repayment while observation is pending. Spec parsed; native hardware journey unrun.
• new onchain-accepted-result.xml — accepted fixed-amount and send-all with Manual coin selection finish local activity and expose distinct exact transaction IDs in Details.

Manual Tests

• Controlled refusal → Send Confirm → Pending → no success/proof or repayment — controlled broadcast-refusal fixture not in Capabilities.
• Controlled response loss → Send Confirm → Pending → retain the exact transaction ID when supplied and block retry after restart — broadcast-response-loss fixture not in Capabilities.
• regression: fail outcome persistence after Accepted → preserve the known result in the running app; restart with unresolved guard → no repayment — storage-fault injection not in Capabilities.
• regression: interrupt ordinary Accepted local follow-up → restart → Send → finish saved metadata/activity and show the original exact transaction ID without another broadcast — interruption/storage-fault injection after acceptance not in Capabilities.
• regression: physical Trezor Shop payment → approve transaction → original hardware payment/proof follow-up survives transport changes — physical USB and BLE transport not in Capabilities.

Automated Checks

• added OnchainSendAttemptServiceTests.swift — durable admission, outcome persistence, exact observation and callback failure release with zero dispatch; failed release storage remains guarded.
• updated PaykitPaymentProofServiceTests.swift, PaykitPaymentStateBackupTests.swift — proof acceptance evidence, request protection and a suspended Lightning save cannot overwrite the concurrent on-chain start marker.
• updated HwFundingSignerTests.swift, HwWalletManagerTests.swift — original hardware account context and verified Shop versus Pending navigation.
• updated ChannelPurchaseFlow.swift, UtxoSelectionTests.swift — matching outcome API integration.
• updated TransferServiceActivityTests.swift — original local follow-up and actual reopened hardware Pending navigation; delayed verification restores original Core Sent/contact/tags before proof delivery. Existing accepted-order tests inject the geo decision while production retains the same geographic check.
• ran published dependency validation — source 773792d, archive SHA-256 1955a179fdb6acc5159ead68225dd27029d2f2a74c4b8b13164038eb9f95462f, and downloaded/SwiftPM simulator binaries agree.

Local verification: c339e9e merges master 1eabf70; the simulator build and 263 affected tests passed (0 failures/skips), including hardware authorization/retry, request/proof protection, exact outcome reconciliation, transfer activity and contact lifecycle. All tracked source hashes and the rc68 simulator binary match the tested snapshot. The base-inherited Paykit rc56 pin and API adaptation are preserved. Standard Debug geographic checks remain enabled; local-only -packageFingerprintPolicy warn handles the existing VSS fingerprint conflict.

Earlier feedback validation at 2039d39: 99 affected tests passed; three semantic regressions failed on its preceding head and passed with those fixes. This is historical evidence, not a fresh red run for the merge.

Earlier CI at 2039d39 passed unit/integration/build checks; the E2E run was cancelled and its aggregate status failed. The cancellation cause remains unknown. No rerun of that unchanged cancelled run was requested; checks for the new merge head are pending.

Prior rc68 validation: 101 affected tests and funded fixed/Manual Max native journeys passed with independently verified exact backend transactions. Those device runs were not repeated for this batch. The updated hardware journey, physical hardware and controlled native fault scenarios remain unrun; no new hardware Preview is claimed. Missing original metadata after a failed write or crash cannot restore lost tags.

[ovitrif]

I pushed e447862 with original Shop and order follow-up recovery, exact native observation acknowledgement, earlier-payment isolation, shared verified proof metadata and original transfer accounting across wallet changes. The focused simulator build and 90 tests passed.

This addresses accepted payments becoming stuck after local proof/activity/tracking failures without funding again. Process loss before a durable result still remains guarded. Corrected artifacts, hardware proof integration and current consumer journeys remain draft gates.

[ovitrif]

I pushed 9c84067 with the hardware Shop acceptance gates. A Core txid remains an original lookup candidate until a fresh exact outgoing observation in the original hardware wallet verifies it. Only that positive result can create Shop proof, Sent activity or Success; Pending retains the original payer/request/wallet and does not inspect an unrelated Savings guard.

The frozen batch passed its simulator build and 70 focused tests with no failures or skips. The UI, payer and activity regressions failed before their fixes; mocks do not establish a real hardware acceptance journey. Physical hardware and native fault fixtures remain QA gaps.

This remains draft while current rc68 remote consumption, affected tests and fixed/Manual Max native journeys finish. Historical rc67 media is labelled accordingly.

[ovitrif]

I pushed f16736d with the production rc68 package pin and current journey documentation. The normal SwiftPM revision is 773792d; the published archive, Package.swift checksum and extracted/resolved simulator framework independently match the canonical artifact.

The current simulator build and 101 affected software, hardware and backup tests passed. The matching installed app completed funded fixed 1,000-sat and Manual Max 198,745-sat native sends; both exact UI transaction IDs matched native successful-broadcast logs and independent backend lookup. Current Preview replaces the historical accepted-send media.

The hardware Shop journey is specified and parsed but unrun; controlled native refusal/response-loss and physical USB/BLE remain explicit QA gaps. Autopilot Max remains outside this fix.

[jvsena42]

One HIGH, one MEDIUM and two LOW inline. I reviewed this as a funds change. The refusal lockout and the legacy-proof gap are shared with synonymdev/bitkit-android#1384.

Checked and clean:

• Only NodeError maps to .preDispatch; in rc.68 send_with_broadcast_result errors only before submission. Panics and other errors stay .unresolved, and the non-cancellable queue records the outcome.
• Success is shown only on .accepted, and Electrum returns Accepted only for a matching txid. Observation promotion needs BDK sync events. HW Shop needs a matching getTransactionDetail with sent > 0.
• admit is synchronous, so a concurrent send sees .pending.
• The new keychain entry holds no key material and uses the existing accessibility/group, and full wipes cover it.
• The ldk-node rc.66 → rc.68 bump carries the broadcast-result API with no storage migrations. It also brings rc.67 coin-selection changes.

Pre-existing and out of scope: ordinary HW sends and Boost still use the legacy broadcast.

[jvsena42]

A refused broadcast locks on-chain sends for good.

Unknown and Rejected both block the next send, and the only exit is observing that exact txid on chain. There is no release on sync and no user action; only a wallet reset or reinstall clears the stored attempt. Ordinary sends, Shop payments and Blocktank funding all go through the same guard.

In ldk-node rc.68, classify_electrum_broadcast (src/chain/electrum.rs) returns Rejected only for -26 non-final. Every other daemon refusal becomes Unknown, including mempool min fee not met, min relay fee not met, txn-mempool-conflict and bad-txns-inputs-missingorspent. Those mean the backend did not take the transaction, and nothing rebroadcasts a user send, so no Received/Confirmed event ever arrives. A Slow or Custom fee during a mempool purge, or a send built on stale UTXO state, leaves the wallet unable to send on-chain while its funds stay in BDK.

Out of Scope accepts indefinite blocking for a lost result. This is a definite, common refusal that lands in the same state.

Fix: allow a release once a post-refusal sync shows the txid unknown to the backend and the attempt's inputs still unspent. If it could surface later, require the next send to spend at least one of those inputs so it conflicts rather than double-pays.

Same in synonymdev/bitkit-android#1384.

[ovitrif]

The lockout is real when a refused transaction never surfaces. It is an explicit limitation of this bounded change, including refusal as well as a lost response.

rc.68 Rejected reports a recognized refusal and explicitly allows earlier delivery: outcome contract. The pinned electrum-client 0.24.1 retries transport failures up to three times after the original call. A later protocol refusal returns without the earlier transport-error history; a request may already have been written before a receive failure. The final refusal therefore does not prove that earlier delivery or acceptance never occurred. See the retry wrapper. This is a possibility supported by the transport contract, not a claim that it happened in the tested journeys.

Only a proven pre-dispatch failure releases this guard. Relabelling fee/conflict/missing-input responses as Rejected would preserve diagnostics but would not prove retry safety. One backend reporting absence and unspent inputs cannot retract previously delivered bytes or an in-flight request.

Requiring a later send to conflict with the original adds a replacement/recovery policy, which the agreed scope excludes. I’m keeping the guard and clarifying the refusal limitation. Does this remain a blocker for the bounded acceptance-reporting change, or can the recovery policy be considered separately?

[coreyphillips]

Two independent reviews.

needs changing before merge

• Pre-broadcast hook failure leaves an unclearable guard that blocks every on-chain send (Bitkit/Services/OnchainSendAttemptService.swift:218). If beforeBroadcastAttempt throws, the pending attempt is left in the keychain, and it can never be resolved. OnchainSendAttemptService.send (Bitkit/Services/OnchainSendAttemptService.swift, the do { try await beforeBroadcastAttempt() } catch { throw OnchainSendAttemptError.unresolved } block) first persists a .pending attempt via admit. Then it rethrows .unresolved without calling clearBeforeDispatch. At that point the node has not been called, so the attempt has txid == nil. Every release path needs a txid or a non-pending status: - observeConfirmedTransaction matches on attempt.txid. - resumeAcceptedOrdinarySend, resumeAcceptedRequestSend and resumeAcceptedTransfer all require a txid. - admit refuses while blocksNewSend is true. The only exit is Keychain.wipeEntireKeychain through a wallet reset. Ordinary sends, send-all, Shop payments and transfer-to-spending funding all stay blocked indefinitely. This is reachable. In SendConfirmationView the hook is markOnchainPaymentStarted, and that throws in several cases: - sdk.identityStatus() throws or returns no key (currentIdentity). - The prepared proof is no longer found (requestUnavailable). - The keychain read or write fails. The user then sees "An earlier on-chain send is unresolved" and a Pending screen saying the transaction "may have been sent". Neither is true. testCallbackNodeErrorAndCancellationRetainGuardWithoutDispatch asserts exactly this state: node.calls == 0 with the .pending guard retained. So the behaviour is deliberate, but its consequence is not the accepted "lost result" trade-off from the issue. In that case a transaction may exist. Here it is known that nothing was dispatched. I confirmed this by tracing every function that writes or clears the store in OnchainSendAttemptService.swift, and by reading the test above. I did not run it on a device.

worth doing, does not block

• Geoblock after accepted funding blocks all on-chain sends (Bitkit/Services/TransferService.swift:45). If createTransfer fails after funding is accepted, all on-chain sends stay blocked. TransferViewModel now routes transfer bookkeeping through resumeAcceptedTransfer, which calls TransferService.createTransfer and propagates its errors. createTransfer throws when GeoService.shared.isGeoBlocked is true for a to-spending LSP transfer. If geoblock status turns true after the funding transaction is accepted, the attempt stays .accepted with localFollowupComplete == false. The WalletViewModel startup resume fails the same way on every launch while geoblocked, and admit refuses every new on-chain send in the meantime. Before this PR the tracking failure was logged and ignored. Consider bypassing the geoblock check when a funding txid for the order already exists, since that check guards creating a new order, not recording one already paid.
• New user-facing send and pending strings bypass localization (Bitkit/Views/Wallets/Send/SendPendingScreen.swift:451). None of the new user-facing messages go through t(...), although the surrounding code localizes its strings. Affected strings: - The Pending screen messages ("Earlier on-chain payment", "Transaction ID: …", the rejected, unknown and follow-up texts). - The SendConfirmationView toasts ("Broadcast rejected", "Broadcast unconfirmed"). - The OnchainSendAttemptError.errorDescription values. - The TransferViewModel AppError messages. Non-English users will see English on the exact screens meant to stop them paying twice.
• Transfer recovery can create duplicate records (Bitkit/Services/OnchainSendAttemptService.swift:373). Accepted transfer restoration can run twice because restoreAcceptedTransfer suspends before setting localFollowupComplete. The direct payOrder recovery and confirmation callback can both pass the guard, then race through TransferService.createTransfer, whose order lookup and insert are not atomic. This can persist duplicate records for one order and double-count the pending transfer balance.
• Hardware Shop resolution loses wallet scope (Bitkit/Services/PaykitPaymentProofService.swift:461). A delayed hardware Shop resolution carries its hardware wallet ID, but AppScene.associateResolvedPaykitOnchainPayment calls findActivity and setContact without that ID. Both default to the main wallet, then the resolution is consumed even when lookup fails. If the send screen is closed, the hardware activity permanently misses its contact association.

[ovitrif]

Pushed 2039d39 to address callback admission, proof mutation races, and delayed/reopened hardware follow-up. Callback errors clear only the exact attempt before native dispatch; a failed clear remains guarded. Proof mutations now serialize without holding the lock through SDK network work. Delayed verification restores original Sent/contact/tags before delivery, and Pending can reopen after the resolution event was consumed. Accepted payments are never broadcast again.

Local verification: simulator build and 99 affected tests passed against published rc68 (0 failures/skips). The three regressions failed on the preceding head. Geographic test state is isolated while the production check remains unchanged.

Prior native fixed/Manual Max media is historical for this batch. Hardware journey and controlled native faults remain unrun. Previous-head E2E failures remain unresolved; new-head CI is pending. Refusal lockout and software legacy migration remain open review discussions.

[jvsena42]

Re-checked 2039d39. The callback-failure MEDIUM and the lock LOW are resolved, and the greptile threads on delayed HW verification and the pending screen check out. No new findings.

Still open, waiting on my decision: the refusal lockout thread and the legacy-proof migration thread, where you asked whether they block this PR.

[ovitrif]

I’m moving this PR back to draft while synonymdev/ldk-node#119 awaits approval. The early app review was valuable: it validated the integration against rc68 and caught issues that have now been addressed.

The app integration depends on the node proposal being accepted. I’ll revisit readiness once LDK is approved, and update and revalidate the integration if the node API changes.

[ovitrif]

Pushed c339e9e to resolve the conflicts with master 1eabf70. The merge retains current contact/payment authorization before software dispatch and every hardware retry, together with the original payer/request/wallet context, positive-acceptance gates and duplicate-payment protection.

Validation: simulator build passed and 263 affected tests passed with 0 failures/skips. The exact tracked source and resolved rc68 simulator binary match the tested snapshot. The base-inherited Paykit rc56 update is preserved; LDK rc68 and Core are unchanged.

The PR remains draft pending LDK approval. New hardware/native-fault journeys and media were not run; the existing Preview is historical. The earlier cancelled E2E run remains unattributed; current-head checks are pending. No refusal-release or recovery-policy changes were added.

[ben-kaufman]

On the first hardware attempt, this authorization can fail after prepareHardwareContactPayment has persisted paymentStarted = true. The coordinator then drops the signed payment without broadcasting, but cancelHardwareContactPayment only calls cancelPreparation, which keeps the started hardware proof. The request stays in flight with no transaction ID. Could we release the exact proof when authorization fails before any broadcast, while keeping attempted retries guarded, and cover the durable proof in the denied-first-attempt test?

[ben-kaufman]

If SDK proof submission fails after the hardware activity is saved, the next reconciliation runs this again and overwrites any contact the user has since removed or reassigned in Details. Could we record completion of the local follow-up and preserve later contact edits when retrying proof submission?

[ben-kaufman]

This callback also resumes attempts whose local follow-up is already complete. If the user removes or changes the activity's contact before a delayed transaction event arrives, localFollowup.save passes the original contact back to createSentOnchainActivityFromSendResult, which overwrites the user's edit. Could we skip completed follow-up here and cover a contact edit followed by the received event?