Handle overriden htlcs in remote close scenario (#697)

When we just signed an outgoing htlc, it is only present in the next
remote commit (which will become the remote commit once the current one
is revoked).

If we unilaterally close the channel, and our commitment is confirmed,
then the htlc will never reach the chain, it has been "overriden" and
should be failed ASAP. This is correctly handled since
6d5ec8c.

But if remote unilaterally close the channel with its *current*
commitment (that doesn't contain the htlc), then the same thing happens:
the htlc is also "overriden", and we should fail it.

This fixes #691.

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

@@ -1172,8 +1172,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
         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)
+      val overridenHtlcs = Closing.overriddenHtlcs(d.commitments.localCommit, d.commitments.remoteCommit, d.commitments.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit), 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")

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

@@ -732,7 +732,6 @@ 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.
@@ -745,10 +744,26 @@ object Helpers {
       * @param log
       * @return
       */
-    def overriddenHtlcs(localCommit: LocalCommit, remoteCommit: RemoteCommit, tx: Transaction)(implicit log: LoggingAdapter): Set[UpdateAddHtlc] =
+    def overriddenHtlcs(localCommit: LocalCommit, remoteCommit: RemoteCommit, nextRemoteCommit_opt: Option[RemoteCommit], tx: Transaction)(implicit log: LoggingAdapter): Set[UpdateAddHtlc] =
       if (localCommit.publishableTxs.commitTx.tx.txid == tx.txid) {
+        // our commit got confirmed, so any htlc that we signed but they didn't sign will never reach the chain
+        val mostRecentRemoteCommit = nextRemoteCommit_opt.getOrElse(remoteCommit)
         // 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)
+        mostRecentRemoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add) -- localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add)
+      } else if (remoteCommit.txid == tx.txid) {
+        // their commit got confirmed
+        nextRemoteCommit_opt match {
+          case Some(nextRemoteCommit) =>
+            // we had signed a new commitment but they committed the previous one
+            // any htlc that we signed in the new commitment that they didn't sign will never reach the chain
+            nextRemoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add) -- localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add)
+          case None =>
+            // their last commitment got confirmed, so no htlcs will be overriden, they will timeout or be fulfilled on chain
+            Set.empty
+        }
+      } else if (nextRemoteCommit_opt.map(_.txid) == Some(tx.txid)) {
+        // their last commitment got confirmed, so no htlcs will be overriden, they will timeout or be fulfilled on chain
+        Set.empty
       } else Set.empty
 
     /**

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

@@ -263,21 +263,20 @@ class ClosingStateSpec extends TestkitBaseClass with StateTestsHelperMethods {
   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
+      val aliceCommitTx = alice.stateData.asInstanceOf[DATA_NORMAL].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]
+      // note that bob doesn't receive the new sig!
       // then we make alice unilaterally close the channel
       alice ! Error("00" * 32, "oops".getBytes)
-      alice2blockchain.expectMsg(PublishAsap(aliceCommitTx)) // commit tx
+      alice2blockchain.expectMsg(PublishAsap(aliceCommitTx))
       awaitCond(alice.stateName == CLOSING)
-      val initialState = alice.stateData.asInstanceOf[DATA_CLOSING]
-      assert(initialState.localCommitPublished.isDefined)
+      val aliceData = alice.stateData.asInstanceOf[DATA_CLOSING]
+      assert(aliceData.localCommitPublished.isDefined)
       relayer.expectMsgType[LocalChannelDown]
 
       // actual test starts here
@@ -289,6 +288,32 @@ class ClosingStateSpec extends TestkitBaseClass with StateTestsHelperMethods {
     }
   }
 
+  test("recv BITCOIN_TX_CONFIRMED (remote commit with htlcs only signed by local in next remote commit)") { case (alice, bob, alice2bob, bob2alice, _, bob2blockchain, relayer, _) =>
+    within(30 seconds) {
+      val sender = TestProbe()
+      val bobCommitTx = bob.stateData.asInstanceOf[DATA_NORMAL].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 believe bob unilaterally close the channel
+      alice ! WatchEventSpent(BITCOIN_FUNDING_SPENT, bobCommitTx)
+      awaitCond(alice.stateName == CLOSING)
+      val aliceData = alice.stateData.asInstanceOf[DATA_CLOSING]
+      assert(aliceData.remoteCommitPublished.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(bobCommitTx), 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)