From 2fe01194970014700d1b509a08a3f2e118163562 Mon Sep 17 00:00:00 2001 From: t-bast Date: Mon, 16 Oct 2023 11:06:12 +0200 Subject: [PATCH] Fix `update_fee` reserve requirement mismatch 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. --- .../fr/acinq/lightning/channel/Commitments.kt | 13 ++++++++++--- .../lightning/channel/states/NormalTestsCommon.kt | 3 ++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt b/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt index 1408bbb26..b0309c6fd 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt @@ -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() @@ -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 { diff --git a/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt index 281c5e950..0494f27c2 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt @@ -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.