Skip to content

Commit

Permalink
Fix update_fee reserve requirement mismatch (#551)
Browse files Browse the repository at this point in the history
The splicing update removed the `channelReserve` field from `LocalParams`
and always uses a value of 1% of the channel capacity. But that is incorrect
for channels that were created before that update and haven't been migrated.

For such channels, we will use an incorrect channel reserve which can end
up force-closing channels when receiving otherwise valid `update_fee`
messages. We temporarily ignore the reserve in this computation and will
re-enable it once all users have been migrated to use splicing.

We also fix a mismatch between the local and remote dust limit used when
computing commit tx fees.
  • Loading branch information
t-bast authored Oct 16, 2023
1 parent 8c8a38c commit 6e4bc39
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
13 changes: 10 additions & 3 deletions src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ data class Commitment(
val incomingHtlcs = reduced.htlcs.incomings()

// note that the initiator pays the fee, so if sender != initiator, both sides will have to afford this payment
val fees = commitTxFee(params.remoteParams.dustLimit, reduced)
val fees = commitTxFee(params.localParams.dustLimit, reduced)
// NB: we don't enforce the initiatorFeeReserve (see sendAdd) because it would confuse a remote initiator that doesn't have this mitigation in place
// We could enforce it once we're confident a large portion of the network implements it.
val missingForSender = reduced.toRemote - remoteChannelReserve(params).toMilliSatoshi() - (if (params.localParams.isInitiator) 0.sat else fees).toMilliSatoshi()
Expand Down Expand Up @@ -388,8 +388,15 @@ data class Commitment(
// It is easier to do it here because under certain (race) conditions spec allows a lower-than-normal fee to be paid,
// and it would be tricky to check if the conditions are met at signing
// (it also means that we need to check the fee of the initial commitment tx somewhere)
val fees = commitTxFee(params.remoteParams.dustLimit, reduced)
val missing = reduced.toRemote.truncateToSatoshi() - remoteChannelReserve(params) - fees
val fees = commitTxFee(params.localParams.dustLimit, reduced)
// TODO:
// When migrating to the dual-funded model, we removed the explicit channel reserve from LocalParams.
// For channels that were created before the splicing update, this can result in a mismatch where we think
// the channel reserve is bigger than what is actually is, and incorrectly reject the remote update_fee.
// We temporarily ignore the channel reserve to avoid unnecessary force-close.
// We should restore the correct calculation that takes the reserve into account once all users have migrated.
// val missing = reduced.toRemote.truncateToSatoshi() - remoteChannelReserve(params) - fees
val missing = reduced.toRemote.truncateToSatoshi() - fees
return if (missing < 0.sat) {
Either.Left(CannotAffordFees(params.channelId, -missing, remoteChannelReserve(params), fees))
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1419,7 +1419,8 @@ class NormalTestsCommon : LightningTestSuite() {
assertEquals(bob.commitments.copy(changes = bob.commitments.changes.copy(remoteChanges = bob.commitments.changes.remoteChanges.copy(proposed = bob.commitments.changes.remoteChanges.proposed + fee2))), bob2.commitments)
}

@Test
// TODO: restore this test once we take the reserve into account (see canReceiveFee)
@Ignore
fun `recv UpdateFee -- sender cannot afford it`() {
val (alice, bob) = reachNormal()
// We put all the balance on Bob's side, so that Alice cannot afford a feerate increase.
Expand Down

0 comments on commit 6e4bc39

Please sign in to comment.