Fail unsigned outgoing htlcs on force-close (#1832)

Fail outgoing _unsigned_ htlcs on force close, just like we do when disconnected.

Fixes #1829.

Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

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

@@ -1038,12 +1038,13 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
     case Event(INPUT_DISCONNECTED, d: DATA_NORMAL) =>
       // we cancel the timer that would have made us send the enabled update after reconnection (flappy channel protection)
       cancelTimer(Reconnected.toString)
-      // if we have pending unsigned htlcs, then we cancel them and advertise the fact that the channel is now disabled
+      // if we have pending unsigned htlcs, then we cancel them and generate an update with the disabled flag set, that will be returned to the sender in a temporary channel failure
       val d1 = if (d.commitments.localChanges.proposed.collectFirst { case add: UpdateAddHtlc => add }.isDefined) {
         log.debug("updating channel_update announcement (reason=disabled)")
         val channelUpdate = Announcements.makeChannelUpdate(nodeParams.chainHash, nodeParams.privateKey, remoteNodeId, d.shortChannelId, d.channelUpdate.cltvExpiryDelta, d.channelUpdate.htlcMinimumMsat, d.channelUpdate.feeBaseMsat, d.channelUpdate.feeProportionalMillionths, d.commitments.capacity.toMilliSatoshi, enable = false)
+        // NB: the htlcs stay in the commitments.localChange, they will be cleaned up after reconnection
         d.commitments.localChanges.proposed.collect {
-          case add: UpdateAddHtlc => relayer ! RES_ADD_SETTLED(d.commitments.originChannels(add.id), add, HtlcResult.Disconnected(channelUpdate))
+          case add: UpdateAddHtlc => relayer ! RES_ADD_SETTLED(d.commitments.originChannels(add.id), add, HtlcResult.DisconnectedBeforeSigned(channelUpdate))
         }
         d.copy(channelUpdate = channelUpdate)
       } else {
@@ -1790,7 +1791,8 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
         case data: HasCommitments =>
           val replyTo = if (c.replyTo == ActorRef.noSender) sender else c.replyTo
           replyTo ! RES_SUCCESS(c, data.channelId)
-          handleLocalError(ForcedLocalCommit(data.channelId), data, Some(c))
+          val failure = ForcedLocalCommit(data.channelId)
+          handleLocalError(failure, data, Some(c))
         case _ => handleCommandError(CommandUnavailableInThisState(d.channelId, "forceclose", stateName), c)
       }
 
@@ -1888,12 +1890,14 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
       }
   }
 
+  /** Metrics */
   onTransition {
     case state -> nextState if state != nextState =>
       if (state != WAIT_FOR_INIT_INTERNAL) Metrics.ChannelsCount.withTag(Tags.State, state.toString).decrement()
       if (nextState != WAIT_FOR_INIT_INTERNAL) Metrics.ChannelsCount.withTag(Tags.State, nextState.toString).increment()
   }
 
+  /** Check pending settlement commands */
   onTransition {
     case _ -> CLOSING =>
       PendingCommandsDb.getSettlementCommands(nodeParams.db.pendingCommands, nextStateData.asInstanceOf[HasCommitments].channelId) match {
@@ -1914,6 +1918,17 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
       }
   }
 
