Merge pull request #5017 from Roasbeef/default-max-parts

routing: if MaxShardAmt is set, then use that as a ceiling for our splits, use default of 16 for MaxParts

Author: cfromknecht

cmd/lncli/cmd_pay.go

@@ -14,12 +14,14 @@ import (
 	"strings"
 	"time"
 
+	"github.com/btcsuite/btcutil"
 	"github.com/jedib0t/go-pretty/table"
 	"github.com/jedib0t/go-pretty/text"
 	"github.com/lightninglabs/protobuf-hex-display/jsonpb"
 	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/lnrpc/routerrpc"
 	"github.com/lightningnetwork/lnd/lntypes"
+	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/record"
 	"github.com/lightningnetwork/lnd/routing/route"
 	"github.com/urfave/cli"
@@ -62,7 +64,7 @@ var (
 		Name: "max_parts",
 		Usage: "the maximum number of partial payments that may be " +
 			"used",
-		Value: 1,
+		Value: routerrpc.DefaultMaxParts,
 	}
 
 	jsonFlag = cli.BoolFlag{
@@ -71,6 +73,20 @@ var (
 			"messages. Set by default on Windows because table " +
 			"formatting is unsupported.",
 	}
+
+	maxShardSizeSatFlag = cli.UintFlag{
+		Name: "max_shard_size_sat",
+		Usage: "the largest payment split that should be attempted if " +
+			"payment splitting is required to attempt a payment, " +
+			"specified in satoshis",
+	}
+
+	maxShardSizeMsatFlag = cli.UintFlag{
+		Name: "max_shard_size_msat",
+		Usage: "the largest payment split that should be attempted if " +
+			"payment splitting is required to attempt a payment, " +
+			"specified in milli-satoshis",
+	}
 )
 
 // paymentFlags returns common flags for sendpayment and payinvoice.
@@ -115,6 +131,7 @@ func paymentFlags() []cli.Flag {
 			Usage: "allow sending a circular payment to self",
 		},
 		dataFlag, inflightUpdatesFlag, maxPartsFlag, jsonFlag,
+		maxShardSizeSatFlag, maxShardSizeMsatFlag,
 	}
 }
 
@@ -354,6 +371,23 @@ func sendPaymentRequest(ctx *cli.Context,
 
 	req.MaxParts = uint32(ctx.Uint(maxPartsFlag.Name))
 
+	switch {
+	// If the max shard size is specified, then it should either be in sat
+	// or msat, but not both.
+	case ctx.Uint64(maxShardSizeMsatFlag.Name) != 0 &&
+		ctx.Uint64(maxShardSizeSatFlag.Name) != 0:
+		return fmt.Errorf("only --max_split_size_msat or " +
+			"--max_split_size_sat should be set, but not both")
+
+	case ctx.Uint64(maxShardSizeMsatFlag.Name) != 0:
+		req.MaxShardSizeMsat = ctx.Uint64(maxShardSizeMsatFlag.Name)
+
+	case ctx.Uint64(maxShardSizeSatFlag.Name) != 0:
+		req.MaxShardSizeMsat = uint64(lnwire.NewMSatFromSatoshis(
+			btcutil.Amount(ctx.Uint64(maxShardSizeSatFlag.Name)),
+		))
+	}
+
 	// Parse custom data records.
 	data := ctx.String(dataFlag.Name)
 	if data != "" {

lnrpc/routerrpc/router.pb.go

@@ -253,6 +253,14 @@ message SendPaymentRequest {
     that show which htlcs are still in flight are suppressed.
     */
     bool no_inflight_updates = 18;
+
+    /*
+    The largest payment split that should be attempted when making a payment if
+    splitting is necessary. Setting this value will effectively cause lnd to
+    split more aggressively, vs only when it thinks it needs to. Note that this
+    value is in milli-satoshis.
+    */
+    uint64 max_shard_size_msat = 21;
 }
 
 message TrackPaymentRequest {

lnrpc/routerrpc/router.proto

@@ -1405,6 +1405,11 @@
           "type": "boolean",
           "format": "boolean",
           "description": "If set, only the final payment update is streamed back. Intermediate updates\nthat show which htlcs are still in flight are suppressed."
+        },
+        "max_shard_size_msat": {
+          "type": "string",
+          "format": "uint64",
+          "description": "The largest payment split that should be attempted when making a payment if\nsplitting is necessary. Setting this value will effectively cause lnd to\nsplit more aggressively, vs only when it thinks it needs to. Note that this\nvalue is in milli-satoshis."
         }
       }
     },

