Skip to content

fix: enforce max send fee drains confirmed amount #1147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
coreyphillips wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix: enforce max send fee drains confirmed amount #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6780822
fix: verify max onchain send at selected speed
coreyphillips Aug 11, 2026
e00b553
fix: reuse send fee rates for drain check
coreyphillips Aug 11, 2026
96d17bd
fix: pin max onchain send to selected speed
coreyphillips Sep 2, 2026
f3f402e
chore: merge master into spar/issue-1144
coreyphillips Sep 3, 2026
b27f23d
fix: skip max recheck for hardware sends
coreyphillips Sep 3, 2026
bdf6963
fix: settle max send before onchain pay
coreyphillips Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions app/src/main/java/to/bitkit/repositories/LightningRepo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1514,6 +1514,21 @@ class LightningRepo @Inject constructor(
}
}

/** Max onchain amount sendable at [speed], i.e. the spendable balance minus the send-all mining fee */
suspend fun estimateMaxSendOnchain(
address: Address? = null,
speed: TransactionSpeed? = null,
feeRates: FeeRates? = null,
): Result<ULong> = withContext(bgDispatcher) {
runSuspendCatching {
val spendableSats = getBalancesAsync().getOrThrow().spendableOnchainBalanceSats
if (spendableSats == 0uL) return@runSuspendCatching 0uL

val fee = estimateSendAllFee(address = address, speed = speed, feeRates = feeRates).getOrThrow()
spendableSats.safe() - fee.safe()

@jvsena42 jvsena42 Sep 4, 2026

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.

🔵 Cover fee >= spendable and the coin-selection path

fee >= spendable is untested: spendableSats.safe() - fee.safe() saturates to 0, takeIf { it > 0uL } makes it look like "no estimate", and refreshMaxSendOnchain then falls back to amount == maxSendOnchainSats, which still flags a drain LDK will reject. Fail-closed but unverified - add a LightningRepoTest case for fee > spendable returning 0, and a send-flow case asserting the fallback does not set isMaxAmount when the estimate is unavailable.

}
}
Comment on lines +1517 to +1530

@jvsena42 jvsena42 Aug 19, 2026

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.

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 coreyphillips Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


suspend fun getFeeRateForSpeed(
speed: TransactionSpeed,
feeRates: FeeRates? = null,
Expand Down
70 changes: 61 additions & 9 deletions app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2082,6 +2082,7 @@ class AppViewModel @Inject constructor(
it.copy(
amount = amount,
isAmountInputValid = validateAmount(amount),
isMaxAmount = false,
confirmedWarnings = persistentListOf(),
)
}
Expand Down Expand Up @@ -2358,6 +2359,7 @@ class AppViewModel @Inject constructor(
_sendUiState.update {
it.copy(selectedUtxos = utxos.toImmutableList())
}
refreshMaxSendOnchain()
refreshOnchainFeeUi()
setSendEffect(SendEffect.NavigateToConfirm)
}
Expand Down Expand Up @@ -3240,6 +3242,7 @@ class AppViewModel @Inject constructor(
state.copy(
amount = 0u,
isAmountInputValid = false,
isMaxAmount = false,
)
}
}
Expand Down Expand Up @@ -3355,6 +3358,9 @@ class AppViewModel @Inject constructor(
return
}

// pay the amount & drain flag the refresh settled on, not the ones it is about to replace
onchainSendRefreshJob?.join()

val amount = _sendUiState.value.amount

val lnurl = _sendUiState.value.lnurl
Expand Down Expand Up @@ -3729,13 +3735,14 @@ class AppViewModel @Inject constructor(
amount: ULong,
tags: List<String> = emptyList(),
): Result<Txid> {
val state = _sendUiState.value
return lightningRepo.sendOnChain(
address = address,
sats = amount,
speed = _sendUiState.value.speed,
utxosToSpend = _sendUiState.value.selectedUtxos,
isMaxAmount = _sendUiState.value.payMethod == SendMethod.ONCHAIN &&
amount == walletRepo.balanceState.value.maxSendOnchainSats,
speed = state.speed,
utxosToSpend = state.selectedUtxos,
feeRates = state.feeRates,
isMaxAmount = state.payMethod == SendMethod.ONCHAIN && state.isMaxAmount,
tags = tags,
)
}
Expand Down Expand Up @@ -3787,19 +3794,21 @@ class AppViewModel @Inject constructor(
}
}

