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
aarani · Apr 18, 2023 · 8dfb1cb · 8dfb1cb
1 parent 20b4f13
commit 8dfb1cb
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 21 deletions.
diff --git a/src/DotNetLightning.Core/Channel/Channel.fs b/src/DotNetLightning.Core/Channel/Channel.fs
@@ -1471,14 +1471,15 @@ and Channel =
             let remoteChannelKeys =
                 this.SavedChannelState.StaticChannelConfig.RemoteChannelPubKeys
 
-            let lastCommitFeeSatoshi =
-                this.SavedChannelState.StaticChannelConfig.FundingScriptCoin.TxOut.Value
-                - (this.SavedChannelState.LocalCommit.PublishableTxs.CommitTx.Value.TotalOut)
+            let! idealFee =
+                this.FirstClosingFee
+                    localShutdownScriptPubKey
+                    remoteShutdownScriptPubKey
+                |> expectTransactionError
 
-            do!
-                checkRemoteProposedHigherFeeThanBaseFee
-                    lastCommitFeeSatoshi
-                    msg.FeeSatoshis
+            let maxFee =
+                this.SavedChannelState.StaticChannelConfig.LocalParams.MutualCloseMaxFeeMultiplier
+                * idealFee
 
             do!
                 checkRemoteProposedFeeWithinNegotiatedRange
@@ -1550,6 +1551,12 @@ and Channel =
                             nextClosingFee
                         |> expectTransactionError
 
+                    if this.SavedChannelState.StaticChannelConfig.IsFunder
+                       && nextClosingFee > maxFee then
+                        return!
+                            Error
+                            <| ProposalExceedsMaxFee(nextClosingFee, maxFee)
+
                     let nextState =
                         { this.NegotiatingState with
                             LocalClosingFeesProposed =

diff --git a/src/DotNetLightning.Core/Channel/ChannelError.fs b/src/DotNetLightning.Core/Channel/ChannelError.fs
@@ -62,11 +62,11 @@ type ChannelError =
         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
@@ -108,8 +108,8 @@ type ChannelError =
         | CannotSignCommitmentBeforeRevocation -> Ignore
         | InsufficientConfirmations(_, _) -> Ignore
         | InvalidOperationAddHTLC _ -> Ignore
-        | RemoteProposedHigherFeeThanBaseFee(_, _) -> Close
         | RemoteProposedFeeOutOfNegotiatedRange(_, _, _) -> Close
+        | ProposalExceedsMaxFee(_, _) -> Ignore
 
     member this.Message =
         match this with
@@ -161,12 +161,6 @@ type ChannelError =
             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
@@ -178,6 +172,11 @@ type ChannelError =
                 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 ->
@@ -409,12 +408,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>)

diff --git a/src/DotNetLightning.Core/Channel/ChannelOperations.fs b/src/DotNetLightning.Core/Channel/ChannelOperations.fs
@@ -80,6 +80,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/ChannelTests.fs b/tests/DotNetLightning.Core.Tests/ChannelTests.fs
@@ -103,6 +103,7 @@ let tests =
                         ToSelfDelay = 144us |> BlockHeightOffset16
                         MaxAcceptedHTLCs = 1000us
                         Features = FeatureBits.Zero
+                        MutualCloseMaxFeeMultiplier = 3
                     }
 
                 let remoteParams: RemoteParams =

diff --git a/tests/DotNetLightning.Core.Tests/TransactionTests.fs b/tests/DotNetLightning.Core.Tests/TransactionTests.fs
@@ -101,6 +101,7 @@ let testList =
                         ToSelfDelay = 144us |> BlockHeightOffset16
                         MaxAcceptedHTLCs = 1000us
                         Features = FeatureBits.Zero
+                        MutualCloseMaxFeeMultiplier = 3
                     }
 
                 let remoteLocalParam: LocalParams =
@@ -112,6 +113,7 @@ let testList =
                         ToSelfDelay = 144us |> BlockHeightOffset16
                         MaxAcceptedHTLCs = 1000us
                         Features = FeatureBits.Zero
+                        MutualCloseMaxFeeMultiplier = 3
                     }
 
                 let remoteParam: RemoteParams =
@@ -532,6 +534,7 @@ let testList =
                         ToSelfDelay = 144us |> BlockHeightOffset16
                         MaxAcceptedHTLCs = 1000us
                         Features = FeatureBits.Zero
+                        MutualCloseMaxFeeMultiplier = 3
                     }
 
                 let remoteLocalParam: LocalParams =
@@ -543,6 +546,7 @@ let testList =
                         ToSelfDelay = 144us |> BlockHeightOffset16
                         MaxAcceptedHTLCs = 1000us
                         Features = FeatureBits.Zero
+                        MutualCloseMaxFeeMultiplier = 3
                     }
 
                 let remoteParams: RemoteParams =
@@ -887,6 +891,7 @@ let testList =
                         ToSelfDelay = 144us |> BlockHeightOffset16
                         MaxAcceptedHTLCs = 1000us
                         Features = FeatureBits.Zero
+                        MutualCloseMaxFeeMultiplier = 3
                     }
 
                 let remoteLocalParam: LocalParams =
@@ -898,6 +903,7 @@ let testList =
                         ToSelfDelay = 144us |> BlockHeightOffset16
                         MaxAcceptedHTLCs = 1000us
                         Features = FeatureBits.Zero
+                        MutualCloseMaxFeeMultiplier = 3
                     }
 
                 let remoteParam: RemoteParams =