Skip to content

fix: add retry to coin selection loading - #1288

Open
jvsena42 wants to merge 14 commits into
masterfrom
fix/616-coin-selection-load-retry
Open

jvsena42 wants to merge 14 commits into
masterfrom
fix/616-coin-selection-load-retry

Conversation

@jvsena42

@jvsena42 jvsena42 commented Sep 16, 2026

Copy link
Copy Markdown
Member

Fixes #616
Related to #1275

This PR adds a bounded retry and an inline error with a Retry button to the coin selection load, and stops activity updates from reloading the list.

Description

  • Retries loading spendable outputs up to 3 times (1s, then 2s apart) when the node is not set up or not running, so a load that races the node start does not fail on the first attempt.
  • Leaves other failures unretried, including the node run timeout (the repo already waits up to 1 minute for the node) and fee calculation errors, so the worst-case wait stays the same as before.
  • Replaces the raw English error toast with an inline, localized error message and a Retry button when no coins could be loaded; the message stays visible and the button shows its loading state while a retry runs.
  • Centers the error message and its Retry button vertically in the space the UTXO list would occupy, instead of pinning them under the title.
  • Shows a spinner while the first load is in progress and nothing is listed yet, centered in that same space so it does not jump when the error replaces it.
  • Loads UTXOs once per required amount and address, and passes activity updates to the view model separately, so a new activity no longer reloads the list or resets a manual selection.
  • Adds a changelog fragment.

The race that originally caused the "Failed to load utxos" toast was fixed by c32193f and 27d4f0c; #616 is now about the error and retry handling, and could be retitled to match.

Out of Scope

  • SendCoinSelectionScreen.kt: going back from Send Confirm to Coin Selection still re-runs the load. A successful reload replaces a manual selection with all UTXOs, and a failed reload with coins already listed leaves the stale list without an error, because the error block only renders on an empty list. The same applies to a toggle made during the up-to-3s retry window. Deferred to a follow-up.
  • SendCoinSelectionViewModel.kt: ServiceError.NodeNotStarted and NodeRunTimeoutError are not retried.
  • LightningRepo: it already logs each failed listSpendableOutputs attempt, so a persistent failure logs once per attempt plus the final view model error. Repo logging is unchanged.
  • changelog.d/next/616.fixed.md: the fragment says coin selection "retries while the Lightning node is starting", but the retry covers the node not being set up or not running. A node that is still starting is already handled by the repo's wait. The wording can be tightened before release.
  • Pre-existing differences from the Figma frame (no ₿ prefix on amounts, no divider above the first row, no inline tag chips) are left as is.
  • Journey: journeys/coin-selection/manual-coin-selection-load.xml is committed here (85757ed), with its rows in journeys/README.md. It is Android-only for now; iOS has SendUtxoSelectionView but no load error or retry to assert on.
  • SendCoinSelectionScreen.kt: the CoinSelectionLoadError and CoinSelectionRetry test tags do not reach the accessibility tree, so android layout cannot see them and a journey cannot assert on them; they still work with Compose onNodeWithTag. This is pre-existing and not specific to this PR — SecondaryButton applies its modifier to a plain Box wrapper, so every SecondaryButton test tag in the app behaves the same way (verified against ResetToDefault on the Electrum Server screen). Left as is.
  • iOS parity: check whether bitkit-ios toasts or retries when its coin selection fails to load UTXOs.

Design

Send (Paste) (On-chain) › Coin selection. The loading and error/retry states have no design.

Preview

All media below was captured from the PR head build, on a regtest emulator with Coin Selection set to Manual. The loaded-state media is from 2026-09-17; the loading and error/retry media was re-captured on 2026-09-18, after the states were centered.

Figma Loaded Selection kept after activity sync
Figma coin selection Manual coin selection loads all UTXOs with no toast Deselected UTXO still off after a new activity synced

The full flow, from the wallet home through a bitcoin: deeplink to Coin Selection and a manual deselection:

v01-manual-coin-selection-flow.mp4

The five below were captured with a temporary local change that made listSpendableOutputs fail with NodeNotSetup for the first 6 calls. That change is not part of this PR.

Loading Load error Retry in progress Retry failed again Retry succeeds
Centered loading spinner while retrying Centered inline load error with Retry Retry in progress Retry failed again Retry succeeds and UTXOs load

