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

lnwire+htlcswitch: report htlc accept height #3414

Merged
merged 5 commits into from Sep 16, 2019

Conversation

@joostjager
Copy link
Collaborator

@joostjager joostjager commented Aug 19, 2019

Implements lightning/bolts#608 by extending the incorrect_or_unknown_payment_details message with the height at which the failed htlc was accepted.

@joostjager joostjager changed the title lnwire+htlcswitch: report htlc accept height [wip] [no review] lnwire+htlcswitch: report htlc accept height [wip] [don't review] Aug 23, 2019
@joostjager joostjager force-pushed the report-accept-height branch from 630fe2a to 7de1e62 Sep 5, 2019
@joostjager joostjager changed the title lnwire+htlcswitch: report htlc accept height [wip] [don't review] lnwire+htlcswitch: report htlc accept height Sep 5, 2019
@joostjager joostjager force-pushed the report-accept-height branch 2 times, most recently from 2ca1e27 to ae146db Sep 5, 2019
@joostjager joostjager requested a review from cfromknecht Sep 5, 2019
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

first pass looks pretty good, only one rpc-related comment 👍

@@ -239,6 +239,9 @@ message Failure {
the failure message. Position zero is the sender node.
**/
uint32 failure_source_index = 8;

/// A failure type-dependent block height.
uint32 height = 9;
Copy link
Collaborator

@cfromknecht cfromknecht Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps something like accept_height, to mimic what the invoice htlc's will show?

Copy link
Collaborator Author

@joostjager joostjager Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but wasn't sure. The idea of the message Failure is that it isn't a oneof to keep it simple. Fields are reused in different messages. This height is meant as a generic height field associated with a failure. But yes, I could also make it more specific.

Copy link
Collaborator

@halseth halseth left a comment

Looks more or less good to me :)

invoices/invoiceregistry.go Outdated Show resolved Hide resolved
lnwire/onion_error_test.go Outdated Show resolved Hide resolved
lnwire/onion_error.go Show resolved Hide resolved
@joostjager joostjager force-pushed the report-accept-height branch from eab6793 to ec8602b Sep 10, 2019
@joostjager joostjager requested a review from halseth Sep 10, 2019
@joostjager joostjager force-pushed the report-accept-height branch from ec8602b to 3894c26 Sep 11, 2019
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

LGTM! great commit structure 👌

@cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented Sep 11, 2019

invoices/invoiceregistry_test.go:127:29: undefined: testInvoiceCltvDelta

@joostjager joostjager force-pushed the report-accept-height branch from 3894c26 to 91766ed Sep 11, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Sep 11, 2019

Test failure fixed

Copy link
Collaborator

@halseth halseth left a comment

LGTM 👡

Hash: payHash,
})

for key, htlc := range invoice.Htlcs {
Copy link
Collaborator

@halseth halseth Sep 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add similar comment as for the settle case

Copy link
Collaborator Author

@joostjager joostjager Sep 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added. While writing the comment, I also realized that the invoice update could be a bit stricter. See commit channeldb+invoices: make htlc cancelation stricter

invoices/invoiceregistry.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the report-accept-height branch 2 times, most recently from 343c376 to 81d6637 Sep 13, 2019
@joostjager joostjager requested a review from halseth Sep 13, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Sep 13, 2019

Requires #3503 to be merged so that the linter is tamed first.

@joostjager joostjager force-pushed the report-accept-height branch from 81d6637 to 783d9db Sep 13, 2019
@@ -640,7 +640,10 @@ func (i *InvoiceRegistry) CancelInvoice(payHash lntypes.Hash) error {
canceledHtlcs := make(
map[channeldb.CircuitKey]*channeldb.HtlcAcceptDesc,
)
for key := range invoice.Htlcs {
for key, htlc := range invoice.Htlcs {
if htlc.State == channeldb.HtlcStateCancelled {
Copy link
Collaborator

@halseth halseth Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we can cancel a settled one?

Copy link
Collaborator Author

@joostjager joostjager Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline. No htlc should be in the settled state here, because then the invoice state would have been settled. Added extra check to return an error if this happens.

joostjager added 5 commits Sep 16, 2019
Previously it was possible to cancel a canceled htlc. This would
subtract the htlc amount from the invoice amount again.
This commit modifies hodl htlc notification from invoice registry from a
single notification per hash to distinct notifications per htlc. This
prepares for htlc-specific information (accept height) to be added to the
notification.
This is a preparation for passing back the accept height in the
incorrect payment details failure message to the sender.
This test relied on specific internals of the failure encode function.
Changes to failure message pointer receiver everywhere subtly broke this
test.
Extends the invalid payment details failure with the new accept height
field. This allows sender to distinguish between a genuine invalid
details situation and a delay caused by intermediate nodes.
@joostjager joostjager force-pushed the report-accept-height branch from 783d9db to f60e4b1 Sep 16, 2019
@joostjager joostjager merged commit 95502da into lightningnetwork:master Sep 16, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants