fix: enforce max send fee drains confirmed amount

[coreyphillips]

Fixes #1144

This PR keeps max on-chain sends in step with the fee speed and recipient they are sent at, so the amount shown on the confirm screen is the amount that goes out.

Description

• Recomputes the max sendable amount for the actual recipient at the selected fee speed whenever the on-chain send is refreshed (continue from amount entry, speed change, coin selection, funding source switch).
• When the entered amount reaches that max, lowers the amount to it and flags the send as a max send, so the confirmed figure matches what the send delivers. Below the max the send stays an exact-amount send.
• Passes the same fee rate snapshot to both the max estimate and the send, so the two can no longer be built from different fee tables.
• Falls back to the cached wallet max only when the estimate is unavailable, and leaves hardware wallet sends untouched since their available amount is tracked separately.
• Adds a repository helper that derives the max from the spendable balance minus the send-all fee for a given address, speed and fee rates.
• Merges latest master, folding the max recheck into the cancellable on-chain refresh job introduced there.

Preview

N/A, the only visible change is the confirm amount updating when the fee speed changes.

QA Notes

Manual Tests

• 1a. Send → Amount → tap Max → Confirm → switch speed to Fast: displayed amount lowers to the max at Fast and the send completes with no on-chain balance left behind.
  • 1b. Same flow → switch speed to Slow: displayed amount stays as confirmed and exactly that amount is delivered.
• 2. Send → Amount → tap Max → Confirm with a Taproot recipient (bcrt1p...) at the default speed: send completes with no on-chain balance left behind.
• 3. regression: Send → Amount below max → Confirm → change speed: amount is unchanged and exactly that amount is delivered.
• 4. regression: Hardware wallet funding source → Amount → tap Max → Confirm → change speed: amount follows the hardware wallet's available balance.
• 5. regression: Send → Amount → Coin Selection → Continue → Confirm: fee and amount still refresh.

Automated Checks

• Unit tests added: cover a speed change lowering a max send to the max at the new speed, exact sends below the max, and the cached-max fallback when the estimate fails in AppViewModelSendFlowTest.kt.
• Unit tests added: cover the send-all fee subtraction and the zero spendable balance shortcut in LightningRepoTest.kt.
• Local verification: just compile, just test, just lint.

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

[jvsena42]

Reviewed the drain/max-send change and tested it on device (dev build, regtest, funded wallet). One of the findings is a reproduced regression — details inline.

[jvsena42]

Address-type mismatch makes this comparison apples-to-oranges — max-send is broken for P2TR/P2SH/P2PKH recipients.

This recomputes the max using the recipient address, but the cached value it gets compared against on L2897 (walletRepo.balanceState.value.maxSendOnchainSats) comes from DeriveBalanceStateUseCase.getMaxSendAmount, which passes address = null and therefore falls back to cacheStore.onchainAddress — our own receive address. calculateSendAllFee depends on the output script size, so the two values differ for any recipient whose script type differs from selectedAddressType, with no speed change at all.

Reproduced on regtest (balance 1,000,000 sats, default speed):

• P2TR recipient → Sending exact amount '999890' instead of draining, max at speed 'Medium' is '999878' → isMaxAmount = false → AppError='The available funds are insufficient to cover the transaction' and an "Error Sending" toast.
• P2WPKH recipient, same wallet and speed → isMaxAmount = true, drain succeeds (txid 498ced33…).

So MAX now fails for P2TR/P2SH/P2PKH recipients, and symmetrically for P2WPKH recipients once the user switches selectedAddressType to Taproot. iOS avoids this because its MAX amount is itself derived from calculateMaxSendableAmount(address: recipient, rate: selected).

Suggest comparing like with like: either recompute with the same address the cached max used, or derive the MAX button amount from the recipient + selected rate.

[coreyphillips]

Good catch, fixed. Dropped the equality check entirely. The max is now recomputed for the recipient address at the selected speed in refreshMaxSendOnchain, so the cached own-address value never gates the drain. It only remains as a fallback when the estimate is unavailable.

[jvsena42]

The intended "fall back to an exact-amount send" path can never succeed.

Whenever maxAtSelectedSpeed < amount, the headroom left over is exactly the 1-output send-all fee at the default rate, while an exact-amount send needs a 2-output tx (recipient + change) at the selected — higher — rate. That is always more, so returning false here doesn't degrade to an exact-amount send, it degrades to an insufficient-funds error. This is what the P2TR repro above ends in.

Consider clamping amount to maxAtSelectedSpeed (and updating the displayed amount/fee when the speed changes, as iOS does), or surfacing an actionable "reduce amount" error instead of the generic LDK failure.

[coreyphillips]

Agreed, that path was unfundable by construction. There is no exact-amount fallback now: when the amount reaches the max at the selected speed we lower the amount to that max and drain, so the confirmed figure matches what is delivered.

[jvsena42]

TransactionSpeed has no toString, so this logs Failed to recompute max send amount for speed 'to.bitkit.models.TransactionSpeed$Medium@987bb0b' on device. Use the existing state.speed.serialized() to keep the reference traceable.

[coreyphillips]

Fixed, the remaining log uses state.speed.serialized().

