Send-max computed at the default fee speed, but the drain happens at the selected speed (#1144)

[coreyphillips]

Closes #1144

.../java/to/bitkit/repositories/LightningRepo.kt | 15 +++
.../main/java/to/bitkit/viewmodels/AppViewModel.kt | 27 ++++-
.../to/bitkit/repositories/LightningRepoTest.kt | 49 ++++++++
.../bitkit/viewmodels/AppViewModelSendFlowTest.kt | 132 +++++++++++++++++++++
changelog.d/next/1144.fixed.md | 1 +
5 files changed, 223 insertions(+), 1 deletion(-)

[greptile-apps]

Greptile Summary

The PR prevents an on-chain max send from draining at a fee speed different from the speed used to calculate the confirmed amount.

• Adds a repository API to estimate the maximum sendable amount for a specified address, fee speed, and fee-rate snapshot.
• Rechecks the maximum at the selected speed before enabling send-all behavior.
• Falls back to sending the confirmed amount exactly when the estimates differ or recomputation fails.
• Adds repository and send-flow tests for matching, mismatched, failed, and zero-balance estimates.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified.

The selected-speed estimate is passed consistently into the drain decision, subtraction safely saturates at zero, and mismatched or unavailable estimates avoid sending more than the amount the user confirmed.

Important Files Changed

Filename	Overview
app/src/main/java/to/bitkit/repositories/LightningRepo.kt	Adds selected-speed maximum estimation using the spendable balance and existing saturating fee subtraction.
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt	Revalidates cached max amounts against the selected fee speed before enabling on-chain drain mode.
app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt	Covers selected-speed fee subtraction and the zero-spendable-balance shortcut.
app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt	Covers drain eligibility when estimates match and exact-send fallback on mismatch or estimation failure.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User confirms cached max amount] --> B{Amount equals cached max?}
    B -- No --> E[Send exact amount]
    B -- Yes --> C[Estimate max at selected fee speed]
    C --> D{Estimate succeeds and matches?}
    D -- Yes --> F[Send using drain mode]
    D -- No --> E
    E --> G[LightningRepo sendOnChain]
    F --> G

Loading

_{Reviews (1): Last reviewed commit: "fix: verify max onchain send at selected..." | Re-trigger Greptile}

[coreyphillips]

Review by codex (round 1): The approach is sound, but the actual drain can still use a different fee-rate snapshot than the new max validation. Existing test results show 1,919 tests passing, including both affected suites. A fresh targeted run was blocked by the sandbox denying the Gradle cache lock.

• [blocking] Use the same fee rates for validation and sending (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:2882)
  At line 2901, shouldDrainOnchain validates the amount using state.feeRates, but the sendOnChain call at line 2882 does not pass those rates. It therefore receives feeRates = null, and LightningRepo.kt:1333 fetches a fresh fee table. If rates changed, the guard can approve draining at one rate and the transaction can drain at another, reproducing the confirmed-amount mismatch. I confirmed this through the production call path to BlocktankService.getFees, which refreshes rates. Snapshot the send state and pass the same speed and feeRates to both operations, then assert this in the send-flow test.

[coreyphillips]

Applied the review's blocking fix: sendOnchain now snapshots the send state once and passes the same speed and feeRates to both estimateMaxSendOnchain (the drain guard) and sendOnChain, so the guard and the transaction can no longer be built from different fee tables. Updated the send-flow test to use a non-null FeeRates and assert both calls receive it. Targeted unit tests and detekt pass; committed as e00b553, not pushed.

• Fixed: Use the same fee rates for validation and sending

[coreyphillips]

Machine state for resuming this PR. Safe to ignore.

{
  "version": 1,
  "round": 1,
  "next_actor": "claude",
  "status": "pending",
  "ledger": {},
  "filed": []
}

[coreyphillips]

Review by claude (round 2): The PR mirrors the iOS approach: recompute the max sendable amount at confirm time with the selected speed/fee rates and fall back to an exact-amount send when the cached max no longer matches. Direction is right, plumbing state.feeRates into both the drain check and sendOnChain is a good touch, and the new unit tests pass (./gradlew testDevDebugUnitTest --tests "*AppViewModelSendFlowTest*" --tests "*LightningRepoTest*" -> BUILD SUCCESSFUL). But the equality check compares two estimates that are not computed against the same address (and sometimes not against a real estimate at all), so it degrades to an exact-amount send in cases where the speed never changed, and the degraded send is arithmetically guaranteed to be unfundable.

