Require strict re-signing before shutdown update

Whenever we exchange `shutdown`, we now require that new closing txs
are signed before allowing another `shutdown` message to be sent to
start a new signing round.

This creates more risk of deadlock when one side fails to send their
sigs, where we'll need to disconnect to start a new signing round.
But that shouldn't happen if nodes are honest and not buggy, so it
probably doesn't matter. If nodes are buggy or malicious, we will
need to force-close anyway.

Author: t-bast

eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelData.scala

@@ -536,36 +536,18 @@ object SpliceStatus {
   case object SpliceAborted extends SpliceStatus
 }
 
-case class ClosingCompleteSent(closingComplete: ClosingComplete, closingFeerate: FeeratePerKw)
-
-sealed trait OnRemoteShutdown
-object OnRemoteShutdown {
-  /** When receiving the remote shutdown, we sign a new version of our closing transaction. */
-  case class SignTransaction(closingFeerate: FeeratePerKw) extends OnRemoteShutdown
-  /** When receiving the remote shutdown, we don't sign a new version of our closing transaction, but our peer may sign theirs. */
-  case object WaitForSigs extends OnRemoteShutdown
-}
-
 sealed trait ClosingNegotiation {
   def localShutdown: Shutdown
-  // When we disconnect, we discard pending signatures.
-  def disconnect(): ClosingNegotiation.WaitingForRemoteShutdown = this match {
-    case status: ClosingNegotiation.WaitingForRemoteShutdown => status
-    case status: ClosingNegotiation.SigningTransactions => status.closingCompleteSent_opt.map(_.closingFeerate) match {
-      // If we were waiting for their signature, we will send closing_complete again after exchanging shutdown.
-      case Some(closingFeerate) if status.closingSigReceived_opt.isEmpty => ClosingNegotiation.WaitingForRemoteShutdown(status.localShutdown, OnRemoteShutdown.SignTransaction(closingFeerate))
-      case _ => ClosingNegotiation.WaitingForRemoteShutdown(status.localShutdown, OnRemoteShutdown.WaitForSigs)
-    }
-    case status: ClosingNegotiation.WaitingForConfirmation => ClosingNegotiation.WaitingForRemoteShutdown(status.localShutdown, OnRemoteShutdown.WaitForSigs)
-  }
+  /** Closing feerate for our closing transaction. */
+  def closingFeerate: FeeratePerKw
 }
 object ClosingNegotiation {
   /** We've sent a new shutdown message: we wait for their shutdown message before taking any action. */
-  case class WaitingForRemoteShutdown(localShutdown: Shutdown, onRemoteShutdown: OnRemoteShutdown) extends ClosingNegotiation
-  /** We've exchanged shutdown messages: at least one side will send closing_complete to renew their closing transaction. */
-  case class SigningTransactions(localShutdown: Shutdown, remoteShutdown: Shutdown, closingCompleteSent_opt: Option[ClosingCompleteSent], closingSigSent_opt: Option[ClosingSig], closingSigReceived_opt: Option[ClosingSig]) extends ClosingNegotiation
-  /** We've signed a new closing transaction and are waiting for confirmation or to initiate RBF. */
-  case class WaitingForConfirmation(localShutdown: Shutdown, remoteShutdown: Shutdown) extends ClosingNegotiation
+  case class WaitingForRemoteShutdown(localShutdown: Shutdown, closingFeerate: FeeratePerKw) extends ClosingNegotiation
+  /** We've exchanged shutdown messages: we both send closing_complete to renew the closing transactions. */
+  case class SigningTransactions(localShutdown: Shutdown, remoteShutdown: Shutdown, closingFeerate: FeeratePerKw, closingCompleteSent_opt: Option[ClosingComplete], closingSigSent_opt: Option[ClosingSig], closingSigReceived_opt: Option[ClosingSig]) extends ClosingNegotiation
+  /** We've signed new closing transactions and are waiting for confirmation or to initiate RBF. */
+  case class WaitingForConfirmation(localShutdown: Shutdown, remoteShutdown: Shutdown, closingFeerate: FeeratePerKw) extends ClosingNegotiation
 }
 
 sealed trait ChannelData extends PossiblyHarmful {

eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala

@@ -117,6 +117,7 @@ case class InvalidAnnouncementSignatures           (override val channelId: Byte
 case class InvalidCommitmentSignature              (override val channelId: ByteVector32, fundingTxId: TxId, fundingTxIndex: Long, unsignedCommitTx: Transaction) extends ChannelException(channelId, s"invalid commitment signature: fundingTxId=$fundingTxId fundingTxIndex=$fundingTxIndex commitTxId=${unsignedCommitTx.txid} commitTx=$unsignedCommitTx")
 case class InvalidHtlcSignature                    (override val channelId: ByteVector32, txId: TxId) extends ChannelException(channelId, s"invalid htlc signature: txId=$txId")
 case class CannotGenerateClosingTx                 (override val channelId: ByteVector32) extends ChannelException(channelId, "failed to generate closing transaction: all outputs are trimmed")
+case class ShutdownWaitingForSigs                  (override val channelId: ByteVector32) extends ChannelException(channelId, "received unexpected shutdown while signing closing transactions")
 case class MissingCloseSignature                   (override val channelId: ByteVector32) extends ChannelException(channelId, "closing_complete is missing a signature for a closing transaction including our output")
 case class InvalidCloseSignature                   (override val channelId: ByteVector32, txId: TxId) extends ChannelException(channelId, s"invalid close signature: txId=$txId")
 case class UnexpectedClosingComplete               (override val channelId: ByteVector32, fees: Satoshi, lockTime: Long) extends ChannelException(channelId, s"unexpected closing_complete with fees=$fees and lockTime=$lockTime: we already sent closing_sig, you must send shutdown first")

eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala

@@ -1727,77 +1727,69 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
 
   when(NEGOTIATING_SIMPLE)(handleExceptions {
     case Event(remoteShutdown: Shutdown, d: DATA_NEGOTIATING_SIMPLE) =>
+      val localScript = d.status.localShutdown.scriptPubKey
+      val remoteScript = remoteShutdown.scriptPubKey
       d.status match {
         case status: ClosingNegotiation.WaitingForRemoteShutdown =>
           // We have already sent our shutdown. Now that we've received theirs, we're ready to sign closing transactions.
-          // If we don't have a closing feerate, we don't need to create a new version of our closing transaction (which
-          // can happen after a reconnection for example).
-          status.onRemoteShutdown match {
-            case OnRemoteShutdown.SignTransaction(closingFeerate) =>
-              val localScript = status.localShutdown.scriptPubKey
-              val remoteScript = remoteShutdown.scriptPubKey
-              MutualClose.makeSimpleClosingTx(nodeParams.currentBlockHeight, keyManager, d.commitments.latest, localScript, remoteScript, closingFeerate) match {
-                case Left(f) =>
-                  log.warning("cannot create local closing txs, waiting for remote closing_complete: {}", f.getMessage)
-                  val status1 = ClosingNegotiation.SigningTransactions(status.localShutdown, remoteShutdown, None, None, None)
-                  stay() using d.copy(status = status1)
-                case Right((closingTxs, closingComplete)) =>
-                  log.debug("signing local mutual close transactions: {}", closingTxs)
-                  val status1 = ClosingNegotiation.SigningTransactions(status.localShutdown, remoteShutdown, Some(ClosingCompleteSent(closingComplete, closingFeerate)), None, None)
-                  stay() using d.copy(status = status1, proposedClosingTxs = d.proposedClosingTxs :+ closingTxs) storing() sending closingComplete
-              }
-            case OnRemoteShutdown.WaitForSigs =>
-              val status1 = ClosingNegotiation.SigningTransactions(status.localShutdown, remoteShutdown, None, None, None)
+          MutualClose.makeSimpleClosingTx(nodeParams.currentBlockHeight, keyManager, d.commitments.latest, localScript, remoteScript, status.closingFeerate) match {
+            case Left(f) =>
+              log.warning("cannot create local closing txs, waiting for remote closing_complete: {}", f.getMessage)
+              val status1 = ClosingNegotiation.SigningTransactions(status.localShutdown, remoteShutdown, status.closingFeerate, None, None, None)
               stay() using d.copy(status = status1)
+            case Right((closingTxs, closingComplete)) =>
+              log.debug("signing local mutual close transactions: {}", closingTxs)
+              val status1 = ClosingNegotiation.SigningTransactions(status.localShutdown, remoteShutdown, status.closingFeerate, Some(closingComplete), None, None)
+              stay() using d.copy(status = status1, proposedClosingTxs = d.proposedClosingTxs :+ closingTxs) storing() sending closingComplete
           }
-        case status: ClosingNegotiation.SigningTransactions =>
-          // We were in the middle of signing transactions: we restart a signing round from scratch.
-          // If we were waiting for their signature, we will send closing_complete again after exchanging shutdown.
-          val localShutdown = status.localShutdown
-          val onRemoteShutdown = status.closingCompleteSent_opt.map(_.closingFeerate) match {
-            case Some(closingFeerate) if status.closingSigReceived_opt.isEmpty => OnRemoteShutdown.SignTransaction(closingFeerate)
-            case _ => OnRemoteShutdown.WaitForSigs
-          }
-          val status1 = ClosingNegotiation.WaitingForRemoteShutdown(localShutdown, onRemoteShutdown)
-          self ! remoteShutdown
-          stay() using d.copy(status = status1) sending localShutdown
+        case _: ClosingNegotiation.SigningTransactions =>
+          // We were in the middle of signing transactions: sending shutdown is forbidden at that point.
+          context.system.scheduler.scheduleOnce(2 second, peer, Peer.Disconnect(remoteNodeId))
+          stay() sending Warning(d.channelId, ShutdownWaitingForSigs(d.channelId).getMessage)
         case status: ClosingNegotiation.WaitingForConfirmation =>
           // Our peer wants to create a new version of their closing transaction. We don't need to update our version of
-          // the closing transaction: we re-send our previous shutdown and wait for their closing_complete.
+          // the closing transaction: we use the same parameters as we did in the previous signing round.
           val localShutdown = status.localShutdown
-          val status1 = ClosingNegotiation.SigningTransactions(localShutdown, remoteShutdown, None, None, None)
-          stay() using d.copy(status = status1) sending localShutdown
+          MutualClose.makeSimpleClosingTx(nodeParams.currentBlockHeight, keyManager, d.commitments.latest, localScript, remoteScript, status.closingFeerate) match {
+            case Left(f) =>
+              log.warning("cannot create local closing txs, waiting for remote closing_complete: {}", f.getMessage)
+              val status1 = ClosingNegotiation.SigningTransactions(localShutdown, remoteShutdown, status.closingFeerate, None, None, None)
+              stay() using d.copy(status = status1) sending localShutdown
+            case Right((closingTxs, closingComplete)) =>
+              log.debug("signing local mutual close transactions: {}", closingTxs)
+              val status1 = ClosingNegotiation.SigningTransactions(localShutdown, remoteShutdown, status.closingFeerate, Some(closingComplete), None, None)
+              stay() using d.copy(status = status1, proposedClosingTxs = d.proposedClosingTxs :+ closingTxs) storing() sending Seq(localShutdown, closingComplete)
+          }
       }
 
     case Event(closingComplete: ClosingComplete, d: DATA_NEGOTIATING_SIMPLE) =>
       d.status match {
         case _: ClosingNegotiation.WaitingForRemoteShutdown =>
           log.info("ignoring remote closing_complete, we've sent shutdown to initiate a new signing round")
-          stay()
+          stay() sending Warning(d.channelId, UnexpectedClosingComplete(d.channelId, closingComplete.fees, closingComplete.lockTime).getMessage)
         case _: ClosingNegotiation.WaitingForConfirmation =>
           log.info("ignoring closing_complete, we've already sent closing_sig: peer must send shutdown again before closing_complete")
           stay() sending Warning(d.channelId, UnexpectedClosingComplete(d.channelId, closingComplete.fees, closingComplete.lockTime).getMessage)
         case status: ClosingNegotiation.SigningTransactions if status.closingSigSent_opt.nonEmpty =>
-          log.info("ignoring closing_complete, we've already sent closing_sig: peer must send shutdown again before closing_complete")
+          log.info("ignoring closing_complete, we've already sent closing_sig: peer must send closing_sig, then shutdown again before closing_complete")
           stay() sending Warning(d.channelId, UnexpectedClosingComplete(d.channelId, closingComplete.fees, closingComplete.lockTime).getMessage)
         case status: ClosingNegotiation.SigningTransactions =>
           val localScript = status.localShutdown.scriptPubKey
           val remoteScript = status.remoteShutdown.scriptPubKey
           MutualClose.signSimpleClosingTx(keyManager, d.commitments.latest, localScript, remoteScript, closingComplete) match {
             case Left(f) =>
-              // This may happen if scripts were updated concurrently, so we simply ignore failures.
               log.warning("invalid closing_complete: {}", f.getMessage)
-              stay()
+              stay() sending Warning(d.channelId, f.getMessage)
             case Right((signedClosingTx, closingSig)) =>
               log.debug("signing remote mutual close transaction: {}", signedClosingTx.tx)
               val status1 = status.closingCompleteSent_opt match {
                 // We've sent closing_complete: we may be waiting for their closing_sig.
                 case Some(_) => status.closingSigReceived_opt match {
-                  case Some(_) => ClosingNegotiation.WaitingForConfirmation(status.localShutdown, status.remoteShutdown)
+                  case Some(_) => ClosingNegotiation.WaitingForConfirmation(status.localShutdown, status.remoteShutdown, status.closingFeerate)
                   case None => status.copy(closingSigSent_opt = Some(closingSig))
                 }
-                // We haven't sent closing_complete: we're not waiting for their closing_sig'.
-                case None => ClosingNegotiation.WaitingForConfirmation(status.localShutdown, status.remoteShutdown)
+                // We haven't sent closing_complete: we're not waiting for their closing_sig.
+                case None => ClosingNegotiation.WaitingForConfirmation(status.localShutdown, status.remoteShutdown, status.closingFeerate)
               }
               val d1 = d.copy(status = status1, publishedClosingTxs = d.publishedClosingTxs :+ signedClosingTx)
               stay() using d1 storing() calling doPublish(signedClosingTx, localPaysClosingFees = false) sending closingSig
@@ -1818,14 +1810,13 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
         case status: ClosingNegotiation.SigningTransactions =>
           MutualClose.receiveSimpleClosingSig(keyManager, d.commitments.latest, d.proposedClosingTxs.last, closingSig) match {
             case Left(f) =>
-              // This may happen if scripts were updated concurrently, so we simply ignore failures.
               log.warning("invalid closing_sig: {}", f.getMessage)
-              stay()
+              stay() sending Warning(d.channelId, f.getMessage)
             case Right(signedClosingTx) =>
               log.debug("received signatures for local mutual close transaction: {}", signedClosingTx.tx)
               val status1 = status.closingSigSent_opt match {
                 // We have already signed their transaction: both local and remote closing transactions have been updated.
-                case Some(_) => ClosingNegotiation.WaitingForConfirmation(status.localShutdown, status.remoteShutdown)
+                case Some(_) => ClosingNegotiation.WaitingForConfirmation(status.localShutdown, status.remoteShutdown, status.closingFeerate)
                 // We haven't sent closing_sig yet: they may send us closing_complete to update their closing transaction.
                 case None => status.copy(closingSigReceived_opt = Some(closingSig))
               }
@@ -1859,17 +1850,17 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
           log.info("we're already waiting for our peer to send their shutdown message, no need to send ours again")
           handleCommandError(ClosingAlreadyInProgress(d.channelId), c)
         case _: ClosingNegotiation.SigningTransactions =>
-          val status1 = ClosingNegotiation.WaitingForRemoteShutdown(localShutdown, OnRemoteShutdown.SignTransaction(closingFeerate))
-          stay() using d.copy(status = status1) storing() sending localShutdown
+          log.info("we're in the middle of signing closing transactions, we should finish this round before starting a new signing session")
+          handleCommandError(ClosingAlreadyInProgress(d.channelId), c)
         case _: ClosingNegotiation.WaitingForConfirmation =>
-          val status1 = ClosingNegotiation.WaitingForRemoteShutdown(localShutdown, OnRemoteShutdown.SignTransaction(closingFeerate))
+          val status1 = ClosingNegotiation.WaitingForRemoteShutdown(localShutdown, closingFeerate)
           stay() using d.copy(status = status1) storing() sending localShutdown
       }
 
     case Event(e: Error, d: DATA_NEGOTIATING_SIMPLE) => handleRemoteError(e, d)
 
     case Event(INPUT_DISCONNECTED, d: DATA_NEGOTIATING_SIMPLE) =>
-      val status1 = d.status.disconnect()
+      val status1 = ClosingNegotiation.WaitingForRemoteShutdown(d.status.localShutdown, d.status.closingFeerate)
       goto(OFFLINE) using d.copy(status = status1)
 
   })
@@ -2526,10 +2517,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
     case Event(_: ChannelReestablish, d: DATA_NEGOTIATING_SIMPLE) =>
       // We retransmit our shutdown: we may have updated our script and they may not have received it.
       val localShutdown = d.status.localShutdown
-      val status1 = d.status match {
-        case status: ClosingNegotiation.WaitingForRemoteShutdown => status.copy(localShutdown = localShutdown)
-        case _ => ClosingNegotiation.WaitingForRemoteShutdown(localShutdown, OnRemoteShutdown.WaitForSigs)
-      }
+      val status1 = ClosingNegotiation.WaitingForRemoteShutdown(localShutdown, d.status.closingFeerate)
       goto(NEGOTIATING_SIMPLE) using d.copy(status = status1) sending localShutdown
 
     // This handler is a workaround for an issue in lnd: starting with versions 0.10 / 0.11, they sometimes fail to send

eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/CommonHandlers.scala

@@ -139,12 +139,12 @@ trait CommonHandlers {
     MutualClose.makeSimpleClosingTx(nodeParams.currentBlockHeight, keyManager, commitments.latest, localScript, remoteScript, closingFeerate) match {
       case Left(f) =>
         log.warning("cannot create local closing txs, waiting for remote closing_complete: {}", f.getMessage)
-        val status = ClosingNegotiation.SigningTransactions(localShutdown, remoteShutdown, None, None, None)
+        val status = ClosingNegotiation.SigningTransactions(localShutdown, remoteShutdown, closingFeerate, None, None, None)
         val d = DATA_NEGOTIATING_SIMPLE(commitments, status, Nil, Nil)
         goto(NEGOTIATING_SIMPLE) using d storing() sending toSend
       case Right((closingTxs, closingComplete)) =>
         log.debug("signing local mutual close transactions: {}", closingTxs)
-        val status = ClosingNegotiation.SigningTransactions(localShutdown, remoteShutdown, Some(ClosingCompleteSent(closingComplete, closingFeerate)), None, None)
+        val status = ClosingNegotiation.SigningTransactions(localShutdown, remoteShutdown, closingFeerate, Some(closingComplete), None, None)
         val d = DATA_NEGOTIATING_SIMPLE(commitments, status, closingTxs :: Nil, Nil)
         goto(NEGOTIATING_SIMPLE) using d storing() sending toSend :+ closingComplete
     }