From 374b659476ac46d5ebd02791e43c8f4011711e52 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 11 Jul 2024 11:55:26 +0200 Subject: [PATCH 1/5] routing: don't assume HTLC amount for custom channel It seems like the amount given to the getBandwidth is 0 in some cases (keysend) and non-zero in other cases (invoice payment). If we use custom channels, then that amount will likely not be the actual HTLC amount we're going to put on chain. So we have to use a zero amount for the MayAddOutgoingHtlc check, otherwise we might seem to go below the local channel reserve even though we'll end up sending a very small amount only. And since the check here is just to find out we're not already at the maximum HTLC capacity for this channel, we can safely use the amount of 0 for the check, which is handled properly in the link logic. --- routing/bandwidth.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/routing/bandwidth.go b/routing/bandwidth.go index 1fb96e8ef..fa133b21f 100644 --- a/routing/bandwidth.go +++ b/routing/bandwidth.go @@ -138,6 +138,13 @@ func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID, auxBandwidth lnwire.MilliSatoshi auxBandwidthDetermined bool + + // htlcAmount is the amount we're going to use to check if we + // can add another HTLC to the channel. If the external traffic + // shaper is handling the channel, we'll use 0 to just sanity + // check the number of HTLCs on the channel, since we don't know + // the actual HTLC amount that will be sent. + htlcAmount = amount ) err = fn.MapOptionZ(b.trafficShaper, func(ts TlvTrafficShaper) error { fundingBlob := link.FundingCustomBlob() @@ -171,6 +178,15 @@ func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID, auxBandwidthDetermined = true + // We don't know the actual HTLC amount that will be sent using + // the custom channel. But we'll still want to make sure we can + // add another HTLC, using the MayAddOutgoingHtlc method below. + // Passing 0 into that method will use the minimum HTLC value + // for the channel, which is okay to just check we don't exceed + // the max number of HTLCs on the channel. A proper balance + // check is done elsewhere. + htlcAmount = 0 + return nil }) if err != nil { @@ -180,11 +196,11 @@ func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID, return 0 } - // If our link isn't currently in a state where it can add - // another outgoing htlc, treat the link as unusable. - if err := link.MayAddOutgoingHtlc(amount); err != nil { + // If our link isn't currently in a state where it can add another + // outgoing htlc, treat the link as unusable. + if err := link.MayAddOutgoingHtlc(htlcAmount); err != nil { log.Warnf("ShortChannelID=%v: cannot add outgoing "+ - "htlc: %v", cid, err) + "htlc with amount %v: %v", cid, htlcAmount, err) return 0 } From 4f2c75f62047579f249ed5914042a0cc6337f342 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 11 Jul 2024 19:01:04 +0200 Subject: [PATCH 2/5] htlcswitch: also set packet amount on modified forward This fixes an issue where the switch's forwarding logic would think the bandwidth to forward an HTLC was insufficient for a custom channel HTLC, because we only overwrote the HTLC's amount and not the packet's (which is just a short cut struct member anyway). --- htlcswitch/interceptable_switch.go | 1 + itest/lnd_forward_interceptor_test.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/htlcswitch/interceptable_switch.go b/htlcswitch/interceptable_switch.go index 1060867a7..67832a985 100644 --- a/htlcswitch/interceptable_switch.go +++ b/htlcswitch/interceptable_switch.go @@ -658,6 +658,7 @@ func (f *interceptedForward) ResumeModified( switch htlc := f.packet.htlc.(type) { case *lnwire.UpdateAddHTLC: outgoingAmountMsat.WhenSome(func(amount lnwire.MilliSatoshi) { + f.packet.amount = amount htlc.Amount = amount }) diff --git a/itest/lnd_forward_interceptor_test.go b/itest/lnd_forward_interceptor_test.go index 00050cec6..3b1e61e22 100644 --- a/itest/lnd_forward_interceptor_test.go +++ b/itest/lnd_forward_interceptor_test.go @@ -407,7 +407,7 @@ func testForwardInterceptorModifiedHtlc(ht *lntest.HarnessTest) { customRecords[crKey] = crValue action := routerrpc.ResolveHoldForwardAction_RESUME_MODIFIED - newOutgoingAmountMsat := packet.OutgoingAmountMsat + 4000 + newOutgoingAmountMsat := packet.OutgoingAmountMsat err := bobInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ IncomingCircuitKey: packet.IncomingCircuitKey, From e8bf89160deb8cafba73c66c763c37315db1bb01 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 11 Jul 2024 16:09:53 -0700 Subject: [PATCH 3/5] itest: don't modify HTLC amount in testForwardInterceptorWireRecords The goal of the test is just to make sure that we can pick up the wire records. With the prior bug fix, if we also modify the outgoing amount here, the normal checks to ensure that the fee has been paid will trigger with this larger amount, which wasn't factored in during initial route creation. --- itest/lnd_forward_interceptor_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/itest/lnd_forward_interceptor_test.go b/itest/lnd_forward_interceptor_test.go index 3b1e61e22..cf870c2ab 100644 --- a/itest/lnd_forward_interceptor_test.go +++ b/itest/lnd_forward_interceptor_test.go @@ -539,7 +539,7 @@ func testForwardInterceptorWireRecords(ht *lntest.HarnessTest) { require.Equal(ht, []byte("test"), val) action := routerrpc.ResolveHoldForwardAction_RESUME_MODIFIED - newOutgoingAmountMsat := packet.OutgoingAmountMsat + 800 + newOutgoingAmountMsat := packet.OutgoingAmountMsat err := bobInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ IncomingCircuitKey: packet.IncomingCircuitKey, From 14a6f73258df0f23b817b869e431fb76db8331c3 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 11 Jul 2024 19:47:34 -0700 Subject: [PATCH 4/5] routing: only set firstHopBlob if we have custom records --- routing/pathfind.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/routing/pathfind.go b/routing/pathfind.go index 7b7c31893..a68816ae3 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -1007,18 +1007,22 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, continue } - firstHopTLVs := lnwire.CustomRecords( - r.FirstHopCustomRecords, - ) - firstHopData, err := firstHopTLVs.Serialize() - if err != nil { - return nil, 0, err + var firstHopBlob fn.Option[tlv.Blob] + if r.FirstHopCustomRecords != nil { + firstHopTLVs := lnwire.CustomRecords( + r.FirstHopCustomRecords, + ) + firstHopData, err := firstHopTLVs.Serialize() + if err != nil { + return nil, 0, err + } + + firstHopBlob = fn.Some(firstHopData) } edge := edgeUnifier.getEdge( netAmountReceived, g.bandwidthHints, - partialPath.outboundFee, - fn.Some[tlv.Blob](firstHopData), + partialPath.outboundFee, firstHopBlob, ) if edge == nil { From d80bca8c6ffe460dd8ec5ad0b911a0f858b5e74a Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 11 Jul 2024 19:48:47 -0700 Subject: [PATCH 5/5] routing: skip amtInRange for custom HTLCs We might be trying to send an invoice amount that's greater than the size of the channel, but once you factor in the custom channel logic, an actual HTLC can be sent over the channel to pay that larger payment. As a result, we'll skip over this check if a have a custom HTLC. --- routing/unified_edges.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routing/unified_edges.go b/routing/unified_edges.go index 8481a2f83..4c895920e 100644 --- a/routing/unified_edges.go +++ b/routing/unified_edges.go @@ -234,7 +234,7 @@ func (u *edgeUnifier) getEdgeLocal(netAmtReceived lnwire.MilliSatoshi, amt := netAmtReceived + lnwire.MilliSatoshi(inboundFee) // Check valid amount range for the channel. - if !edge.amtInRange(amt) { + if htlcBlob.IsNone() && !edge.amtInRange(amt) { log.Debugf("Amount %v not in range for edge %v", netAmtReceived, edge.policy.ChannelID)