routing: update route hints when seeing a ChannelUpdate msg by yyforyongyu · Pull Request #5218 · lightningnetwork/lnd

yyforyongyu · 2021-04-16T16:54:45Z

The issue is defined in #2540.

In short, because we treat the RouteHints as ephemeral info which doesn't belong to db, when we first heard them from the invoice and convert them into ChannelEdgePolicy, we didn't update them after the edge updates them. This PR takes the ChannelUpdate message wrapped in the error message found in sendPayment, and applies the update if it was meant for the ChannelEdgePolicy of the private channel.

The Change

In order to be able to update private edge policies, some refactor has been done, as in,

shardHandler has to be aware of PaymentSession to access and modify the additionalEdges.
we now handle send error messages in shardHandler, decide whether the update is for private or public channels. PaymentSession will handle it first if the update is for the private channel, using the new method UpdateAdditionalEdge. Otherwise, we pass it to ChannelRouter to handle it.

The Question

Although we treat the RouteHints as ephemeral info, it actually isn't. The route hints are stored in payReq during payment initialization, which means, users will see inconsistent records once the private ChannelEdgePolicy is updated. The question is, should we go further to change the route hints stored in the payReq?

The Alternative

Another thought is to treat the ChannelEdgePolicy created from RouteHints as normal policies, which means we can save them to db and process them using the logic path created for public channel. The downside is a) we might see the db grows too large (maybe prune after each payment?) and b) users could slowly build up the graph using the route hints thus breach privacy.

This PR took the tests from #2151, rebased, refactored, and included in this PR. There is still this excellent test TestSendPaymentPrivateEdgeDirection, which I will include in another PR to keep the current one compact and focused.

Fixes #2540.
Fixes #4807.

halseth

should we go further to change the route hints stored in the payReq

I don't think we should, since it will make the invoice invalid. For record keeping it feels like e rather should store it unchanged, the information we learn during path finding won't be used again later anyway.

halseth · 2021-04-20T15:23:14Z

routing/payment_lifecycle.go

is an alternative to pass the payment session to this method?

To processSendError itself? IMO this is a better change as it moves the error handling closer to the source in a sense as the shard handler is what's responsible for retrying, tracking the set of in-flight shards, reporting the failure to mission control, etc.

is an alternative to pass the payment session to this method?

Yeah that's indeed an alternative, and probably a change easier to make. However, after going through the current design, I think our ChannelRouter is made intentionally to not care about in-memory states such as PaymentSession. If we want to go the alternative way, ChannelRouter would need to store PaymentSession in its struct to be able to use it inside the method processSendError. Plus what @Roasbeef said above.

Roasbeef · 2021-04-20T16:26:01Z

The question is, should we go further to change the route hints stored in the payReq?

As mentioned above, if we do this, then the sig on the invoice will no longer validate as it's now essentially invalid. It's also the case that w/e we update it to can also be invalidated when the private channel updates their policies once again. Early in the network private channels didn't update policies much as there was no clear use, but lately many wallets implement "on the fly" channel creation and use a change in the "fake" private channel chan update to have the sender retry once the channels parmaters have been finalized.

Roasbeef

Nice job resurrecting such an old PR! This'll really help out many of the popular LN wallets that utilize private channel updates to improve UX by opening channels on the fly.

Roasbeef · 2021-04-20T16:28:20Z

routing/pathfind_test.go

FWIW new tests don't really use the old static graph file and instead opt tin create a graph on the fly using some of the newly added test helpers. However I understand this was a revived PR and at the time of writing of this original commit, this was the only/preferred way to do things.

Got it. So are you suggesting we switch these tests to make the graph created on the fly in this PR?

And unrelated to this PR, we probably want to refactor old tests to get rid of the static graph file someday?

Got it. So are you suggesting we switch these tests to make the graph created on the fly in this PR?

Don't think it's a blocker, as this commit only exists as we salvaged the OG commit from the contributor.

routing/router_test.go

routing/router.go

Roasbeef · 2021-04-20T16:35:54Z

routing/payment_lifecycle.go

To processSendError itself? IMO this is a better change as it moves the error handling closer to the source in a sense as the shard handler is what's responsible for retrying, tracking the set of in-flight shards, reporting the failure to mission control, etc.

routing/payment_lifecycle_test.go

routing/payment_session.go

Roasbeef · 2021-04-20T16:44:56Z

routing/payment_lifecycle.go

Alternatively, couldn't we check that the chan ID being updated is amongst the set of ephemeral edges?

This is changed, now we first lookup the edge policy inside additionalEdges, then decide which one to update.

Roasbeef · 2021-04-20T16:46:00Z

routing/payment_session.go

We can do a map look up here (of the outer map) to avoid the outer loop iteration.

Yeah my bad, Changed.

lntest/itest/lnd_routing_test.go

halseth

Great work on this PR and added tests! 👍 Nothing major from me, looks pretty good.

routing/payment_lifecycle_test.go

routing/router_test.go

routing/router.go

halseth · 2021-04-30T10:39:09Z

routing/payment_lifecycle.go

style nit on this and more:

err := p.handleFailureMessage( &attempt.Route, failureSourceIdx, failureMessage, ) if err != nil {

Wait I thought the one-liner was the preferable style...?

usually only when it actually fits on a single line :)

Ha got it. Guess I went too far on the one-line style.

Changed it back. Like it when we can decrease the total number of lines.

halseth · 2021-04-30T10:40:02Z

routing/payment_session_source.go

hm this wasn't set at all before? What consequences did that have?

I'd say nothing since previously we didn't update the channel edge policy created from route hints.