+  /** Fail outgoing unsigned htlcs right away when transitioning from NORMAL to CLOSING */
+  onTransition {
+    case NORMAL -> CLOSING =>
+      nextStateData match {
+        case d: DATA_CLOSING =>
+          d.commitments.localChanges.proposed.collect {
+            case add: UpdateAddHtlc => relayer ! RES_ADD_SETTLED(d.commitments.originChannels(add.id), add, HtlcResult.ChannelFailureBeforeSigned)
+          }
+      }
+  }
+
   /*
           888    888        d8888 888b    888 8888888b.  888      8888888888 8888888b.   .d8888b.
           888    888       d88888 8888b   888 888  "Y88b 888      888        888   Y88b d88P  Y88b
@@ -2578,4 +2593,3 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
   initialize()
 
 }
-

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

@@ -214,8 +214,9 @@ object HtlcResult {
   sealed trait Fail extends HtlcResult
   case class RemoteFail(fail: UpdateFailHtlc) extends Fail
   case class RemoteFailMalformed(fail: UpdateFailMalformedHtlc) extends Fail
-  case class OnChainFail(cause: ChannelException) extends Fail
-  case class Disconnected(channelUpdate: ChannelUpdate) extends Fail { assert(!Announcements.isEnabled(channelUpdate.channelFlags), "channel update must have disabled flag set") }
+  case class OnChainFail(cause: Throwable) extends Fail
+  case object ChannelFailureBeforeSigned extends Fail
+  case class DisconnectedBeforeSigned(channelUpdate: ChannelUpdate) extends Fail { assert(!Announcements.isEnabled(channelUpdate.channelFlags), "channel update must have disabled flag set") }
 }
 final case class RES_ADD_SETTLED[+O <: Origin, +R <: HtlcResult](origin: O, htlc: UpdateAddHtlc, result: R) extends CommandSuccess[CMD_ADD_HTLC]
 

eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/ChannelRelay.scala

@@ -81,7 +81,8 @@ object ChannelRelay {
       case f: HtlcResult.RemoteFail => CMD_FAIL_HTLC(originHtlcId, Left(f.fail.reason), commit = true)
       case f: HtlcResult.RemoteFailMalformed => CMD_FAIL_MALFORMED_HTLC(originHtlcId, f.fail.onionHash, f.fail.failureCode, commit = true)
       case _: HtlcResult.OnChainFail => CMD_FAIL_HTLC(originHtlcId, Right(PermanentChannelFailure), commit = true)
-      case f: HtlcResult.Disconnected => CMD_FAIL_HTLC(originHtlcId, Right(TemporaryChannelFailure(f.channelUpdate)), commit = true)
+      case HtlcResult.ChannelFailureBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(PermanentChannelFailure), commit = true)
+      case f: HtlcResult.DisconnectedBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(TemporaryChannelFailure(f.channelUpdate)), commit = true)
     }
   }
 

eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala

@@ -114,7 +114,11 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
         case HtlcResult.OnChainFail(cause) =>
           // if the outgoing htlc is being resolved on chain, we treat it like a local error but we cannot retry
           handleLocalFail(d, cause, isFatal = true)
-        case HtlcResult.Disconnected(_) =>
+        case HtlcResult.ChannelFailureBeforeSigned =>
+          // the channel that we wanted to use for the outgoing htlc has failed before that htlc was signed, we may
+          // retry with another channel
+          handleLocalFail(d, ChannelFailureException, isFatal = false)
+        case HtlcResult.DisconnectedBeforeSigned(_) =>
           // a disconnection occured before the outgoing htlc got signed
           // again, we consider it a local error and treat is as such
           handleLocalFail(d, DisconnectedException, isFatal = false)
@@ -347,6 +351,7 @@ object PaymentLifecycle {
 
   /** custom exceptions to handle corner cases */
   case object UpdateMalformedException extends RuntimeException("first hop returned an UpdateFailMalformedHtlc message")
+  case object ChannelFailureException extends RuntimeException("a channel failure occured with the first hop")
   case object DisconnectedException extends RuntimeException("a disconnection occurred with the first hop")
   // @formatter:on
 

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

@@ -1915,6 +1915,21 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     awaitCond(alice.stateName == NORMAL)
   }
 
+  test("recv CMD_FORCECLOSE (with pending unsigned htlcs)") { f =>
+    import f._
+    val sender = TestProbe()
+    val (_, htlc1) = addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    val aliceData = alice.stateData.asInstanceOf[DATA_NORMAL]
+    assert(aliceData.commitments.localChanges.proposed.size == 1)
+
+    // actual test starts here
+    alice ! CMD_FORCECLOSE(sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_FORCECLOSE]]
+    val addSettled = relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.ChannelFailureBeforeSigned.type]]
+    assert(addSettled.htlc == htlc1)
+  }
+
   def testShutdown(f: FixtureParam, script_opt: Option[ByteVector]): Unit = {
     import f._
     val bobParams = bob.stateData.asInstanceOf[DATA_NORMAL].commitments.localParams
@@ -2395,6 +2410,22 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(claimFee == expectedFee)
   }
 
