Skip to content

feat: move LN channel balances to spark - #1774

Open
ovitrif wants to merge 34 commits into
feat/breez-spark-lightningfrom
feat/move-to-spark
Open

ovitrif wants to merge 34 commits into
feat/breez-spark-lightningfrom
feat/move-to-spark

Conversation

@ovitrif

@ovitrif ovitrif commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1768

Twin: synonymdev/bitkit-ios#1212
Dependency: #1697
Companion: synonymdev/bitkit-e2e-tests#297

This PR adds Move to Spark: a one-time sheet and an action that pays the balance of a Lightning channel to the user's own Spark wallet over the channel, then offers to close the emptied channel. Blocktank is closing its channels, and moving the balance over Lightning costs only the routing fee, where a close followed by a deposit costs on-chain and claim fees and 3 confirmations. It stacks on #1697: review it against that base; it merges after #1697.

Description

  • Adds MoveToSparkRepo, which quotes the move against a new amountless Spark invoice: it starts from what the channels can send and leaves room for the routing fee. The fee shown is LDK's estimate plus a 2 sat margin, and the payment's routing fee is capped at exactly that.
  • Adds LightningRepo.payInvoiceOverLdk and estimateLdkRoutingFeesForAmount, so the move always goes over the channels and never falls back to Spark; the default shouldPayWithSpark routing is unchanged.
  • Adds the Move to Spark screen: quote, swipe to move, then "Moved to Spark" with Close, which opens the existing cooperative close; after a partial move it offers Move The Rest instead.
  • Shows "Amount too large" with Move Half when no route carries the amount, down to a 1,000 sat minimum, so a large balance can move in parts; any other failure, such as an expired invoice or a stopped node, shows "Could not move" with Try Again.
  • Waits up to 2 minutes for the payment's outcome. A payment still in flight then shows "Moving to Spark" with OK only, since its amount may still return to the channel; a failure that arrives later shows "Could not move" and the usual failed notice.
  • Adds a timed sheet that tells channel holders Blocktank is closing its connections and opens Move to Spark; it shows once, at the top priority, and is checked again once channels and Spark have loaded after a cold start.
  • Adds a Move To Spark button to Spending and Lightning Connections while a channel can still send at least 1,000 sats to a usable Spark wallet, after the fee reserve.
  • Keeps the move's own send and receive from raising the usual payment sent or received notices, in the foreground and from the background service, since the screen reports the outcome.
  • Records a move as one Activity row, the channel send, with the moved amount and the routing fee, whichever leg arrives first; the Spark receive with the same payment hash only marks a pending or failed send as succeeded.
  • Fits the smallest screens: button labels stay on one line at 360 dp and on an iPhone SE, the buttons sit at the bottom with the illustration taking the flexible height, and headlines put the white words first and the shorter accent last, as master's screens do.

