Core: remove the commit fee clamp, add max fee

This commit removes the check that compares mutual close fee with commit tx fee, this check is outdated and has been changed by this spec PR [1]. To prevent unreasonably high fee, this commit introduces MutualCloseMaxFeeMultiplier setting. [1] lightning/bolts#847
nblockchain · Apr 12, 2023 · 846c243 · 846c243
1 parent ea73caf
commit 846c243
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 24 deletions.
diff --git a/src/DotNetLightning.Core/Channel/Channel.fs b/src/DotNetLightning.Core/Channel/Channel.fs
@@ -1113,9 +1113,17 @@ and Channel = {
             | (None, None) ->
                 Error ReceivedClosingSignedBeforeSendingOrReceivingShutdown
         let remoteChannelKeys = this.SavedChannelState.StaticChannelConfig.RemoteChannelPubKeys
-        let lastCommitFeeSatoshi =
-            this.SavedChannelState.StaticChannelConfig.FundingScriptCoin.TxOut.Value - (this.SavedChannelState.LocalCommit.PublishableTxs.CommitTx.Value.TotalOut)
-        do! checkRemoteProposedHigherFeeThanBaseFee lastCommitFeeSatoshi msg.FeeSatoshis
+
+        let! idealFee =
+            this.FirstClosingFee
+                localShutdownScriptPubKey
+                remoteShutdownScriptPubKey
+                    |> expectTransactionError
+
+        let maxFee =
+            this.SavedChannelState.StaticChannelConfig.LocalParams.MutualCloseMaxFeeMultiplier
+            * idealFee
+
         do!
             checkRemoteProposedFeeWithinNegotiatedRange
                 (List.tryHead this.NegotiatingState.LocalClosingFeesProposed)
@@ -1146,18 +1154,20 @@ and Channel = {
         if (areWeInDeal || hasTooManyNegotiationDone) then
             return this, MutualClose (finalizedTx, None)
         else
-            let lastLocalClosingFee = this.NegotiatingState.LocalClosingFeesProposed |> List.tryHead
-            let! localF = 
-                match lastLocalClosingFee with
-                | Some v -> Ok v
-                | None ->
-                    this.FirstClosingFee
-                        localShutdownScriptPubKey
-                        remoteShutdownScriptPubKey
-                    |> expectTransactionError
+            let maybeLastLocalClosingFee = this.NegotiatingState.LocalClosingFeesProposed |> List.tryHead
+            let localF = 
+                match maybeLastLocalClosingFee with
+                | Some lastLocalClosingFee -> lastLocalClosingFee
+                | None -> idealFee
+
             let nextClosingFee =
                 Channel.NextClosingFee (localF, msg.FeeSatoshis)
-            if (Some nextClosingFee = lastLocalClosingFee) then
+
+            if this.SavedChannelState.StaticChannelConfig.IsFunder && nextClosingFee > maxFee then
+                 return!
+                     Error <| ProposalExceedsMaxFee(nextClosingFee, maxFee)
+
+            if (Some nextClosingFee = maybeLastLocalClosingFee) then
                 return this, MutualClose (finalizedTx, None)
             else if (nextClosingFee = msg.FeeSatoshis) then
                 // we have reached on agreement!

diff --git a/src/DotNetLightning.Core/Channel/ChannelError.fs b/src/DotNetLightning.Core/Channel/ChannelError.fs
@@ -53,8 +53,8 @@ type ChannelError =
     | FundingTxNotGiven of msg: string
     | OnceConfirmedFundingTxHasBecomeUnconfirmed of height: BlockHeight * depth: BlockHeightOffset32
     | CannotCloseChannel of msg: string
-    | RemoteProposedHigherFeeThanBaseFee of baseFee: Money * proposedFee: Money
     | RemoteProposedFeeOutOfNegotiatedRange of ourPreviousFee: Money * theirPreviousFee: Money * theirNextFee: Money
+    | ProposalExceedsMaxFee of proposalFee: Money * maxFee: Money
     | NoUpdatesToSign
     | CannotSignCommitmentBeforeRevocation
     | InsufficientConfirmations of requiredDepth: BlockHeightOffset32 * currentDepth: BlockHeightOffset32
@@ -95,8 +95,8 @@ type ChannelError =
         | CannotSignCommitmentBeforeRevocation -> Ignore
         | InsufficientConfirmations(_, _) -> Ignore
         | InvalidOperationAddHTLC _ -> Ignore
-        | RemoteProposedHigherFeeThanBaseFee(_, _) -> Close
         | RemoteProposedFeeOutOfNegotiatedRange(_, _, _) -> Close
+        | ProposalExceedsMaxFee(_, _) -> Ignore
 
     member this.Message =
         match this with
@@ -125,15 +125,17 @@ type ChannelError =
             sprintf "once confirmed funding tx has become less confirmed than threshold %A! This is probably caused by reorg. current depth is: %A " height depth
         | ReceivedShutdownWhenRemoteHasUnsignedOutgoingHTLCs msg ->
             sprintf "They sent shutdown msg (%A) while they have pending unsigned HTLCs, this is protocol violation" msg
-        | RemoteProposedHigherFeeThanBaseFee(baseFee, proposedFee) ->
-            "remote proposed a closing fee higher than commitment fee of the final commitment transaction. "
-            + sprintf "commitment fee=%A; fee remote proposed=%A;" baseFee proposedFee
         | RemoteProposedFeeOutOfNegotiatedRange(ourPreviousFee, theirPreviousFee, theirNextFee) ->
             "remote proposed a closing fee which was not strictly between the previous fee that \
             we proposed and the previous fee that they proposed. "
             + sprintf
                 "our previous fee = %A; their previous fee = %A; their next fee = %A"
                 ourPreviousFee theirPreviousFee theirNextFee
+        | ProposalExceedsMaxFee(proposalFee, maxFee) ->
+            sprintf
+                "latest fee proposal (%i) exceeds max fee (%i)"
+                proposalFee.Satoshi
+                maxFee.Satoshi
         | CryptoError cryptoError ->
             sprintf "Crypto error: %s" cryptoError.Message
         | TransactionRelatedErrors transactionErrors ->
@@ -352,12 +354,6 @@ module internal ChannelError =
     let receivedShutdownWhenRemoteHasUnsignedOutgoingHTLCs msg =
         msg |> ReceivedShutdownWhenRemoteHasUnsignedOutgoingHTLCs |> Error
 
-    let checkRemoteProposedHigherFeeThanBaseFee baseFee proposedFee =
-        if (baseFee < proposedFee) then
-            RemoteProposedHigherFeeThanBaseFee(baseFee, proposedFee) |> Error
-        else
-            Ok()
-
     let checkRemoteProposedFeeWithinNegotiatedRange (ourPreviousFeeOpt: Option<Money>)
                                                     (theirPreviousFeeOpt: Option<Money>)
                                                     (theirNextFee: Money) =

diff --git a/src/DotNetLightning.Core/Channel/ChannelOperations.fs b/src/DotNetLightning.Core/Channel/ChannelOperations.fs
@@ -67,6 +67,11 @@ type LocalParams = {
     ToSelfDelay: BlockHeightOffset16
     MaxAcceptedHTLCs: uint16
     Features: FeatureBits
+    // MutualCloseMaxFeeMultiplier is a multiplier we'll apply to the ideal fee
+    // of the funder, to decide when the negotiated fee is too high. By
+    // default, we want to bail out if we attempt to negotiate a fee that's
+    // 3x higher than our ideal fee.
+    MutualCloseMaxFeeMultiplier: int
 }
 
 type RemoteParams = {

diff --git a/tests/DotNetLightning.Core.Tests/TransactionTests.fs b/tests/DotNetLightning.Core.Tests/TransactionTests.fs
@@ -63,6 +63,7 @@ let testList = testList "transaction tests" [
             ToSelfDelay = 144us |> BlockHeightOffset16
             MaxAcceptedHTLCs = 1000us
             Features = FeatureBits.Zero
+            MutualCloseMaxFeeMultiplier = 3
         }
 
         let remoteLocalParam : LocalParams = {
@@ -73,6 +74,7 @@ let testList = testList "transaction tests" [
             ToSelfDelay = 144us |> BlockHeightOffset16
             MaxAcceptedHTLCs = 1000us
             Features = FeatureBits.Zero
+            MutualCloseMaxFeeMultiplier = 3
         }
 
         let remoteParam : RemoteParams = {