fix: retain prepared transaction fees

[ben-kaufman]

This PR retains each prepared transaction's exact mining fee before broadcast so an accepted payment can complete local bookkeeping while transaction history catches up.

iOS already retained ordinary-send fees; its narrower replacement-recovery counterpart is synonymdev/bitkit-ios#915.

Description

• Saves the exact fee with its candidate transaction in the existing encrypted attempt record before dispatch.
• Uses only the proven winner's fee for local activity; unknown outcomes, another candidate's fee and mismatched requests cannot complete a payment.
• Preserves write-failure blocking, restart recovery and the existing transaction-history fallback when no local fee was retained.

Out of Scope

• Shared backup format, native APIs, Paykit SDK, Homeserver and Noise: unchanged; reset/cross-device restore still requires the existing history fallback.
• Payment acceptance, broadcast, identity checks and safety waits: unchanged. Fee evidence is not payment evidence.
• Performance PR fix: defer proof delivery after durable handoff #1458 and minimum-amount PR fix: validate minimum paykit request amounts #1457 remain separate. This is not a confirmed explanation of the reported long-lived payment failures or a measured speedup.

Design

N/A - no UI changes.

Preview

N/A - recovery bookkeeping only.

QA Notes

Journeys

• updated journeys/send/onchain-original-payment-retry.xml - common retry UI actions are unchanged; its description now points to the shared manual fault recipe.

Manual Tests

• In a disposable regtest fixture, follow journeys/send/prepared-fee-recovery.md: delay transaction-history details after Accepted, restart after receipt persistence, and exercise replacement/original winners -> exact winner fee and one local activity/proof, with no extra send; native history fault injection is not in Capabilities.
• Fail receipt persistence before dispatch and exercise Unknown, wallet/request mismatch and backup replay -> no unguarded broadcast or completion; storage/evidence fault injection is not in Capabilities.

Automated Checks

• added OnchainSendAttemptStoreTest.kt - persisted original/replacement fees survive file/fsync reopen; conflicting fees fail closed; local backup replay preserves private evidence.
• added LightningRepoTest.kt - accepted winners use only their own fee, including zero; Unknown and mismatched shop requests do not complete.
• updated OnchainSendCoordinatorTest.kt - receipt fees are durable before dispatch, failed storage prevents dispatch, and discarded candidates lose only their own metadata.
• updated LightningRepoTest.kt - a missing successor fee never borrows the original candidate's fee and still uses the existing history fallback.

Final local validation on master 3cac93b0: compilation and 3,948 tests across 222 suites passed. Complete format/Detekt reports contain the same 1,981 verified baseline findings, with zero introduced findings or suppressions; all 31 test compiler diagnostics match the base. Three tests added, none removed. Repository-native review, production-polish sweep and a fresh frozen correctness/quality gate completed with no outstanding findings.

The author validation above did not run the controlled device/payment checklist. Piotr subsequently reported focused Android device checks in review 5489664744, using the clean 10e00654 PR APK on a dedicated disposable staging-regtest wallet; the subsequent 46a9a9de commit changes only documentation. The run covered a dropped original broadcast held Pending, invalid-fee feedback, exact 703-sat fee approval for the original 1,000-sat recipient amount, normal warning/PIN cancellation with no broadcast, one accepted same-input replacement, and one correct activity after full app restart. These are reviewer-reported results, not a new run by this documentation update. The earlier isolated Accepted-payment fixture remains preserved and was not resumed.

The four manual cases, file name and recipe now match iOS #915, with original-send/replacement and platform adaptations recorded in both READMEs. This documentation-only follow-up changes no source or tests. Full Detekt/format findings remain identical to verified base. The full controlled manual matrix remains incomplete: accepted recovery with history withheld, interruption before local follow-up, a genuinely confirmed original-winner race, prepared-receipt persistence failure, identity/request switches, full backup replay and absent-fee fallback were not verified on device in that run. Live Shop proof delivery was not exercised. Approval and the focused checks do not establish a complete fault-matrix pass or a measured speedup.

