handle sync errors in SHUTDOWN and refactor

ACINQ · Oct 25, 2021 · d8b2cbe · d8b2cbe
1 parent 6d396ce
commit d8b2cbe
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 47 deletions.
diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala
@@ -1703,25 +1703,9 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder, remo
 
     case Event(channelReestablish: ChannelReestablish, d: DATA_NORMAL) =>
       Syncing.checkSync(keyManager, d, channelReestablish) match {
-        case res: SyncResult.LocalLateProven =>
-          log.warning(s"counterparty proved that we have an outdated (revoked) local commitment!!! ourLocalCommitmentNumber=${res.ourLocalCommitmentNumber} theirRemoteCommitmentNumber=${res.theirRemoteCommitmentNumber}")
-          // their data checks out, we indeed seem to be using an old revoked commitment, and must absolutely *NOT* publish it, because that would be a cheating attempt and they
-          // would punish us by taking all the funds in the channel
-          handleOutdatedCommitment(channelReestablish, d)
-        case res: Syncing.SyncResult.LocalLateUnproven =>
-          log.warning(s"our local commitment is in sync, but counterparty says that they have a more recent remote commitment than the one we know of (they could be lying)!!! ourRemoteCommitmentNumber=${res.ourRemoteCommitmentNumber} theirCommitmentNumber=${res.theirLocalCommitmentNumber}")
-          // there is no way to make sure that they are saying the truth, the best thing to do is ask them to publish their commitment right now
-          // maybe they will publish their commitment, in that case we need to remember their commitment point in order to be able to claim our outputs
-          // note that if they don't comply, we could publish our own commitment (it is not stale, otherwise we would be in the case above)
-          handleOutdatedCommitment(channelReestablish, d)
-        case res: Syncing.SyncResult.RemoteLying =>
-          // they are deliberately trying to fool us into thinking we have a late commitment
-          log.warning(s"counterparty is lying about us having an outdated commitment!!! ourLocalCommitmentNumber=${res.ourLocalCommitmentNumber} theirRemoteCommitmentNumber=${res.theirRemoteCommitmentNumber}")
-          handleLocalError(InvalidRevokedCommitProof(d.channelId, res.ourLocalCommitmentNumber, res.theirRemoteCommitmentNumber, res.invalidPerCommitmentSecret), d, Some(channelReestablish))
-        case SyncResult.RemoteLate =>
-          log.warning("counterparty appears to be using an outdated commitment, they may request a force-close, standing by...")
-          stay()
-        case res: SyncResult.InSync =>
+        case syncFailure: SyncResult.Failure =>
+          handleSyncFailure(channelReestablish, syncFailure, d)
+        case syncSuccess: SyncResult.Success =>
           var sendQueue = Queue.empty[LightningMessage]
           // normal case, our data is up-to-date
 
@@ -1735,18 +1719,10 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder, remo
           }
 
           // we may need to retransmit updates and/or commit_sig and/or revocation
-          sendQueue = sendQueue ++ res.retransmit
+          sendQueue = sendQueue ++ syncSuccess.retransmit
 
           // then we clean up unacknowledged updates
-          log.debug("discarding proposed OUT: {}", d.commitments.localChanges.proposed.map(Commitments.msg2String(_)).mkString(","))
-          log.debug("discarding proposed IN: {}", d.commitments.remoteChanges.proposed.map(Commitments.msg2String(_)).mkString(","))
-          val commitments1 = d.commitments.copy(
-            localChanges = d.commitments.localChanges.copy(proposed = Nil),
-            remoteChanges = d.commitments.remoteChanges.copy(proposed = Nil),
-            localNextHtlcId = d.commitments.localNextHtlcId - d.commitments.localChanges.proposed.collect { case u: UpdateAddHtlc => u }.size,
-            remoteNextHtlcId = d.commitments.remoteNextHtlcId - d.commitments.remoteChanges.proposed.collect { case u: UpdateAddHtlc => u }.size)
-          log.debug(s"localNextHtlcId=${d.commitments.localNextHtlcId}->${commitments1.localNextHtlcId}")
-          log.debug(s"remoteNextHtlcId=${d.commitments.remoteNextHtlcId}->${commitments1.remoteNextHtlcId}")
+          val commitments1 = Commitments.discardUnsignedUpdates(d.commitments)
 
           commitments1.remoteNextCommitInfo match {
             case Left(_) =>
@@ -1816,16 +1792,13 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder, remo
 
     case Event(channelReestablish: ChannelReestablish, d: DATA_SHUTDOWN) =>
       Syncing.checkSync(keyManager, d, channelReestablish) match {
-        case res: SyncResult.InSync =>
-          val commitments1 = d.commitments.copy(
-            localChanges = d.commitments.localChanges.copy(proposed = Nil),
-            remoteChanges = d.commitments.remoteChanges.copy(proposed = Nil),
-            localNextHtlcId = d.commitments.localNextHtlcId - d.commitments.localChanges.proposed.collect { case u: UpdateAddHtlc => u }.size,
-            remoteNextHtlcId = d.commitments.remoteNextHtlcId - d.commitments.remoteChanges.proposed.collect { case u: UpdateAddHtlc => u }.size)
-          val sendQueue = Queue.empty[LightningMessage] ++ res.retransmit :+ d.localShutdown
+        case syncFailure: SyncResult.Failure =>
+          handleSyncFailure(channelReestablish, syncFailure, d)
+        case syncSuccess: SyncResult.Success =>
+          val commitments1 = Commitments.discardUnsignedUpdates(d.commitments)
+          val sendQueue = Queue.empty[LightningMessage] ++ syncSuccess.retransmit :+ d.localShutdown
           // BOLT 2: A node if it has sent a previous shutdown MUST retransmit shutdown.
           goto(SHUTDOWN) using d.copy(commitments = commitments1) sending sendQueue
-        case _ => ??? // TODO
       }
 
     case Event(_: ChannelReestablish, d: DATA_NEGOTIATING) =>
@@ -2295,6 +2268,29 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder, remo
     stay() using d.copy(channelUpdate = channelUpdate1) storing()
   }
 