Out of Scope

  • Activity: a move shows as an outgoing payment for now; a designed Move to Spark row is bug: move to spark activity loses direction and fee #1819.
  • Design: an AI-made first version from existing components; a Figma design and designer review follow.
  • Review follow-ups: move() and MoveToSparkViewModel unit tests, keeping the outcome after leaving the screen, and keeping the move's hash across a restart — chore: move to spark review follow-ups #1802.
  • Emergency exit: Move to Spark should not be offered while a Spark emergency exit runs, but isExiting comes with the exit PR (feat: spark emergency exit #1782), not this PR's base. Every Move to Spark check goes through MoveToSparkMath.isSparkUsable in MoveToSparkRepo.kt, so whichever of the two PRs lands second changes that one line to spark.isReady && !spark.isExiting.

Design

N/A — no design available. UI pending design: this is an AI-made first version from existing components; a Figma design and designer review follow.

Preview

Android emulator (Pixel profile) at 896 × 1994, 320 dpi, at 2x speed.

Sheet on Home, Later Too large, Move Half Move the rest Close afterwards
rec-sheet2x.mp4
rec-too-large2x.mp4
rec-move-rest2x.mp4
rec-close2x.mp4

Screens

Android emulator (Pixel profile) at 896 × 1994, 320 dpi, in flow order. Bitkit draws dark only, its system bars included.

Post-update sheet Spending entry Connections entry Finding a route
Too large Confirm: amount and fee Moving Done: part moved
Done: close offer Close confirmation After the close Could not move

QA Notes

Setup

  • A local Spark regtest: the spark profile from feat: local spark regtest network bitkit-docker#37 (./spark up), which runs three Spark operators, the SSP with its Lightning node, the LNURL and data-sync servers and a chain API on bitkit-docker's chain, compatible with Spark SDK 0.28. It does not publish the SSP node's P2P port (9735); publish it in a compose override to open a channel from the app.
  • A dev build (gradlew assembleDevDebug) with SPARK_LOCAL_CONFIG_B64 set to ./spark config --base64 and the Spark chain service pointed at http://127.0.0.1:23090/api, then adb reverse for 23535–23537, 23049, 23090 and the SSP node's P2P port, and Electrum (60001). This PR has no switch for it; the app-side settings land in separate PRs.
  • In the app, fund Savings (./spark send <address> 500000), then open Lightning Connections, Manual Setup, and open a channel to the SSP node; confirm it with ./spark mine 6.
  • Manual test 1 needs a route limit the stack cannot set: the tested build carried a QA hook, a qa-move file in the app's files folder (limit=<sats>, fail, slow).

Journeys

  • J1: new move-to-spark.xml — sheet once, entry button, move with routing fee, close afterwards, nothing for a wallet without a channel

Manual Tests

  • 1. With a route that cannot carry the balance, open Move To Spark → "Amount too large" offers to move half; repeating it moves a part that routes, and Done offers Move The Rest — needs a route limit the local stack cannot set (tested with a local route-limit hook)

Automated Checks

  • added MoveToSparkRepoTest.kt — quote fits the routing fee under the channel limit, grows back once the real fee is known, keeps a smaller target, offers a move only when 1,000 sats fit beside the fee reserve, maps no route (including a send that finds no route on a live invoice) to too large, refuses with nothing to move or Spark not ready
  • added ActivityServiceMoveToSparkTest.kt and a SparkRepoTest.kt case — a move keeps one outgoing Activity row with the routing fee, whichever leg arrives first
  • updated NotifyPaymentReceivedHandlerTest.kt and AppViewModelSendFlowTest.kt — no received notice for the move, the new sheet wired in

Evidence

Tested at eccf3648a on the stack and build from Setup, Android emulator (Pixel profile) at 896 × 1994, 320 dpi, one 400,000 sat channel to the SSP; the Preview and Screens above are this run. Later commits (f7dd3ec and 3341321) change only the in-flight screen and Activity rows, which this run does not show.

  • J1 sheet: once the channel confirmed, the sheet showed on Home; Later closed it.
  • 1: with the route limit, the full balance showed "Amount too large"; Move Half moved a part with a 0 sat routing fee (direct channel), and Done offered Move The Rest.
  • J1 move and close: Move To Spark moved the rest, Done offered Close, and Close then Close returned to Lightning Connections with no open connection and the balance on Spark.
  • Could not move: with the fail hook, the screen showed "Could not move" with Try Again.

Gaps

  • Only direct channels to the SSP were tested; a multi-hop route with a real routing fee was not set up locally.
  • J1's "not again after relaunch" and "no channel, no sheet" steps were not repeated in this run.
  • The Activity rows and the in-flight screen were not driven on a device; unit tests cover them.

Mainnet tests (run by the author)

These run on real devices with mainnet funds, by the author. Reviewers and review bots don't need to run them, and they don't block review.

Round 1

None yet.

Round 2 (pending)

  • M1: intro — after updating with a funded channel, the post-update sheet shows once on Home and opens Move to Spark
  • M2: move sheet — Move To Spark quotes the channel balance with a routing fee that fits under the channel's limit
  • M3: move — swiping moves the balance over the channel to the Spark wallet, with a routing fee no higher than the one shown
  • M4: channel close — Close Connection on Done closes the emptied channel cooperatively
  • M5: balance after — Spark holds the moved amount and the closed channel's remainder returns to Savings

Models used

  • Planning/scoping: claude-opus-5-5 (reasoning: medium)
  • Implementation: claude-opus-5-5 (reasoning: medium)
  • Review: claude-opus-5-5 (reasoning: Not exposed)

Metadata note: the review subagents' run records give the model but not a reasoning setting.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 50f10b3 (run).

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

@ovitrif

ovitrif commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 95f993e89: the Move To Spark button in Lightning Connections now sits below the open channels instead of above the Spark section, next to the channel it moves and clear of the Spark row and detail that the Spark exit work is changing on the base branch. Layout only; compile and lint pass.

@ovitrif

ovitrif commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 84f49451a..62526d3bf, a small-screen pass on the Move to Spark screens:

  • Button labels stay on one line at 360 dp: Later / Move Now, Cancel / Move Half, Later / Move Rest, Later / Close (the amount left the button; Close replaces Close Connection).
  • Bottom buttons sit at the screen bottom; the illustration takes the flexible height and shrinks so the sheet copy stays whole.
  • Headlines put the white words first and the shorter accent last, as master's screens do, with the purple accent of the Lightning screens.
  • All 15 locales follow the same headline shape and short labels.

Screenshots at 360 dp are in Preview. just compile, just lint and just test pass.

@ovitrif
ovitrif force-pushed the feat/breez-spark-lightning branch from ab34526 to ab1b09d Compare November 5, 2026 02:00
@ovitrif
ovitrif force-pushed the feat/move-to-spark branch from 62526d3 to 967292c Compare November 5, 2026 02:10
@ovitrif

ovitrif commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto the rebuilt Spark base ab1b09d14, which drops the emergency exit, and force-pushed with a lease (this PR is a draft): 62526d3bf → 967292c09.

  • ContentView and LightningConnectionsViewModel conflicts kept the new base's lines and only the Move to Spark route, entry and canMoveToSpark additions.
  • 967292c09 adapts to the removed exit: Spark readiness is SparkState.isReady (the removed isUsable also excluded a running exit), and the screen formats amounts itself instead of using the exit's helper.

just compile, just lint and just test pass. The device runs and screens in the description were taken before the rebase on the same Move to Spark code.

@ovitrif

ovitrif commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Merged the new base head 4632e7057 (Spark chain service on Bitkit electrs) into this PR as fe4488ae5. It merged without conflicts and changes only the base's Env.kt, SparkService.kt and SparkChainServiceTest.kt; no Move to Spark file changed. just compile and SparkChainServiceTest.kt pass locally; CI runs the full suite.

@ovitrif

ovitrif commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Reran the failed CI jobs at fe4488ae5. compose-ui / ui-tests hit the 45-minute job limit inside the emulator step without writing a test report, and the build gate failed only because of that. Build and unit tests passed, the previous head and the base's 4632e7057 passed the same checks, and the merge changed no Move to Spark file.

@ovitrif ovitrif changed the title feat: channel balances move to spark at low fees feat: move LN channel balances to spark Nov 5, 2026
@ovitrif

ovitrif commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Dropped "at the lowest fee" from this PR's changelog line, to match the new title: the line now says what the change does, not how cheaply.

@ovitrif

ovitrif commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed cae73d244: every Move to Spark check, including the move's own guard, now goes through MoveToSparkMath.isSparkUsable. Move to Spark must not be offered during a Spark emergency exit, and isExiting arrives with #1782, not this PR's base. Whichever of the two PRs lands second changes that one line to spark.isReady && !spark.isExiting; this is now under Out of Scope in the description. just compile, MoveToSparkRepoTest.kt and just lint pass.

@ovitrif

ovitrif commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Merged the new base head 4e22b9ca3 (Breez SDK 0.28.0) into this PR as 09219e448. It merged without conflicts; Move to Spark's code needed no change for the 0.28 API. just compile, just test and just lint pass locally.

@ovitrif

ovitrif commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

Pushed d8b7d32c2: fixes from an independent pre-review pass, and the Spark base 2f483bd14 (master merged into #1697) merged in.

  • Only a missing route (RouteNotFound, ROUTE_NOT_FOUND, RETRIES_EXHAUSTED) shows "Amount too large"; any other failure, such as an expired invoice or a stopped node, shows "Could not move" with Try Again.
  • The fee shown is LDK's estimate plus the 2 sat margin, and the payment's routing fee cap is exactly that, so it never pays more than shown.
  • While a payment is still in flight after the 2 minute wait, Done offers only OK, not Close Connection.
  • The background service no longer posts a received notice for the move; a failure after the screen stopped waiting raises the usual failed notice.
  • A second move call while one is in progress does nothing.

The remaining low-severity items are in #1802. just compile, just lint and just test pass.

@ovitrif
ovitrif marked this pull request as ready for review November 6, 2026 17:19

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta 891630d49..91b4a4eaa is clean: e06ac74cf treats a PaymentSendingFailed on an unexpired invoice as no route (the Android side of the LOW on synonymdev/bitkit-ios#1212), 91b4a4eaa only names arguments, 7d9bd0f4d offers the move only when the minimum still fits after the first fee guess, ac748932e is journey text. The rest comes from the base, reviewed on #1697.

Device gate (partial): 91b4a4eaa — not run yet; transfer/move-to-spark.xml needs a channel and a Spark regtest wallet.

@ovitrif

ovitrif commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 9b473fe61 and 91b4a4eaa: merged the Spark base c85259fe2. Its payInvoiceWithLdk takes a payment deadline now, so the channel-only send passes none, which fixes the compile error on the merged head. just compile, just lint and just test pass.

@pwltr pwltr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 91b4a4e.

Medium candidate — Retain hashless settlement failures until the payment ID is known. MoveToSparkRepo.kt:133: the settlement listener begins filtering events while paymentId is null; the ID is assigned only after payInvoiceOverLdk returns. A supported PaymentFailed event with a null hash can arrive independently before that return and be rejected despite belonging to the transfer. The SharedFlow does not replay it once the ID becomes known, so the two-minute timeout reports InFlight and skips the failure or smaller-payment recovery flow. Buffer or reconcile early hashless failures when the matching ID becomes available. This is supported by static source ordering and existing event-shape tests; production timing was not reproduced, and native library internals were unavailable, so it remains a candidate.

Review details

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

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: Full reassessment of the whole pull request at 91b4a4ea, against merge base c85259fe. The comparison base changed and no coverage baseline was supplied, so this pass covers the entire PR.

No new actionable code findings.

The quote keeps the moved amount plus the shown routing fee (the LDK estimate, rounded down, plus 2 sats) within the usable outbound limit, and the channel-only send caps the fee at that amount. The send uses the Spark invoice’s one-hour expiry. While this process still remembers the payment hash, foreground send/receive notices and the background payment notification are suppressed. If the payment has no result after two minutes, the screen offers only OK.

The same quote math, 1,000 sat minimum, fee cap, entry points, and move-to-spark journey match bitkit-ios#1212 at b6049d0a. iOS keeps the swipe in progress until the payment settles; Android shows the in-flight result after two minutes. bitkit-e2e-tests#297 at b004e5ed dismisses MoveToSparkIntroLater, which this sheet exposes. Spark createInvoice(amount, description, expiry) and SparkState.isReady match on bitkit-android#1697 at 85986cde and on this PR’s base.

Unit, lint, and UI checks passed at this revision. Local e2e failed, including onboarding waiting for home on a new wallet, before a channel exists for this sheet. Device testing was not performed. The author’s journey evidence is from earlier revisions and still needs the PR’s local Spark regtest setup.

Suggested additional test cases

  • Android, dev build on the PR’s local Spark regtest, with one channel that can send at least 1,000 sats: swipe to move and leave the payment unsettled for more than 2 minutes. The screen shows the in-flight text and only OK. After the payment settles, Spark holds the amount and the channel can still be closed from Lightning Connections.

Earlier findings reconciled at this revision: 5 verified fixed, 1 not substantiated.

Reviewer model: grok-4.7
Reasoning effort: xhigh

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Device gate (partial): 91b4a4eaa. Journeys: 0 passed, 0 failed, 1 partial. perf: not relevant, skipped.

transfer/move-to-spark.xml:

  • Steps 1–3 passed. The Move To Spark sheet showed, Later closed it, and it did not come back after a relaunch.
    • On the first launch the sheet came after Backup. canMove was still false at the 2 s timed-sheet check, so the priority-4 Backup sheet won. This is expected ordering, not a defect.
  • Step 4 did not hold, because of the environment. The local bitcoind was reset mid-run and rolled back 13 blocks. The funding tx lost its confirmations (Funding transaction was un-confirmed. Locked at 3 confs, now have 0 confs) and LDK closed the channel, so canMove went false and the Move button was gone. A second channel never confirmed on the reset chain.
  • Steps 5–8 not run. They need the PR's local spark-sdk regtest/local stack. Its ghcr.io/breez/spark-operator image is private ("unauthorized"), so I could not start it. I used Lightspark's hosted regtest for Spark, plus a 400k Manual Setup channel to the local LND.

Setup: Pixel_9 emulator, E2E=true E2E_BACKEND=local build with local VSS, lnurl-auth and electrs/bitcoind, and Spark on Lightspark's hosted regtest. Fresh wallet. No crashes.

⚠️ The move itself (steps 5–8) still needs a run with access to the spark-operator image. The code scan has nothing above LOW, so this is the only thing between this PR and approval.

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

@ovitrif

ovitrif commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Medium candidate — Retain hashless settlement failures until the payment ID is known. […] Buffer or reconcile early hashless failures when the matching ID becomes available.

@pwltr Fixed in eccf364: while the payment id is still unknown, the listener keeps every PaymentFailed without a hash, and once payInvoiceOverLdk returns the id, a kept failure with that id settles the move at once, so it reaches Amount too large or Could not move instead of the two-minute in-flight outcome. move() itself still has no unit test; that stays in #1802.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta review 91b4a4eaa..eccf3648a: no findings. A hashless PaymentFailed that arrives before payInvoiceOverLdk returns its id is now queued, and matched to sentId afterwards. A failure for another payment can't match, and the in-flight timeout path is unchanged.

Device gate: still partial (steps 5–8 of move-to-spark.xml need the private spark-operator image), as in my last review.

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

@ben-kaufman ben-kaufman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new material findings at eccf3648a.

Coverage limits remain: the actual move has no current-head device run, multi-hop routing fees are untested, move/view-model tests are deferred to #1802, and the separate security passes did not complete.

Reviewer model: gpt-6.1-sol
Reasoning effort: xhigh

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: Follow-up at eccf3648, re-reviewing the delta since 91b4a4ea against the completed baseline review. Merge base is still c85259fe. This pass inspected MoveToSparkRepo.payAndWait, the channel-only send it calls, and the move screen's failure and in-flight results. Inherited coverage: quote and fee reserve, channel-only send and invoice expiry before an id exists, notice suppression, the intro sheet and Spending/Connections entry, the iOS comparison at b6049d0a, the e2e intro selector at b004e5ed, and Spark createInvoice / isReady on the #1697 base.

No new actionable code findings.

A hashless PaymentFailed that arrives before payInvoiceOverLdk returns is queued and, when its payment id is the id that send returns, ends the move as too large or failed. The two-minute in-flight result remains the path when that id has no matching event. The invoice hash is still recorded before that result, so foreground and background notice suppression still applies to this early failure. The five resolved threads on this PR were checked at this head; the payment-id match, raw route error, live channel state, reported-hash suppression, and 1,000 sat fee reserve are still in the code.

Device testing: not performed in this review. Validation was source inspection only. MoveToSparkRepoTest covers the quote and fee math.

Suggested additional test cases

  • Android unit (MoveToSparkRepoTest): with Spark ready and a channel above the minimum, emit PaymentFailed (paymentHash null, paymentId equal to the id payInvoiceOverLdk will return, reason ROUTE_NOT_FOUND) before that call returns. Expected: move() fails with MoveToSparkError.TooLarge.

Non-blocking consistency suggestions

  • bitkit-ios#1212 at 0a7b433e starts listening in sendOverChannels only after send returns, and paymentFailed matches only when paymentHash equals that id. The same early hashless failure would leave the iOS move screen unchanged.

Reviewer model: grok-4.7
Reasoning effort: xhigh

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta eccf3648a..f7dd3ecb7: no findings. The in-flight Done screen now has its own title and illustration, and still offers only OK, so closing the channel stays hidden while the payment may return. in_flight_title and the new in_flight_text are translated in every values-* file.

Device gate: still partial (needs the private spark-operator image).

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

@ovitrif

ovitrif commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed f7dd3ec and 3341321:

  • A move still in flight after the 2-minute wait now shows Moving to Spark with the lightning illustration, and its text no longer suggests closing the connection (all locales).
  • A move keeps one Activity row, the channel send, outgoing with the moved amount and the routing fee, whichever leg arrives first: SparkRepo skips a Spark receive whose hash already has a sent row, and ActivityService lets the send replace a received row with its hash. Tests cover both arrival orders. This is the interim rule from the iOS twin's review (feat: move LN channel balances to spark bitkit-ios#1212); a designed transfer row stays in bug: move to spark activity loses direction and fee #1819.

just compile, just test and just lint pass.

@ovitrif

ovitrif commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Steps 5–8 not run. They need the PR's local spark-sdk regtest/local stack. Its ghcr.io/breez/spark-operator image is private ("unauthorized"), so I could not start it.

@jvsena42 The QA Notes Setup now uses the spark profile from synonymdev/bitkit-docker#37 instead. It pins the last public operator and an SSP patched for Spark SDK 0.28, so it starts without the private image. I ran J1 and manual test 1 on it at this PR's head today, and the Preview and Screens are from that run. Two things the profile doesn't do yet: it doesn't publish the SSP node's P2P port (9735), so add a compose override to open a channel from the app, and the app needs the local-stack build settings from the Setup until those land in their own PRs.

… feat/move-to-spark

# Conflicts:
#	app/src/main/java/to/bitkit/repositories/SparkRepo.kt
@ovitrif

ovitrif commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed a6a8432: merged the Spark base 767f0eb0e. The one conflict was in SparkRepo.recordActivity: both guards stay, the move's receive skip and the base's skip of a later attempt on an already-paid invoice. just compile, just test and just lint pass.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta f7dd3ecb7..a6a84323d: no findings. The moves channel send and its Spark receive share one hash, and the send now owns the row whichever leg arrives first. SparkRepo.recordActivity skips the Spark receive when a SENT row exists, and ActivityService lets a non-failed outbound LDK payment replace a RECEIVED row through the existing update path. A failed channel send cant replace it, and Spark only receives when the send succeeds.

Device gate: pending. It can now run on the bitkit-docker#37 spark profile.

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

@pwltr pwltr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed a6a8432.

Medium — Completed Spark receipt can leave the send activity pending or failed. SparkRepo.recordActivity discards every receive update when an existing row is a send, including when that send is still pending or failed. CoreService.processBolt11 can also replace a completed receive with a pending send. With a delayed or missing LDK terminal update after timeout or restart, completed settlement is therefore not reflected in activity. Preserve the outbound direction and fee while reconciling terminal receipt evidence.

Same-quote retry idempotency remains an unconfirmed Medium candidate: the LDK send implementation and UI retry caller were outside the frozen scope and their transport was rejected. No duplicate payment is established. This was static source review; runtime validation remains unperformed.

Review details

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

Security review
Model: Kimi K3
Reasoning effort: high

A completed Spark receive marks its pending channel send succeeded, and only a succeeded send replaces the Spark receive row.
@ovitrif

ovitrif commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Medium — Completed Spark receipt can leave the send activity pending or failed.

@pwltr Fixed in c5327fc:

  • A completed Spark receive now marks its move's send row succeeded when that row is still pending or failed. The row keeps the outbound direction, the moved amount and the routing fee.
  • processBolt11 replaces the Spark receive row only with a succeeded send. A pending or failed update never undoes a succeeded row, so a later LDK sync still reporting the send as pending leaves it settled.
  • New tests cover both orders: a completed receive after a pending send, and a pending send after a completed receive, then its success.

@ovitrif
ovitrif requested a review from pwltr November 9, 2026 18:17

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta a6a84323d..6ca4d3fa2 (c5327fc): no findings. A completed Spark receive marks the move's pending channel send SUCCEEDED, and only a SUCCEEDED send replaces the receive row. A pending or failed update can no longer downgrade a settled row. The send's own later success still updates its fee through the normal path.

Device gate: in progress on the bitkit-docker spark profile.

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

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device gate passed at a6a8432. It ran on the bitkit-docker spark profile (cdd294a) on a Pixel_9 emulator with a fresh to.bitkit.dev, with Savings funded and a 400k channel opened to the SSP.

  • J1 (move-to-spark.xml): PASS.
    • With no channel there is no sheet and no button.
    • The intro sheet shows once the channel is ready and doesn't come back after Later and a relaunch.
    • The quote, the swipe and the Done screen work, as do both closes. Spark ends at 394 930, which is 196 478 + 198 452.
    • Activity shows one "Sent · Move to Spark" row per move, with no received row.
  • Manual test 1: PASS. I used a local route-limit hook (limit=250000), since the PR ships none. "Amount too large" offered Move Half at 196 478, the move paid ₿0, and Done offered Move Rest.

The commits since then (3341321, c5327fc) change only how the move's activity row is recorded and settled. I code-reviewed them, and unit tests cover them.

Not exercised, because no QA case covers them: the in-flight screen, "Could not move", and a multi-hop route with a real fee.

Evidence: .ai/gate_spark_1774_1794249927/ (screens, layout dumps, log excerpt).

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

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.

6 participants