+  test("recv WatchFundingSpentTriggered (their commit w/ pending unsigned htlcs)") { f =>
+    import f._
+    val sender = TestProbe()
+    val (_, htlc1) = addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    val aliceData = alice.stateData.asInstanceOf[DATA_NORMAL]
+    assert(aliceData.commitments.localChanges.proposed.size == 1)
+
+    // actual test starts here
+    // bob publishes his current commit tx
+    val bobCommitTx = bob.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.commitTxAndRemoteSig.commitTx.tx
+    alice ! WatchFundingSpentTriggered(bobCommitTx)
+    val addSettled = relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.ChannelFailureBeforeSigned.type]]
+    assert(addSettled.htlc == htlc1)
+  }
+
   test("recv WatchFundingSpentTriggered (their *next* commit w/ htlc)") { f =>
     import f._
 
@@ -2456,6 +2487,29 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(getClaimHtlcTimeoutTxs(rcp).length === 2)
   }
 
+  test("recv WatchFundingSpentTriggered (their *next* commit w/ pending unsigned htlcs)") { f =>
+    import f._
+    val sender = TestProbe()
+    addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    // alice sign but we intercept bob's revocation
+    alice ! CMD_SIGN()
+    alice2bob.expectMsgType[CommitSig]
+    alice2bob.forward(bob)
+    bob2alice.expectMsgType[RevokeAndAck]
+    val (_, htlc2) = addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    val aliceData = alice.stateData.asInstanceOf[DATA_NORMAL]
+    assert(aliceData.commitments.localChanges.proposed.size == 1)
+
+    // actual test starts here
+    // bob publishes his current commit tx
+    val bobCommitTx = bob.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.commitTxAndRemoteSig.commitTx.tx
+    alice ! WatchFundingSpentTriggered(bobCommitTx)
+    val addSettled = relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.ChannelFailureBeforeSigned.type]]
+    assert(addSettled.htlc == htlc2)
+  }
+
   test("recv WatchFundingSpentTriggered (revoked commit)") { f =>
     import f._
     // initially we have :
@@ -2566,6 +2620,29 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     htlcPenaltyTxs.foreach(htlcPenaltyTx => Transaction.correctlySpends(htlcPenaltyTx, Seq(revokedTx), ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS))
   }
 
+  test("recv WatchFundingSpentTriggered (revoked commit w/ pending unsigned htlcs)") { f =>
+    import f._
+    val sender = TestProbe()
+    addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    crossSign(alice, bob, alice2bob, bob2alice)
+    val bobRevokedCommitTx = bob.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.commitTxAndRemoteSig.commitTx.tx
+    addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    crossSign(alice, bob, alice2bob, bob2alice)
+    val (_, htlc3) = addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    val aliceData = alice.stateData.asInstanceOf[DATA_NORMAL]
+    assert(aliceData.commitments.localChanges.proposed.size == 1)
+
+    // actual test starts here
+    // bob publishes his current commit tx
+
+    alice ! WatchFundingSpentTriggered(bobRevokedCommitTx)
+    val addSettled = relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.ChannelFailureBeforeSigned.type]]
+    assert(addSettled.htlc == htlc3)
+  }
+
   test("recv Error") { f =>
     import f._
     val (ra1, htlca1) = addHtlc(250000000 msat, alice, bob, alice2bob, bob2alice)
@@ -2654,6 +2731,20 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(localCommitPublished.commitTx.txid == bobCommitTx.txid)
   }
 
+  test("recv Error (with pending unsigned htlcs)") { f =>
+    import f._
+    val sender = TestProbe()
+    val (_, htlc1) = addHtlc(10000 msat, alice, bob, alice2bob, bob2alice, sender.ref)
+    sender.expectMsgType[RES_SUCCESS[CMD_ADD_HTLC]]
+    val aliceData = alice.stateData.asInstanceOf[DATA_NORMAL]
+    assert(aliceData.commitments.localChanges.proposed.size == 1)
+
+    // actual test starts here
+    alice ! Error(ByteVector32.Zeroes, "oops")
+    val addSettled = relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.ChannelFailureBeforeSigned.type]]
+    assert(addSettled.htlc == htlc1)
+  }
+
   test("recv WatchFundingDeeplyBuriedTriggered", Tag(StateTestsTags.ChannelsPublic)) { f =>
     import f._
     alice ! WatchFundingDeeplyBuriedTriggered(400000, 42, null)
@@ -2808,8 +2899,12 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     // actual test starts here
     Thread.sleep(1100)
     alice ! INPUT_DISCONNECTED
-    assert(relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.Disconnected]].htlc.paymentHash === htlc1.paymentHash)
-    assert(relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.Disconnected]].htlc.paymentHash === htlc2.paymentHash)
+    val addSettled1 = relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult]]
+    assert(addSettled1.htlc == htlc1)
+    assert(addSettled1.result.isInstanceOf[HtlcResult.DisconnectedBeforeSigned])
+    val addSettled2 = relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult]]
+    assert(addSettled2.htlc == htlc2)
+    assert(addSettled2.result.isInstanceOf[HtlcResult.DisconnectedBeforeSigned])
     assert(!Announcements.isEnabled(channelUpdateListener.expectMsgType[LocalChannelUpdate].channelUpdate.channelFlags))
     awaitCond(alice.stateName == OFFLINE)
   }