lnrpc/routerrpc/router.swagger.json

@@ -24,6 +24,15 @@ import (
 	"github.com/lightningnetwork/lnd/zpay32"
 )
 
+const (
+	// DefaultMaxParts is the default number of splits we'll possibly use
+	// for MPP when the user is attempting to send a payment.
+	//
+	// TODO(roasbeef): make this value dynamic based on expected number of
+	// attempts for given amount
+	DefaultMaxParts = 16
+)
+
 // RouterBackend contains the backend implementation of the router rpc sub
 // server calls.
 type RouterBackend struct {
@@ -554,14 +563,23 @@ func (r *RouterBackend) extractIntentFromSendRequest(
 	}
 	payIntent.CltvLimit = cltvLimit
 
-	// Take max htlcs from the request. Map zero to one for backwards
-	// compatibility.
+	// Attempt to parse the max parts value set by the user, if this value
+	// isn't set, then we'll use the current default value for this
+	// setting.
 	maxParts := rpcPayReq.MaxParts
 	if maxParts == 0 {
-		maxParts = 1
+		maxParts = DefaultMaxParts
 	}
 	payIntent.MaxParts = maxParts
 
+	// If this payment had a max shard amount specified, then we'll apply
+	// that now, which'll force us to always make payment splits smaller
+	// than this.
+	if rpcPayReq.MaxShardSizeMsat > 0 {
+		shardAmtMsat := lnwire.MilliSatoshi(rpcPayReq.MaxShardSizeMsat)
+		payIntent.MaxShardAmt = &shardAmtMsat
+	}
+
 	// Take fee limit from request.
 	payIntent.FeeLimit, err = lnrpc.UnmarshallAmt(
 		rpcPayReq.FeeLimitSat, rpcPayReq.FeeLimitMsat,

lnrpc/routerrpc/router_backend.go

@@ -204,6 +204,7 @@ out:
 		FinalCltvDelta: int32(carolPayReq.CltvExpiry),
 		TimeoutSeconds: 60,
 		FeeLimitMsat:   noFeeLimitMsat,
+		MaxParts:       1,
 	}
 	sendAndAssertFailure(
 		t, net.Alice,
@@ -240,6 +241,7 @@ out:
 		FinalCltvDelta: int32(carolPayReq.CltvExpiry),
 		TimeoutSeconds: 60,
 		FeeLimitMsat:   noFeeLimitMsat,
+		MaxParts:       1,
 	}
 	sendAndAssertFailure(
 		t, net.Alice,
@@ -300,6 +302,7 @@ out:
 				PaymentRequest: carolInvoice2.PaymentRequest,
 				TimeoutSeconds: 60,
 				FeeLimitMsat:   noFeeLimitMsat,
+				MaxParts:       1,
 			},
 		)
 
@@ -332,6 +335,7 @@ out:
 		PaymentRequest: carolInvoice3.PaymentRequest,
 		TimeoutSeconds: 60,
 		FeeLimitMsat:   noFeeLimitMsat,
+		MaxParts:       1,
 	}
 	sendAndAssertFailure(
 		t, net.Alice,
@@ -381,6 +385,7 @@ out:
 			PaymentRequest: carolInvoice.PaymentRequest,
 			TimeoutSeconds: 60,
 			FeeLimitMsat:   noFeeLimitMsat,
+			MaxParts:       1,
 		},
 		lnrpc.PaymentFailureReason_FAILURE_REASON_NO_ROUTE,
 	)

lntest/itest/lnd_multi-hop-error-propagation_test.go

