Skip to content

fix: retain prepared transaction fees - #1459

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

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

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

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

N/A - not drivable; see Manual Tests.

Manual Tests

  • In a disposable regtest fixture, follow 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.

Controlled device/payment checks above remain unrun. The isolated earlier Accepted-payment fixture was preserved and not resumed; no new payment, funding or broadcast was performed for this fix.

@ben-kaufman
ben-kaufman marked this pull request as ready for review October 11, 2026 05:07
@greptile-apps

greptile-apps Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence 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

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 10e0065 (run).

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

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

This branch has not been deployed

No deployments
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