[ovitrif]

Closes #1855
Closes #1849
Closes #1868

This PR fixes four Spark payment gaps found in review after #1697 merged. The main one: a restored wallet could pay an invoice over its channels that Spark had already paid.

Description

• Restored wallet pays a Spark-paid invoice again (bug: restored wallet pays a spark-paid invoice again over channels #1855, from @pwltr's review feat: add spark via breez sdk #1697 (review)):
  • A full restore now records its time before any category is restored, so a restore with no metadata backup is held too.
  • After a restore, a payment over channels of a still-valid invoice dated before the restore, or whose date can't be read, is refused until the invoice expires, unless Spark's list shows it pending or paid. Spark's Synced event doesn't end this, because it can fire before the payment history syncs. The user is asked to request a new invoice.
  • Spark's own send follows the same rule: it refuses such an invoice when its list lacks it, shows it failed, or can't be read.
  • The refusal can end earlier once the SDK reports its payment-history sync: bug: synced fires before payment history has synced breez/spark-sdk#1314.
  • The sends Spark lists keep their invoice expiries, so they stay protected until those invoices expire, after a restart too.
  • Invoices created after the restore pay over channels normally, even during a Spark outage.
  • Installs that never restored are never held: every Spark Lightning send saves its hash before it goes out, and no released build sent over Spark.
• The node side of the same gap:
  • A restore copied the old device's node send list as complete, so while the node was down, Spark could pay what the node already paid.
  • The same rule now holds there: invoices dated before the restore wait until the node lists its payments, and those payments are then added to the cached list.
• A Spark payment past its request deadline (from @coreyphillips's review feat: pay contacts via spark #1754 (review), which feat: pay contacts via spark #1754 is blocked on):
  • The deadline is checked again after the uncertainty write, right before the send.
  • A refusal clears both holds.
• Spark pays while the node is down: the send sheet no longer waits for the node when Spark can pay the invoice.
• Spending kept after moving the whole balance to Savings (bug: spending balance kept after a full withdrawal #1849): after a full withdrawal, a 0 balance read while Spark's last sync failed is real, so Spending clears.

Out of Scope

• A payee reissuing the same payment hash in a new invoice dated after the restore: that invoice passes the date check, so it could be paid again over the other rail before that rail lists its sends.
• A payee whose clock runs fast can make an invoice from just before the restore look newer.
• A backup taken while its node list was not yet complete keeps refusing Spark sends until the node lists once, as master did before this PR.
• iOS: the same restore gap is fixed in feat: add spark via breez sdk bitkit-ios#1128, which hasn't merged yet.
• Translations of wallet__payment_restored_invoice: it is in English for now; translations follow in chore: translate spark strings #1840.

Design

N/A — no UI changes.

Preview

N/A — the refusal copy is unchanged.

QA Notes

Setup

A local regtest stack (bitkit-docker) with Spark regtest. Build devDebug from this PR's head.

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

• 1. Pay an invoice with Spark, restore the wallet from a backup taken before that payment, and pay the same unexpired invoice again before Spark syncs → the payment is refused over channels. After Spark syncs, Bitkit shows it as already paid. — needs a wallet restore from an older backup, which the journeys can't stage.
• 2. After that restore, pay a new invoice while Spark is offline → it pays over channels. — needs a Spark outage.
• 3. With the node stopped and Spark ready, pay an invoice Spark can pay → it pays over Spark with no wait for the node. — needs a stopped node.

Automated Checks

• added SparkRepoTest.kt — a pre-restore invoice is refused until Spark lists its sends; a post-restore one pays; a never-restored install is never held; a pre-restore invoice Spark's synced list lacks is refused over the node until it expires; a ready Spark that hasn't synced since the restore refuses an invoice from before it; a send whose deadline passes during the uncertainty write is refused; a 0 read after a full withdrawal drops the Spending balance
• added LightningRepoTest.kt — a pre-restore invoice is refused over Spark until the node lists its payments; a post-restore one pays with the node down; the node's listing ends the hold and seeds its hashes
• added BackupRepoTest.kt — a restore sets both holds and keeps the node list's seeded flag
• added SendSheetSyncOverlayTest.kt — the sheet doesn't wait for the node when Spark can pay

Evidence

• Each test failed before its fix and passes after it.
• just compile, just lint and just test pass on this PR's head.

Gaps

• Manual tests 1–3 were not run on a device.

Models used

• Planning/scoping: Claude Opus 5.5 (reasoning: high)
• Implementation: Claude Opus 5.5 (reasoning: high)
• Review: Not performed

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-11T13:15:57.661704Z	39a9317	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[greptile-apps]

Confidence Score: 2/5

[High impact] The PR changes when the wallet can send Lightning payments across Spark and node channels.

The PR needs changes before merging because restored wallets can still pay some invoices twice and Spark confirmation still waits for the node.

Findings

1.  Spark payments still wait ▶
2.  Missing metadata skips payment holds ▶
3.  Paid invoices lose protection ▶

Summary

This PR adds restore-time payment holds for Spark and node history, moves the final Spark deadline check after both cache writes, and clears the shown Spending balance after a full withdrawal.

• Restored wallets block older cross-rail payments until history is listed.
• Spark stops a payment when its request deadline has passed.
• The send sheet skips the node wait when Spark can pay an invoice.
• Spending shows zero after Spark moves the full balance to Savings.

Explicitly deferred by ovitrif in the supplied PR description: same-hash reissued invoices, fast payee clocks, and backups with an incomplete initial node list.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Restore wallet] --> B{Metadata applied?}
  B -->|Yes| C[Set history holds]
  B -->|No| D[No new history holds]
  C --> E[Each rail lists its sends]
  E --> F[Seed hashes and clear hold]
  F --> G[Trim cached hashes]
  C --> H[Older invoices wait while history is unavailable]
  D --> I[Node may pay a Spark-paid invoice]
  G --> J[Hashes without expiry can be dropped above 100]

Loading

_{Reviews (1) · Last reviewed commit: "fix: hold pre-restore invoices until the..." · Reviewed by Greptile}

[github-actions]

Regtest APK

Built from 7a80258 (run).

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

[talosmachina]

1 finding (1 P1). Holds a restored wallet's pre-restore invoices off the other rail until each rail lists its sends, re-checks the Spark deadline after the uncertainty write, lets Spark pay with the node down, and drops Spending after a full withdrawal. Reviewed 45413814, full tier.

What I checked, and 5 candidates I ruled out

Read in full: the changed functions in SparkRepo.kt (payInvoice, earlierLightningSend, syncActivities, seedRestoredSendHashes, refreshState, withdrawAll), LightningRepo.kt (payInvoice, earlierNodeSend, payInvoiceWithLdk, keepLdkSendHashes, executeWhenNodeRunning), CacheStore.kt seed/trim helpers, BackupRepo.restoreMetadataBackup, SendSheet.kt, SendConfirmScreen.kt
Call sites traced: earlierLightningSend (one caller, payInvoiceWithLdk), SparkError.SendRefused consumers (PaykitPaymentProofRepo.failLightningPayment, QuickPayRepo.classifyDispatchError), isNodeRunning into SendConfirmContent
CI: green at review time (build-and-test, lint, detekt, compose-ui); e2e-tests-local - lightning_security still pending
Build: not built here; on bitkit-android the review is reasoned from the code and CI

Ruled out

• New SendRefused from the node path misread as "nothing left": it is thrown before onBeforeSend and before LDK is called, so Paykit/QuickPay treating it as a pre-dispatch rejection is correct.
• Seeding from earlierLightningSend without the sync check: that list is read only when canListPayments() holds, so it is a synced list.
• Seed racing a restore: both seeders no-op once the *UnseededSince field is null, and updateData re-checks inside the transaction.
• Deadline refusal leaking the uncertain flag: the new order removes both the uncertain send and the hash on refusal, covered by the new test.
• withdrewAll hiding funds that arrive later: it clears on the first non-zero read, which the new test exercises.
• Cold reader: the run on the finding did not finish inside this turn, so it is unconfirmed by a second model; the trace below is the evidence.

Merge confidence: 3/5, one P1: the node-down Spark send still stops on the Confirm screen.

Reviewer model: Claude Opus 5.5
Reasoning effort: Not exposed (the run metadata available to this reviewer does not carry it)

[coreyphillips]

Two independent reviews.

needs changing before merge

• Confirmation still blocks Spark sends when the node is stopped (app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt:182-187)

worth doing, does not block

• A concurrent refresh can clear withdrewAll and bring the stale balance back (app/src/main/java/to/bitkit/repositories/SparkRepo.kt:578)

nits

• Every node send now decodes the invoice through coreService for the hold check (app/src/main/java/to/bitkit/repositories/LightningRepo.kt:1664)

the reviewers disagree, your call

• Spark hold is never armed when a restore applies no metadata cache (app/src/main/java/to/bitkit/repositories/BackupRepo.kt:1205-1213)
• Partial payment-history sync releases the restore hold (app/src/main/java/to/bitkit/repositories/SparkRepo.kt:1095-1098)
• Cache trimming removes protection for restored unexpired invoices (app/src/main/java/to/bitkit/data/CacheStore.kt:219-223)

[pwltr]

Reviewed 4541381.

A restore now holds each rail off invoices created before it until that rail's own synced list seeds the kept send hashes, and the seeding reads Spark's list only once Spark has synced. The deadline is checked after the uncertainty write, and a full withdrawal no longer keeps the old Spending balance.

High candidate — A restore without its metadata backup sets no hold, so the node can pay a Spark-paid invoice again. app/src/main/java/to/bitkit/repositories/BackupRepo.kt:1210 sets the holds only while applying the restored metadata cache. When the metadata backup is missing or fails to download, performFullRestoreFromLatestBackup continues without it (:1070), leaving no hold and no kept send hashes. Before Spark's first synced listing, earlierLightningSend then returns no earlier send, and payInvoiceWithLdk can pay an unexpired invoice Spark paid before the restore. This is the missing-metadata half of the #1697 finding. Setting the holds when a full restore starts, whatever the metadata outcome, would close it. Whether the payee would accept the second payment has not been shown.

Review details

General review
Model: Claude Opus 5.5
Reasoning effort: high

[jvsena42]

Code scan at 45413814d: no new findings beyond the open threads. I added a second consequence to greptile's expiry thread at SparkRepo.kt:1039: after an unsynced restart, Spark itself re-sends a seeded hash.

Checked and clean:

• Double pay across rails after a metadata restore: sparkHistoryUnseededSince and ldkHistoryUnseededSince live in AppCacheData (CacheStore.kt:349-359). Old backups decode with them null, and the restore always overwrites both with the restore time (BackupRepo.kt:1205-1213). Listed Spark sends are seeded into sparkSendHashes when the hold ends, so startedSend holds them after a relaunch. The iOS twin misses that step; I raised it on synonymdev/bitkit-ios#1128.
• No permanent hold: every hold is limited to invoices dated before the restore. It ends on Synced, on syncActivities, or on a successful list.
• Spark paying with the node down (2b240a573): the Spark path still runs earlierNodeSend, which refuses while the node list is unseeded.
• Balance after a full withdrawal (14813371d): display only.
• Kill paths: the marker is persisted before seeding, and a kill between addSparkSendHash, addSparkUncertainSend and the send leaves the hash held.

The restore-without-metadata path and Confirm waiting for the node are greptile's open P1s.

Spark is in no release yet (v3.1.0 predates the #1697 merge), so the changelog fragment describes a bug no released build had.

Device gate (partial): 45413814d — not run yet. Manual tests 1-3 from the PR body, plus a force-stop and relaunch with Spark unreachable before paying again, need a funded Spark regtest wallet and a restore. I'll run them on a later pass.

Reviewer model: Claude Fable 5.2 (claude-fable-5-2), Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed

[ovitrif]

Edited the body to close #1868 on merge, since a0e995a here fixes it; I reopened that issue, which I had closed before this PR merged.

[ovitrif]

Pushed f7ebbb8: the restore refusal now lasts until the invoice expires, and Spark's own send uses the same check.

@coreyphillips

• Blocking point (the 7-day window): addressed in 3214ec8. The window is gone on both rails: a still-valid invoice dated before the restore that Spark hasn't listed as pending or paid stays refused until its own expiry, and one whose date can't be read stays refused. The trade-off is that a long-expiry invoice can't be paid over channels after a restore; the user is asked for a new one. The refusal can end earlier once bug: synced fires before payment history has synced breez/spark-sdk#1314 gives a payment-history sync signal.
• Spark's own hold (r4540745651): addressed in 9d5e267 and f7ebbb8. Spark's send now uses the node's check, so such an invoice is refused with RestoredInvoiceUnverified whenever Spark's list lacks it, shows it as failed, or can't be read. Tests: "a synced spark whose list lacks a pre-restore invoice refuses to send it", "a synced spark whose list can't be read refuses a pre-restore invoice", "a pre-restore invoice that outlasts a week is still refused on both rails on day 8".

Tests: compile, detekt and unit tests pass.

Re-review is needed only because of this push: cb9a7af...f7ebbb8

[coreyphillips]

Rechecked after 3 new commits (9d5e267, 3214ec8, f7ebbb8). Nothing blocking remains.

addressed

• Partial payment-history sync releases the restore hold (app/src/main/java/to/bitkit/repositories/SparkRepo.kt:1095-1098). Fixed: A partial list can still seed and clear sparkHistoryUnseededSince, but that no longer lets the invoice through. In earlierLightningSend (SparkRepo.kt:956-965, 939-948), NotFound, an un-retried FAILED and an unreadable list all reach refuseUnverifiedRestoredInvoice (990-998). After 3214ec8 that refuses until the invoice expires, with no week cap. I traced each branch and confirmed LightningRepo.kt:1684 is the only node send, behind this check. I did not run the tests.
• Spark's own pre-restore hold ends on a list that can lack its sends (app/src/main/java/to/bitkit/repositories/SparkRepo.kt:308-322). Fixed: In payInvoice, NotFound, Found(FAILED) and an Unavailable read after seeding now fall to the else at SparkRepo.kt:317-319, which applies the node's refuseUnverifiedRestoredInvoice rule. The throw comes before onBeforeSend and the hash writes, so a refusal leaves nothing behind. I checked this by reading the when guards and the tests at SparkRepoTest.kt:344-399. I did not run them.

new since the last review

nits

• Comments still say a listing or a week ends the restore hold (app/src/main/java/to/bitkit/data/CacheStore.kt:366-369)

[ovitrif]

Pushed 2738531: comments and a log line only, from @coreyphillips's nit; no behaviour change. Compile and detekt pass.

Re-review is needed only because of this push: f7ebbb8...2738531

[jvsena42]

Device gate (partial) at cb9a7af06, on the local Spark regtest stack (bitkit-docker spark profile, Gate_Throwaway emulator). All five cases pass. The head has since moved four commits (9d5e26773..273853135), which change payInvoice and refuseUnverifiedRestoredInvoice. I'll review that delta and re-run what it touches.

To stage a restore without the payment: the metadata backup already held the Spark-paid hash, so after the phrase restore I removed that hash and its expiry from app_cache.json with run-as. That stands in for a backup taken before the payment.

Case	Result
1. Pre-restore invoice X that Spark already paid, before Spark syncs	Pass. Over Spark (no channel, operators down) and over channels it is refused: "Bitkit can't confirm this invoice wasn't paid before you restored your wallet. Please request a new invoice." After Spark syncs and re-seeds, paying X shows "Bitcoin Sent" with no new send on either rail, and the payee has a single inbound record.
2. New invoice while Spark is offline	Pass, paid over channels (PaymentSuccessful, 7,000 sats to the SSP node). A first attempt to another node failed with ROUTE_NOT_FOUND because the SSP won't forward from a private channel, a limit of this stack.
3. Node down, Spark ready	Pass. Confirm opened with no sync overlay, and the payment went over Spark (Sent Spark payment '01a1cbc6…').
4. Pre-restore invoice Y that Spark never paid, over channels after Spark's sync	Pass, refused with the new wallet__payment_restored_invoice copy.
5. Relaunch with Spark unreachable after its first sync, then pay X again	Pass. "Payment Pending", with no LDK or Spark send and no new activity row.

17:41:34.364 WARN [SparkRepo.kt:318] Refused a Spark send of '1aaecbe8…' until Spark lists its sends
17:44:45.496 WARN [SparkRepo.kt:948] Refused a node send until Spark lists its earlier sends once
17:54:46.572 WARN [SparkRepo.kt:999] Refused a node send of a pre-restore invoice Spark hasn't listed
18:05:06.705 ERROR Error executing 'payInvoice' [PaymentPendingException='Payment pending']

Small UX notes, not findings:

• Paying an already-paid invoice again shows the "Bitcoin Sent" success sheet, with no sign that nothing new was sent.
• An invoice paid before the restore shows "Payment Pending" while Spark is unreachable.
• The restored-invoice refusal offers "Try Again", which can't succeed.

Device gate (partial): cb9a7af06 (head 273853135; the new commits are not yet reviewed or driven).

Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed

[jvsena42]

Re-review of 9d5e26773, 3214ec889, f7ebbb888 and 273853135: no findings. Approving on the gate posted above (cb9a7af06, all five cases pass) plus this review.

• Spark rail (9d5e26773, f7ebbb888): payInvoice now runs refuseUnverifiedRestoredInvoice when Spark's list is unreadable or doesn't hold the hash, the same rule as the node's send. It runs before onBeforeSend, sendsInFlight and the hash writes, so a refusal leaves nothing behind. An invoice Spark listed as paid or pending still returns its hash or PaymentPendingException.
• No 7-day cap (3214ec889): the refusal now lasts until the invoice expires. On the Spark path the expiry is always known, from the parsed invoice. On the node path an undecodable invoice is refused, which fails closed, and an invoice that won't decode wouldn't pay anyway. holdsRestoredInvoices() now stays true after a restore, which costs one invoice decode per node send.
• Consequence: after a restore, an invoice made before it that Spark never paid can't be paid on either rail until it expires, and the copy asks for a new invoice. That is a deliberate trade against a double payment.

The gate drove the node-side refusal of a pre-restore invoice Spark hadn't listed (case 4). The Spark-side version added here is covered by the new SparkRepoTest cases, not driven.

Device gate: cb9a7af06 (head 273853135: the delta above, reviewed) — 5 manual cases passed on the local Spark regtest stack.

Reviewer model: Claude Fable 5.2 (claude-fable-5-2), Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed

[coreyphillips]

Rechecked after 1 new commit (2738531). Nothing blocking remains.

still open

• Comments still say a listing or a week ends the restore hold (app/src/main/java/to/bitkit/data/CacheStore.kt:366-369). Since 3214ec8 the hold ends only when the invoice expires, and it applies on both rails, but two comments still describe the old rule. The KDoc on AppCacheData.sparkRestoredAt (CacheStore.kt:366-369) says "for a while after it the node refuses", which is now wrong on both counts. The comment on the isUnlistedSinceRestore branch (SparkRepo.kt:312-316), and its log line "until Spark lists its sends", imply that a Spark listing ends the refusal. It doesn't: the else branch refuses the same invoice after the listing. The branch is also nearly redundant now, because the restore code sets sparkHistoryUnseededSince and sparkRestoredAt to the same time. Claude still sees it: Both places you named are fixed. The KDoc at CacheStore.kt:366-369 now says both rails refuse until expiry, and the comment and log line at SparkRepo.kt:312-316 no longer say a listing ends the refusal. I checked these against refuseUnverifiedRestoredInvoice, which keys only on sparkRestoredAt and expiry. Two comments from earlier commits on this branch still say a listing ends the hold. The KDoc on seedRestoredSendHashes (SparkRepo.kt:1081) says seeding means "the hold on older invoices ends", but seeding clears only sparkHistoryUnseededSince and the expiry hold stays. BackupRepo.kt:1051-1052 says the restore arms holds "until that rail lists its own sends", but the same update arms sparkRestoredAt, which lasts until expiry.

new since the last review

worth doing, does not block

• New refusal string wallet__payment_restored_invoice has no translations (app/src/main/res/values/strings.xml:1502)

[ovitrif]

Pushed 7a80258: comments only, no behaviour change. Compile and detekt pass.

@coreyphillips

• The two comments your recheck found are fixed: the seedRestoredSendHashes KDoc and the restore comment in BackupRepo now say seeding ends only the wait for Spark's first listing, and an invoice Spark hasn't listed stays refused until it expires.
• Translations of wallet__payment_restored_invoice: out of scope, tracked in chore: translate spark strings #1840 with the other Spark keys. The body's Out of Scope now says so.

@jvsena42 Thanks for the device run. On your three UX notes:

• "Bitcoin Sent" for an invoice that is already paid, and "Payment Pending" while Spark can't confirm one, are the existing outcomes for those cases, so they stay as they are.
• "Try Again" on the restored-invoice refusal comes from the generic failure sheet, and can't succeed until the invoice expires. The message itself asks for a new invoice; a tailored action is a design change, so this PR leaves it.

Re-review is needed only because of this push: 2738531...7a80258

[pwltr]

Reviewed 7a80258.

No consequential correctness finding was established by the static General review.

Review details

General review
Model: GPT-6.1 Sol
Reasoning effort: high

[ovitrif]

Cancelled and reran the e2e run: its lightning_security job sat for 1 h 40 min at "Create AVD and generate snapshot", which holds the required e2e-status check. No code change.

[ovitrif]

Merging: @pwltr approved the head, 7a80258, and @jvsena42 and @coreyphillips approved 2738531. The commits after that change comments only. All checks are green, and no thread is open.

The lightning_security e2e job needed two reruns. On the first run it stalled while creating the emulator. On the second, its tests passed, but the artifact upload failed during GitHub's outage. The device run is @jvsena42's gate at cb9a7af, where all five cases pass. The Spark-side refusal added after that is covered by SparkRepoTest.

This closes #1849, #1855 and #1868.