Handle overriden htlcs in remote close scenario

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)