pay: respect maxfeepercent when choosing a shadow route

[darosior]

We claim to do so in the manpage. I was wrong

fixes #3684

[kirelagin]

I gave the docs a more thorough read, and, it turns out, they are actually correct:

Route randomization will never exceed maxfeepercent of the payment. Route randomization and shadow routing will not take routes that would exceed maxdelay.

So, it only claims that route randomisation respects maxfeepercent, not shadow routing.

Anyway, docs aside, there are two things here:

1. No way of disabling shadow routing. As I understand now, this was intentional, however I think it would still be good to have an option for disabling it (e.g. my use-case is that I control multiple nodes connected to each other and I would like to be able to distribute liquidity among them through direct channels, and it is easier when the amounts being transferred are not fuzzed).
2. Fuzzing small amounts breaks the transfers as the payee rejects them.

I think that this PR is definitely an improvement on (1), as it makes the behaviour more predictable.

It also resolves (2), assuming that maxfeepercent cannot be set to more than 100. The receiving node rejects the transfer if the amount is twice that of the invoice. However, I am worried that it effectively completely disables shadow routing for small amounts, although, I suppose, it can be desirable in certain cases. Therefore, probably, not as part of this PR, but I think it is worth considering lifting the restriction on “twice the amount” on the receiving side for small payments (the RFC only says SHOULD, not MUST fail).

[darosior]

So, it only claims that route randomisation respects maxfeepercent, not shadow routing.

Right, amended the OP.

No way of disabling shadow routing.

Actually you can in developer mode this PR hid a disabling parameter (use_shadow) behind #if DEVELOPER :-).

It also resolves (2), assuming that maxfeepercent cannot be set to more than 100

I think it can, but It does default to 50%.

I am worried that it effectively completely disables shadow routing for small amounts, although

It doesn't if the receiving peer has channel signaling a base_fee which is less than amount * maxfeepercent.
However it does in most cases and I'm looking forward to Rusty and/or Christian feedback on this : it seems a lesser evil than effectively paying more than twice the requested amount.

I think it is worth considering lifting the restriction on “twice the amount” on the receiving side for small payments

Maybe, or put a treshold for it to take effect ? Like >10sat.

[darosior]

Don't know if I introduced it with this patch, but test_peerinfo seems flaky on Travis : I had to restart the VALGRIND=1 ARCH=64 DEVELOPER=1 COMPILER=gcc TEST_GROUP=3 TEST_GROUP_COUNT=12 SOURCE_CHECK_ONLY=false job twice (and another one once).

[darosior]

Hmm I'm thinking that maybe we should still add the CLTV delta in this case, just not the amount.

[rustyrussell]

OK, so the test sometimes passes before the changes anyway, so let's make it firmer.

Otherwise, excellent. Needs "Changelog-Fixed: pay: don't send occasional wrong amount for tiny amounts" and "Changelog-Changed: pay: respect maxfeepercent when creating shadow routes" though.

diff --git a/tests/test_pay.py b/tests/test_pay.py
index c1373652f..af37eb0f9 100644
--- a/tests/test_pay.py
+++ b/tests/test_pay.py
@@ -462,18 +462,22 @@ def test_pay_maxfee_shadow(node_factory):
         lambda: len(l1.rpc.listchannels(source=l2.info["id"])["channels"]) > 1
     )
 
-    # A tiny amount, we must not add the base_fee between l2 and l3
-    amount = 2
-    bolt11 = l2.rpc.invoice(amount, "tiny", "tiny")["bolt11"]
-    pay_status = l1.rpc.pay(bolt11)
-    assert pay_status["amount_msat"] == Millisatoshi(amount)
-
-    # A bigger amount, shadow routing could have been used but we set a low
-    # maxfeepercent.
-    amount = 20000
-    bolt11 = l2.rpc.invoice(amount, "bigger", "bigger")["bolt11"]
-    pay_status = l1.rpc.pay(bolt11, maxfeepercent="0.000001")
-    assert pay_status["amount_msat"] == Millisatoshi(amount)
+    # shadow routes are a bit random, so run multiple times.
+    for i in range(5):
+        # A tiny amount, we must not add the base_fee between l2 and l3
+        amount = 2
+        bolt11 = l2.rpc.invoice(amount, "tiny.{}".format(i), "tiny")["bolt11"]
+        pay_status = l1.rpc.pay(bolt11)
+        assert pay_status["amount_msat"] == Millisatoshi(amount)
+
+    # shadow routes are a bit random, so run multiple times.
+    for i in range(5):
+        # A bigger amount, shadow routing could have been used but we set a low
+        # maxfeepercent.
+        amount = 20000
+        bolt11 = l2.rpc.invoice(amount, "big.{}".format(i), "bigger")["bolt11"]
+        pay_status = l1.rpc.pay(bolt11, maxfeepercent="0.000001")
+        assert pay_status["amount_msat"] == Millisatoshi(amount)
 
 
 def test_sendpay(node_factory):

[rustyrussell]

1. No way of disabling shadow routing. As I understand now, [this was intentional](https://github.com/ElementsProject/lightning/pull/3212#discussion_r342386943), however I think it would still be good to have an option for disabling it (e.g. my use-case is that I control multiple nodes connected to each other and I would like to be able to distribute liquidity among them through direct channels, and it is easier when the amounts being transferred are not fuzzed).

You can always use the lowlevel getroute and sendpay APIs if you want finer control (they can literally be chained together), or if they're direct peers you can make a route pretty easily without getroute at all?

[darosior]

OK, so the test sometimes passes before the changes anyway, so let's make it firmer.

Ok, I usually comment out the randomization to test shadow routing :)

lightning/plugins/pay.c

Lines 1107 to 1108 in 34ed209

	if (pseudorand(2) == 0)
	return start_pay_attempt(cmd, pc, "Initial attempt");

[kirelagin]

You can always use the lowlevel getroute and sendpay APIs if you want finer control (they can literally be chained together),

Yes, that’s what I was considering, but I would also need to waitsendpay and, in addition to that, I’m not sure if there are any potential unexpected transient failures that pay would retry for me.

or if they're direct peers you can make a route pretty easily without getroute at all?

For that I'd need to know the parameters of the channel, and rather than keeping track of them, it’s much easier to just cal getroute to be honest.

[rustyrussell]

Yes, pay will retry, but via a different path (if any available) which is probably not what you want? It won't wait for peers to come online or anything.

[rustyrussell]

Ack 7993285