[jvsena42]

Same here — '${state.speed}' renders as to.bitkit.models.TransactionSpeed$Medium@987bb0b. Use state.speed.serialized().

[coreyphillips]

Fixed, that log is gone and the one left uses serialized().

[jvsena42]

This duplicates DeriveBalanceStateUseCase.getMaxSendAmount with different inputs, and the drain decision is exact equality between the two.

getMaxSendAmount (~L214) computes the same quantity but fetches fee rates fresh via blocktank.getFees() and applies the 1%-of-balance fallback; this one takes the send-sheet snapshot state.feeRates (captured in resetSendState) and has no fallback. Since shouldDrainOnchain requires amount == maxAtSelectedSpeed exactly, any blocktank rate refresh between the last balance derivation and confirm silently disables drain — same failure mode as the AppViewModel comments.

Having the use case delegate to this new repo method (same address, same rates) would make the two agree by construction rather than by coincidence.

[coreyphillips]

The drain decision no longer compares the two, so they cannot disagree. estimateMaxSendOnchain is used on its own for the send-flow max; getMaxSendAmount keeps its fallback for the balance display.

[ovitrif]

@coreyphillips can you look into this (addressing comments), looks quite important on 1st sight, would like to merge and ship it in upcoming 2.5.0 🙏🏻

[coreyphillips]

@jvsena42 thanks for the device repro, both blockers were real. Pushed 96d17bd.

Rather than gating the drain on equality with the cached max, the send flow now recomputes the max for the actual recipient at the selected speed (refreshMaxSendOnchain, run from refreshOnchainSendIfNeeded). When the amount reaches that max we lower the amount to it and flag the send as a drain, so:

• no more apples-to-oranges comparison, the own-address cached max is only a fallback when the estimate fails
• no unfundable exact-amount fallback, the confirmed amount always matches what the drain delivers
• the confirm screen amount updates on speed change, like iOS
• the extra LDK call moved off the swipe-to-pay path into the existing refresh

Logs now use speed.serialized() and the duplicate warn is gone.

@ovitrif should be good for 2.5.0 once re-checked.

QA notes: max send to a P2TR recipient at default speed should drain, and switching speed on confirm should update the displayed amount. Covered in AppViewModelSendFlowTest.kt.

[ovitrif]

@coreyphillips can you please add PR description details matching our usual format? Ensuring sensitive framing, ofc 🙏🏻

While at it please also update with latest master to resolve conflicts flagged by GitHub.

[coreyphillips]

@ovitrif merged latest master, description updated, ready for re-review.

[ovitrif]

Pay is not sequenced with refreshMaxSendOnchain, so switching a max send to Fast can still drain at the new fee while the confirm amount is the old one.

The hardware early-return in that helper also has no send-flow test that would fail if the guard disappeared.

[ovitrif]

The drain decision now lives in refreshMaxSendOnchain, which this job runs in the background. sendOnchain reads state.isMaxAmount without waiting for that job, and the confirm swipe stays enabled while onchainFeeUi.isLoading is true for software wallets. After Max → Confirm → Fast, a swipe that lands before the refresh finishes still calls sendAllToAddress at the new rate while the header still shows the previous amount, which is the mismatch this change is meant to close. The same write can also restore amount after onAmountChange has already cleared isMaxAmount.

Could we join onchainSendRefreshJob before sending, or disable swipe while the on-chain fee refresh is in flight, so the confirmed amount and drain flag are the ones the user actually paid?

[coreyphillips]

Fixed in bdf6963. proceedWithPayment now joins onchainSendRefreshJob before reading the amount, so the swipe pays the amount and drain flag the refresh settled on. Also guarded the refresh write itself: it no longer applies if amount, address, speed or funding source changed while the estimate was in flight, so it cannot restore an amount onAmountChange already cleared.

Covered by max onchain send waits for the in-flight max refresh before paying and max refresh does not restore the amount after it was edited in AppViewModelSendFlowTest.kt; both fail without the fixes.

[ovitrif]

refreshMaxSendOnchain returns immediately when hardwareWalletId is set, which is what stops a hardware max send from being rewritten using the software wallet's spendable balance. None of the new send-flow tests set a hardware funding source, so removing that guard would not fail the suite.

Could we add a case that a hardware max send keeps hardwareAvailableSats and never calls estimateMaxSendOnchain?

[coreyphillips]

Added hardware max send keeps its available amount and skips the onchain max estimate in AppViewModelSendFlowTest.kt. It switches speed on a hardware max send and asserts the amount tracks hardwareAvailableSats with no estimateMaxSendOnchain call. Verified it fails if the guard is removed.

[coreyphillips]

@ovitrif both addressed in bdf6963.

• pay now joins onchainSendRefreshJob before reading the amount, so a swipe landing mid-refresh pays the settled amount and drain flag
• the refresh write is dropped if amount, address, speed or funding source changed while the estimate was in flight
• added the hardware max send case: amount stays on hardwareAvailableSats and estimateMaxSendOnchain is never called

Three new tests in AppViewModelSendFlowTest.kt, each verified to fail without its fix. compileDevDebugKotlin, testDevDebugUnitTest and detekt pass.