@@ -2851,8 +2946,8 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     // actual test starts here
     Thread.sleep(1100)
     alice ! INPUT_DISCONNECTED
-    assert(relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.Disconnected]].htlc.paymentHash === htlc1.paymentHash)
-    assert(relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.Disconnected]].htlc.paymentHash === htlc2.paymentHash)
+    assert(relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.DisconnectedBeforeSigned]].htlc.paymentHash === htlc1.paymentHash)
+    assert(relayerA.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.DisconnectedBeforeSigned]].htlc.paymentHash === htlc2.paymentHash)
     val update2a = channelUpdateListener.expectMsgType[LocalChannelUpdate]
     assert(update1a.channelUpdate.timestamp < update2a.channelUpdate.timestamp)
     assert(!Announcements.isEnabled(update2a.channelUpdate.channelFlags))

eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala

@@ -361,7 +361,7 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
 
     register.expectMsg(ForwardShortId(paymentFSM, channelId_ab, cmd1))
     val update_bc_disabled = update_bc.copy(channelFlags = Announcements.makeChannelFlags(isNode1 = true, enable = false))
-    sender.send(paymentFSM, addCompleted(HtlcResult.Disconnected(update_bc_disabled)))
+    sender.send(paymentFSM, addCompleted(HtlcResult.DisconnectedBeforeSigned(update_bc_disabled)))
 
     // then the payment lifecycle will ask for a new route excluding the channel
     routerForwarder.expectMsg(defaultRouteRequest(a, d, cfg).copy(ignore = Ignore(Set.empty, Set(ChannelDesc(channelId_ab, a, b)))))

eclair-core/src/test/scala/fr/acinq/eclair/payment/relay/ChannelRelayerSpec.scala

@@ -361,7 +361,8 @@ class ChannelRelayerSpec extends ScalaTestWithActorTestKit(ConfigFactory.load("a
       TestCase(HtlcResult.RemoteFail(UpdateFailHtlc(channelId1, downstream_htlc.id, hex"deadbeef")), CMD_FAIL_HTLC(r.add.id, Left(hex"deadbeef"), commit = true)),
       TestCase(HtlcResult.RemoteFailMalformed(UpdateFailMalformedHtlc(channelId1, downstream_htlc.id, ByteVector32.One, FailureMessageCodecs.BADONION)), CMD_FAIL_MALFORMED_HTLC(r.add.id, ByteVector32.One, FailureMessageCodecs.BADONION, commit = true)),
       TestCase(HtlcResult.OnChainFail(HtlcOverriddenByLocalCommit(channelId1, downstream_htlc)), CMD_FAIL_HTLC(r.add.id, Right(PermanentChannelFailure), commit = true)),
-      TestCase(HtlcResult.Disconnected(u_disabled.channelUpdate), CMD_FAIL_HTLC(r.add.id, Right(TemporaryChannelFailure(u_disabled.channelUpdate)), commit = true))
+      TestCase(HtlcResult.DisconnectedBeforeSigned(u_disabled.channelUpdate), CMD_FAIL_HTLC(r.add.id, Right(TemporaryChannelFailure(u_disabled.channelUpdate)), commit = true)),
+      TestCase(HtlcResult.ChannelFailureBeforeSigned, CMD_FAIL_HTLC(r.add.id, Right(PermanentChannelFailure), commit = true))
     )
 
     testCases.foreach { testCase =>