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)