The centered spinner, the centered load error, the Retry tap, the button's loading state, a second failure and the load that finally succeeds, on that same simulated-failure build:

retry-flow.mp4
Cold start: connecting gate Cold start: loaded once the node runs
Send sheet shows Connecting to network until the node runs Coin selection loads with no toast after a cold start

On a cold start the Send sheet shows "Connecting to network" until the node runs, so the retry path cannot be reached on a healthy wallet.

QA Notes

Manual Tests

  • 1. Settings → Advanced → Coin Selection → Manual → Send on-chain 10 000 sats → Amount → Continue → Coin Selection: every UTXO listed and switched on, no toast, no error block.
  • 2a. Coin Selection → deselect one UTXO → receive and confirm a new on-chain payment while the screen is open: the UTXO stays deselected and TOTAL SELECTED is unchanged.
    • 2b. the list is not reloaded when the new activity appears.
  • 3. Force-stop → open a bitcoin: deeplink with an amount → Continue once the node runs → Coin Selection: UTXOs load with no toast.
  • 4a. Build with listSpendableOutputs forced to return ServiceError.NodeNotSetup → Coin Selection: a spinner centered in the empty list area, then after 3 attempts "Bitkit could not load your coins. Please try again." with Retry, centered in that same area without moving; Continue disabled.
    • 4b. tap Retry: the message stays and the button shows a spinner.
    • 4c. Retry once the forced failure stops: UTXOs load and the error disappears.
  • 5. regression: Settings → Advanced → Coin Selection → Autopilot → Send on-chain: Coin Selection is skipped and Send Confirm opens.

Steps 1 and 2 were run on a regtest emulator and written up as a new journey, committed here at journeys/coin-selection/manual-coin-selection-load.xml:

manual-coin-selection-load.xml
diff --git a/journeys/coin-selection/manual-coin-selection-load.xml b/journeys/coin-selection/manual-coin-selection-load.xml
new file mode 100644
index 000000000..f8100d0ce
--- /dev/null
+++ b/journeys/coin-selection/manual-coin-selection-load.xml
@@ -0,0 +1,40 @@
+<journey name="manual coin selection loads once and keeps the selection">
+  <description>
+    Verifies the Send coin selection screen loads the wallet's UTXOs without a toast, and that a
+    manual selection survives an activity update while the screen is open (the load runs once per
+    amount and address; activity changes only refresh tag lookup).
+
+    The inline load error state (text "Bitkit could not load your coins. Please try again.", testTag
+    "CoinSelectionLoadError", with a Retry button, testTag "CoinSelectionRetry") is not reachable on
+    a healthy wallet: the Send amount screen shows "Connecting to network" (testTag
+    "sync_node_view") until the node runs, and fee calculation falls back to a fixed fee instead of
+    failing. It was checked with a temporary local change that failed 'listSpendableOutputs'.
+    SendCoinSelectionViewModelTest.kt covers it. Both tags sit on a SecondaryButton wrapper and on a
+    Column that carry no semantics of their own, so they do not reach `android layout`; assert the
+    error from its text or a screenshot.
+
+    Precondition: onboarded dev wallet with at least two confirmed Savings UTXOs, funded through the
+    lsp regtest deposit and mine (see ../README.md). Note a Savings address from Receive before
+    starting, and a valid regtest address to send to. Start on the wallet home screen. This journey
+    changes the Coin Selection setting to Manual and adds a 10 000 sat deposit; the last actions
+    restore the previous setting.
+  </description>
+  <actions>
+    <action>Tap the header menu (testTag "HeaderMenu") and then Settings (testTag "DrawerSettings")</action>
+    <action>Tap the Advanced tab (testTag "Tab-advanced") and then Coin Selection (testTag "CoinSelectPreference")</action>
+    <action>Note which method and autopilot mode are checked, then tap Manual (testTag "manual_button")</action>
+    <action>Go back to the wallet home screen</action>
+    <action>Run adb shell am start -a android.intent.action.VIEW -d "bitcoin:&lt;regtest address&gt;?amount=0.0001" to.bitkit.dev</action>
+    <action>Verify the amount field (testTag "SendNumberField") shows 10 000, waiting through "Connecting to network" (testTag "sync_node_view") if it is shown</action>
+    <action>Tap Continue (testTag "ContinueAmount")</action>
+    <action>Verify the coin selection screen (testTag "coin_selection_screen") lists one row per UTXO (testTags starting with "utxo_row_"), all toggled on, with TOTAL REQUIRED and TOTAL SELECTED above zero</action>
+    <action>Verify no error toast appeared and the load error text "Bitkit could not load your coins. Please try again." is not shown</action>
+    <action>Tap the smallest UTXO row once to deselect it</action>
+    <action>Verify TOTAL SELECTED decreased by that UTXO's amount</action>
+    <action>Run ./lsp POST /regtest/chain/deposit '{"address":"&lt;savings address&gt;","amountSat":10000}' and then ./lsp POST /regtest/chain/mine '{"count":1}'</action>
+    <action>Wait about 60 seconds without touching the screen, for the wallet to sync the new activity</action>
+    <action>Verify the coin selection screen still shows the same UTXO rows, the deselected row is still off, and TOTAL SELECTED is unchanged</action>
+    <action>Close the Send sheet without continuing</action>
+    <action>Open Settings, Advanced, Coin Selection again and restore the method and autopilot mode noted earlier</action>
+  </actions>
+</journey>

