From f4efd64ae5a9775b5cabc552fe1665a48efc60b9 Mon Sep 17 00:00:00 2001 From: Pierre-Marie Padiou Date: Fri, 8 Nov 2024 14:04:49 +0100 Subject: [PATCH] Don't relay buggy extra payments (#2937) If a payer is buggy and tries to pay the same invoice multiple times, it can lead to an edge case where the recipient accepted the first one and purchased liquidity for it, but didn't purchase additional liquidity and thus cannot receive the duplicate payments. Also, when replaying parts that were waiting for an on-the-fly funding, we set `commit = false` to all parts, instead of just the last one. This optimization caused the payment to be stuck if the last part was unexpectedly rejected (which would for example happen in the buggy payer case described above, before we rejected those extra payments). --------- Co-authored-by: t-bast --- .../main/scala/fr/acinq/eclair/io/Peer.scala | 10 +++++++--- .../payment/relay/OnTheFlyFunding.scala | 4 +--- .../payment/relay/OnTheFlyFundingSpec.scala | 20 +++++++++---------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala b/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala index 2843d1b9fb..611306659a 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala @@ -44,7 +44,7 @@ import fr.acinq.eclair.remote.EclairInternalsSerializer.RemoteTypes import fr.acinq.eclair.router.Router import fr.acinq.eclair.wire.protocol import fr.acinq.eclair.wire.protocol.FailureMessageCodecs.createBadOnionFailure -import fr.acinq.eclair.wire.protocol.{AddFeeCredit, ChannelTlv, CurrentFeeCredit, Error, HasChannelId, HasTemporaryChannelId, LightningMessage, LiquidityAds, NodeAddress, OnTheFlyFundingFailureMessage, OnionMessage, OnionRoutingPacket, RecommendedFeerates, RoutingMessage, SpliceInit, TlvStream, TxAbort, UnknownMessage, Warning, WillAddHtlc, WillFailHtlc, WillFailMalformedHtlc} +import fr.acinq.eclair.wire.protocol.{AddFeeCredit, ChannelTlv, CurrentFeeCredit, Error, HasChannelId, HasTemporaryChannelId, LightningMessage, LiquidityAds, NodeAddress, OnTheFlyFundingFailureMessage, OnionMessage, OnionRoutingPacket, RecommendedFeerates, RoutingMessage, SpliceInit, TemporaryChannelFailure, TlvStream, TxAbort, UnknownMessage, Warning, WillAddHtlc, WillFailHtlc, WillFailMalformedHtlc} /** * This actor represents a logical peer. There is one [[Peer]] per unique remote node id at all time. @@ -276,8 +276,12 @@ class Peer(val nodeParams: NodeParams, proposal.createFulfillCommands(status.preimage).foreach { case (channelId, cmd) => PendingCommandsDb.safeSend(register, nodeParams.db.pendingCommands, channelId, cmd) } pending.copy(proposed = pending.proposed :+ proposal) case status: OnTheFlyFunding.Status.Funded => - log.info("received extra payment for on-the-fly funding that has already been funded with txId={} (payment_hash={}, amount={})", status.txId, cmd.paymentHash, cmd.amount) - pending.copy(proposed = pending.proposed :+ OnTheFlyFunding.Proposal(htlc, cmd.upstream)) + log.info("rejecting extra payment for on-the-fly funding that has already been funded with txId={} (payment_hash={}, amount={})", status.txId, cmd.paymentHash, cmd.amount) + // The payer is buggy and is paying the same payment_hash multiple times. We could simply claim that + // extra payment for ourselves, but we're nice and instead immediately fail it. + val proposal = OnTheFlyFunding.Proposal(htlc, cmd.upstream) + proposal.createFailureCommands(None).foreach { case (channelId, cmd) => PendingCommandsDb.safeSend(register, nodeParams.db.pendingCommands, channelId, cmd) } + pending } case None => self ! Peer.OutgoingMessage(htlc, d.peerConnection) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/OnTheFlyFunding.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/OnTheFlyFunding.scala index 7ec46939c6..b12316ea62 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/OnTheFlyFunding.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/OnTheFlyFunding.scala @@ -292,9 +292,7 @@ object OnTheFlyFunding { // This lets us detect that this HTLC is an on-the-fly funded HTLC. val htlcFees = LiquidityAds.FundingFee(remainingFees.min(p.maxFees(htlcMinimum)), cmd.status.txId) val origin = Origin.Hot(htlcSettledAdapter.toClassic, p.upstream) - // We only sign at the end of the whole batch. - val commit = p.htlc.id == cmd.proposed.last.htlc.id - val add = CMD_ADD_HTLC(cmdAdapter.toClassic, p.htlc.amount - htlcFees.amount, paymentHash, p.htlc.expiry, p.htlc.finalPacket, p.htlc.blinding_opt, 1.0, Some(htlcFees), origin, commit) + val add = CMD_ADD_HTLC(cmdAdapter.toClassic, p.htlc.amount - htlcFees.amount, paymentHash, p.htlc.expiry, p.htlc.finalPacket, p.htlc.blinding_opt, 1.0, Some(htlcFees), origin, commit = true) cmd.channel ! add remainingFees - htlcFees.amount } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/relay/OnTheFlyFundingSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/relay/OnTheFlyFundingSpec.scala index f538decb78..00ff39e944 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/relay/OnTheFlyFundingSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/relay/OnTheFlyFundingSpec.scala @@ -381,6 +381,7 @@ class OnTheFlyFundingSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike { proposeFunding(40_000_000 msat, CltvExpiry(515), paymentHash2, upstream2.head) signLiquidityPurchase(100_000 sat, LiquidityAds.PaymentDetails.FromFutureHtlc(paymentHash2 :: Nil)) proposeExtraFunding(50_000_000 msat, CltvExpiry(525), paymentHash2, upstream2.last) + register.expectMsgType[Register.Forward[CMD_FAIL_HTLC]] // A third funding is signed coming from a trampoline payment. val paymentHash3 = randomBytes32() @@ -396,16 +397,16 @@ class OnTheFlyFundingSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike { val upstream4 = Upstream.Hot.Trampoline(List(upstreamChannel(60_000_000 msat, CltvExpiry(560), paymentHash4))) proposeFunding(50_000_000 msat, CltvExpiry(516), paymentHash4, upstream4) - // The first three proposals reach their CLTV expiry. + // The first three proposals reach their CLTV expiry (the extra htlc was already failed). peer ! CurrentBlockHeight(BlockHeight(515)) - val fwds = (0 until 6).map(_ => register.expectMsgType[Register.Forward[CMD_FAIL_HTLC]]) + val fwds = (0 until 5).map(_ => register.expectMsgType[Register.Forward[CMD_FAIL_HTLC]]) register.expectNoMessage(100 millis) fwds.foreach(fwd => { assert(fwd.message.reason == Right(UnknownNextPeer())) assert(fwd.message.commit) }) - assert(fwds.map(_.channelId).toSet == (upstream1 ++ upstream2 ++ upstream3.received).map(_.add.channelId).toSet) - assert(fwds.map(_.message.id).toSet == (upstream1 ++ upstream2 ++ upstream3.received).map(_.add.id).toSet) + assert(fwds.map(_.channelId).toSet == (upstream1 ++ upstream2.slice(0, 1) ++ upstream3.received).map(_.add.channelId).toSet) + assert(fwds.map(_.message.id).toSet == (upstream1 ++ upstream2.slice(0, 1) ++ upstream3.received).map(_.add.id).toSet) awaitCond(nodeParams.db.liquidity.listPendingOnTheFlyFunding(remoteNodeId).isEmpty, interval = 100 millis) } @@ -848,6 +849,7 @@ class OnTheFlyFundingSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike { val purchase = signLiquidityPurchase(200_000 sat, LiquidityAds.PaymentDetails.FromFutureHtlcWithPreimage(preimage :: Nil), channelId, fees, fundingTxIndex = 5, htlcMinimum) // We receive the last payment *after* signing the funding transaction. proposeExtraFunding(50_000_000 msat, expiryOut, paymentHash, upstream(2)) + register.expectMsgType[Register.Forward[CMD_FAIL_HTLC]] // Once the splice with the right funding index is locked, we forward HTLCs matching the proposed will_add_htlc. val channelData = makeChannelData(htlcMinimum) @@ -858,17 +860,15 @@ class OnTheFlyFundingSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike { val adds1 = Seq( channel.expectMsgType[CMD_ADD_HTLC], channel.expectMsgType[CMD_ADD_HTLC], - channel.expectMsgType[CMD_ADD_HTLC], ) adds1.foreach(add => { assert(add.paymentHash == paymentHash) assert(add.fundingFee_opt.nonEmpty) assert(add.fundingFee_opt.get.fundingTxId == purchase.txId) }) - adds1.take(2).foreach(add => assert(!add.commit)) - assert(adds1.last.commit) + adds1.foreach(add => assert(add.commit)) assert(adds1.map(_.fundingFee_opt.get.amount).sum == fees.total.toMilliSatoshi) - assert(adds1.map(add => add.amount + add.fundingFee_opt.get.amount).sum == 160_000_000.msat) + assert(adds1.map(add => add.amount + add.fundingFee_opt.get.amount).sum == 110_000_000.msat) channel.expectNoMessage(100 millis) // The recipient fails the payments: we don't relay the failure upstream and will retry. @@ -887,7 +887,6 @@ class OnTheFlyFundingSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike { val adds2 = Seq( channel.expectMsgType[CMD_ADD_HTLC], channel.expectMsgType[CMD_ADD_HTLC], - channel.expectMsgType[CMD_ADD_HTLC], ) adds2.foreach(add => add.replyTo ! RES_SUCCESS(add, purchase.channelId)) channel.expectNoMessage(100 millis) @@ -900,9 +899,8 @@ class OnTheFlyFundingSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike { val fwds = Seq( register.expectMsgType[Register.Forward[CMD_FULFILL_HTLC]], register.expectMsgType[Register.Forward[CMD_FULFILL_HTLC]], - register.expectMsgType[Register.Forward[CMD_FULFILL_HTLC]], ) - val (channelsIn, htlcsIn) = upstream.flatMap { + val (channelsIn, htlcsIn) = upstream.take(2).flatMap { case u: Hot.Channel => Seq(u) case u: Hot.Trampoline => u.received case _: Upstream.Local => Nil