From 6d5ec8c4fa1e7485d15d8baa6afa165445f082ad Mon Sep 17 00:00:00 2001 From: Pierre-Marie Padiou Date: Tue, 31 Jul 2018 16:04:55 +0200 Subject: [PATCH] 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. --- .../fr/acinq/eclair/channel/Channel.scala | 13 +++++++ .../eclair/channel/ChannelExceptions.scala | 1 + .../fr/acinq/eclair/channel/Helpers.scala | 19 +++++++++++ .../channel/states/h/ClosingStateSpec.scala | 34 +++++++++++++++++-- 4 files changed, 65 insertions(+), 2 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala index 034d308e66..184057cf23 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala +++ b/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) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala index 0b5ac59221..e1fdea24bf 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala +++ b/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") 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 index 9139fc6345..b4b71044ca 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala +++ b/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. 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 index 5a16778355..c48f6f6279 100644 --- 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 @@ -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]