/** Reselect utxos for current amount & speed then refresh fees using updated utxos */
/** Recheck the max sendable, reselect utxos for current amount & speed, then refresh fees using updated utxos */
private fun refreshOnchainSendIfNeeded(): Job? {
val currentState = _sendUiState.value
if (currentState.payMethod != SendMethod.ONCHAIN ||
currentState.amount == 0uL ||
currentState.address.isEmpty()
val state = _sendUiState.value
if (state.payMethod != SendMethod.ONCHAIN ||
state.amount == 0uL ||
state.address.isEmpty()
) {
return null
}

updateOnchainFeeUi { it.copy(isLoading = true) }
onchainSendRefreshJob?.cancel()
val job = viewModelScope.launch(bgDispatcher, start = CoroutineStart.LAZY) {
refreshMaxSendOnchain()

@ovitrif ovitrif Sep 3, 2026

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.

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 coreyphillips Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@jvsena42 jvsena42 Sep 4, 2026

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.

🟡 Gate the swipe on the refresh instead of re-deciding the amount after consent

The join() fixes the ordering but not the consent problem, and it lands too late for the Paykit check. validateIncomingPaymentRequest -> hasMismatchedIncomingPaymentRequest runs at :3344 against the pre-refresh amount using strict acceptsPaymentAmount equality; the join at :3362 then lowers amount and sets isMaxAmount, so a refresh still in flight at swipe time gets the mismatch guard to pass on the requested amount and then underpays it - and completeOnchainPaymentProofInBackground posts a proof for the underpaying txid. The swipe is reachable while the refresh runs because SendConfirmScreen:400 only consults onchainFeeUi.isLoading via isHardwareFeeLoading, which is gated on hardwareWalletId != null. Disable SwipeToConfirm while onchainSendRefreshJob is active (or drop the hardwareWalletId != null qualifier on isHardwareFeeLoading), keep join() only as a backstop, and never change amount after the swipe.

val currentState = _sendUiState.value
// preselect utxos for deterministic fee estimation
if (
currentState.hardwareWalletId == null &&
Expand Down Expand Up @@ -3827,6 +3836,48 @@ class AppViewModel @Inject constructor(
return job
}

/**
* Flags the send as a drain when the amount reaches the max sendable to this recipient at the selected speed,

@jvsena42 jvsena42 Sep 4, 2026

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.

⚪ Drop KDoc/inline comments on private functions

AGENTS.md: 'NEVER add code comments to private functions'. New KDoc on private refreshMaxSendOnchain plus the inline comments at 3361 and 3855. Move the rationale into the function/log names or the PR description.

* lowering the amount to that max so the confirmed figure matches what the drain delivers.
*/
private suspend fun refreshMaxSendOnchain() {
val state = _sendUiState.value
if (state.payMethod != SendMethod.ONCHAIN || state.hardwareWalletId != null) return

@ovitrif ovitrif Sep 3, 2026

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.

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 coreyphillips Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

if (state.amount == 0uL || state.address.isEmpty()) return

val max = lightningRepo.estimateMaxSendOnchain(
address = state.address,
speed = state.speed,
feeRates = state.feeRates,
).getOrNull()?.takeIf { it > 0uL }

if (max == null) {
// without an estimate the cached max is the only max-send signal left
_sendUiState.update {
if (it.divergedFrom(state)) return@update it
it.copy(isMaxAmount = it.amount == walletRepo.balanceState.value.maxSendOnchainSats)
}
return
}

val isMaxAmount = state.amount >= max
if (isMaxAmount && state.amount != max) {
Logger.info(
"Lowering amount '${state.amount}' to max '$max' at speed '${state.speed.serialized()}'",
context = TAG,
)
}
_sendUiState.update {
if (it.divergedFrom(state)) return@update it
it.copy(amount = if (isMaxAmount) max else it.amount, isMaxAmount = isMaxAmount)

@jvsena42 jvsena42 Sep 4, 2026

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.

🟡 A drain sends the balance at send time, not the confirmed max

Setting amount = max makes the confirm screen promise a figure the drain does not honour. isMaxAmount routes to LightningService.send -> node.onchainPayment().sendAllToAddress(...), which ignores sats and sweeps whatever is spendable when it runs - and LightningRepo.sendOnChain calls ensureSyncedBeforeSend() immediately before it, so a deposit that confirms between the estimate and the swipe is swept to the recipient too. Pre-existing for an explicit Max send, but this PR widens the trigger from amount == cachedMax to any amount at or above the live speed-specific max, so ordinary sends now take the sweep path. Re-check the max inside sendOnchain after ensureSyncedBeforeSend and abort (or re-confirm) when it moved above the confirmed amount, rather than letting sendAllToAddress define the amount.

}
}

private fun SendUiState.divergedFrom(snapshot: SendUiState) = amount != snapshot.amount ||
address != snapshot.address ||
speed != snapshot.speed ||
hardwareWalletId != snapshot.hardwareWalletId

private suspend fun refreshOnchainFeeUi() = withContext(bgDispatcher) {
val currentState = _sendUiState.value
updateOnchainFeeUi { it.copy(isLoading = true) }
Expand Down Expand Up @@ -4809,6 +4860,7 @@ data class SendUiState(
val isAddressInputValid: Boolean = false,
val amount: ULong = 0u,
val isAmountInputValid: Boolean = false,
val isMaxAmount: Boolean = false,
val isUnified: Boolean = false,
val canSwitchWallet: Boolean = false,
val canSwitchFundingSource: Boolean = false,
Expand Down
49 changes: 49 additions & 0 deletions app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,55 @@ class LightningRepoTest : BaseUnitTest() {
assertEquals(80_000uL, result)
}

@Test
fun `estimateMaxSendOnchain should subtract the send-all fee for the given speed`() = test {
startNodeForTesting()
whenever(lightningService.balances).thenReturn(
BalanceDetails(
totalOnchainBalanceSats = 100_000uL,
spendableOnchainBalanceSats = 80_000uL,
totalAnchorChannelsReserveSats = 0uL,
totalLightningBalanceSats = 0uL,
lightningBalances = emptyList(),
pendingBalancesFromChannelClosures = emptyList(),
),
)
whenever { lightningService.estimateSendAllFee(any(), any()) }.thenReturn(2_000uL)

val result = sut.estimateMaxSendOnchain(
address = "bcrt1qtest",
speed = TransactionSpeed.Fast,
feeRates = FeeRates(fast = 20u, mid = 10u, slow = 5u),
)

assertEquals(78_000uL, result.getOrNull())
verify(lightningService).estimateSendAllFee(address = "bcrt1qtest", satsPerVByte = 20uL)
}

@Test
fun `estimateMaxSendOnchain should return zero when nothing is spendable`() = test {
startNodeForTesting()
whenever(lightningService.balances).thenReturn(
BalanceDetails(
totalOnchainBalanceSats = 100_000uL,
spendableOnchainBalanceSats = 0uL,
totalAnchorChannelsReserveSats = 0uL,
totalLightningBalanceSats = 0uL,
lightningBalances = emptyList(),
pendingBalancesFromChannelClosures = emptyList(),
),
)

val result = sut.estimateMaxSendOnchain(
address = "bcrt1qtest",
speed = TransactionSpeed.Fast,
feeRates = FeeRates(fast = 20u, mid = 10u, slow = 5u),
)

assertEquals(0uL, result.getOrNull())
verify(lightningService, never()).estimateSendAllFee(any(), any())
}

@Test
fun `updateAddressType should fail when already in progress`() = test {
startNodeForTesting()
Expand Down
Loading
Loading