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 <[email protected]>

Author: pm47

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)

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
       }

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