Merge pull request #6200 from Roasbeef/routing-fee-fix

lnrpc: fix the existing routing fee inaccuracy

cmd/lncli/commands.go

@@ -1971,7 +1971,16 @@ var updateChannelPolicyCommand = cli.Command{
 			Usage: "the fee rate that will be charged " +
 				"proportionally based on the value of each " +
 				"forwarded HTLC, the lowest possible rate is 0 " +
-				"with a granularity of 0.000001 (millionths)",
+				"with a granularity of 0.000001 (millionths). Can not " +
+				"be set at the same time as fee_rate_ppm.",
+		},
+		cli.Uint64Flag{
+			Name: "fee_rate_ppm",
+			Usage: "the fee rate ppm (parts per million) that " +
+				"will be charged proportionally based on the value of each " +
+				"forwarded HTLC, the lowest possible rate is 0 " +
+				"with a granularity of 0.000001 (millionths). Can not " +
+				"be set at the same time as fee_rate.",
 		},
 		cli.Int64Flag{
 			Name: "time_lock_delta",
@@ -2032,6 +2041,7 @@ func updateChannelPolicy(ctx *cli.Context) error {
 	var (
 		baseFee       int64
 		feeRate       float64
+		feeRatePpm    uint64
 		timeLockDelta int64
 		err           error
 	)
@@ -2051,8 +2061,12 @@ func updateChannelPolicy(ctx *cli.Context) error {
 	}
 
 	switch {
+	case ctx.IsSet("fee_rate") && ctx.IsSet("fee_rate_ppm"):
+		return fmt.Errorf("fee_rate or fee_rate_ppm can not both be set")
 	case ctx.IsSet("fee_rate"):
 		feeRate = ctx.Float64("fee_rate")
+	case ctx.IsSet("fee_rate_ppm"):
+		feeRatePpm = ctx.Uint64("fee_rate_ppm")
 	case args.Present():
 		feeRate, err = strconv.ParseFloat(args.First(), 64)
 		if err != nil {
@@ -2061,7 +2075,7 @@ func updateChannelPolicy(ctx *cli.Context) error {
 
 		args = args.Tail()
 	default:
-		return fmt.Errorf("fee_rate argument missing")
+		return fmt.Errorf("fee_rate or fee_rate_ppm argument missing")
 	}
 
 	switch {
@@ -2100,7 +2114,6 @@ func updateChannelPolicy(ctx *cli.Context) error {
 
 	req := &lnrpc.PolicyUpdateRequest{
 		BaseFeeMsat:   baseFee,
-		FeeRate:       feeRate,
 		TimeLockDelta: uint32(timeLockDelta),
 		MaxHtlcMsat:   ctx.Uint64("max_htlc_msat"),
 	}
@@ -2120,6 +2133,12 @@ func updateChannelPolicy(ctx *cli.Context) error {
 		}
 	}
 
+	if feeRate != 0 {
+		req.FeeRate = feeRate
+	} else if feeRatePpm != 0 {
+		req.FeeRatePpm = uint32(feeRatePpm)
+	}
+
 	resp, err := client.UpdateChannelPolicy(ctxc, req)
 	if err != nil {
 		return err

docs/release-notes/release-notes-0.14.2.md

@@ -81,6 +81,8 @@ Postgres](https://github.com/lightningnetwork/lnd/pull/6111)
 
 * [Fix an issue that would prevent very old nodes from starting up due to lack of a historical channel bucket](https://github.com/lightningnetwork/lnd/pull/6159)
 
+* [Fixes a bug that would cause incorrect rounding when translating a decimal fee rate to ppm](https://github.com/lightningnetwork/lnd/pull/6200)
+
 
 ## RPC Server
 
@@ -91,6 +93,10 @@ Postgres](https://github.com/lightningnetwork/lnd/pull/6111)
 * [Fix missing label on streamed
   transactions](https://github.com/lightningnetwork/lnd/pull/5854).
 
+* [The `fee_rate_ppm` parameter/argument was added to  
+  update channel policy](https://github.com/lightningnetwork/lnd/pull/5711)
+  to prevent truncation error with tiny fee rates.
+
 * [Closing txid is now
   exposed](https://github.com/lightningnetwork/lnd/pull/6146) inside
   WaitingCloseResp from calling `PendingChannels`.
@@ -99,6 +105,8 @@ Postgres](https://github.com/lightningnetwork/lnd/pull/6111)
   set](https://github.com/lightningnetwork/lnd/pull/6185) on
   `RPCMiddlewareRequest` messages.
 
+* [Adds a new FeeRatePpm to the UpdateChanPolicy call to allow fee rate expression in the native protocol unit](https://github.com/lightningnetwork/lnd/pull/6200)
+
 
 ## Routing
 
@@ -117,6 +125,7 @@ Postgres](https://github.com/lightningnetwork/lnd/pull/6111)
 * Bjarne Magnussen
 * Daniel McNally
 * Elle Mouton
+* Erik Ek
 * Harsha Goli
 * Joost Jager
 * Martin Habovštiak

lnrpc/lightning.proto

@@ -3751,6 +3751,9 @@ message PolicyUpdateRequest {
     // goes up to 6 decimal places, so 1e-6.
     double fee_rate = 4;
 
+    // The effective fee rate in micro-satoshis (parts per million).
+    uint32 fee_rate_ppm = 9;
+
     // The required timelock delta for HTLCs forwarded over the channel.
     uint32 time_lock_delta = 5;
 

lnrpc/lightning.swagger.json

@@ -5767,6 +5767,11 @@
           "format": "double",
           "description": "The effective fee rate in milli-satoshis. The precision of this value\ngoes up to 6 decimal places, so 1e-6."
         },
+        "fee_rate_ppm": {
+          "type": "integer",
+          "format": "int64",
+          "description": "The effective fee rate in micro-satoshis (parts per million)."
+        },
         "time_lock_delta": {
           "type": "integer",
           "format": "int64",

lntest/itest/lnd_channel_policy_test.go

@@ -2,6 +2,7 @@ package itest
 
 import (
 	"context"
+	"math"
 	"strings"
 	"time"
 
@@ -15,6 +16,19 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// assertPolicyUpdate checks that a given policy update has been received by a
+// list of given nodes.
+func assertPolicyUpdate(t *harnessTest, nodes []*lntest.HarnessNode,
+	advertisingNode string, policy *lnrpc.RoutingPolicy,
+	chanPoint *lnrpc.ChannelPoint) {
+
+	for _, node := range nodes {
+		assertChannelPolicyUpdate(
+			t.t, node, advertisingNode, policy, chanPoint, false,
+		)
+	}
+}
+
 // testUpdateChannelPolicy tests that policy updates made to a channel
 // gets propagated to other nodes in the network.
 func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
@@ -45,21 +59,6 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 	// make sure they all receive the expected updates.
 	nodes := []*lntest.HarnessNode{net.Alice, net.Bob}
 
-	// assertPolicyUpdate checks that a given policy update has been
-	// received by a list of given nodes.
-	assertPolicyUpdate := func(nodes []*lntest.HarnessNode,
-		advertisingNode string, policy *lnrpc.RoutingPolicy,
-		chanPoint *lnrpc.ChannelPoint) {
-
-		for _, node := range nodes {
-			assertChannelPolicyUpdate(
-				t.t, node, advertisingNode,
-				policy, chanPoint, false,
-			)
-		}
-
-	}
-
 	// Alice and Bob should see each other's ChannelUpdates, advertising the
 	// default routing policies.
 	expectedPolicy := &lnrpc.RoutingPolicy{
@@ -71,9 +70,11 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 	}
 
 	assertPolicyUpdate(
-		nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint,
+		t, nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint,
+	)
+	assertPolicyUpdate(
+		t, nodes, net.Bob.PubKeyStr, expectedPolicy, chanPoint,
 	)
-	assertPolicyUpdate(nodes, net.Bob.PubKeyStr, expectedPolicy, chanPoint)
 
 	// They should now know about the default policies.
 	for _, node := range nodes {
@@ -144,10 +145,10 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 	}
 
 	assertPolicyUpdate(
-		nodes, net.Bob.PubKeyStr, expectedPolicyBob, chanPoint2,
+		t, nodes, net.Bob.PubKeyStr, expectedPolicyBob, chanPoint2,
 	)
 	assertPolicyUpdate(
-		nodes, carol.PubKeyStr, expectedPolicyCarol, chanPoint2,
+		t, nodes, carol.PubKeyStr, expectedPolicyCarol, chanPoint2,
 	)
 
 	// Check that all nodes now know about the updated policies.
@@ -337,7 +338,9 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 	}
 
 	// Wait for all nodes to have seen the policy update done by Bob.
-	assertPolicyUpdate(nodes, net.Bob.PubKeyStr, expectedPolicy, chanPoint)
+	assertPolicyUpdate(
+		t, nodes, net.Bob.PubKeyStr, expectedPolicy, chanPoint,
+	)
 
 	// Check that all nodes now know about Bob's updated policy.
 	for _, node := range nodes {
@@ -421,10 +424,10 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 	// Wait for all nodes to have seen the policy updates for both of
 	// Alice's channels.
 	assertPolicyUpdate(
-		nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint,
+		t, nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint,
 	)
 	assertPolicyUpdate(
-		nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint3,
+		t, nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint3,
 	)
 
 	// And finally check that all nodes remembers the policy update they
@@ -456,11 +459,11 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 		// of Alice's channels. Carol will not see the last update as
 		// the limit has been reached.
 		assertPolicyUpdate(
-			[]*lntest.HarnessNode{net.Alice, net.Bob},
+			t, []*lntest.HarnessNode{net.Alice, net.Bob},
 			net.Alice.PubKeyStr, expectedPolicy, chanPoint,
 		)
 		assertPolicyUpdate(
-			[]*lntest.HarnessNode{net.Alice, net.Bob},
+			t, []*lntest.HarnessNode{net.Alice, net.Bob},
 			net.Alice.PubKeyStr, expectedPolicy, chanPoint3,
 		)
 		// Check that all nodes remembers the policy update
@@ -481,11 +484,11 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 			expectedPolicy = &prevAlicePolicy
 		}
 		assertPolicyUpdate(
-			[]*lntest.HarnessNode{carol},
+			t, []*lntest.HarnessNode{carol},
 			net.Alice.PubKeyStr, expectedPolicy, chanPoint,
 		)
 		assertPolicyUpdate(
-			[]*lntest.HarnessNode{carol},
+			t, []*lntest.HarnessNode{carol},
 			net.Alice.PubKeyStr, expectedPolicy, chanPoint3,
 		)
 		assertChannelPolicy(
@@ -818,3 +821,89 @@ func testUpdateChannelPolicyForPrivateChannel(net *lntest.NetworkHarness,
 	assertAmountPaid(t, "Alice(local) => Bob(remote)",
 		net.Alice, aliceFundPoint, amtExpected, 0)
 }
+
+// testUpdateChannelPolicyFeeRateAccuracy tests that updating the channel policy
+// rounds fee rate values correctly as well as setting fee rate with ppm works
+// as expected.
+func testUpdateChannelPolicyFeeRateAccuracy(net *lntest.NetworkHarness,
+	t *harnessTest) {
+
+	chanAmt := funding.MaxBtcFundingAmount
+	pushAmt := chanAmt / 2
+
+	// Create a channel Alice -> Bob.
+	chanPoint := openChannelAndAssert(
+		t, net, net.Alice, net.Bob,
+		lntest.OpenChannelParams{
+			Amt:     chanAmt,
+			PushAmt: pushAmt,
+		},
+	)
+	defer closeChannelAndAssert(t, net, net.Alice, chanPoint, false)
+
+	// Nodes that we need to make sure receive the channel updates.
+	nodes := []*lntest.HarnessNode{net.Alice, net.Bob}
+
+	baseFee := int64(1500)
+	timeLockDelta := uint32(66)
+	maxHtlc := uint64(500000)
+	defaultMinHtlc := int64(1000)
+
+	// Originally LND did not properly round up fee rates which caused
+	// inaccuracy where fee rates were simply rounded down due to the
+	// integer conversion.
+	//
+	// We'll use a fee rate of 0.031337 which without rounding up would
+	// have resulted in a fee rate ppm of 31336.
+	feeRate := 0.031337
+
+	// Expected fee rate will be rounded up.
+	expectedFeeRateMilliMsat := int64(math.Round(testFeeBase * feeRate))
+
+	expectedPolicy := &lnrpc.RoutingPolicy{
+		FeeBaseMsat:      baseFee,
+		FeeRateMilliMsat: expectedFeeRateMilliMsat,
+		TimeLockDelta:    timeLockDelta,
+		MinHtlc:          defaultMinHtlc,
+		MaxHtlcMsat:      maxHtlc,
+	}
+
+	req := &lnrpc.PolicyUpdateRequest{
+		BaseFeeMsat:   baseFee,
+		FeeRate:       feeRate,
+		TimeLockDelta: timeLockDelta,
+		MaxHtlcMsat:   maxHtlc,
+		Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{
+			ChanPoint: chanPoint,
+		},
+	}
+
+	ctxb := context.Background()
+	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
+	if _, err := net.Alice.UpdateChannelPolicy(ctxt, req); err != nil {
+		t.Fatalf("unable to get alice's balance: %v", err)
+	}
+
+	// Make sure that both Alice and Bob sees the same policy after update.
+	assertPolicyUpdate(
+		t, nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint,
+	)
+
+	// Now use the new PPM feerate field and make sure that the feerate is
+	// correctly set.
+	feeRatePPM := uint32(32337)
+	req.FeeRate = 0 // Can't set both at the same time.
+	req.FeeRatePpm = feeRatePPM
+	expectedPolicy.FeeRateMilliMsat = int64(feeRatePPM)
+
+	ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
+	if _, err := net.Alice.UpdateChannelPolicy(ctxt, req); err != nil {
+		t.Fatalf("unable to get alice's balance: %v", err)
+	}
+
+	// Make sure that both Alice and Bob sees the same policy after update.
+	assertPolicyUpdate(
+		t, nodes, net.Alice.PubKeyStr, expectedPolicy, chanPoint,
+	)
+
+}

lntest/itest/lnd_test_list_on_test.go

@@ -32,6 +32,10 @@ var allTestCases = []*testCase{
 		name: "update channel policy",
 		test: testUpdateChannelPolicy,
 	},
+	{
+		name: "update channel policy fee rate accuracy",
+		test: testUpdateChannelPolicyFeeRateAccuracy,
+	},
 	{
 		name: "open channel reorg test",
 		test: testOpenChannelAfterReorg,

rpcserver.go

@@ -6297,7 +6297,7 @@ func (r *rpcServer) DecodePayReq(ctx context.Context,
 // Nodes on the network advertise their fee rate using this point as a base.
 // This means that the minimal possible fee rate if 1e-6, or 0.000001, or
 // 0.0001%.
-const feeBase = 1000000
+const feeBase float64 = 1000000
 
 // FeeReport allows the caller to obtain a report detailing the current fee
 // schedule enforced by the node globally for each channel.
@@ -6330,7 +6330,7 @@ func (r *rpcServer) FeeReport(ctx context.Context,
 		// 1mil mSAT sent, so will divide by this to get the proper fee
 		// rate.
 		feeRateFixedPoint := edgePolicy.FeeProportionalMillionths
-		feeRate := float64(feeRateFixedPoint) / float64(feeBase)
+		feeRate := float64(feeRateFixedPoint) / feeBase
 
 		// TODO(roasbeef): also add stats for revenue for each channel
 		feeReports = append(feeReports, &lnrpc.ChannelFeeReport{
@@ -6470,28 +6470,52 @@ func (r *rpcServer) UpdateChannelPolicy(ctx context.Context,
 		return nil, fmt.Errorf("unknown scope: %v", scope)
 	}
 
+	var feeRateFixed uint32
+
 	switch {
-	// As a sanity check, if the fee isn't zero, we'll ensure that the
-	// passed fee rate is below 1e-6, or the lowest allowed non-zero fee
-	// rate expressible within the protocol.
-	case req.FeeRate != 0 && req.FeeRate < minFeeRate:
-		return nil, fmt.Errorf("fee rate of %v is too small, min fee "+
-			"rate is %v", req.FeeRate, minFeeRate)
+	// The request should use either the fee rate in percent, or the new
+	// ppm rate, but not both.
+	case req.FeeRate != 0 && req.FeeRatePpm != 0:
+		errMsg := "cannot set both FeeRate and FeeRatePpm at the " +
+			"same time"
+
+		return nil, status.Errorf(codes.InvalidArgument, errMsg)
+
+	// If the request is using fee_rate.
+	case req.FeeRate != 0:
+		// As a sanity check, if the fee isn't zero, we'll ensure that
+		// the passed fee rate is below 1e-6, or the lowest allowed
+		// non-zero fee rate expressible within the protocol.
+		if req.FeeRate != 0 && req.FeeRate < minFeeRate {
+			return nil, fmt.Errorf("fee rate of %v is too "+
+				"small, min fee rate is %v", req.FeeRate,
+				minFeeRate)
+		}
+
+		// We'll also need to convert the floating point fee rate we
+		// accept over RPC to the fixed point rate that we use within
+		// the protocol. We do this by multiplying the passed fee rate
+		// by the fee base. This gives us the fixed point, scaled by 1
+		// million that's used within the protocol.
+		//
+		// Because of the inaccurate precision of the IEEE 754
+		// standard, we need to round the product of feerate and
+		// feebase.
+		feeRateFixed = uint32(math.Round(req.FeeRate * feeBase))
+
+	// Otherwise, we use the fee_rate_ppm parameter.
+	case req.FeeRatePpm != 0:
+		feeRateFixed = req.FeeRatePpm
+	}
 
 	// We'll also ensure that the user isn't setting a CLTV delta that
 	// won't give outgoing HTLCs enough time to fully resolve if needed.
-	case req.TimeLockDelta < minTimeLockDelta:
+	if req.TimeLockDelta < minTimeLockDelta {
 		return nil, fmt.Errorf("time lock delta of %v is too small, "+
 			"minimum supported is %v", req.TimeLockDelta,
 			minTimeLockDelta)
 	}
 
-	// We'll also need to convert the floating point fee rate we accept
-	// over RPC to the fixed point rate that we use within the protocol. We
-	// do this by multiplying the passed fee rate by the fee base. This
-	// gives us the fixed point, scaled by 1 million that's used within the
-	// protocol.
-	feeRateFixed := uint32(req.FeeRate * feeBase)
 	baseFeeMsat := lnwire.MilliSatoshi(req.BaseFeeMsat)
 	feeSchema := routing.FeeSchema{
 		BaseFee: baseFeeMsat,
@@ -6513,9 +6537,9 @@ func (r *rpcServer) UpdateChannelPolicy(ctx context.Context,
 	}
 
 	rpcsLog.Debugf("[updatechanpolicy] updating channel policy base_fee=%v, "+
-		"rate_float=%v, rate_fixed=%v, time_lock_delta: %v, "+
+		"rate_fixed=%v, time_lock_delta: %v, "+
 		"min_htlc=%v, max_htlc=%v, targets=%v",
-		req.BaseFeeMsat, req.FeeRate, feeRateFixed, req.TimeLockDelta,
+		req.BaseFeeMsat, feeRateFixed, req.TimeLockDelta,
 		minHtlc, maxHtlc,
 		spew.Sdump(targetChans))