+  private def handleSyncFailure(channelReestablish: ChannelReestablish, syncFailure: SyncResult.Failure, d: HasCommitments) = {
+    syncFailure match {
+      case res: SyncResult.LocalLateProven =>
+        log.warning(s"counterparty proved that we have an outdated (revoked) local commitment!!! ourLocalCommitmentNumber=${res.ourLocalCommitmentNumber} theirRemoteCommitmentNumber=${res.theirRemoteCommitmentNumber}")
+        // their data checks out, we indeed seem to be using an old revoked commitment, and must absolutely *NOT* publish it, because that would be a cheating attempt and they
+        // would punish us by taking all the funds in the channel
+        handleOutdatedCommitment(channelReestablish, d)
+      case res: Syncing.SyncResult.LocalLateUnproven =>
+        log.warning(s"our local commitment is in sync, but counterparty says that they have a more recent remote commitment than the one we know of (they could be lying)!!! ourRemoteCommitmentNumber=${res.ourRemoteCommitmentNumber} theirCommitmentNumber=${res.theirLocalCommitmentNumber}")
+        // there is no way to make sure that they are saying the truth, the best thing to do is ask them to publish their commitment right now
+        // maybe they will publish their commitment, in that case we need to remember their commitment point in order to be able to claim our outputs
+        // note that if they don't comply, we could publish our own commitment (it is not stale, otherwise we would be in the case above)
+        handleOutdatedCommitment(channelReestablish, d)
+      case res: Syncing.SyncResult.RemoteLying =>
+        // they are deliberately trying to fool us into thinking we have a late commitment
+        log.warning(s"counterparty is lying about us having an outdated commitment!!! ourLocalCommitmentNumber=${res.ourLocalCommitmentNumber} theirRemoteCommitmentNumber=${res.theirRemoteCommitmentNumber}")
+        handleLocalError(InvalidRevokedCommitProof(d.channelId, res.ourLocalCommitmentNumber, res.theirRemoteCommitmentNumber, res.invalidPerCommitmentSecret), d, Some(channelReestablish))
+      case SyncResult.RemoteLate =>
+        log.warning("counterparty appears to be using an outdated commitment, they may request a force-close, standing by...")
+        stay()
+    }
+  }
+
   private def maybeEmitChannelUpdateChangedEvent(newUpdate: ChannelUpdate, oldUpdate_opt: Option[ChannelUpdate], d: DATA_NORMAL): Unit = {
     if (oldUpdate_opt.isEmpty || !Announcements.areSameIgnoreFlags(newUpdate, oldUpdate_opt.get)) {
       context.system.eventStream.publish(ChannelUpdateParametersChanged(self, d.channelId, newUpdate.shortChannelId, d.commitments.remoteNodeId, newUpdate))

diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
@@ -875,6 +875,22 @@ object Commitments {
     (commitTx, htlcTxs)
   }
 
+  /**
+   * When reconnecting, we drop all outgoing unsigned changes.
+   */
+  def discardUnsignedUpdates(commitments: Commitments)(implicit log: LoggingAdapter): Commitments = {
+    log.debug("discarding proposed OUT: {}", commitments.localChanges.proposed.map(msg2String(_)).mkString(","))
+    log.debug("discarding proposed IN: {}", commitments.remoteChanges.proposed.map(msg2String(_)).mkString(","))
+    val commitments1 = commitments.copy(
+      localChanges = commitments.localChanges.copy(proposed = Nil),
+      remoteChanges = commitments.remoteChanges.copy(proposed = Nil),
+      localNextHtlcId = commitments.localNextHtlcId - commitments.localChanges.proposed.collect { case u: UpdateAddHtlc => u }.size,
+      remoteNextHtlcId = commitments.remoteNextHtlcId - commitments.remoteChanges.proposed.collect { case u: UpdateAddHtlc => u }.size)
+    log.debug(s"localNextHtlcId=${commitments.localNextHtlcId}->${commitments1.localNextHtlcId}")
+    log.debug(s"remoteNextHtlcId=${commitments.remoteNextHtlcId}->${commitments1.remoteNextHtlcId}")
+    commitments1
+  }
+
   def msg2String(msg: LightningMessage): String = msg match {
     case u: UpdateAddHtlc => s"add-${u.id}"
     case u: UpdateFulfillHtlc => s"ful-${u.id}"

diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala
@@ -318,11 +318,12 @@ object Helpers {
     // @formatter:off
     sealed trait SyncResult
     object SyncResult {
-      case class InSync(retransmit: Seq[LightningMessage]) extends SyncResult
-      case class LocalLateProven(ourLocalCommitmentNumber: Long, theirRemoteCommitmentNumber: Long) extends SyncResult
-      case class LocalLateUnproven(ourRemoteCommitmentNumber: Long, theirLocalCommitmentNumber: Long) extends SyncResult
-      case class RemoteLying(ourLocalCommitmentNumber: Long, theirRemoteCommitmentNumber: Long, invalidPerCommitmentSecret: PrivateKey) extends SyncResult
-      case object RemoteLate extends SyncResult
+      case class Success(retransmit: Seq[LightningMessage]) extends SyncResult
+      sealed trait Failure extends SyncResult
+      case class LocalLateProven(ourLocalCommitmentNumber: Long, theirRemoteCommitmentNumber: Long) extends Failure
+      case class LocalLateUnproven(ourRemoteCommitmentNumber: Long, theirLocalCommitmentNumber: Long) extends Failure
+      case class RemoteLying(ourLocalCommitmentNumber: Long, theirRemoteCommitmentNumber: Long, invalidPerCommitmentSecret: PrivateKey) extends Failure
+      case object RemoteLate extends Failure
     }
     // @formatter:on
 
@@ -346,15 +347,15 @@ object Helpers {
             val commitSig = waitingForRevocation.sent
             retransmitRevocation_opt match {
               case None =>
-                SyncResult.InSync(retransmit = signedUpdates :+ commitSig)
+                SyncResult.Success(retransmit = signedUpdates :+ commitSig)
               case Some(revocation) if d.commitments.localCommit.index > waitingForRevocation.sentAfterLocalCommitIndex =>
-                SyncResult.InSync(retransmit = signedUpdates :+ commitSig :+ revocation)
+                SyncResult.Success(retransmit = signedUpdates :+ commitSig :+ revocation)
               case Some(revocation) =>
-                SyncResult.InSync(retransmit = revocation +: signedUpdates :+ commitSig)
+                SyncResult.Success(retransmit = revocation +: signedUpdates :+ commitSig)
             }
           case Left(waitingForRevocation) if remoteChannelReestablish.nextLocalCommitmentNumber == (waitingForRevocation.nextRemoteCommit.index + 1) =>
             // we just sent a new commit_sig, they have received it but we haven't received their revocation
-            SyncResult.InSync(retransmit = retransmitRevocation_opt.toSeq)
+            SyncResult.Success(retransmit = retransmitRevocation_opt.toSeq)
           case Left(waitingForRevocation) if remoteChannelReestablish.nextLocalCommitmentNumber < waitingForRevocation.nextRemoteCommit.index =>
             // they are behind
             SyncResult.RemoteLate
@@ -366,7 +367,7 @@ object Helpers {
             )
           case Right(_) if remoteChannelReestablish.nextLocalCommitmentNumber == (d.commitments.remoteCommit.index + 1) =>
             // they have acknowledged the last commit_sig we sent
-            SyncResult.InSync(retransmit = retransmitRevocation_opt.toSeq)
+            SyncResult.Success(retransmit = retransmitRevocation_opt.toSeq)
           case Right(_) if remoteChannelReestablish.nextLocalCommitmentNumber < (d.commitments.remoteCommit.index + 1) =>
             // they are behind
             SyncResult.RemoteLate