Fail unsigned outgoing htlcs at CLOSING (#660)

This can happen in an unilateral close scenario, when local commit
"wins" the race to the blockchain, and some outgoing htlcs weren't yet
signed by remote.

This fixes #649.

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

@@ -536,6 +536,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
     case Event(fulfill: UpdateFulfillHtlc, d: DATA_NORMAL) =>
       Try(Commitments.receiveFulfill(d.commitments, fulfill)) match {
         case Success(Right((commitments1, origin, htlc))) =>
+          // NB: fulfills must be forwarded to the upstream channel asap, because they allow us to get money
           relayer ! ForwardFulfill(fulfill, origin, htlc)
           stay using d.copy(commitments = commitments1)
         case Success(Left(_)) => stay
@@ -561,6 +562,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
     case Event(fail: UpdateFailHtlc, d: DATA_NORMAL) =>
       Try(Commitments.receiveFail(d.commitments, fail)) match {
         case Success(Right((commitments1, origin, htlc))) =>
+          // TODO NB: fails must not be forwarded to the upstream channel before they are signed, because they cancel the incoming payment
           relayer ! ForwardFail(fail, origin, htlc)
           stay using d.copy(commitments = commitments1)
         case Success(Left(_)) => stay
@@ -570,6 +572,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
     case Event(fail: UpdateFailMalformedHtlc, d: DATA_NORMAL) =>
       Try(Commitments.receiveFailMalformed(d.commitments, fail)) match {
         case Success(Right((commitments1, origin, htlc))) =>
+          // TODO NB: fails must not be forwarded to the upstream channel before they are signed, because they cancel the incoming payment
           relayer ! ForwardFailMalformed(fail, origin, htlc)
           stay using d.copy(commitments = commitments1)
         case Success(Left(_)) => stay
@@ -886,6 +889,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
     case Event(fail: UpdateFailHtlc, d: DATA_SHUTDOWN) =>
       Try(Commitments.receiveFail(d.commitments, fail)) match {
         case Success(Right((commitments1, origin, htlc))) =>
+          // TODO NB: fails must not be forwarded to the upstream channel before they are signed, because they cancel the incoming payment
           relayer ! ForwardFail(fail, origin, htlc)
           stay using d.copy(commitments = commitments1)
         case Success(Left(_)) => stay
@@ -895,6 +899,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
     case Event(fail: UpdateFailMalformedHtlc, d: DATA_SHUTDOWN) =>
       Try(Commitments.receiveFailMalformed(d.commitments, fail)) match {
         case Success(Right((commitments1, origin, htlc))) =>
+          // TODO NB: fails must not be forwarded to the upstream channel before they are signed, because they cancel the incoming payment
           relayer ! ForwardFailMalformed(fail, origin, htlc)
           stay using d.copy(commitments = commitments1)
         case Success(Left(_)) => stay
@@ -1165,6 +1170,14 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
         log.warning(s"failing htlc #${add.id} paymentHash=${add.paymentHash} origin=$origin: htlc timed out")
         relayer ! Status.Failure(AddHtlcFailed(d.channelId, add.paymentHash, HtlcTimedout(d.channelId), origin, None, None))
       }
+      // we also need to fail outgoing htlcs that we know will never reach the blockchain
+      val overridenHtlcs =
+        Closing.overriddenHtlcs(d.commitments.localCommit, d.commitments.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit).getOrElse(d.commitments.remoteCommit), tx)
+      overridenHtlcs.foreach { add =>
+        val origin = d.commitments.originChannels(add.id)
+        log.warning(s"failing htlc #${add.id} paymentHash=${add.paymentHash} origin=$origin: overriden by local commit")
+        relayer ! Status.Failure(AddHtlcFailed(d.channelId, add.paymentHash, HtlcOverridenByLocalCommit(d.channelId), origin, None, None))
+      }
       // then let's see if any of the possible close scenarii can be considered done
       val mutualCloseDone = d.mutualClosePublished.exists(_.txid == tx.txid) // this case is trivial, in a mutual close scenario we only need to make sure that one of the closing txes is confirmed
     val localCommitDone = localCommitPublished1.map(Closing.isLocalCommitDone(_)).getOrElse(false)

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

@@ -49,6 +49,7 @@ case class InvalidFinalScript                  (override val channelId: BinaryDa
 case class FundingTxTimedout                   (override val channelId: BinaryData) extends ChannelException(channelId, "funding tx timed out")
 case class FundingTxSpent                      (override val channelId: BinaryData, spendingTx: Transaction) extends ChannelException(channelId, s"funding tx has been spent by txid=${spendingTx.txid}")
 case class HtlcTimedout                        (override val channelId: BinaryData) extends ChannelException(channelId, "one or more htlcs timed out")
+case class HtlcOverridenByLocalCommit          (override val channelId: BinaryData) extends ChannelException(channelId, "htlc was overriden by local commit")
 case class FeerateTooSmall                     (override val channelId: BinaryData, remoteFeeratePerKw: Long) extends ChannelException(channelId, s"remote fee rate is too small: remoteFeeratePerKw=$remoteFeeratePerKw")
 case class FeerateTooDifferent                 (override val channelId: BinaryData, localFeeratePerKw: Long, remoteFeeratePerKw: Long) extends ChannelException(channelId, s"local/remote feerates are too different: remoteFeeratePerKw=$remoteFeeratePerKw localFeeratePerKw=$localFeeratePerKw")
 case class InvalidCommitmentSignature          (override val channelId: BinaryData, tx: Transaction) extends ChannelException(channelId, s"invalid commitment signature: tx=$tx")

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

@@ -674,6 +674,25 @@ object Helpers {
         }).toSet.flatten
       }
 
+
+    /**
+      * If a local commitment tx reaches min_depth, we need to fail the outgoing htlcs that only us had signed, because
+      * they will never reach the blockchain.
+      *
+      * Those are only present in the remote's commitment.
+      *
+      * @param localCommit
+      * @param remoteCommit
+      * @param tx
+      * @param log
+      * @return
+      */
+    def overriddenHtlcs(localCommit: LocalCommit, remoteCommit: RemoteCommit, tx: Transaction)(implicit log: LoggingAdapter): Set[UpdateAddHtlc] =
+      if (localCommit.publishableTxs.commitTx.tx.txid == tx.txid) {
+        // NB: from the p.o.v of remote, their incoming htlcs are our outgoing htlcs
+        remoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add) -- localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add)
+      } else Set.empty
+
     /**
       * In CLOSING state, when we are notified that a transaction has been confirmed, we check if this tx belongs in the
       * local commit scenario and keep track of it.

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala

@@ -16,6 +16,7 @@
 
 package fr.acinq.eclair.channel.states.h
 
+import akka.actor.Status
 import akka.actor.Status.Failure
 import akka.testkit.{TestFSMRef, TestProbe}
 import fr.acinq.bitcoin.{OutPoint, ScriptFlags, Transaction, TxIn}
@@ -173,7 +174,7 @@ class ClosingStateSpec extends TestkitBaseClass with StateTestsHelperMethods {
     }
   }
 
-  test("recv BITCOIN_FUNDING_SPENT (our commit)") { case (alice, _, _, _, alice2blockchain, _, _, _) =>
+  test("recv BITCOIN_FUNDING_SPENT (local commit)") { case (alice, _, _, _, alice2blockchain, _, _, _) =>
     within(30 seconds) {
       // an error occurs and alice publishes her commit tx
       val aliceCommitTx = alice.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.publishableTxs.commitTx.tx
@@ -259,7 +260,36 @@ class ClosingStateSpec extends TestkitBaseClass with StateTestsHelperMethods {
     }
   }
 
-  test("recv BITCOIN_FUNDING_SPENT (their commit)") { case (alice, bob, alice2bob, bob2alice, alice2blockchain, bob2blockchain, _, bobCommitTxes) =>
+  test("recv BITCOIN_TX_CONFIRMED (local commit with htlcs only signed by local)") { case (alice, bob, alice2bob, bob2alice, alice2blockchain, _, relayer, _) =>
+    within(30 seconds) {
+      val sender = TestProbe()
+      // an error occurs and alice publishes her commit tx
+      val aliceData = alice.stateData.asInstanceOf[DATA_NORMAL]
+      val aliceCommitTx = aliceData.commitments.localCommit.publishableTxs.commitTx.tx
+      // alice sends an htlc
+      val (r, htlc) = addHtlc(4200000, alice, bob, alice2bob, bob2alice)
+      // and signs it (but bob doesn't sign it)
+      sender.send(alice, CMD_SIGN)
+      sender.expectMsg("ok")
+      alice2bob.expectMsgType[CommitSig]
+      // then we make alice unilaterally close the channel
+      alice ! Error("00" * 32, "oops".getBytes)
+      alice2blockchain.expectMsg(PublishAsap(aliceCommitTx)) // commit tx
+      awaitCond(alice.stateName == CLOSING)
+      val initialState = alice.stateData.asInstanceOf[DATA_CLOSING]
+      assert(initialState.localCommitPublished.isDefined)
+      relayer.expectMsgType[LocalChannelDown]
+
+      // actual test starts here
+      // when the commit tx is signed, alice knows that the htlc she sent right before the unilateral close will never reach the chain
+      alice ! WatchEventConfirmed(BITCOIN_TX_CONFIRMED(aliceCommitTx), 0, 0)
+      // so she fails it
+      val origin = alice.stateData.asInstanceOf[DATA_CLOSING].commitments.originChannels(htlc.id)
+      relayer.expectMsg(Status.Failure(AddHtlcFailed(aliceData.channelId, htlc.paymentHash, HtlcOverridenByLocalCommit(aliceData.channelId), origin, None, None)))
+    }
+  }
+
+  test("recv BITCOIN_FUNDING_SPENT (remote commit)") { case (alice, bob, alice2bob, bob2alice, alice2blockchain, bob2blockchain, _, bobCommitTxes) =>
     within(30 seconds) {
       mutualClose(alice, bob, alice2bob, bob2alice, alice2blockchain, bob2blockchain)
       val initialState = alice.stateData.asInstanceOf[DATA_CLOSING]