Update routing hints upon insufficient fees failure. #2151

Bluetegu · 2018-11-05T15:19:38Z

This pull request address issue #2066

Reconstruct the problem:
Setup composed of 3 nodes, A -> B -> C, where B -> C is private.
Create an invoice on C (C:addinvoice with private flag)
Increase fee on B->C (B:updatechanpolicy to a higher fee)
Attempt to pay invoice from A (A:sendpayment --pay_req=xxx)
Receive payment error ("unable to route payment to destination: FeeInsufficient ... ")

After the fix the payment goes through.

A unit test on the 'basic graph' was added. The test adds a private channel between son goku and elst, with lower fees compared with the alternative routes from roasbeef to elst. A fee error is returned on first attempt to route through the private channel, with a higher fee, but still better compared with the alternative routes. The test checks that the after the update the payment is still routed through the private channel but with the updated higher fees.

A bug in the existing test TestSendPaymentErrorRepeatedFeeInsufficient was fixed too. I did a minimal fix. I believe this test can be further improved. The major issue was that the simulated error returned was rejected due to signature failure, and hence didn't simulate correctly the insufficient fees error as intended. In order to correctly sign the errors, an optional private key attribute was added to the basic graph and its parser.

joostjager · 2018-12-05T09:56:44Z

routing/missioncontrol.go

@@ -291,6 +291,27 @@ func generateBandwidthHints(sourceNode *channeldb.LightningNode,
 	return bandwidthHints, nil
 }