Automated Checks

  • Unit tests added in SendCoinSelectionViewModelTest.kt:
    • NodeNotSetup followed by success loads UTXOs.
    • NodeNotRunningError is retried.
    • NodeRunTimeoutError is not retried and sets the error.
    • A persistent NodeNotSetup sets the error after 3 attempts and sends no toast.
    • A non-transient list failure is not retried.
    • A fee calculation failure is not retried.
    • A successful retry clears the error.
    • The error stays visible while a retry runs.
    • Activity updates neither reset a manual selection nor reload.
  • Instrumented tests added in SendCoinSelectionContentTest.kt, which renders SendCoinSelectionContent (renamed from Content and no longer private, so the test can reach it, the same way SendAmountContentTest.kt reaches SendAmountContent):
    • Loading with no coins shows CoinSelectionLoading and neither error tag.
    • A load error with no coins shows CoinSelectionLoadError and CoinSelectionRetry and no spinner.
    • A retry in progress keeps the error and still shows no spinner.
    • Tapping CoinSelectionRetry calls onRetry.
    • A loaded list shows the UTXO row and neither the spinner nor the error.
    • A reload with coins already listed keeps the list instead of the spinner.
  • The centering is layout-only, so no test asserts it; PreviewLoading and PreviewLoadError in SendCoinSelectionScreen.kt cover both centered states.
  • Ran locally: just test file SendCoinSelectionViewModelTest (14 passed), just compile, just test (2608 passed, 0 failed) and just lint. SendCoinSelectionContentTest.kt was compiled only, through just build compileDevDebugAndroidTestKotlin; instrumented tests need a device and were not run here, they run in CI's ui-tests.yml.

jvsena42 and others added 2 commits September 16, 2026 18:14
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jvsena42 jvsena42 self-assigned this Sep 16, 2026
@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with no actionable correctness, security, or repository-rule violations identified.

Summary

This PR improves coin-selection loading and preserves manual selections across activity updates.

  • Adds bounded retries for transient node-state failures.
  • Introduces explicit loading and localized inline error states with manual retry.
  • Separates activity synchronization from UTXO loading.
  • Adds unit coverage for retry, error, and selection-preservation behavior.
  • Adds a changelog fragment.

Diagram

sequenceDiagram
    participant UI as Coin Selection Screen
    participant VM as Coin Selection ViewModel
    participant Repo as Lightning Repository
    UI->>VM: loadUtxos(amount, address)
    VM->>VM: Set loading state
    loop Up to 3 attempts for transient node errors
        VM->>Repo: listSpendableOutputs()
        Repo-->>VM: Outputs or failure
        opt Node not set up/running
            VM->>VM: Delay before retry
        end
    end
    alt Outputs loaded
        VM->>Repo: calculateTotalFee(...)
        VM-->>UI: UTXOs, totals, selection valid
    else Loading failed
        VM-->>UI: Inline error and Retry button
    end
    UI->>VM: Activity updates
    VM->>VM: Update tag lookup input without reloading UTXOs
Loading

Reviews (1) · Last reviewed commit: "fix: stop retrying node run timeout in c..."

@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Regtest APK

Built from e8b5bcb (run).

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

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Approved — no HIGH/MEDIUM findings at head 8e516f904. (Posted as a comment because GitHub does not allow approving your own PR.)

Checked: Traced the retry loop in SendCoinSelectionViewModel.listSpendableOutputsWithRetry (lines 99-108): exactly 3 listSpendableOutputs calls with 1s and 2s delays, the third result is returned as-is, and only NodeNotRunningError and ServiceError.NodeNotSetup trigger a retry, so the bound holds and NodeRunTimeoutError, LdkError and fee errors fall through on the first attempt. Cancellation is sound: loadUtxos cancels the previous job before launching a new one, every stale job is parked at a suspension point (executeWhenNodeRunning's withContext, delay, or calculateTotalFee's withContext) so it resumes with CancellationException, and runSuspendCatching rethrows it, so a cancelled attempt never runs the success or failure branch and cannot overwrite the newer job's state or clear isLoading; executeOperation and calculateTotalFee both rethrow CancellationException rather than folding it into a Result. On a first-load failure availableUtxos is empty and isSelectionValid stays false, so Continue is disabled while the inline error and Retry are shown; SecondaryButton disables itself while isLoading. The known limitation that a failed reload with coins already listed keeps the stale list, and that a selection can be made during the retry window, is stated in the PR body as out of scope and was not re-raised. Splitting the activity effect from the load effect removes the reload on activity change; tag loading via UtxoRow's LaunchedEffect(txid) behaves the same as before since items() had no key and re-loads did not re-run it. PR #1286 is this PR's base branch (fix/894-remove-coin-selection-auto), so there is no merge-semantic conflict, only a stacked dependency. The unit tests run on UnconfinedTestDispatcher with virtual time, so the times(2)/times(3) call counts and the mid-retry isLoading/loadError assertions exercise the new code rather than passing on master.

@jvsena42
jvsena42 added this pull request to stack #1313 September 18, 2026 12:29
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jvsena42
jvsena42 requested a review from ovitrif September 18, 2026 13:26
jvsena42 and others added 2 commits September 18, 2026 10:28
…to' into tmp-1288

# Conflicts:
#	journeys/README.md
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Approved — no HIGH/MEDIUM findings at head a0fd13669. (Posted as a comment because GitHub does not allow approving your own PR.)

Inline: LOW — Journey omits the Received Bitcoin sheet that the self-deposit opens over coin selection; LOW — Sibling journey manual-coin-selection.xml still describes the activity-triggered reload this PR removes

Checked: Round 3 (a0fd136, load-state centering + journeys): Reviewed the delta since 8e516f9; content merged in from #1286 was not reviewed. In SendCoinSelectionScreen.kt (lines 125-146) the loading and error items cannot show at the same time: the spinner needs loadError == null and the error needs loadError != null, so the two fillParentMaxSize() items never stack and push each other off screen. During a retry (isLoading with loadError set) only the error item renders, the message stays, and Retry shows its loading state. Neither item renders once availableUtxos is non-empty, so the UTXO rows are never covered by a full-height placeholder. fillParentMaxSize() sizes each item to the LazyColumn viewport, which is bounded by weight(1f). The totals and Continue sit outside the LazyColumn, and Continue is still gated on isSelectionValid (line 190), which the delta does not touch. The CoinSelectionLoading, CoinSelectionLoadError, CoinSelectionRetry, coin_selection_screen and utxo_row_ tags are all present at head, and every other tag the new journey names exists at head (HeaderMenu, DrawerSettings, Tab-advanced via CustomTabRowWithSpacing, CoinSelectPreference, manual_button, SendNumberField, sync_node_view, ContinueAmount). The error string matches wallet__selection_load_error. No bitkit-ios twin PR exists, and the journey is recorded as not ported in the cross-platform table.

Comment thread journeys/coin-selection/manual-coin-selection-load.xml Outdated
Comment thread journeys/README.md
jvsena42 and others added 2 commits September 18, 2026 11:04
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ovi-reviewer[bot]

This comment was marked as resolved.

ovi-reviewer[bot]

This comment was marked as resolved.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Approved — no blocking HIGH/MEDIUM findings at head 2d6d6053c. (Posted as a comment because GitHub does not allow approving your own PR.)

Checked: Follow-up on f7b31b3, 9fe4283, 378f6df and dd46737. Both journey LOWs are fixed: the load journey now closes the Received sheet with its OK button (new_transaction_sheet / ReceivedTransactionButton, both in NewTransactionSheet.kt) and warns against Back, and the stale "reloads on activity" note is gone from the manual coin selection journey. The only production change renames the private Content to a public SendCoinSelectionContent so SendCoinSelectionContentTest can drive it; the new tests pin the spinner-only, error-with-retry, retrying and list branches and the retry callback. The preview trailing-comma change follows the repo rule.

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve


Reaudit: diff 4 files.

Findings:
N/A

Audit:
Audited - no findings.

QA: emu-1 redroid container on Android 15, dev build 2d6d605

  1. passed: Manual mode listed all UTXOs selected, with no toast or load error.

    Test 1:
    Settings → Advanced → Coin Selection → Manual → Send on-chain 10 000 sats → Amount → Continue → Coin…
    1.mp4
  2. passed: The deselected UTXO and 150 000 sat total survived activity sync without reloading.

    Test 2:
    **2a.** Coin Selection → deselect one UTXO → receive and confirm a new on-chain payment while the screen is…
    2.mp4
  3. passed: A force-stop deeplink loaded all UTXOs with no toast or load error.

    Test 3:
    Force-stop → open a `bitcoin:` deeplink with an amount → Continue once the node runs → Coin Selection: UTXOs…
    3.mp4
  4. passed: Forced failure centered loading and error with Continue disabled; Autopilot still skipped Coin Selection.

    Test 5:
    `regression:` Settings → Advanced → Coin Selection → Autopilot → Send on-chain: Coin Selection is skipped and…
    5.mp4
  5. passed: Retry kept the error visible while the button spinner ran.

    Test 6:
    **4b.** tap Retry: the message stays and the button shows a spinner.
    6.mp4
  6. passed: Retry loaded all UTXOs and cleared the error after failures stopped.

    Test 7:
    **4c.** Retry once the forced failure stops: UTXOs load and the error disappears.
    7.mp4

Tip

Test 1 worth a journey:

  • Open Settings → Advanced → Coin Selection
  • Select Manual
  • Return to the wallet home screen
  • Open a bitcoin deeplink for 10 000 sats
  • Verify the amount and tap Continue
  • Verify every UTXO is listed and selected
  • Verify no toast or load error appears

Test 2 worth a journey:

  • Deselect the smallest UTXO
  • Verify the selected total decreases
  • Receive and confirm a new on-chain payment
  • Wait for wallet activity to sync without leaving the screen
  • Verify the same UTXO rows remain
  • Verify the deselected row and selected total remain unchanged

Test 3 worth a journey:

  • Force-stop the app
  • Open a bitcoin deeplink for 10 000 sats
  • Wait for the amount screen to become ready
  • Tap Continue
  • Verify the UTXOs load
  • Verify no toast or load error appears

Coverage:
Unit tests: 90% - SendCoinSelectionContentTest.kt covers six load, error, Retry and list branches; centering remains preview-only.
QA: 8 of 8 manual tests passed


Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)

ovitrif

This comment was marked as resolved.

Base automatically changed from fix/894-remove-coin-selection-auto to master September 18, 2026 20:30
ovi-reviewer[bot]

This comment was marked as resolved.

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve


Reaudit: diff 0 files.

Findings:
N/A

Audit:
Already done in comment.

QA: carried

  1. ⏭️ skipped: tested on ef39fa1, recording
  2. ⏭️ skipped: tested on ef39fa1, recording
  3. ⏭️ skipped: tested on ef39fa1, recording
  4. ⏭️ skipped: tested on 2d6d605, recording
  5. ⏭️ skipped: tested on ef39fa1, recording
  6. ⏭️ skipped: tested on 2d6d605, recording

Coverage:
QA: 6 of 6 manual tests passed


Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)

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.

Failed to load utxos in manual coin selection

2 participants