rpc+routing: add support for fee limits when finding routes for payments #1113

wpaulino · 2018-04-19T16:49:05Z

This PR adds support for fee limits when finding possible routes for payments.

The fee limit can be specified in two ways: either as a fixed amount of satoshis or as a percentage of the payment's amount.

Replaces #714 and fixes #278.

halseth

Nice job! This feature is a much needed one, as at the moment you might end up paying excessive amounts of fees with no way of controlling it! 🚔

A few comments, main one that we should add test for the new RPC.

Thanks to @djseeds and @sebastiandelgado for getting this started, and @danrobinson for initial review! 👏

halseth · 2018-04-20T11:06:26Z

rpcserver.go

+			btcutil.Amount(feeLimit.GetFixed()),
+		)
+	case *lnrpc.FeeLimit_Percent:
+		return amount * lnwire.MilliSatoshi(feeLimit.GetPercent()/100)


generally best to move the division last to avoid loss of precision, as I think this will become 0 by integer division.

halseth · 2018-04-20T11:07:40Z

rpcserver.go

+		// payment amount as an upper bound since there might be some
+		// routes where you pay more in fees than the amount of the
+		// payment.
+		return maxPaymentMSat


Maybe we should restrict this to 2x payment amount or something?

Yeah I was thinking this too. Not sure if we want to enforce this without adding a no_fee_limit flag though.

If the caller really wants no fee limit, she can set the fee limit to a ridiculously high number.

Still think we need to have a sane default value here.

Forgot to address this - should be good now.

I gave this a spin on testnet and seems o be working as advertised! Only thing I realized is that very small payments might be impossible to get through with this default value, since fees might always be higher. This might be okay though, as you can always increase your fee limit.

Also, for large payments, you might end up paying 2x the amount if the only route is a high fee route (value + fee when fee=value). This might lead to some unpleasant surprises. Should we set a default, hard coded max value? We could even consider doing this at the lncli level. Atm the default value appears to be 0 if you look at lncli payinvoice -h. We could change this to a reasonable value?

halseth · 2018-04-20T11:15:34Z

cmd/lncli/commands.go

-			err      error
-			amount   int64
-		)
+		return sendPaymentRequest(ctx, req)


Nice refactor! Can you add a short comment why we short circuit in case pay_req is set?

halseth · 2018-04-20T11:20:08Z

routing/router_test.go

