Skip to content

Don't relay buggy extra payments #2937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pm47 merged 4 commits into from
Nov 8, 2024
Merged

Don't relay buggy extra payments #2937

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5438bb
Don't relay buggy extra payments
t-bast Nov 1, 2024
0e04e72
fix tests
pm47 Nov 6, 2024
2a8b5b0
remove superfluous signature optimization
pm47 Nov 6, 2024
7f74bb5
fixup! fix tests
pm47 Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 1 addition & 3 deletions eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/OnTheFlyFunding.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
21 changes: 10 additions & 11 deletions eclair-core/src/test/scala/fr/acinq/eclair/payment/relay/OnTheFlyFundingSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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)
}

Expand Down Expand Up @@ -848,6 +849,8 @@ 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)
Expand All @@ -858,17 +861,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.
Expand All @@ -887,7 +888,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)
Expand All @@ -900,9 +900,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
Expand Down
Loading