From b380b0099b439ac1a50ae39c82d2cdb1127ccc2f Mon Sep 17 00:00:00 2001 From: pm47 Date: Tue, 11 Sep 2018 17:35:21 +0200 Subject: [PATCH] handle overriden htlcs in remote close scenario 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 6d5ec8c4fa1e7485d15d8baa6afa165445f082ad. 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. --- .../fr/acinq/eclair/channel/Channel.scala | 3 +- .../fr/acinq/eclair/channel/Helpers.scala | 21 +++++++++-- .../channel/states/h/ClosingStateSpec.scala | 37 ++++++++++++++++--- 3 files changed, 50 insertions(+), 11 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 317986c937..25fc6ee0fa 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 @@ -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") 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 d83e8a8c0a..4e7c82aecd 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 @@ -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 /** 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 c48f6f6279..839b0d2720 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 @@ -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)