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

htlcswitch+invoices: always return incorrect_or_unknown_payment_details #3391

Merged

Conversation

@joostjager
Copy link
Collaborator

commented Aug 9, 2019

In order to prevent information leaks by nodes probing with a payment hash, this commit changes exit hop processing so that it always returns incorrect_or_unknown_payment_details and leaves the prober in the dark about whether an invoice actually exists.

This implements part of lightningnetwork/lightning-rfc#608 by removing the deprecated failure message FinalExpiryTooSoon. This won't affect payment reliability, because this error was considered final just as IncorrectOrUnknownPaymentDetails is.

The follow up of this PR is #3186 in which the accepted height is reported, allowing senders to distinguish between incorrect details and delays on the forward path.

joostjager added 5 commits Jul 4, 2019
Align naming better with the lightning spec. Not the full name of the
failure (FailIncorrectOrUnknownPaymentDetails) is used, because this
would cause too many long lines in the code.
This commit fixes exit hop behavior to be in line with the lightning
spec.
Previously the time lock in the onion payload was reported. This is no
new information to the sender.
In order to prevent information leaks by nodes probing with a payment
hash, this commit changes exit hop processing so that it always returns
incorrect_or_unknown_payment_details and leaves the prober in the dark
about whether an invoice actually exists.
Copy link
Member

left a comment

LGTM 🛩

No major blockers, just some comments about leaving in some of the 4existing information for logging purposes on our end (the receiving end).

@@ -1025,9 +1020,8 @@ func TestChannelLinkMultiHopInsufficientPayment(t *testing.T) {
).Wait(30 * time.Second)
if err == nil {
t.Fatal("error haven't been received")
} else if !strings.Contains(err.Error(), "insufficient capacity") {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 9, 2019

Member

It may still be useful to report the error string we give out, for the sake of the RPC client. So we'd also extend this new assertFailureCode method to still assert our internal string.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 10, 2019

Author Collaborator

This sounds like a test that does not belong here. Is it right that you want tests that ensure that we return a specific string for an error over the rpc interface? In that case I'd think we should add those to lnwire. But how relevant is this really? We move to structured errors for SendToRoute, so for that call these strings won't appear anymore. Also SendPayment is moving to higher level structured messages.

htlcswitch/link.go Show resolved Hide resolved
// HtlcCancelReason defines reasons for which htlcs can be canceled.
type HtlcCancelReason uint8

const (

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 9, 2019

Member

It may still be useful to log this information internally for debugging purposes (dev using two nodes on simnet for example).

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 10, 2019

Author Collaborator

We do, see the debugLog statements in NotifyExitHopHtlc

Copy link
Collaborator

left a comment

LGTM

@cfromknecht cfromknecht merged commit 9a5ac78 into lightningnetwork:master Aug 13, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.02%) to 60.701%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.