• [blocking] Drain check compares estimates made against different addresses, so ordinary max sends degrade to a send that cannot be funded (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  The cached max is DeriveBalanceStateUseCase.getMaxSendAmount (line 214-228), which calls lightningRepo.estimateSendAllFee(speed = speed) with address = null. LightningRepo.estimateSendAllFee (line 1479) then resolves address ?: cacheStore.data.first().onchainAddress, i.e. the wallet's OWN next receive address. The new shouldDrainOnchain (AppViewModel.kt:2899-2903) recomputes via estimateMaxSendOnchain(address = address, ...) with the DESTINATION address. LightningService.estimateSendAllFee (line 1210-1222) forwards the address to node.onchainPayment().calculateSendAllFee(address, retainReserves, feeRate), whose result depends on the output script length. A P2WPKH output is 31 bytes vs 43 for P2TR, so sending max to a bc1p address from a P2WPKH wallet yields feeDest = feeOwn + ~12 vB * rate even when the selected speed IS the default speed. The equality amount != maxAtSelectedSpeed then holds, drain is disabled, and the code sends amount == cachedMax non-max. That send needs cachedMax + feeDest (plus a change output, ~31 more vB) while only spendable == cachedMax + feeOwn exists, so it is short by construction and LDK must fail with insufficient funds. Same failure, much larger, whenever the cached max fell back to Defaults.fallbackFeePercent (Env.kt:251 = 0.1, i.e. 10% of spendable) because the estimate errored: the recomputed max will never match, so every max send degrades and fails. Net effect: a plain max send that worked on master now errors for common address-type combinations. Confirmed by reading the exact call sites above; the new AppViewModel test max onchain send falls back to exact amount when selected speed changes the max already asserts precisely this degrade behavior (isMaxAmount=false with sats = cachedMax). I did not run against a real LDK node. Suggested direction: recompute BOTH sides against the same destination address (compare max-at-selected-speed vs max-at-the-speed the cached value used), or treat amount >= maxAtSelectedSpeed as drain-eligible and only degrade when the selected rate is genuinely cheaper, rather than exact equality against a differently-parameterized estimate.
• [non-blocking] Degrade path has no user-facing handling when the amount exceeds the max at the selected speed (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  When the selected speed really is more expensive than the default (the exact scenario in issue Send-max computed at the default fee speed, but the drain happens at the selected speed #1144), shouldDrainOnchain correctly refuses to drain, but nothing clamps the amount or tells the user. The send then fails deep in LDK and surfaces as a generic send error with no explanation that the faster fee no longer fits the confirmed amount. Recomputing the max on setTransactionSpeed (AppViewModel.kt:1640-1668) and updating/invalidating the amount on the confirm screen, or showing a targeted error, would make the fix visible rather than just non-destructive.
• [non-blocking] [out of scope] estimateMaxSendOnchain ignores selectedUtxos, so the check is meaningless under coin control (app/src/main/java/to/bitkit/repositories/LightningRepo.kt)
  estimateMaxSendOnchain derives from getBalancesAsync().spendableOnchainBalanceSats, i.e. the full spendable balance, while the send may be restricted to state.selectedUtxos. With manual coin selection the recomputed max describes a different UTXO set than the send, so the equality result is effectively arbitrary (it will almost always mismatch and degrade). The pre-existing cached max has the same limitation, so this is not a regression, but the new confirm-time check inherits it.
• [non-blocking] Extra LDK round trip on every max-send confirm (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  shouldDrainOnchain runs estimateMaxSendOnchain (balances + calculateSendAllFee on the LDK service queue) synchronously inside the confirm path, after the user has already swiped to pay. It is only paid on the max-amount path, so the cost is bounded, but it adds latency at the most sensitive moment; caching the fee rate that produced the displayed max (the issue's alternative suggestion) would avoid the extra call entirely.
• [nit] Log wording (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  Logger.info("Sending exact amount '$amount' instead of draining, ...") is clear, but the Logger.warn above it logs the failure that estimateSendAllFee/getFeeRateForSpeed already logged internally (LightningRepo.kt:1507-1511), which the repo guidance discourages as duplicate error logging.

[coreyphillips]

Review by claude (round 2): The PR stops the wallet from draining at a fee speed the confirmed amount didn't account for by recomputing the max at the selected speed/rates and only setting isMaxAmount when it still matches exactly, and it now threads state.feeRates into sendOnChain so the check and the send use the same rates. The repo helper is clean and the new unit tests pass (verified: ./gradlew :app:testDevDebugUnitTest --tests '*AppViewModelSendFlowTest' --tests '*LightningRepoTest' -> BUILD SUCCESSFUL). The problem is the fallback: when the recomputed max is lower than the confirmed amount, the code sends the stale exact amount, which cannot fund itself, so the send fails instead of sending the wrong amount. That converts the issue's exact scenario (max send, then pick Fast) from 'sends less than displayed' into 'send always errors', and the address asymmetry makes it fire even without a speed change. The guard direction is right; the fallback needs rethinking (recompute/refresh the displayed amount, or drain when the recomputed max is <= the confirmed amount).

• [blocking] Exact-amount fallback is arithmetically guaranteed to fail when the selected speed costs more (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  shouldDrainOnchain returns false when maxAtSelectedSpeed != amount, and sendOnChain then takes the non-max path: LightningService.send -> node.onchainPayment().sendToAddress(amountSats = sats, ...) (LightningService.kt:940), where the mining fee is paid on top of amountSats. The confirmed amount is the cached max, i.e. spendable - feeAtDefaultSpeed (DeriveBalanceStateUseCase.getMaxSendAmount, lines 214-228). Funding that send at the selected speed requires amount + feeAtSelectedSpeed <= spendable, i.e. feeAtSelectedSpeed <= feeAtDefaultSpeed. Whenever the user picks a faster-than-default speed (the exact case in Send-max computed at the default fee speed, but the drain happens at the selected speed #1144), that is false and ldk-node returns InsufficientFunds. So the flow the issue describes now reliably errors at swipe-to-pay, and nothing in the UI lowers the amount for the user (setTransactionSpeed at AppViewModel.kt:1641 leaves state.amount untouched; validateAmount at :1794 still checks against the cached max). Confirmed by reading the code paths and the arithmetic; I did not run against a live node, so the only assumption is that sendToAddress errors rather than shaving the output, which is why sendAllToAddress exists as a separate API. Suggested direction: if maxAtSelectedSpeed < amount, either drain at the selected speed after updating the confirmed amount (and re-confirming), or recompute state.amount when the speed changes so the confirmed figure always matches the speed it will be sent at.
• [blocking] Drain check compares against a max computed for a different address, so it mismatches even at the default speed (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  The cached maxSendOnchainSats comes from lightningRepo.estimateSendAllFee(speed = defaultSpeed) with address = null (DeriveBalanceStateUseCase.kt:221), which LightningRepo.estimateSendAllFee resolves to cacheStore.data.first().onchainAddress, the wallet's own receive address (LightningRepo.kt:1479). shouldDrainOnchain instead passes the destination address (AppViewModel.kt:2900). node.onchainPayment().calculateSendAllFee(address = ...) builds the tx against that address, so the send-all fee differs with the output script type (e.g. P2WPKH 31 vB vs P2TR 43 vB, ~12 vB, times the rate). Result: for a max send to an address whose type differs from the wallet's own, the strict equality check fails even when the user never touched the speed, drain is disabled, and the send then hits the same insufficient-funds path as the finding above. Same class of false mismatch arises from the rate source asymmetry (cached max resolves rates via coreService.blocktank.getFees(); the check uses the state.feeRates snapshot taken in resetSendState) and from any balance change between balance derivation and confirm. Exact equality across two differently-parameterised estimates is too brittle to gate a drain on.
• [non-blocking] Extra LDK round-trip added to the swipe-to-pay path (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  shouldDrainOnchain runs getBalancesAsync plus calculateSendAllFee synchronously between the user's confirmation and the send, on every max on-chain send. It is bounded work, but it lands on the most latency-sensitive step of the flow, after the swipe. If the guard survives in some form, consider computing it during refreshOnchainSendIfNeeded (where the fee estimates are already refreshed on speed change) and caching the result in SendUiState, so confirm-time work stays a comparison.
• [non-blocking] [out of scope] Confirm screen never refreshes the max amount when the speed changes (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  setTransactionSpeed (AppViewModel.kt:1641) refreshes fee estimates and UTXO selection but leaves state.amount at the value computed from the default-speed max, and validateAmount (:1794) only ever compares against walletRepo.balanceState.value.maxSendOnchainSats, which is derived at the default speed. This is the underlying cause of Send-max computed at the default fee speed, but the drain happens at the selected speed #1144 and predates the PR; fixing it at this layer (recompute the max for the selected speed and destination when the speed changes) would make the confirm-time guard largely unnecessary. Filing as follow-up rather than a change request on this diff.
• [nit] estimateMaxSendOnchain drops the percentage fallback used elsewhere (app/src/main/java/to/bitkit/repositories/LightningRepo.kt)
  DeriveBalanceStateUseCase.getMaxSendAmount falls back to FALLBACK_FEE_PERCENT of the balance when the send-all fee estimate fails; the new estimateMaxSendOnchain propagates the failure instead. That is a defensible choice for a drain guard (the caller treats failure as 'do not drain'), but the two now disagree about what the max is under estimator failure, which is worth a comment or a shared helper if the method gets reused.