+// UpdateEdgePolicy updates the edge policy of the routing hints kept in the
+// payment session. The number of hints updated is returned.
+func (p *paymentSession) UpdateEdgePolicy(e *channeldb.ChannelEdgePolicy) uint32 {


It seems the return value is used nowhere and can just as well be removed.

joostjager · 2018-12-05T10:03:12Z

routing/router.go

@@ -1838,6 +1840,8 @@ func (r *ChannelRouter) sendPayment(payment *LightningPayment,
 			// newly updated fees.
 			case *lnwire.FailFeeInsufficient:


This is not the only case where we can get back a channel update. For example, FailIncorrectCltvExpiry. I think we should try to update additionalEdges also in those cases.

Can the update maybe be done inside applyChannelUpdate ?

routing/router_test.go

routing/pathfind_test.go

routing/router_test.go

joostjager · 2018-12-05T10:23:36Z

routing/router_test.go

+	t.Parallel()
+
+	const startingBlockHeight = 101
+	ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath)


There are currently two ways of writing these kind of path finding tests. One is the way you did it, using the json file. The other option is used in for example TestChannelUpdateValidation. It allows programmatically building the test graph and generates some values like the keys by itself (as the actual value isn't important for the tests, it just needs a value). You could consider using that instead. I think it is clearer than keep adding new channels to the json file.

I think that you will still have the problem of needing a private key even if you use TestChannelUpdateValidation approach, as the validation of the error message is part of the tested code. Anyhow, I like the approach of using the basic graph in many tests all based on a non trivial topology. It requires the writer of the test to rethink whether the logic indeed covers all scenarios, beyond the simplest topology needed for testing the particular feature or fix she was aiming to do.

That's okay, just wanted to mention the option.

joostjager · 2018-12-05T10:25:55Z

Needs to be rebased

joostjager · 2018-12-05T10:29:03Z

Also, have a look at the commit message format described in https://dev.lightning.community/contribute/. We add the subsystem prefix to the message.

Bluetegu · 2018-12-05T17:30:32Z

Thanks for the review!
I rebased and changed the commit message(s). The code in router.go looks much better all around.
I addressed most of the comments. Please see responses to individual comments inline.

Bluetegu · 2018-12-08T21:05:58Z

I rebased again, but for some reason the third CI test (coverage) fails in auto pilot and I don't think this has anything to do with the code modification in this PR. I forced push twice already and got the same error there.

joostjager · 2018-12-10T09:31:48Z

routing/router.go

@@ -1818,6 +1820,12 @@ func (r *ChannelRouter) sendPayment(payment *LightningPayment,
 				update *lnwire.ChannelUpdate,
 				pubKey *btcec.PublicKey) {

+				err := ValidateChannelUpdateAnn(pubKey, update)
+				if err != nil {
+					log.Errorf("Unable to validate channel update: %v", err)


This line and others too exceed the max line width of 80 chars (project coding standard).

The log line is of 77 width. I believe I kept the <80 width everywhere.

I have it at 96 chars. Do you have tab size 8?

tab size 4. Using vscode.

The standard is 80 chars max @ tab size 8.

Sorry about that. Fixed.

joostjager · 2018-12-10T09:32:37Z

routing/router.go

@@ -1818,6 +1820,12 @@ func (r *ChannelRouter) sendPayment(payment *LightningPayment,
 				update *lnwire.ChannelUpdate,
 				pubKey *btcec.PublicKey) {

+				err := ValidateChannelUpdateAnn(pubKey, update)


Why is this added here? Validation is also done inside r.applyChannelUpdate.

I agree this is ugly. My excuse it that I'm trying to minimize the changes I do in code to those mandatory for this PR. Therefore, if validation fails, processChannelUpdateAndRetry should immediately return, and should not update paySession at all (neither update hints or report edge policy failure), so the check must be there. However we can not rely on the error returned by applyChannelUpdate, as it may well error in other scenarios (i.e. not validation errors) which are perfectly ok, and do require retries. Initially I thought of doing the validation much earlier, i.e. before the switch on the error types, but this looks like a change that is too major for this PR; hence my feeling was that adding validation here is the right way to go, with only bad consequence being that its checked twice.

I think it is ok to not update the payment session if the graph update fails. ReportEdgeFailure is important to call in case of a validation error. If we get an incorrect update we don't want to try that channel anymore.

Ok. I removed the extra validation and returned if applyChannelUpdate fails after the edge is pruned.

I'm uncomfortable with the handling of error messages that fail in signature validation, as they do prune edges. Doesn't that allow an attacker to send error messages and cause the routing to fail?

joostjager · 2018-12-10T09:33:45Z

routing/router.go

+						FeeBaseMSat:               uBaseFee,
+						FeeProportionalMillionths: uFeeRate,
+					})
+


You've added it in here, but for other cases where r.applyChannelUpdate is called directly, the payment session isn't updated.

My idea is that the hints are updated only when a retry is aimed at. It now handles all those cases (not only insufficient fees). Otherwise the vertex or channel is removed completely, and there is no need for the update. I thought that is the whole purpose of defining processChannelUpdateAndRetry (which includes the paySession in its closure).
Said in another way, it doesn't seem right to me to plug it into applyChannelUpdate, exactly because this function is called for changing database edges/vertexes (and don't have paySession in closure).

Yes, I think you're right. All other calls to applyChannelUpdate prune immediately, so those edges will never show up in subsequent payment attempts anymore.

routing/router_test.go

joostjager · 2018-12-10T09:45:38Z

routing/router_test.go

+	t.Parallel()
+
+	const startingBlockHeight = 101
+	ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath)


That's okay, just wanted to mention the option.

routing/router_test.go

joostjager · 2018-12-10T11:05:22Z

routing/missioncontrol.go

+	// run over hints and update policy if edge id matches
+	for _, edges := range p.additionalEdges {
+		for _, edge := range edges {
+			if edge.ChannelID == e.ChannelID {


Are you updating the channel in both directions? I think you may also need to match the pubkey in additional edges to update only a single direction.

The channel is updated only in one direction. That is, the node generates an error on the outgoing channel policy, and you update the hints accordingly. Am I missing something here?

I mean, suppose you have two additional edges relating to the same channel id but in opposite direction. They will both be updated?

Yes. I changed it such that it takes direction into account too, such that policy will be updated only if channel ID and direction both match.
I think this also fixes a general hidden bug in routing through hint hops, not related to this PR, as additionalEdges policies didn't take into account direction. That is, the Flags were not initialized in NewPaymentSession, and routing assumes the direction is initialized in all edge policies.

Bluetegu · 2018-12-10T20:04:25Z

Ready for inspection :-)
By some miracle the cover test passed too.
Please see inline my response to the review issues.
Thanks!

routing/router_test.go

joostjager · 2018-12-11T08:00:30Z

routing/missioncontrol.go

+			if edge.ChannelID == e.ChannelID &&
+				edge.Flags&lnwire.ChanUpdateDirection ==
+					e.Flags&lnwire.ChanUpdateDirection {
+				edge.FeeBaseMSat = e.FeeBaseMSat


New line after wrapped line

joostjager · 2018-12-11T08:08:55Z

routing/router.go

+				var direction lnwire.ChanUpdateFlag
+				if failedEdge.direction == 1 {
+					direction = lnwire.ChanUpdateDirection
+				}


failedEdge.direction can be used directly.

Not sure. failedEdge direction is unit8 while Flags are unit16.

Ok, you may need to do a type cast, but the if statement is not necessary.

joostjager · 2018-12-11T08:10:00Z

routing/missioncontrol.go

+// UpdateEdgePolicy updates the edge policy of the routing hints kept in the
+// payment session.
+func (p *paymentSession) UpdateEdgePolicy(e *channeldb.ChannelEdgePolicy) {
+


remove newline

joostjager · 2018-12-11T08:10:10Z

routing/missioncontrol.go

+// payment session.
+func (p *paymentSession) UpdateEdgePolicy(e *channeldb.ChannelEdgePolicy) {
+
+	// run over hints and update policy if edge id and direction matches


Capital at beginning of sentence. Period at end.

joostjager · 2018-12-11T08:15:39Z

routing/router.go

-			log.Error(err)
-			return err
-		}
+		if exists {


Is this a fix in itself? Ideally this could be yet another separate commit, but if you find it too small for that, it's fine with me.

I had to add it as otherwise you would get errors on applyChannelUpdate when trying to update private channels hops.

I think maybe the proper fix here is something like this: ad92e5c (feel free to cherry-pick it into this PR if needed).

I believe then you wouldn't have to flip the AssumeChannelValid in the config during tests?

Created #2549

This can be removed now that #2549 is merged.

joostjager · 2018-12-11T08:17:00Z

routing/router.go

+				uBaseFee := lnwire.MilliSatoshi(update.BaseFee)
+				uFeeRate := lnwire.MilliSatoshi(update.FeeRate)
+				uMinHTLC := lnwire.MilliSatoshi(
+					update.HtlcMinimumMsat)


This vars are introduced because of line length I assume. But maybe that indicates that now is the time to extract this closure to a separate function.

Yes. But I don't get much if I move it to a separate function defined within the closure, as the names are so long here that it doesn't make it much better. I rather leave it as is. Defining a separate function altogether for an internal code that is called once is not something I personally prefer.

There are plenty of internal functions that are called only once, it does help to make the code more understandable. Inline functions can be useful especially if a lot of local vars are bound.

How about just creating a function taking failedEdge and update and returning the ChannelEdgePolicy? Or better, updating the signature of UpdateEdgePolicy to take failedEdge and update. Also consistent with other pay session funcs that take an edgeLocator too.

Yes, this is the way to go. Thanks!

joostjager · 2018-12-11T08:17:29Z

routing/router_test.go

+
+	// The route should pass via the private channel
+	if !route.containsChannel(privateChannelID) {
+		t.Fatalf("route did not pass through private channel between pham nuwen " +


Line length.

routing/router_test.go

joostjager · 2018-12-11T08:20:27Z

routing/missioncontrol.go

@@ -219,9 +226,9 @@ func (m *missionControl) NewPaymentSession(routeHints [][]HopHint,
 					hopHint.FeeProportionalMillionths,
 				),
 				TimeLockDelta: hopHint.CLTVExpiryDelta,
+				Flags:         direction,


This was indeed not set, but I think it didn't have impact before, because that flag was never read right?

I think it did have impact, i.e. it was an open bug, but the situations where it matters were rare enough such that it wasn't noticed. I haven't fully thought this out. What happened was that when I tried to fix UpdateEdgePolicy to consider direction as well, following your comment, the (new) test failed, and adding direction here fixed it.

Yes, it was an omission that surfaced when you fixed UpdateEdgePolicy. But I think that before that, the situations where it mattered where non-existent. Just interested to know whether that was true. In any case, it is fixed now.

Bluetegu · 2018-12-11T10:36:31Z

Changes made.
The CI cover test failed, but I believe its a bug in the CI test itself (there is an open PR that fixes the coverage test).
Please see inline my response to the review issues.
Thanks!

btcdanny · 2019-01-27T19:04:11Z

@halseth Hi Johan, Is the block tag necessary? Let's try to push this to completion. Thanks!

This would be great if possible, as we at Fastbitcoins are encountering the issue this PR addresses.

halseth · 2019-01-28T08:29:49Z

@Bluetegu Yes, let's do that :) I created a smaller PR that addresses the exact thing that this is blocked by: #2549

Bluetegu · 2019-01-28T09:35:18Z

Ok, great. Let me know if anything is needed on my side.

halseth · 2019-02-14T14:34:09Z

@Bluetegu #2549 is now merged, we can move this forward :)

Bluetegu · 2019-02-14T14:50:40Z

@halseth Thanks!

halseth

I think this is close after a rebase! That should let you remove the AssumeChanValid hack now that #2549 is merged.

Again, really good find for the direction pruning bug, and the added test case is 🔥

halseth · 2019-03-28T12:36:10Z

routing/router_test.go

+	// before further processing an edge update. Toggel AssumeChannelValid
+	// to avoid dropping the channel update, as the utxo set query is
+	// not mocked in this test.
+	ctx.router.cfg.AssumeChannelValid = true


Is this still needed?

halseth · 2019-03-28T12:39:18Z

routing/router_test.go

+// r3: C->E is pruned
+// leaving r4 as viable 4th route
+//
+// This test was added to test direction initialization in NewPaymentSession;


Excellent test description! 👍

halseth · 2019-03-28T12:40:21Z

routing/router_test.go

+	ctx, cleanUp, err := createTestCtxFromGraphInstance(startingBlockHeight,
+		testGraph)
+
+	defer cleanUp()


move defer cleanUp() after error check

halseth · 2019-03-28T12:44:10Z

routing/router_test.go

+	// before further processing an edge update. Toggel AssumeChannelValid
+	// to avoid dropping the channel update, as the utxo set query is
+	// not mocked in this test.
+	ctx.router.cfg.AssumeChannelValid = true


not needed anymore I think

halseth · 2019-03-28T12:44:24Z

routing/router_test.go

+	// not mocked in this test.
+	ctx.router.cfg.AssumeChannelValid = true
+
+	// Send payment


nit: missing period.

halseth · 2019-03-28T12:49:52Z

routing/payment_session.go

@@ -35,6 +35,51 @@ type paymentSession struct {
 	preBuiltRoutes []*Route
 }

+// edgeLocatorOfEdgePolicy returns the edge locator corresponding to
+// the channel edge policy
+func edgeLocatorOfEdgePolicy(ep *channeldb.ChannelEdgePolicy) *edgeLocator {


just return non-pointer value.

halseth · 2019-03-28T12:50:19Z

routing/payment_session.go

+
+// UpdateEdgePolicy updates edge policy of the routing hints kept in the
+// payment session.
+func (p *paymentSession) UpdateEdgePolicy(el *edgeLocator,


also better to take non-pointer for the edgeLocator here.

halseth · 2019-03-28T12:51:21Z

routing/router.go

-			log.Error(err)
-			return err
-		}
+		if exists {


This can be removed now that #2549 is merged.

halseth · 2019-03-28T12:52:39Z

routing/router_test.go

+
+	const startingBlockHeight = 101
+	ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath)
+	defer cleanUp()


same comment wrt cleanup

halseth · 2019-03-28T12:53:01Z

routing/router_test.go

+	// before further processing an edge update. Toggel AssumeChannelValid
+	// to avoid dropping the channel update, as the utxo set query is
+	// not mocked in this test.
+	ctx.router.cfg.AssumeChannelValid = true


can be removed

and its parser. Required to correctly sign and later verify error messages in some test scenarios.

The simulated error returned was rejected due to signature failure, and didn't simulate correctly the insufficient fees error as intended. Fix error by including correct signature.

Hints are updated if the error message is either: - FailFeeInsufficient - FailAmountBelowMinimum - FailIncorrectCltvExpiry

Bluetegu · 2019-04-06T18:51:01Z

I did the rebase, but I cannot figure out why the new direction unitest TestSendPaymentPrivateEdgeDirection fails. It didn't fail in the version before the rebase.
I started a new position and unfortunately I no longer have time to work on LND. I tried to fix this issue the last two weekends without success.
I uploaded my latest attempt. The last commit include all the unfinished code.
I hope someone can continue and complete this work (as well as the other of my open PRs), as it contains fixes for two bugs.
I'll do my best to answer questions etc., and can do some work in weekend, but my bandwidth is very limited now.

halseth · 2019-04-08T12:00:51Z

@Bluetegu No worries! Thanks for your excellent contributions 👍

halseth · 2020-07-17T10:15:03Z

Somebody should pick this up and rebase the still relevant parts.

Roasbeef · 2021-06-25T01:43:15Z

Replaced by #5332

halseth added routing payments Related to invoices/payments P3 might get fixed, nice to have labels Nov 7, 2018

halseth added this to the 0.6 milestone Nov 7, 2018

Roasbeef requested a review from joostjager December 4, 2018 04:14

joostjager reviewed Dec 5, 2018

View reviewed changes

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch from 57fbf15 to cc5444f Compare December 5, 2018 16:56

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch 5 times, most recently from 1ba5022 to 4b1f171 Compare December 8, 2018 20:51

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch from 4b1f171 to 2ef99a0 Compare December 9, 2018 16:58

joostjager suggested changes Dec 10, 2018

View reviewed changes

joostjager reviewed Dec 10, 2018

View reviewed changes

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch 7 times, most recently from cf3f300 to a0ccce4 Compare December 10, 2018 19:41

joostjager suggested changes Dec 11, 2018

View reviewed changes

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch from a0ccce4 to d469a0e Compare December 11, 2018 10:06

joostjager mentioned this pull request Jan 28, 2019

LND fails a payment on getting FeeInsufficient instead of retrying it with updated fees #2540

Closed

Roasbeef added this to the 0.6 milestone Jan 28, 2019

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch 2 times, most recently from 373efcd to 5868c85 Compare February 10, 2019 09:58

halseth removed the blocked label Feb 14, 2019

wpaulino removed this from the 0.6 milestone Mar 25, 2019

halseth suggested changes Mar 28, 2019

View reviewed changes

High Priority automation moved this from Final Testing -- Ready For Merge to Needs review Mar 28, 2019

Bluetegu added 4 commits March 30, 2019 17:38

routing: add optional private key attribute to the basic graph file

3f982da

and its parser. Required to correctly sign and later verify error messages in some test scenarios.

routing: fix TestSendPaymentErrorRepeatedFeeInsufficient unit test.

fdeb896

The simulated error returned was rejected due to signature failure, and didn't simulate correctly the insufficient fees error as intended. Fix error by including correct signature.

router: fix NewPaymentSession not taking edge direction into account.

40de2ed

routing: update routing hints upon receipt of error messages.

718a2e0

Hints are updated if the error message is either: - FailFeeInsufficient - FailAmountBelowMinimum - FailIncorrectCltvExpiry

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch from 5868c85 to 0f1d233 Compare April 6, 2019 18:26

Rebase work. Error still appears in direction testings.

3d2cea0

Bluetegu force-pushed the retry-upon-onion-error-private-2066 branch from 0f1d233 to 3d2cea0 Compare April 6, 2019 18:37

halseth added help wanted needs rebase PR has merge conflicts labels Jul 17, 2020

joostjager mentioned this pull request Jan 26, 2021

Encountering AmountBelowMinimum routing error in pathfinding is not gracefully handled #4807

Closed

yyforyongyu self-assigned this Apr 10, 2021

yyforyongyu mentioned this pull request Apr 16, 2021

routing: update route hints when seeing a ChannelUpdate msg #5218

Closed

Roasbeef closed this Jun 25, 2021

		@@ -1838,6 +1840,8 @@ func (r ChannelRouter) sendPayment(payment LightningPayment,
		// newly updated fees.
		case *lnwire.FailFeeInsufficient:

Update routing hints upon insufficient fees failure. #2151

Update routing hints upon insufficient fees failure. #2151

Conversation

Bluetegu commented Nov 5, 2018

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

joostjager commented Dec 5, 2018

joostjager commented Dec 5, 2018

Bluetegu commented Dec 5, 2018

Bluetegu commented Dec 8, 2018

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

joostjager Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bluetegu commented Dec 10, 2018

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

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

Bluetegu commented Dec 11, 2018

btcdanny commented Jan 27, 2019

halseth commented Jan 28, 2019

Bluetegu commented Jan 28, 2019

halseth commented Feb 14, 2019

Bluetegu commented Feb 14, 2019

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

Bluetegu commented Apr 6, 2019

halseth commented Apr 8, 2019

joostjager Dec 10, 2018 •

edited

Loading