Skip to content
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

[reliable payments] Move payment error deobfuscation to router #3063

Merged
merged 10 commits into from May 17, 2019

Conversation

@halseth
Copy link
Collaborator

@halseth halseth commented May 9, 2019

This PR consists of two important pieces of #2761:

Make router<->switch interaction async

We define PaymentResult and the method GetPaymentResult
exposed by the switch.

This lets us distinguish an critical error from a actual payment result
(success or failure). This is important since we know that we can only
attempt another payment when a final result from the previous payment
attempt is received.

In order to achieve this we let the router handling generating paymentIDs,
since they are used to later query the switch for the final result.

Move payment error deobfuscation

In this commit we move handing the deobfuscator from the router to the
switch from when the payment is initiated, to when the result is
queried.

We do this because only the router can recreate the deobfuscator after a
restart, and we are preparing for being able to handle results across
restarts.

Since the deobfuscator cannot be nil anymore, we can also get rid of
that special case.

Split off from #2761

@halseth halseth force-pushed the router-error-deobfuscation branch 2 times, most recently from d1dc602 to 8575d62 May 9, 2019
@halseth halseth force-pushed the router-error-deobfuscation branch 2 times, most recently from ba14744 to 77ffe6c May 9, 2019
routing/router.go Outdated Show resolved Hide resolved
channeldb/graph_test.go Outdated Show resolved Hide resolved
htlcswitch/link_test.go Show resolved Hide resolved
@halseth halseth force-pushed the router-error-deobfuscation branch 5 times, most recently from d9ccc34 to 80c2dfc May 14, 2019
@halseth halseth requested a review from joostjager May 14, 2019
@halseth
Copy link
Collaborator Author

@halseth halseth commented May 14, 2019

PTAL @Roasbeef

Copy link
Collaborator

@joostjager joostjager left a comment

Looking good

routing/mock_test.go Show resolved Hide resolved
htlcswitch/payment_result.go Outdated Show resolved Hide resolved
htlcswitch/payment_result.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
routing/mock_test.go Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

@halseth completed an initial pass, most everything looks good though i think there's a potential race between GetPaymentResult and removePendingPayment.

routing/mock_test.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
server.go Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
@halseth halseth force-pushed the router-error-deobfuscation branch 9 times, most recently from f4a6cd4 to a3fbc87 May 16, 2019
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/payment_result.go Show resolved Hide resolved
@halseth halseth force-pushed the router-error-deobfuscation branch 2 times, most recently from ebcfe7b to 7e552a0 May 16, 2019
@cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented May 16, 2019

needs rebase as well :)

halseth added 10 commits May 16, 2019
The switch satisfies this interface, and makes it easy to mock the send
method from the router.
This commit moves the responsibility of generating a unique payment ID
from the switch to the router. This will make it easier for the router
to keep track of which HTLCs were successfully forwarded onto the
network, as it can query the switch for existing HTLCs as long as the
paymentIDs are kept.

The router is expected to maintain a map from paymentHash->paymentID,
such that they can be replayed on restart. This also lets the router
check the status of a sent payment after a restart, by querying the
switch for the paymentID in question.
We will later persist the fields necessary to decrypt a received error.
With the following commits, it'll become important to not resuse
paymentIDs, since there is no way to tell whether the HTLC in question
has already been forwarded and settled/failed.

We clarify this in the SendHTLC comments, and alter the tests to not
attempt to resend an HTLC with a duplicate payment ID.
This lets us distinguish an critical error from a actual payment result
(success or failure). This is important since we know that we can only
attempt another payment when a final result from the previous payment
attempt is received.
We extract the session key such that we can later store it across
restarts.
In this commit we move handing the deobfuscator from the router to the
switch from when the payment is initiated, to when the result is
queried.

We do this because only the router can recreate the deobfuscator after a
restart, and we are preparing for being able to handle results across
restarts.

Since the deobfuscator cannot be nil anymore, we can also get rid of
that special case.
@halseth halseth force-pushed the router-error-deobfuscation branch from 7e552a0 to cd02c22 May 16, 2019
@halseth
Copy link
Collaborator Author

@halseth halseth commented May 16, 2019

Rebased :) PTAL @joostjager @cfromknecht

Copy link
Collaborator

@cfromknecht cfromknecht left a comment

Another step closer to reliable payments! LGTM 👌

Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🌋

@Roasbeef Roasbeef merged commit 30cb667 into lightningnetwork:master May 17, 2019
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants