Track previous fee proposed by remote peer during fee negotiation

The lightning spec requires us to fail a channel if the remote peer
proposes a closing fee which is not strictly between the previous fees
proposed by both sides. This was not being enforced.

This commit adds a field to NegotiatingData which tracks this value so
that a protocol violation of this sort can be caught.

src/DotNetLightning.Core/Channel/Channel.fs

@@ -933,7 +933,8 @@ module Channel =
                                 RemoteNextCommitInfoOpt = state.RemoteNextCommitInfoOpt
                                 LocalShutdown = localShutdownScriptPubKey
                                 RemoteShutdown = msg.ScriptPubKey
-                                ClosingFeesProposed = [ closingFee ]
+                                LocalClosingFeesProposed = [ closingFee ]
+                                RemoteClosingFeeProposed = None
                             }
                             return [
                                 AcceptedShutdownWhenNoPendingHTLCs(
@@ -946,7 +947,8 @@ module Channel =
                                 RemoteNextCommitInfoOpt = state.RemoteNextCommitInfoOpt
                                 LocalShutdown = localShutdownScriptPubKey
                                 RemoteShutdown = msg.ScriptPubKey
-                                ClosingFeesProposed = []
+                                LocalClosingFeesProposed = []
+                                RemoteClosingFeeProposed = None
                             }
                             return [ AcceptedShutdownWhenNoPendingHTLCs(None, nextState) ]
                     else
@@ -973,7 +975,13 @@ module Channel =
                 let remoteChannelKeys = cm.RemoteChannelPubKeys
                 let lastCommitFeeSatoshi =
                     cm.FundingScriptCoin.TxOut.Value - (cm.LocalCommit.PublishableTxs.CommitTx.Value.TotalOut)
-                do! checkRemoteProposedHigherFeeThanBefore lastCommitFeeSatoshi msg.FeeSatoshis
+                do! checkRemoteProposedHigherFeeThanBaseFee lastCommitFeeSatoshi msg.FeeSatoshis
+                do!
+                    checkRemoteProposedFeeWithinNegotiatedRange
+                        (List.tryHead state.LocalClosingFeesProposed)
+                        (Option.map (fun (fee, _sig) -> fee) state.RemoteClosingFeeProposed)
+                        msg.FeeSatoshis
+
                 let! closingTx, closingSignedMsg =
                     Closing.makeClosingTx (
                         cs.ChannelPrivKeys,
@@ -995,19 +1003,19 @@ module Channel =
                         ])
                     |> expectTransactionError
                 let maybeLocalFee =
-                    state.ClosingFeesProposed
+                    state.LocalClosingFeesProposed
                     |> List.tryHead
                 let areWeInDeal = Some(msg.FeeSatoshis) = maybeLocalFee
                 let hasTooManyNegotiationDone =
-                    (state.ClosingFeesProposed |> List.length) >= cs.ChannelOptions.MaxClosingNegotiationIterations
+                    (state.LocalClosingFeesProposed |> List.length) >= cs.ChannelOptions.MaxClosingNegotiationIterations
                 if (areWeInDeal || hasTooManyNegotiationDone) then
                     return!
                         Closing.handleMutualClose
                             finalizedTx
                             state
                             state.RemoteNextCommitInfoOpt
                 else
-                    let lastLocalClosingFee = state.ClosingFeesProposed |> List.tryHead
+                    let lastLocalClosingFee = state.LocalClosingFeesProposed |> List.tryHead
                     let! localF = 
                         match lastLocalClosingFee with
                         | Some v -> Ok v
@@ -1031,7 +1039,9 @@ module Channel =
                         // we have reached on agreement!
                         let negoData = {
                             state with
-                            ClosingFeesProposed = nextClosingFee :: state.ClosingFeesProposed
+                            LocalClosingFeesProposed =
+                                nextClosingFee :: state.LocalClosingFeesProposed
+                            RemoteClosingFeeProposed = Some (msg.FeeSatoshis, msg.Signature)
                         }
                         return!
                             Closing.handleMutualClose
@@ -1052,7 +1062,9 @@ module Channel =
                             |> expectTransactionError
                         let nextState = {
                             state with
-                            ClosingFeesProposed = nextClosingFee :: state.ClosingFeesProposed
+                            LocalClosingFeesProposed =
+                                nextClosingFee :: state.LocalClosingFeesProposed
+                            RemoteClosingFeeProposed = Some (msg.FeeSatoshis, msg.Signature)
                         }
                         return [ WeProposedNewClosingSigned(closingSignedMsg, nextState) ]
             }

src/DotNetLightning.Core/Channel/ChannelError.fs

@@ -50,7 +50,8 @@ type ChannelError =
     | OnceConfirmedFundingTxHasBecomeUnconfirmed of height: BlockHeight * depth: BlockHeightOffset32
     | CannotCloseChannel of msg: string
     | UndefinedStateAndCmdPair of state: ChannelState * cmd: ChannelCommand
-    | RemoteProposedHigherFeeThanBefore of previous: Money * current: Money
+    | RemoteProposedHigherFeeThanBaseFee of baseFee: Money * proposedFee: Money
+    | RemoteProposedFeeOutOfNegotiatedRange of ourPreviousFee: Money * theirPreviousFee: Money * theirNextFee: Money
     // ---- invalid command ----
     | InvalidOperationAddHTLC of InvalidOperationAddHTLCError
     // -------------------------
@@ -82,7 +83,8 @@ type ChannelError =
         | CannotCloseChannel _ -> Ignore
         | UndefinedStateAndCmdPair _ -> Ignore
         | InvalidOperationAddHTLC _ -> Ignore
-        | RemoteProposedHigherFeeThanBefore(_, _) -> Close
+        | RemoteProposedHigherFeeThanBaseFee(_, _) -> Close
+        | RemoteProposedFeeOutOfNegotiatedRange(_, _, _) -> Close
 
     member this.Message =
         match this with
@@ -111,9 +113,15 @@ type ChannelError =
             sprintf "They sent shutdown msg (%A) while they have pending unsigned HTLCs, this is protocol violation" msg
         | UndefinedStateAndCmdPair (state, cmd) ->
             sprintf "DotNetLightning does not know how to handle command (%A) while in state (%A)" cmd state
-        | RemoteProposedHigherFeeThanBefore(prev, current) ->
-            "remote proposed a commitment fee higher than the last commitment fee in the course of fee negotiation"
-            + sprintf "previous fee=%A; fee remote proposed=%A;" prev current
+        | 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
         | CryptoError cryptoError ->
             sprintf "Crypto error: %s" cryptoError.Message
         | TransactionRelatedErrors transactionErrors ->
@@ -318,11 +326,30 @@ module internal ChannelError =
     let undefinedStateAndCmdPair state cmd =
         UndefinedStateAndCmdPair (state, cmd) |> Error
 
-    let checkRemoteProposedHigherFeeThanBefore prev curr =
-        if (prev < curr) then
-            RemoteProposedHigherFeeThanBefore(prev, curr) |> 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) =
+        match (ourPreviousFeeOpt, theirPreviousFeeOpt) with
+        | (Some ourPreviousFee, Some theirPreviousFee) ->
+            let feeWithinRange =
+                ((theirNextFee < theirPreviousFee) && (theirNextFee > ourPreviousFee)) ||
+                ((theirNextFee < ourPreviousFee) && (theirNextFee > theirPreviousFee))
+            if feeWithinRange then
+                Ok ()
+            else
+                RemoteProposedFeeOutOfNegotiatedRange(
+                    ourPreviousFee,
+                    theirPreviousFee,
+                    theirNextFee
+                ) |> Error
+        | _ -> Ok ()
+
 module internal OpenChannelMsgValidation =
     let checkMaxAcceptedHTLCs (msg: OpenChannelMsg) =
         if (msg.MaxAcceptedHTLCs < 1us) || (msg.MaxAcceptedHTLCs > 483us) then

src/DotNetLightning.Core/Channel/ChannelTypes.fs

@@ -61,7 +61,8 @@ module Data =
         RemoteNextCommitInfoOpt: Option<RemoteNextCommitInfo>
         LocalShutdown: ShutdownScriptPubKey
         RemoteShutdown: ShutdownScriptPubKey
-        ClosingFeesProposed: List<Money>
+        LocalClosingFeesProposed: List<Money>
+        RemoteClosingFeeProposed: Option<Money * LNECDSASignature>
     }
 
     type ClosingData = {