routing/payment_session.go

routing/payment_lifecycle.go

lntest/itest/lnd_routing_test.go

yyforyongyu · 2021-05-01T08:28:55Z

Tests still fail, related to #5259

halseth

Only nits from me at this point. LGTM 🚀

halseth · 2021-05-03T07:29:53Z

routing/payment_session.go

why not just return the error?

Yeah I thought about that too. Then I thought it might be better to have the same structure as the method p.router.applyChannelUpdate. Plus the error found in this one won't be handled except for logging.

Roasbeef

LGTM 🏎

Roasbeef · 2021-05-06T02:58:49Z

routing/pathfind_test.go

Got it. So are you suggesting we switch these tests to make the graph created on the fly in this PR?

Don't think it's a blocker, as this commit only exists as we salvaged the OG commit from the contributor.

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.

This commit refactors some of the tests in router_test.go to use the require package.

This commit moves the handleSendError method from ChannelRouter to shardHandler. In doing so, shardHandler can now apply updates to the in-memory paymentSession if they are found in the error message.

This commit adds the method UpdateAdditionalEdge in PaymentSession, which allows the addtional channel edge policy to be updated from a ChannelUpdate message. Another method, GetAdditionalEdgePolicy is added to allow querying additional edge policies.

This commit adds payment session to shardHandler to enable private edge policies being updated in shardHandler. The relevant interface and mock are updated. From now on, upon seeing a ChannelUpdate message, shardHandler will first try to find the target policy in additionalEdges and update it. If nothing found, it will then check the database for edge policy to update.

Roasbeef · 2021-06-25T01:42:48Z

Merged as part of #5332.

yyforyongyu requested review from Roasbeef and halseth April 16, 2021 16:55

halseth suggested changes Apr 20, 2021

View reviewed changes

Roasbeef added this to the 0.13.0 milestone Apr 20, 2021

Roasbeef added P2 should be fixed if one has time bug fix routing nodes labels Apr 20, 2021

Roasbeef requested changes Apr 20, 2021

View reviewed changes

Roasbeef mentioned this pull request Apr 20, 2021

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

Closed

yyforyongyu force-pushed the 2540-fix-retry-err branch 5 times, most recently from 3be6464 to 78ed2df Compare April 29, 2021 03:14

Roasbeef requested review from Roasbeef and halseth April 29, 2021 20:58

yyforyongyu force-pushed the 2540-fix-retry-err branch 3 times, most recently from 7c7da01 to f0285b9 Compare April 30, 2021 08:49

halseth suggested changes Apr 30, 2021

View reviewed changes

yyforyongyu force-pushed the 2540-fix-retry-err branch 2 times, most recently from b007d7f to 981b406 Compare May 1, 2021 08:27

yyforyongyu requested a review from halseth May 1, 2021 08:30

halseth approved these changes May 3, 2021

View reviewed changes

yyforyongyu force-pushed the 2540-fix-retry-err branch 2 times, most recently from aa1fac2 to 875a70d Compare May 5, 2021 10:16

Roasbeef approved these changes May 6, 2021

View reviewed changes

halseth enabled auto-merge May 6, 2021 08:31

yyforyongyu force-pushed the 2540-fix-retry-err branch 3 times, most recently from cd829e6 to 203fb86 Compare May 20, 2021 00:30

Roasbeef requested a review from cfromknecht May 20, 2021 00:34

yyforyongyu and others added 17 commits May 26, 2021 20:32

routing: make payment lifecycle test more verbose

e0a9c2e

routing: add private key attribute in router test

f3b9b19

routing: fix TestSendPaymentErrorRepeatedFeeInsufficient

d35304d

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.

routing: test private edge on fee error

f873c74

routing: parse Channel ID from json file

c74a754

routing: rm hardcoded channel id in router test

5c3cd27

routing: use require in router test

cd6f483

This commit refactors some of the tests in router_test.go to use the require package.

routing: use shardHandler to process err in SendToRoute

5de0ca2

routing: move sendErr handling in shardHandler

ef12011

This commit moves the handleSendError method from ChannelRouter to shardHandler. In doing so, shardHandler can now apply updates to the in-memory paymentSession if they are found in the error message.

routing: refactor TestSendPaymentErrorFeeInsufficientPrivateEdge

940ec75

routing: add TestSendPaymentPrivateEdgeUpdateFeeExceedsLimit

109e4da

itest: moving routing related tests into one file

0b10206

itest: fix make lint

04beb84

itest: test channel policy update in private channels

98822a1

itest: silence depreciation warnings

427fa22

yyforyongyu force-pushed the 2540-fix-retry-err branch 2 times, most recently from b52b646 to 427fa22 Compare May 26, 2021 12:43

yyforyongyu mentioned this pull request May 26, 2021

routing: fix payment state and refactor payment lifecycle tests #5332

Merged

Roasbeef mentioned this pull request Jun 15, 2021

payment failure with wallet that open channels on the fly GaloyMoney/blink#32

Closed

Roasbeef modified the milestones: 0.13.0, 0.13.1 Jun 15, 2021

Roasbeef closed this Jun 25, 2021

yyforyongyu deleted the 2540-fix-retry-err branch June 25, 2021 07:29

Conversation

yyforyongyu commented Apr 16, 2021 • edited by Roasbeef Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Change

The Question

The Alternative

Uh oh!

halseth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Apr 20, 2021

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu commented May 1, 2021

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

yyforyongyu commented Apr 16, 2021 •

edited by Roasbeef

Loading

halseth left a comment •

edited

Loading