@@ -28,6 +28,7 @@ type integratedRoutingContext struct {
 	target *mockNode
 
 	amt         lnwire.MilliSatoshi
+	maxShardAmt *lnwire.MilliSatoshi
 	finalExpiry int32
 
 	mcCfg          MissionControlConfig
@@ -151,6 +152,10 @@ func (c *integratedRoutingContext) testPayment(maxParts uint32,
 		MaxParts:       maxParts,
 	}
 
+	if c.maxShardAmt != nil {
+		payment.MaxShardAmt = c.maxShardAmt
+	}
+
 	session, err := newPaymentSession(
 		&payment, getBandwidthHints,
 		func() (routingGraph, func(), error) {

routing/integrated_routing_context_test.go

@@ -89,6 +89,7 @@ type mppSendTestCase struct {
 	graph           func(g *mockGraph)
 	expectedFailure bool
 	maxParts        uint32
+	maxShardSize    btcutil.Amount
 }
 
 const (
@@ -208,6 +209,33 @@ var mppTestCases = []mppSendTestCase{
 		expectedFailure:  true,
 		maxParts:         10,
 	},
+
+	// Test that if maxShardSize is set, then all attempts are below the
+	// max shard size, yet still sum up to the total payment amount. A
+	// payment of 30k satoshis with a max shard size of 10k satoshis should
+	// produce 3 payments of 10k sats each.
+	{
+		name:             "max shard size clamping",
+		graph:            onePathGraph,
+		amt:              30_000,
+		expectedAttempts: 3,
+		expectedSuccesses: []expectedHtlcSuccess{
+			{
+				amt:   10_000,
+				chans: []uint64{chanSourceIm1, chanIm1Target},
+			},
+			{
+				amt:   10_000,
+				chans: []uint64{chanSourceIm1, chanIm1Target},
+			},
+			{
+				amt:   10_000,
+				chans: []uint64{chanSourceIm1, chanIm1Target},
+			},
+		},
+		maxParts:     1000,
+		maxShardSize: 10_000,
+	},
 }
 
 // TestMppSend tests that a payment can be completed using multiple shards.
@@ -229,6 +257,11 @@ func testMppSend(t *testing.T, testCase *mppSendTestCase) {
 
 	ctx.amt = lnwire.NewMSatFromSatoshis(testCase.amt)
 
+	if testCase.maxShardSize != 0 {
+		shardAmt := lnwire.NewMSatFromSatoshis(testCase.maxShardSize)
+		ctx.maxShardAmt = &shardAmt
+	}
+
 	attempts, err := ctx.testPayment(testCase.maxParts)
 	switch {
 	case err == nil && testCase.expectedFailure:

routing/integrated_routing_test.go

@@ -230,6 +230,18 @@ func (p *paymentSession) RequestRoute(maxAmt, feeLimit lnwire.MilliSatoshi,
 
 	finalHtlcExpiry := int32(height) + int32(finalCltvDelta)
 
+	// Before we enter the loop below, we'll make sure to respect the max
+	// payment shard size (if it's set), which is effectively our
+	// client-side MTU that we'll attempt to respect at all times.
+	maxShardActive := p.payment.MaxShardAmt != nil
+	if maxShardActive && maxAmt > *p.payment.MaxShardAmt {
+		p.log.Debug("Clamping payment attempt from %v to %v due to "+
+			"max shard size of %v", maxAmt,
+			*p.payment.MaxShardAmt, maxAmt)
+
+		maxAmt = *p.payment.MaxShardAmt
+	}
+
 	for {
 		// We'll also obtain a set of bandwidthHints from the lower
 		// layer for each of our outbound channels. This will allow the
@@ -279,7 +291,8 @@ func (p *paymentSession) RequestRoute(maxAmt, feeLimit lnwire.MilliSatoshi,
 			}
 
 			if !p.payment.DestFeatures.HasFeature(lnwire.MPPOptional) {
-				p.log.Debug("not splitting because destination doesn't declare MPP")
+				p.log.Debug("not splitting because " +
+					"destination doesn't declare MPP")
 
 				return nil, errNoPathFound
 			}

routing/payment_session.go

@@ -1684,6 +1684,14 @@ type LightningPayment struct {
 	// MaxParts is the maximum number of partial payments that may be used
 	// to complete the full amount.
 	MaxParts uint32
+
+	// MaxShardAmt is the largest shard that we'll attempt to split using.
+	// If this field is set, and we need to split, rather than attempting
+	// half of the original payment amount, we'll use this value if half
+	// the payment amount is greater than it.
+	//
+	// NOTE: This field is _optional_.
+	MaxShardAmt *lnwire.MilliSatoshi
 }
 
 // SendPayment attempts to send a payment as described within the passed