Skip to content

Commit

Permalink
Relay HTLC failure when inactive revoked commit confirms (#2801)
Browse files Browse the repository at this point in the history 
When an inactive revoked commitment containing outgoing HTLCs
is confirmed, we must consider the outgoing HTLC failed and relay that
failure upstream. Because of the alternative commit tx mechanism,
those inactive revoked commitments match the remote commit,
so we were waiting for tx confirmation before failing back upstream.
  • Loading branch information
@t-bast
t-bast authored Jan 2, 2024
1 parent 3a49f5d commit 61f1e1f
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 24 deletions.
39 changes: 24 additions & 15 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1342,6 +1342,12 @@ object Helpers {
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
htlcsInRemoteCommit.collect(incoming) -- localCommit.spec.htlcs.collect(outgoing)
} else if (d.revokedCommitPublished.map(_.commitTx.txid).contains(tx.txid)) {
// a revoked commitment got confirmed: we will claim its outputs, but we also need to fail htlcs that are pending in the latest commitment:
// - outgoing htlcs that are in the local commitment but not in remote/nextRemote have already been fulfilled/failed so we don't care about them
// - outgoing htlcs that are in the remote/nextRemote commitment may not really be overridden, but since we are going to claim their output as a
// punishment we will never get the preimage and may as well consider them failed in the context of relaying htlcs
nextRemoteCommit_opt.getOrElse(remoteCommit).spec.htlcs.collect(incoming)
} else if (remoteCommit.txid == tx.txid) {
// their commit got confirmed
nextRemoteCommit_opt match {
Expand All @@ -1354,26 +1360,29 @@ object Helpers {
Set.empty
}
} else if (nextRemoteCommit_opt.map(_.txid).contains(tx.txid)) {
// incoming htlcs that have been removed from their commitment are either fulfilled or failed:
// - if they were fulfilled, we already relayed the preimage upstream
// - if they were failed, we need to relay the failure upstream since those htlcs will never reach the chain
val settledHtlcs = remoteCommit.spec.htlcs.collect(incoming) -- nextRemoteCommit_opt.map(_.spec.htlcs.collect(incoming)).getOrElse(Set.empty)
val failedHtlcs = d.commitments.latest.changes.remoteChanges.all.collect {
case f: UpdateFailHtlc => f.id
case f: UpdateFailMalformedHtlc => f.id
}.toSet
settledHtlcs.filter(htlc => failedHtlcs.contains(htlc.id))
} else if (d.revokedCommitPublished.map(_.commitTx.txid).contains(tx.txid)) {
// a revoked commitment got confirmed: we will claim its outputs, but we also need to fail htlcs that are pending in the latest commitment:
// - outgoing htlcs that are in the local commitment but not in remote/nextRemote have already been fulfilled/failed so we don't care about them
// - outgoing htlcs that are in the remote/nextRemote commitment may not really be overridden, but since we are going to claim their output as a
// punishment we will never get the preimage and may as well consider them failed in the context of relaying htlcs
nextRemoteCommit_opt.getOrElse(remoteCommit).spec.htlcs.collect(incoming)
// we must fail htlcs that have been removed from the next commitment
recentlyFailedHtlcs(remoteCommit, nextRemoteCommit_opt, d.commitments.changes)
} else {
Set.empty
}
}

/**
* Returns HTLCs that have been failed and removed from the next remote commitment.
* We need to propagate their failure upstream if we don't receive the remote signature to remove them from our local commitment.
*/
def recentlyFailedHtlcs(remoteCommit: RemoteCommit, nextRemoteCommit_opt: Option[RemoteCommit], changes: CommitmentChanges): Set[UpdateAddHtlc] = {
// Incoming htlcs that have been removed from their commitment are either fulfilled or failed:
// - if they were fulfilled, we already relayed the preimage upstream
// - if they were failed, we need to relay the failure upstream since those htlcs will never reach the chain
val settledHtlcs = remoteCommit.spec.htlcs.collect(incoming) -- nextRemoteCommit_opt.map(_.spec.htlcs.collect(incoming)).getOrElse(Set.empty)
val failedHtlcs = changes.remoteChanges.all.collect {
case f: UpdateFailHtlc => f.id
case f: UpdateFailMalformedHtlc => f.id
}.toSet
settledHtlcs.filter(htlc => failedHtlcs.contains(htlc.id))
}

/**
* 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.
Expand Down
21 changes: 16 additions & 5 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1617,20 +1617,31 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
active = commitment +: Nil,
inactive = Nil
)
// we reset the state
// We reset the state to match the commitment that confirmed.
val d1 = d.copy(commitments = commitments1)
// This commitment may be revoked: we need to verify that its index matches our latest known index before overwriting our previous commitments.
if (commitment.localCommit.commitTxAndRemoteSig.commitTx.tx.txid == tx.txid) {
// our local commit has been published from the outside, it's unexpected but let's deal with it anyway
// Our local commit has been published from the outside, it's unexpected but let's deal with it anyway.
spendLocalCurrent(d1)
} else if (commitment.remoteCommit.txid == tx.txid && commitment.remoteCommit.index == d.commitments.remoteCommitIndex) {
// counterparty may attempt to spend its last commit tx at any time
handleRemoteSpentCurrent(tx, d1)
} else if (commitment.nextRemoteCommit_opt.exists(_.commit.txid == tx.txid) && commitment.remoteCommit.index == d.commitments.remoteCommitIndex && d.commitments.remoteNextCommitInfo.isLeft) {
// counterparty may attempt to spend its last commit tx at any time
handleRemoteSpentNext(tx, d1)
} else {
// counterparty may attempt to spend a revoked commit tx at any time
// Our counterparty is trying to broadcast a revoked commit tx (cheating attempt).
// We need to fail pending outgoing HTLCs: we must do it here because we're overwriting the commitments data, so we won't be able to do it afterwards.
val remoteCommit = d.commitments.latest.remoteCommit
val nextRemoteCommit_opt = d.commitments.latest.nextRemoteCommit_opt.map(_.commit)
val pendingOutgoingHtlcs = nextRemoteCommit_opt.getOrElse(remoteCommit).spec.htlcs.collect(DirectedHtlc.incoming)
val failedHtlcs = Closing.recentlyFailedHtlcs(remoteCommit, nextRemoteCommit_opt, d.commitments.changes)
(pendingOutgoingHtlcs ++ failedHtlcs).foreach { add =>
d.commitments.originChannels.get(add.id) match {
case Some(origin) =>
log.info(s"failing htlc #${add.id} paymentHash=${add.paymentHash} origin=$origin: overridden by revoked remote commit")
relayer ! RES_ADD_SETTLED(origin, add, HtlcResult.OnChainFail(HtlcOverriddenByLocalCommit(d.channelId, add)))
case None => ()
}
}
handleRemoteSpentOther(tx, d1)
}
case None =>
Expand Down
26 changes: 22 additions & 4 deletions eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import fr.acinq.eclair.channel.publish.TxPublisher.{PublishFinalTx, PublishRepla
import fr.acinq.eclair.channel.states.ChannelStateTestsBase.{FakeTxPublisherFactory, PimpTestFSM}
import fr.acinq.eclair.channel.states.ChannelStateTestsTags.{AnchorOutputsZeroFeeHtlcTxs, NoMaxHtlcValueInFlight, ZeroConf}
import fr.acinq.eclair.channel.states.{ChannelStateTestsBase, ChannelStateTestsTags}
import fr.acinq.eclair.payment.relay.Relayer
import fr.acinq.eclair.testutils.PimpTestProbe.convert
import fr.acinq.eclair.transactions.DirectedHtlc.{incoming, outgoing}
import fr.acinq.eclair.transactions.Transactions
Expand Down Expand Up @@ -188,6 +189,11 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
assert(initialState.commitments.latest.localCommit.spec.toLocal == 770_000_000.msat)
assert(initialState.commitments.latest.localCommit.spec.toRemote == 665_000_000.msat)

alice2relayer.expectMsgType[Relayer.RelayForward]
alice2relayer.expectMsgType[Relayer.RelayForward]
bob2relayer.expectMsgType[Relayer.RelayForward]
bob2relayer.expectMsgType[Relayer.RelayForward]

TestHtlcs(htlcsAliceToBob, htlcsBobToAlice)
} else {
val initialState = alice.stateData.asInstanceOf[DATA_NORMAL]
Expand Down Expand Up @@ -1604,6 +1610,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
// bob makes a payment
val (preimage, add) = addHtlc(10_000_000 msat, bob, alice, bob2alice, alice2bob)
crossSign(bob, alice, bob2alice, alice2bob)
alice2relayer.expectMsgType[Relayer.RelayForward]
fulfillHtlc(add.id, preimage, alice, bob, alice2bob, bob2alice)
crossSign(alice, bob, alice2bob, bob2alice)

Expand Down Expand Up @@ -1642,6 +1649,8 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
alice ! WatchTxConfirmedTriggered(BlockHeight(400000), 42, aliceMainPenalty)
// alice's htlc-penalty txs confirm
aliceHtlcsPenalty.foreach { tx => alice ! WatchTxConfirmedTriggered(BlockHeight(400000), 42, tx) }
val settledOutgoingHtlcs = htlcs.aliceToBob.map(_ => alice2relayer.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.OnChainFail]].htlc).toSet
assert(settledOutgoingHtlcs == htlcs.aliceToBob.map(_._2).toSet)

checkPostSpliceState(f, spliceOutFee = 0.sat)

Expand Down Expand Up @@ -1818,8 +1827,16 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
// bob makes a payment that is only applied to splice 2
val (preimage, add) = addHtlc(10_000_000 msat, bob, alice, bob2alice, alice2bob)
crossSign(bob, alice, bob2alice, alice2bob)
alice2relayer.expectMsgType[Relayer.RelayForward]
fulfillHtlc(add.id, preimage, alice, bob, alice2bob, bob2alice)
crossSign(alice, bob, alice2bob, bob2alice)
bob2relayer.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.OnChainFail]]

// alice adds an outgoing htlc that is only applied to splice 2
val pendingOutgoingHtlc = addHtlc(15_000_000 msat, alice, bob, alice2bob, bob2alice)
crossSign(alice, bob, alice2bob, bob2alice)
bob2relayer.expectMsgType[Relayer.RelayForward]
val htlcs1 = htlcs.copy(aliceToBob = htlcs.aliceToBob ++ Seq(pendingOutgoingHtlc))

// funding tx1 confirms
alice ! WatchFundingConfirmedTriggered(BlockHeight(400000), 42, fundingTx1)
Expand All @@ -1835,13 +1852,13 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
val aliceCommitTx2 = assertPublished(alice2blockchain, "commit-tx")
assertPublished(alice2blockchain, "local-anchor")
val claimMainDelayed2 = assertPublished(alice2blockchain, "local-main-delayed")
htlcs.aliceToBob.map(_ => assertPublished(alice2blockchain, "htlc-timeout"))
htlcs1.aliceToBob.map(_ => assertPublished(alice2blockchain, "htlc-timeout"))

alice2blockchain.expectWatchTxConfirmed(aliceCommitTx2.txid)
alice2blockchain.expectWatchTxConfirmed(claimMainDelayed2.txid)
alice2blockchain.expectMsgType[WatchOutputSpent] // local-anchor
htlcs.aliceToBob.foreach(_ => assert(alice2blockchain.expectMsgType[WatchOutputSpent].txId == aliceCommitTx2.txid))
htlcs.bobToAlice.foreach(_ => assert(alice2blockchain.expectMsgType[WatchOutputSpent].txId == aliceCommitTx2.txid))
htlcs1.aliceToBob.foreach(_ => assert(alice2blockchain.expectMsgType[WatchOutputSpent].txId == aliceCommitTx2.txid))
htlcs1.bobToAlice.foreach(_ => assert(alice2blockchain.expectMsgType[WatchOutputSpent].txId == aliceCommitTx2.txid))
alice2blockchain.expectNoMessage(100 millis)

// bob's revoked tx wins
Expand All @@ -1850,7 +1867,6 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
val aliceClaimMain = assertPublished(alice2blockchain, "remote-main-delayed")
val aliceMainPenalty = assertPublished(alice2blockchain, "main-penalty")
val aliceHtlcsPenalty = htlcs.aliceToBob.map(_ => assertPublished(alice2blockchain, "htlc-penalty")) ++ htlcs.bobToAlice.map(_ => assertPublished(alice2blockchain, "htlc-penalty"))

alice2blockchain.expectWatchTxConfirmed(bobRevokedCommitTx.txid)
alice2blockchain.expectWatchTxConfirmed(aliceClaimMain.txid)
assert(alice2blockchain.expectMsgType[WatchOutputSpent].txId == bobRevokedCommitTx.txid) // main-penalty
Expand All @@ -1864,6 +1880,8 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
alice2blockchain.expectWatchTxConfirmed(aliceMainPenalty.txid)
alice ! WatchTxConfirmedTriggered(BlockHeight(400000), 42, aliceMainPenalty)
aliceHtlcsPenalty.foreach { tx => alice ! WatchTxConfirmedTriggered(BlockHeight(400000), 42, tx) }
val settledOutgoingHtlcs = htlcs1.aliceToBob.map(_ => alice2relayer.expectMsgType[RES_ADD_SETTLED[Origin, HtlcResult.OnChainFail]].htlc).toSet
assert(settledOutgoingHtlcs == htlcs1.aliceToBob.map(_._2).toSet)

// alice's final commitment includes the initial htlcs, but not bob's payment
checkPostSpliceState(f, spliceOutFee = 0 sat)
Expand Down

0 comments on commit 61f1e1f

Please sign in to comment.