Relay partially failed htlcs when closing (#1706)

If a channel closes when we've received an UpdateFailHtlc, signed it but not yet received our peer's revocation, we need to fail the htlc upstream. That specific scenario was not correctly handled, resulting in upstream htlcs that were not failed which would force our upstream peer to close the channel.
ACINQ · Feb 24, 2021 · bf2a35f · bf2a35f
1 parent a3c477e
commit bf2a35f
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 3 deletions.
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
@@ -954,10 +954,10 @@ object Helpers {
       val remoteCommit = d.commitments.remoteCommit
       val nextRemoteCommit_opt = d.commitments.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit)
       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)
+        // our commit got confirmed, so any htlc that is in their commitment but not in ours will never reach the chain
+        val htlcsInRemoteCommit = remoteCommit.spec.htlcs ++ nextRemoteCommit_opt.map(_.spec.htlcs).getOrElse(Set.empty)
         // NB: from the p.o.v of remote, their incoming htlcs are our outgoing htlcs
-        mostRecentRemoteCommit.spec.htlcs.collect(incoming) -- localCommit.spec.htlcs.collect(outgoing)
+        htlcsInRemoteCommit.collect(incoming) -- localCommit.spec.htlcs.collect(outgoing)
       } else if (remoteCommit.txid == tx.txid) {
         // their commit got confirmed
         nextRemoteCommit_opt match {

diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala
@@ -517,6 +517,39 @@ class ClosingStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(closingState.claimHtlcDelayedTxs.size === 1)
   }
 
+  test("recv BITCOIN_TX_CONFIRMED (local commit with fail not acked by remote)") { f =>
+    import f._
+    val listener = TestProbe()
+    system.eventStream.subscribe(listener.ref, classOf[PaymentSettlingOnChain])
+    val (_, htlc) = addHtlc(25000000 msat, alice, bob, alice2bob, bob2alice)
+    crossSign(alice, bob, alice2bob, bob2alice)
+    failHtlc(htlc.id, bob, alice, bob2alice, alice2bob)
+    bob ! CMD_SIGN()
+    bob2alice.expectMsgType[CommitSig]
+    bob2alice.forward(alice)
+    alice2bob.expectMsgType[RevokeAndAck]
+    alice2bob.forward(bob)
+    alice2bob.expectMsgType[CommitSig]
+    alice2bob.forward(bob)
+    bob2alice.expectMsgType[RevokeAndAck]
+    // note that alice doesn't receive the last revocation
+    // then we make alice unilaterally close the channel
+    val closingState = localClose(alice, alice2blockchain)
+    assert(closingState.commitTx.txOut.length === 2) // htlc has been removed
+
+    // actual test starts here
+    channelUpdateListener.expectMsgType[LocalChannelDown]
+    assert(closingState.htlcSuccessTxs.isEmpty && closingState.htlcTimeoutTxs.isEmpty && closingState.claimHtlcDelayedTxs.isEmpty)
+    // when the commit tx is confirmed, alice knows that the htlc will never reach the chain
+    alice ! WatchEventConfirmed(BITCOIN_TX_CONFIRMED(closingState.commitTx), 0, 0, closingState.commitTx)
+    // so she fails it
+    val origin = alice.stateData.asInstanceOf[DATA_CLOSING].commitments.originChannels(htlc.id)
+    relayerA.expectMsg(RES_ADD_SETTLED(origin, htlc, HtlcResult.OnChainFail(HtlcOverriddenByLocalCommit(channelId(alice), htlc))))
+    // the htlc will not settle on chain
+    listener.expectNoMsg(2 seconds)
+    relayerA.expectNoMsg(100 millis)
+  }
+
   test("recv BITCOIN_TX_CONFIRMED (remote commit with htlcs only signed by local in next remote commit)") { f =>
     import f._
     val listener = TestProbe()