-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
routing: update route hints when seeing a ChannelUpdate msg #5218
routing: update route hints when seeing a ChannelUpdate msg #5218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
func (p *shardHandler) handleSendError(attempt *channeldb.HTLCAttemptInfo, | ||
sendErr error) error { | ||
|
||
reason := p.router.processSendError( | ||
attempt.AttemptID, &attempt.Route, sendErr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is an alternative to pass the payment session to this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -118,11 +118,14 @@ type testGraph struct { | |||
|
|||
// testNode represents a node within the test graph above. We skip certain | |||
// information such as the node's IP address as that information isn't needed | |||
// for our tests. | |||
// for our tests. Private keys are optional. If set, they should be consistent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. So are you suggesting we switch these tests to make the graph created on the fly in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And unrelated to this PR, we probably want to refactor old tests to get rid of the static graph file someday?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
func (p *shardHandler) handleSendError(attempt *channeldb.HTLCAttemptInfo, | ||
sendErr error) error { | ||
|
||
reason := p.router.processSendError( | ||
attempt.AttemptID, &attempt.Route, sendErr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.go
Outdated
// Apply channel update to additional edges first. If no update is | ||
// performed, we will continue to update the channel edge policy in our | ||
// db. | ||
if p.paySession != nil && p.paySession.UpdateAdditionalEdge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, couldn't we check that the chan ID being updated is amongst the set of ephemeral edges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changed, now we first lookup the edge policy inside additionalEdges
, then decide which one to update.
routing/payment_session.go
Outdated
target := route.NewVertex(pubKey) | ||
|
||
for v, edges := range p.additionalEdges { | ||
if v != target { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do a map look up here (of the outer map) to avoid the outer loop iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah my bad, Changed.
3be6464
to
78ed2df
Compare
7c7da01
to
f0285b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this PR and added tests! 👍 Nothing major from me, looks pretty good.
routing/payment_lifecycle.go
Outdated
// If we received an unknown failure message from a node along the | ||
// route, the failure message will be nil. | ||
failureMessage := rtErr.WireMessage() | ||
if err := p.handleFailureMessage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit on this and more:
err := p.handleFailureMessage(
&attempt.Route, failureSourceIdx, failureMessage,
)
if err != nil {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait I thought the one-liner was the preferable style...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually only when it actually fits on a single line :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha got it. Guess I went too far on the one-line style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it back. Like it when we can decrease the total number of lines.
routing/payment_session_source.go
Outdated
@@ -138,6 +140,7 @@ func RouteHintsToEdges(routeHints [][]zpay32.HopHint, target route.Vertex) ( | |||
hopHint.FeeProportionalMillionths, | |||
), | |||
TimeLockDelta: hopHint.CLTVExpiryDelta, | |||
LastUpdate: time.Now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm this wasn't set at all before? What consequences did that have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say nothing since previously we didn't update the channel edge policy created from route hints.
b007d7f
to
981b406
Compare
Tests still fail, related to #5259 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nits from me at this point. LGTM 🚀
// UpdateAdditionalEdge updates the channel edge policy for a private edge. It | ||
// validates the message signature and checks it's up to date, then applies the | ||
// updates to the supplied policy. It returns a boolean to indicate whether | ||
// there's an error when applying the updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just return the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
aa1fac2
to
875a70d
Compare
cd829e6
to
203fb86
Compare
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.
b52b646
to
427fa22
Compare
Merged as part of #5332. |
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 intoChannelEdgePolicy
, we didn't update them after the edge updates them. This PR takes theChannelUpdate
message wrapped in the error message found insendPayment
, and applies the update if it was meant for theChannelEdgePolicy
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 ofPaymentSession
to access and modify theadditionalEdges
.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 methodUpdateAdditionalEdge
. Otherwise, we pass it toChannelRouter
to handle it.The Question
Although we treat the
RouteHints
as ephemeral info, it actually isn't. The route hints are stored inpayReq
during payment initialization, which means, users will see inconsistent records once the privateChannelEdgePolicy
is updated. The question is, should we go further to change the route hints stored in thepayReq
?The Alternative
Another thought is to treat the
ChannelEdgePolicy
created fromRouteHints
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.