[greptile-apps]

Confidence Score: 5/5

[Critical impact] The PR appears safe to merge; no actionable issue was found.

Summary

Retains each prepared transaction’s mining fee in the existing encrypted attempt record. Local completion can use the winning transaction’s own fee without waiting for transaction history.

• Accepted on-chain payments use the saved fee for their winning transaction.

ben-kaufman explicitly leaves shared backup formats unchanged; reset and cross-device restore still use the history fallback. Controlled device checks remain unrun, as stated in the PR.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Prepare transaction] --> B[Save candidate ID and exact fee]
    B -->|Save succeeds| C[Broadcast prepared transaction]
    B -->|Save fails| D[Do not broadcast]
    C --> E{Positive payment evidence?}
    E -->|No| F[Keep payment unresolved]
    E -->|Yes| G[Select winning transaction]
    G --> H{Winner fee retained?}
    H -->|Yes| I[Save winning fee]
    H -->|No| J[Read exact fee from transaction history]
    J -->|Available| I
    J -->|Unavailable| K[Keep local completion pending]
    I --> L[Save local activity]
    L --> M[Complete local followup]

Loading

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

[github-actions]

Regtest APK

Built from 46a9a9d (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

[talosmachina]

No findings. Persists each prepared candidate's exact mining fee in the attempt record before dispatch, and uses only the winner's own fee when local follow-up runs ahead of transaction history. Reviewed 10e0065, full tier on the changed paths. Not built here: Bitkit Android reviews are reasoned from the code and from CI.

What I checked, and 5 candidates I ruled out

Read: the diff whole; in OnchainSendAttemptStore.kt, retainPreparedReceipt, discardUnsubmittedRetryCandidate, the broadcast-failure reset, restoreActive, load; in LightningRepo.kt, finishOnchainSendLocally and winningCandidateFee; in LightningService.kt, prepareOnchainSend and observedOriginalSendFee; ActiveOnchainAttemptBackup.from/restored.
Call sites traced: the only OnchainPreparedReceipt producer (prepareOnchainSend, fee from prepared.miningFeeSats() of the same signed tx as txid); every write to candidateTxids.
CI: build (assembleDevDebug + testDevDebugUnitTest), lint, detekt, build-local green; e2e-tests-local shards still pending at review time.

Ruled out

• Wrong candidate's fee: the map is keyed by the lowercased txid of the receipt that produced it, and lookup uses attempt.txid, so a successor never reads the original's entry (the existing successor test now asserts this with an original-only entry).
• load() invariant tripping on old or reset records: every path that drops a txid from candidateTxids (discard, broadcast-failure reset) drops it from candidateMiningFees too, and records without the field decode to emptyMap().
• Restore replay wiping local fees: restoreActive normalises the field before comparing and persists existing, so an identical replay keeps local evidence; backup export never carries the field.
• Fee overwriting on retry of the same txid: retainedMiningFees refuses a differing fee for a known txid before persisting.
• Changed authorization equality: authorizeOnchainRecovery and onRecoveredTransfer compare whole attempts, but the new field changes in the same write as candidateTxids/candidateFeeRates, which those checks already tolerate.

Merge confidence: 4/5, no findings, but the changed files were read in their touched regions rather than whole and the e2e shards had not finished.

[piotr-iohk]

QA review

Scope: Full review of the complete PR diff against merge base 3cac93b0, at 10e00654. No saved assessment baseline was available.

No new actionable code findings.

Local completion can use the accepted candidate's own prepared mining fee, including a real zero, while transaction history is still unavailable. The lookup is keyed by that winner's transaction id. A winner with no retained fee still uses the existing exact history calculation, and an unknown outcome or a shop request/transaction that does not match the guarded attempt stays unresolved. A conflicting fee for one transaction id fails before that receipt is saved, and a failed receipt write still prevents dispatch. Same-device backup replay keeps the local fee map. The shared backup still omits it, so a fresh restore waits for transaction history, which matches this PR's backup boundary.

Compared with bitkit-ios#915 at 6973637: iOS also keys exact fees by candidate, and an ordinary iOS send copies the prepared fee into follow-up state at prepare time. This Android change leaves the shared follow-up fee at 0 until local completion.

Validation: build (unit tests), lint, and detekt passed on this revision. I inspected the new store, coordinator, and follow-up tests and did not re-run them. e2e-status passed; those suites do not withhold transaction history or fail receipt persistence. The PR's manual cases cover that runtime fault setup.

Device testing: not performed in this review.

Ready for device testing.

[jvsena42]

No HIGH or MEDIUM finding at 10e0065. Approving with 1 LOW open (inline, on the new journeys file). It can be fixed here or moved to a follow-up issue, your choice; please reply on the thread saying which, with the issue link if it is a follow-up.

Checked and clean:

• Approved fee = broadcast fee = recorded fee. OnchainSendCoordinator.retryOriginal prepares once, approvePrepared shows receipt.miningFeeSats, retainPreparedReceipt stores that receipt's fee under receipt.txid.lowercase(), and submit broadcasts the same PreparedOnchainSend. The initial send in LightningRepo uses the same object the same way. A txid commits to its inputs and outputs, so a retained fee cannot go stale for its key; a fee-rate, UTXO or RBF change is a new txid with its own entry.
• Winner selection. winningCandidateFee looks up attempt.txid.lowercase(), and attempt.txid is only set from the native outcome or the observed transaction. A successor never reads the original's entry and the reverse. retainWinningFee still checks positive evidence, wallet index and txid before writing.
• No second broadcast. The map is read only in finishOnchainSendLocally; broadcastPreparedCandidate, blocksNextSend and admit are untouched. A conflicting fee for one txid fails in retainedMiningFees before the receipt is saved.
• Persisted format. candidateMiningFees: Map<String, ULong> = emptyMap() decodes to the default from a record written by master, and the load() invariant passes on an empty map. v2.5.0 has no OnchainSendAttemptStore, so there is no installed base for this state.
• keys ⊆ candidateTxids on every write path: add, discard, deadline reset, same-attempt restore (keeps the local map), other-attempt restore (empty map from the backup).
• Backup. ActiveOnchainAttemptBackup.from/restored neither export nor import the field, and restoreActive normalises it before the equality check, so an identical replay keeps local fees and a backup cannot supply one.
• Cancel and Back. Initial send: a cancel before the receipt is retained deletes the empty guard; after it but before dispatch the guard is released on the next current() and the fee entry goes with it. Recovery: declining in approvePrepared persists nothing; a failed authorizeOriginal or a pre-dispatch broadcast failure runs discardUnsubmittedRetryCandidate, which drops txid, rate and fee together; PaymentDeadlineExpired clears all three.
• Shop and Unknown. completeAcceptedShopFollowup still gates on request id, txid and positive evidence; a retained fee on an Unknown attempt does nothing.
• The diff adds no ULong arithmetic.

CI is green at this head (build with unit tests, lint, detekt, e2e).

⚠️ Device gate: not run — prepared-fee-recovery.md needs a controlled native transaction-history delay and a prepared-receipt persistence fault, which are not available on my setup and are not in the Capabilities table. The approval rests on the code scan, the unit tests and CI; both Manual Tests are still unrun.

⚠️ perf: not measured — needs the same delayed-history fixture. The PR removes one getTransactionDetails round trip from local completion; that claim rests on reading the code.

[piotr-iohk]

QA review

Scope: Follow-up review of the documentation delta since 10e00654, at 46a9a9de. Inherited the completed baseline QA review: prepared-fee persistence, winner selection, local-completion callers, backup omission, the listed unit tests, and the iOS fee-retention comparison at 6973637. This pass inspected journeys/send/prepared-fee-recovery.md, the retry description pointer, and the README row, and checked those claims against the unchanged Android paths and bitkit-ios#915 at f56b0fb.

No new actionable code findings.

The shared manual file is byte-identical on both platforms, and the twelve retry actions match. iOS Max recovery still has no editable fee field; candidateFeeSats stores the original prepared fee and replacement fees. Android still keeps original and replacement fees in candidateMiningFees. A restart after replacement acceptance, before local follow-up, can write that winner's retained fee into Sent activity without transaction history or another broadcast. Same-device backup replay keeps the local fee map; a fresh restore still waits for history. The earlier cross-platform file mismatch on prepared-fee-recovery.md is addressed by this delta.

Validation: build, lint, and detekt passed on this revision. Production and unit-test sources are unchanged since the baseline, so those tests were not re-run. e2e still had shards running, and multi_address_local had failed with no log available yet. The PR's manual cases in journeys/send/prepared-fee-recovery.md remain the runtime check for withheld history, receipt-persistence failure, and replacement-fee selection.

Device testing: not performed in this review.

[piotr-iohk]

Approved at 46a9a9de6484ca89f473621b11c89d1ec641df40.

Device testing was performed at 10e0065472f25307224ee37b6d74204cd284bc63. I verified the complete subsequent commit: it changes only the journey README, retry journey description and manual test recipe; application source, tests and runtime behavior are unchanged.

Focused device checks passed on a dedicated Pixel 6 emulator (4 cores, 4 GB RAM), with exactly one verified Bitkit installation and a fresh disposable staging-regtest wallet. This run used the clean PR APK with no runtime test seams; broadcast response loss was controlled through a local Electrum proxy.

• Dropped original broadcast: all three native attempts were intercepted without forwarding. Independent staging verification reported the transaction absent. Bitkit retained Payment Pending and exposed Retry original payment.
• Invalid 1,000 sat/vB input displayed fee validation feedback.
• Valid 5 sat/vB retry displayed an exact 703 sat mining fee for the original 1,000 sat recipient amount.
• Exact fee approval and the normal fee-over-half warning preceded the normal payment PIN.
• Canceling at PIN preserved the original Pending transaction and forwarded no broadcast.
• Reopening, reapproving and entering PIN produced one accepted replacement. Independent raw-transaction verification confirmed the same original input set, recipient script and 1,000 sat amount, with the exact 703 sat fee.
• Full app stop/relaunch and unlock retained one sent activity showing 1,000 sat payment + 703 sat fee. The proxy still recorded only one forwarded replacement.

Original dropped transaction: e67ac370f1282f7106a28eec7e51a5f04f7d91a626a48cc5e8d6189e990ff1b3 (141 sat fee).
Accepted replacement: a4722f54b41d74dc4ff4e0f8abad01c9543a79c42998dc612ba3528e8635a6bf (703 sat fee).

Coverage remains scoped: this does not establish the complete manual fault matrix. Accepted recovery with transaction history withheld, interruption before local follow-up, a confirmed original-winner race, prepared-receipt persistence failure, identity/request switches, full backup replay and absent-fee fallback were not verified on device in this run. Live Shop proof delivery was not exercised.

[jvsena42]

Approving at 46a9a9d. No HIGH, MEDIUM or LOW open: the one LOW from my review at 10e0065 (the fee-recovery checks living in different journey files on the two platforms) is fixed and resolved.

The only commit since that review, 46a9a9d, touches journeys/ only (README.md, send/onchain-original-payment-retry.xml, send/prepared-fee-recovery.md). The code I traced at 10e0065 is unchanged, so that review's checked-and-clean list still stands. CI is green at this head.

⚠️ Device gate: not run — prepared-fee-recovery.md needs controlled native transaction-history, broadcast-response and persistence faults, which are not available on my setup; the file itself now says they are not journey capabilities. The approval rests on the code scan, the unit tests and CI; the four manual cases are still unrun.

⚠️ perf: not measured — needs the same delayed-history fixture. The removed getTransactionDetails round trip rests on reading the code.