@@ -992,8 +1004,10 @@ func TestAddEdgeUnknownVertexes(t *testing.T) {

 	// Should still be able to find the routes, and the info should be
 	// updated.
-	routes, err = ctx.router.FindRoutes(targetNode, paymentAmt,
-		defaultNumRoutes, DefaultFinalCLTVDelta)
+	routes, err = ctx.router.FindRoutes(


Could add a short test here for the fee limit exceeded case?

halseth · 2018-04-20T11:24:14Z

rpcserver.go

@@ -1832,6 +1852,10 @@ func (r *rpcServer) SendPayment(paymentStream lnrpc.Lightning_SendPaymentServer)
 					p.cltvDelta = uint16(nextPayment.FinalCltvDelta)
 				}

+				p.feeLimit = calculateFeeLimit(


Make an addition to the integration tests to check that percentage and absolute fee limits are working correctly?

halseth · 2018-04-23T16:44:31Z

lnd_test.go

+	}
+
+	// Wait for Alice to receive the channel update.
+	time.Sleep(5 * time.Second)


we should try to eliminate sleeps during integration tests, as they are becoming quite time consuming. Is there any way you could subscribe to graph updates to determine when Alice has seen the policy update? (maybe the policy update test does this).

halseth · 2018-04-23T16:45:56Z

lnd_test.go

+	for _, channel := range listResp.Channels {
+		switch channel.RemotePubkey {
+		case net.Alice.PubKeyStr:
+			aliceBobChanID = channel.ChanId


You could get these from openChannelAndAssert

EDIT: Since these are short channel IDs I realized you probably can't that easily, so this approach is fine :)

halseth

Starting to look very good to me! One final comment about shortening the test, then I will do a final review after squash an rebase 👍

halseth · 2018-04-30T09:16:13Z

lnd_test.go

+
+	// waitForChanUpdate is a helper closure that will wait for an update
+	// for a specific channel and assert its routing policy.
+	waitForChanUpdate := func(graphUpdates chan *lnrpc.GraphTopologyUpdate,


There's a waitForChannelUpdate elsewhere in this file. Is it possible to unify them, or are they checking different criteria?

halseth · 2018-05-07T07:30:50Z

lnd_test.go

+}
+
+// assertChannelPolicy asserts that the passed node's known channel
+// policy for the passed chanPoint is consistent with Bob's current


halseth · 2018-05-07T07:32:00Z

lnd_test.go

+
+// waitForChannelUpdate waits for a node to receive updates from the advertising
+// node for the specified channels.
+func waitForChannelUpdate(t *harnessTest, graphUpdates chan *lnrpc.GraphTopologyUpdate,


halseth · 2018-05-07T07:34:06Z

lnd_test.go

+	for _, channel := range listResp.Channels {
+		switch channel.RemotePubkey {
+		case net.Alice.PubKeyStr:
+			aliceBobChanID = channel.ChanId


EDIT: Since these are short channel IDs I realized you probably can't that easily, so this approach is fine :)

halseth · 2018-05-07T10:36:24Z

Still think we need a default fee cutoff set. Otherwise this LGTM 👍

Roasbeef · 2018-06-02T01:07:13Z

Needs a rebase to address proto conflicts.

wpaulino · 2018-06-07T02:34:38Z

Rebased.

halseth · 2018-06-07T08:13:46Z

Needs rebase and probably some tweaking again after sendToRoute merge 😅

wpaulino · 2018-06-12T03:31:48Z

Rebased.

Roasbeef

LGTM 👾

Can you try to rebase s.t, the binaries are never even added in the first place?

Also, FWIW the current impl is a bit off, as it'll just error out, rather then seeking the set of paths that respect the feel limit. However, with #1321, we'll have the opportunity to implement a proper greedy solution.

Roasbeef · 2018-06-13T00:50:22Z

cmd/lncli/commands.go

@@ -2547,6 +2614,8 @@ func queryRoutes(ctx *cli.Context) error {
 		return fmt.Errorf("amt argument missing")
 	}

+	feeLimit = calculateFeeLimit(ctx, amt)
+


The debug binaries have been committed.

- Extend SendRequest and QueryRoutesRequest protos - newRoute function takes fee limit and cuts off routes that exceed it - queryRoutes, payInvoice and sendPayment commands take the feeLimit inputs and pass them down to newRoute - When no feeLimit is included, don't enforce any feeLimits at all (by setting feeLimit to maxValue)

halseth suggested changes Apr 20, 2018

View reviewed changes

halseth mentioned this pull request Apr 20, 2018

Add constant and percentage-based fee limits to payments #714

Closed

wpaulino force-pushed the fee-cutoff branch from 8fac48c to 3a4c627 Compare April 23, 2018 06:55

halseth suggested changes Apr 23, 2018

View reviewed changes

halseth reviewed Apr 30, 2018

View reviewed changes

wpaulino force-pushed the fee-cutoff branch from 1dbeb4f to 3fb40a5 Compare April 30, 2018 21:30

Roasbeef added this to the 0.5 milestone May 2, 2018

halseth reviewed May 7, 2018

View reviewed changes

wpaulino force-pushed the fee-cutoff branch 3 times, most recently from 7b9faf0 to 0388447 Compare May 7, 2018 20:51

wpaulino force-pushed the fee-cutoff branch 2 times, most recently from 68182b0 to 980ba6b Compare June 1, 2018 00:21

wpaulino force-pushed the fee-cutoff branch from 980ba6b to f9f0545 Compare June 5, 2018 00:30

joostjager mentioned this pull request Jun 7, 2018

routing: use correct fees by searching backwards #1321

Closed

wpaulino force-pushed the fee-cutoff branch from f9f0545 to 5d7f4a7 Compare June 12, 2018 03:14

Roasbeef previously approved these changes Jun 13, 2018

View reviewed changes

sebastiandelgado and others added 8 commits June 12, 2018 18:14

lnrpc: use a protobuf oneof for the different types of fee limits

df6d75a

routing: denote a payment's fee limit in millisatoshis instead

6a6de81

routing: allow specifying a fee limit during route construction

ddf8f2c

rpcserver: add fee limit support for routing related RPCs

63f079d

cmd/lncli: add fee limit flags to routing related commands

a9d873d

lnd_test: refactor out waitForChannelUpdate and assertChannelPolicy

b2585f3

lnd_test: add route fee cutoff test

70a52c9

wpaulino dismissed Roasbeef’s stale review via 70a52c9 June 13, 2018 01:16

wpaulino force-pushed the fee-cutoff branch from 5d7f4a7 to 70a52c9 Compare June 13, 2018 01:16

Roasbeef merged commit 26636ce into lightningnetwork:master Jun 13, 2018

wpaulino deleted the fee-cutoff branch June 13, 2018 02:38

nothingmuch referenced this pull request in BlueWallet/LndHub Mar 20, 2019

FIX: fee_limit when paying

0fed216

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc+routing: add support for fee limits when finding routes for payments #1113

rpc+routing: add support for fee limits when finding routes for payments #1113

wpaulino commented Apr 19, 2018

halseth left a comment

halseth Apr 20, 2018

wpaulino Apr 23, 2018

halseth Apr 20, 2018

wpaulino Apr 23, 2018

halseth Apr 23, 2018

halseth May 7, 2018

wpaulino May 7, 2018

halseth May 8, 2018

halseth Apr 20, 2018

wpaulino Apr 23, 2018

halseth Apr 20, 2018

wpaulino Apr 23, 2018

halseth Apr 20, 2018

wpaulino Apr 23, 2018

halseth Apr 23, 2018

wpaulino Apr 24, 2018

halseth Apr 23, 2018

halseth May 7, 2018

halseth left a comment

halseth Apr 30, 2018

wpaulino Apr 30, 2018

halseth May 7, 2018

wpaulino May 7, 2018

halseth May 7, 2018

halseth May 7, 2018

halseth commented May 7, 2018

Roasbeef commented Jun 2, 2018

wpaulino commented Jun 7, 2018

halseth commented Jun 7, 2018

wpaulino commented Jun 12, 2018

Roasbeef left a comment

Roasbeef Jun 13, 2018

rpc+routing: add support for fee limits when finding routes for payments #1113

rpc+routing: add support for fee limits when finding routes for payments #1113

Conversation

wpaulino commented Apr 19, 2018

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented May 7, 2018

Roasbeef commented Jun 2, 2018

wpaulino commented Jun 7, 2018

halseth commented Jun 7, 2018

wpaulino commented Jun 12, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment