Merge pull request #10125 from ziggie1984/fix-payment-send-local-chan

bugfix payment lifecycle payment attempts

docs/release-notes/release-notes-0.19.2.md

@@ -113,5 +113,10 @@ much more slowly.
 ## Tooling and Documentation
 
 # Contributors (Alphabetical Order)
+
+* Calvin Zachman 
+* George Tsagkarelis
 * hieblmi
+* Oliver Gugger
 * Yong Yu
+* Ziggie

docs/release-notes/release-notes-0.19.3.md

@@ -29,6 +29,11 @@
   imported in a watch-only (e.g. remote-signing)
   setup](https://github.com/lightningnetwork/lnd/pull/10119).
 
+- [Fixed](https://github.com/lightningnetwork/lnd/pull/10125) a case in the
+  payment lifecycle where we would retry the same route over and over again in
+  situations where the sending amount would violate the channel policy
+  restriction (min,max HTLC).
+
 # New Features
 
 ## Functional Enhancements
@@ -77,3 +82,4 @@
 * Olaoluwa Osuntokun
 * Oliver Gugger
 * Yong Yu
+* Ziggie

htlcswitch/link.go

@@ -2547,36 +2547,12 @@ func (l *channelLink) canSendHtlc(policy models.ForwardingPolicy,
 	heightNow uint32, originalScid lnwire.ShortChannelID,
 	customRecords lnwire.CustomRecords) *LinkError {
 
-	// As our first sanity check, we'll ensure that the passed HTLC isn't
-	// too small for the next hop. If so, then we'll cancel the HTLC
-	// directly.
-	if amt < policy.MinHTLCOut {
-		l.log.Warnf("outgoing htlc(%x) is too small: min_htlc=%v, "+
-			"htlc_value=%v", payHash[:], policy.MinHTLCOut,
-			amt)
-
-		// As part of the returned error, we'll send our latest routing
-		// policy so the sending node obtains the most up to date data.
-		cb := func(upd *lnwire.ChannelUpdate1) lnwire.FailureMessage {
-			return lnwire.NewAmountBelowMinimum(amt, *upd)
-		}
-		failure := l.createFailureWithUpdate(false, originalScid, cb)
-		return NewLinkError(failure)
-	}
-
-	// Next, ensure that the passed HTLC isn't too large. If so, we'll
-	// cancel the HTLC directly.
-	if policy.MaxHTLC != 0 && amt > policy.MaxHTLC {
-		l.log.Warnf("outgoing htlc(%x) is too large: max_htlc=%v, "+
-			"htlc_value=%v", payHash[:], policy.MaxHTLC, amt)
-
-		// As part of the returned error, we'll send our latest routing
-		// policy so the sending node obtains the most up-to-date data.
-		cb := func(upd *lnwire.ChannelUpdate1) lnwire.FailureMessage {
-			return lnwire.NewTemporaryChannelFailure(upd)
-		}
-		failure := l.createFailureWithUpdate(false, originalScid, cb)
-		return NewDetailedLinkError(failure, OutgoingFailureHTLCExceedsMax)
+	// Validate HTLC amount against policy limits.
+	linkErr := l.validateHtlcAmount(
+		policy, payHash, amt, originalScid, customRecords,
+	)
+	if linkErr != nil {
+		return linkErr
 	}
 
 	// We want to avoid offering an HTLC which will expire in the near
@@ -2591,6 +2567,7 @@ func (l *channelLink) canSendHtlc(policy models.ForwardingPolicy,
 			return lnwire.NewExpiryTooSoon(*upd)
 		}
 		failure := l.createFailureWithUpdate(false, originalScid, cb)
+
 		return NewLinkError(failure)
 	}
 
@@ -2606,7 +2583,8 @@ func (l *channelLink) canSendHtlc(policy models.ForwardingPolicy,
 	// We now check the available bandwidth to see if this HTLC can be
 	// forwarded.
 	availableBandwidth := l.Bandwidth()
-	auxBandwidth, err := fn.MapOptionZ(
+
+	auxBandwidth, externalErr := fn.MapOptionZ(
 		l.cfg.AuxTrafficShaper,
 		func(ts AuxTrafficShaper) fn.Result[OptionalBandwidth] {
 			var htlcBlob fn.Option[tlv.Blob]
@@ -2624,8 +2602,10 @@ func (l *channelLink) canSendHtlc(policy models.ForwardingPolicy,
 			return l.AuxBandwidth(amt, originalScid, htlcBlob, ts)
 		},
 	).Unpack()
-	if err != nil {
-		l.log.Errorf("Unable to determine aux bandwidth: %v", err)
+	if externalErr != nil {
+		l.log.Errorf("Unable to determine aux bandwidth: %v",
+			externalErr)
+
 		return NewLinkError(&lnwire.FailTemporaryNodeFailure{})
 	}
 
@@ -2645,6 +2625,7 @@ func (l *channelLink) canSendHtlc(policy models.ForwardingPolicy,
 			return lnwire.NewTemporaryChannelFailure(upd)
 		}
 		failure := l.createFailureWithUpdate(false, originalScid, cb)
+
 		return NewDetailedLinkError(
 			failure, OutgoingFailureInsufficientBalance,
 		)
@@ -4716,3 +4697,71 @@ func (l *channelLink) processLocalUpdateFailHTLC(ctx context.Context,
 	// Immediately update the commitment tx to minimize latency.
 	l.updateCommitTxOrFail(ctx)
 }
+
+// validateHtlcAmount checks if the HTLC amount is within the policy's
+// minimum and maximum limits. Returns a LinkError if validation fails.
+func (l *channelLink) validateHtlcAmount(policy models.ForwardingPolicy,
+	payHash [32]byte, amt lnwire.MilliSatoshi,
+	originalScid lnwire.ShortChannelID,
+	customRecords lnwire.CustomRecords) *LinkError {
+
+	// In case we are dealing with a custom HTLC, we don't need to validate
+	// the HTLC constraints.
+	//
+	// NOTE: Custom HTLCs are only locally sourced and will use custom
+	// channels which are not routable channels and should have their policy
+	// not restricted in the first place. However to be sure we skip this
+	// check otherwise we might end up in a loop of sending to the same
+	// route again and again because link errors are not persisted in
+	// mission control.
+	if fn.MapOptionZ(
+		l.cfg.AuxTrafficShaper,
+		func(ts AuxTrafficShaper) bool {
+			return ts.IsCustomHTLC(customRecords)
+		},
+	) {
+
+		l.log.Debugf("Skipping htlc amount policy validation for " +
+			"custom htlc")
+
+		return nil
+	}
+
+	// As our first sanity check, we'll ensure that the passed HTLC isn't
+	// too small for the next hop. If so, then we'll cancel the HTLC
+	// directly.
+	if amt < policy.MinHTLCOut {
+		l.log.Warnf("outgoing htlc(%x) is too small: min_htlc=%v, "+
+			"htlc_value=%v", payHash[:], policy.MinHTLCOut,
+			amt)
+
+		// As part of the returned error, we'll send our latest routing
+		// policy so the sending node obtains the most up to date data.
+		cb := func(upd *lnwire.ChannelUpdate1) lnwire.FailureMessage {
+			return lnwire.NewAmountBelowMinimum(amt, *upd)
+		}
+		failure := l.createFailureWithUpdate(false, originalScid, cb)
+
+		return NewLinkError(failure)
+	}
+
+	// Next, ensure that the passed HTLC isn't too large. If so, we'll
+	// cancel the HTLC directly.
+	if policy.MaxHTLC != 0 && amt > policy.MaxHTLC {
+		l.log.Warnf("outgoing htlc(%x) is too large: max_htlc=%v, "+
+			"htlc_value=%v", payHash[:], policy.MaxHTLC, amt)
+
+		// As part of the returned error, we'll send our latest routing
+		// policy so the sending node obtains the most up-to-date data.
+		cb := func(upd *lnwire.ChannelUpdate1) lnwire.FailureMessage {
+			return lnwire.NewTemporaryChannelFailure(upd)
+		}
+		failure := l.createFailureWithUpdate(false, originalScid, cb)
+
+		return NewDetailedLinkError(
+			failure, OutgoingFailureHTLCExceedsMax,
+		)
+	}
+
+	return nil
+}

itest/list_on_test.go

@@ -242,6 +242,10 @@ var allTestCases = []*lntest.TestCase{
 		Name:     "wumbo channels",
 		TestFunc: testWumboChannels,
 	},
+	{
+		Name:     "max htlc path payment",
+		TestFunc: testMaxHtlcPathPayment,
+	},
 	{
 		Name:     "max htlc pathfind",
 		TestFunc: testMaxHtlcPathfind,

itest/lnd_max_htlc_path_test.go

@@ -0,0 +1,74 @@
+package itest
+
+import (
+	"github.com/lightningnetwork/lnd/lnrpc"
+	"github.com/lightningnetwork/lnd/lnrpc/routerrpc"
+	"github.com/lightningnetwork/lnd/lntest"
+)
+
+// testMaxHtlcPathPayment tests that when a payment is attempted, the path
+// finding logic correctly takes into account the max_htlc value of the first
+// channel.
+func testMaxHtlcPathPayment(ht *lntest.HarnessTest) {
+	// Create a channel Alice->Bob.
+	chanPoints, nodes := ht.CreateSimpleNetwork(
+		[][]string{nil, nil}, lntest.OpenChannelParams{
+			Amt: 1000000,
+		},
+	)
+	alice, bob := nodes[0], nodes[1]
+	chanPoint := chanPoints[0]
+
+	// Alice and Bob should have one channel open with each other now.
+	ht.AssertNodeNumChannels(alice, 1)
+	ht.AssertNodeNumChannels(bob, 1)
+
+	// Define a max_htlc value that is lower than the default.
+	const (
+		maxHtlcMsat   = 50000000
+		minHtlcMsat   = 1000
+		timeLockDelta = 80
+		baseFeeMsat   = 0
+		feeRate       = 0
+	)
+
+	// Update Alice's channel policy to set the new max_htlc value.
+	req := &lnrpc.PolicyUpdateRequest{
+		Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{
+			ChanPoint: chanPoint,
+		},
+		MaxHtlcMsat:   maxHtlcMsat,
+		MinHtlcMsat:   minHtlcMsat,
+		BaseFeeMsat:   baseFeeMsat,
+		FeeRate:       feeRate,
+		TimeLockDelta: timeLockDelta,
+	}
+	alice.RPC.UpdateChannelPolicy(req)
+
+	expectedPolicy := &lnrpc.RoutingPolicy{
+		FeeBaseMsat:      baseFeeMsat,
+		FeeRateMilliMsat: feeRate,
+		TimeLockDelta:    timeLockDelta,
+		MinHtlc:          minHtlcMsat,
+		MaxHtlcMsat:      maxHtlcMsat,
+	}
+
+	// Wait for the policy update to propagate to Bob.
+	ht.AssertChannelPolicyUpdate(
+		bob, alice, expectedPolicy, chanPoint, false,
+	)
+
+	// Create an invoice for an amount greater than the max htlc value.
+	invoiceAmt := int64(maxHtlcMsat + 10_000_000)
+	invoice := &lnrpc.Invoice{ValueMsat: invoiceAmt}
+	resp := bob.RPC.AddInvoice(invoice)
+
+	// Attempt to pay the invoice from Alice. The payment should be
+	// splitted into two parts, one for the max_htlc value and one for the
+	// remaining amount and succeed.
+	payReq := &routerrpc.SendPaymentRequest{
+		PaymentRequest: resp.PaymentRequest,
+		FeeLimitMsat:   noFeeLimitMsat,
+	}
+	ht.SendPaymentAssertSettled(alice, payReq)
+}

routing/bandwidth.go

@@ -24,9 +24,9 @@ type bandwidthHints interface {
 	availableChanBandwidth(channelID uint64,
 		amount lnwire.MilliSatoshi) (lnwire.MilliSatoshi, bool)
 
-	// firstHopCustomBlob returns the custom blob for the first hop of the
-	// payment, if available.
-	firstHopCustomBlob() fn.Option[tlv.Blob]
+	// isCustomHTLCPayment returns true if this payment is a custom payment.
+	// For custom payments policy checks might not be needed.
+	isCustomHTLCPayment() bool
 }
 
 // getLinkQuery is the function signature used to lookup a link.
@@ -207,8 +207,23 @@ func (b *bandwidthManager) availableChanBandwidth(channelID uint64,
 	return bandwidth, true
 }
 
-// firstHopCustomBlob returns the custom blob for the first hop of the payment,
-// if available.
-func (b *bandwidthManager) firstHopCustomBlob() fn.Option[tlv.Blob] {
-	return b.firstHopBlob
+// isCustomHTLCPayment returns true if this payment is a custom payment.
+// For custom payments policy checks might not be needed.
+func (b *bandwidthManager) isCustomHTLCPayment() bool {
+	return fn.MapOptionZ(b.firstHopBlob, func(blob tlv.Blob) bool {
+		customRecords, err := lnwire.ParseCustomRecords(blob)
+		if err != nil {
+			log.Warnf("failed to parse custom records when "+
+				"checking if payment is custom: %v", err)
+
+			return false
+		}
+
+		return fn.MapOptionZ(
+			b.trafficShaper,
+			func(s htlcswitch.AuxTrafficShaper) bool {
+				return s.IsCustomHTLC(customRecords)
+			},
+		)
+	})
 }

routing/integrated_routing_context_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/lightningnetwork/lnd/kvdb"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/routing/route"
-	"github.com/lightningnetwork/lnd/tlv"
 	"github.com/lightningnetwork/lnd/zpay32"
 	"github.com/stretchr/testify/require"
 )
@@ -36,8 +35,8 @@ func (m *mockBandwidthHints) availableChanBandwidth(channelID uint64,
 	return balance, ok
 }
 
-func (m *mockBandwidthHints) firstHopCustomBlob() fn.Option[tlv.Blob] {
-	return fn.None[tlv.Blob]()
+func (m *mockBandwidthHints) isCustomHTLCPayment() bool {
+	return false
 }
 
 // integratedRoutingContext defines the context in which integrated routing

routing/unified_edges.go

@@ -265,12 +265,13 @@ func (u *edgeUnifier) getEdgeLocal(netAmtReceived lnwire.MilliSatoshi,
 		// Add inbound fee to get to the amount that is sent over the
 		// local channel.
 		amt := netAmtReceived + lnwire.MilliSatoshi(inboundFee)
-
 		// Check valid amount range for the channel. We skip this test
-		// for payments with custom HTLC data, as the amount sent on
-		// the BTC layer may differ from the amount that is actually
-		// forwarded in custom channels.
-		if bandwidthHints.firstHopCustomBlob().IsNone() &&
+
+		// for payments with custom htlc data we skip the amount range
+		// check because the amt of the payment does not relate to the
+		// actual amount carried by the HTLC but instead is encoded in
+		// the blob data.
+		if !bandwidthHints.isCustomHTLCPayment() &&
 			!edge.amtInRange(amt) {
 
 			log.Debugf("Amount %v not in range for edge %v",