Skip to content

lnwire+htlcswitch+routing: report and interpret incorrect_or_unknown_payment_details height#3186

Closed
joostjager wants to merge 2 commits into
lightningnetwork:masterfrom
joostjager:expiry-info-leak
Closed

lnwire+htlcswitch+routing: report and interpret incorrect_or_unknown_payment_details height#3186
joostjager wants to merge 2 commits into
lightningnetwork:masterfrom
joostjager:expiry-info-leak

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Jun 11, 2019 •
edited
Loading

This pr extends failure result interpretation by inspecting the height at which the receiver accepted the htlc. This allows senders to distinguish between incorrect details and delays on the forward path.

@joostjager joostjager requested a review from cfromknecht June 11, 2019 06:58
@joostjager joostjager force-pushed the expiry-info-leak branch 2 times, most recently from 85c5b0f to d571917 Compare June 12, 2019 10:19
@joostjager
Copy link
Copy Markdown
Contributor Author

joostjager commented Jun 12, 2019

Added commit lnwire+htlcswitch+routing: report height for invalid payment details to show what we can do with lightning/bolts#608

@Roasbeef Roasbeef added P2 should be fixed if one has time privacy General label for issues/PRs related to the privacy implications of using the software routing labels Jun 18, 2019
@joostjager joostjager force-pushed the expiry-info-leak branch 3 times, most recently from bd91412 to df0b256 Compare June 25, 2019 14:14
@wpaulino wpaulino added this to the 0.7.1 milestone Jul 2, 2019
@wpaulino wpaulino added the v0.7.1 label Jul 2, 2019
cfromknecht
cfromknecht reviewed Jul 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

nice changes! nice to see some of the cancel reasons being removed and simplifications of the error responses

Comment thread lnwire/onion_error.go Outdated
Comment thread invoices/invoiceregistry.go Outdated
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht Jul 4, 2019

Choose a reason for hiding this comment

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

is primary reason for doing this that it doesn't require database changes? and defer storing accepted height to do in one refactor?

Copy link
Copy Markdown
Contributor Author

@joostjager joostjager Jul 4, 2019

Choose a reason for hiding this comment

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

AcceptedHeight is complicated because there can be multiple held htlcs with different accept heights. This is meant as a stop gap until we have an invoice registry that tracks each htlc paying to an invoice.

@joostjager
Copy link
Copy Markdown
Contributor Author

joostjager commented Jul 4, 2019

I already renamed the failure message type, but changed the failure code itself now too.

@joostjager joostjager requested a review from cfromknecht July 4, 2019 10:20
@Roasbeef Roasbeef modified the milestones: 0.7.1, 0.8 Jul 9, 2019
@wpaulino wpaulino removed the v0.7.1 label Jul 9, 2019
cfromknecht
cfromknecht reviewed Jul 10, 2019
View reviewed changes
Comment thread lnwire/onion_error.go Outdated
Comment thread lnwire/onion_error.go Outdated
@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Aug 5, 2019

Looks like we can move forward with this after the latest spec meeting.

@joostjager joostjager requested a review from Roasbeef as a code owner August 6, 2019 08:45
@joostjager joostjager force-pushed the expiry-info-leak branch 3 times, most recently from e314e53 to 3ee2264 Compare August 6, 2019 09:40
@Roasbeef Roasbeef removed their request for review August 15, 2019 23:53
@joostjager joostjager changed the title lnwire+htlcswitch+routing: report and interpret incorrect_or_unknown_payment_details height [no review] lnwire+htlcswitch+routing: report and interpret incorrect_or_unknown_payment_details height [don't review] Aug 23, 2019
@joostjager joostjager force-pushed the expiry-info-leak branch 3 times, most recently from 7f7cd72 to 576f3fe Compare September 9, 2019 11:03
@joostjager joostjager changed the title lnwire+htlcswitch+routing: report and interpret incorrect_or_unknown_payment_details height [don't review] lnwire+htlcswitch+routing: report and interpret incorrect_or_unknown_payment_details height Sep 9, 2019
@joostjager joostjager added blocked and removed blocked labels Sep 9, 2019
@joostjager joostjager requested a review from Roasbeef September 9, 2019 11:07
@joostjager
Copy link
Copy Markdown
Contributor Author

joostjager commented Sep 9, 2019

This is now rebased and can be re-reviewed

@cfromknecht
Copy link
Copy Markdown
Contributor

cfromknecht commented Sep 16, 2019

@joostjager needs rebase

@joostjager
routing: interpret incorrect payment details reported height
74fd3e9
@joostjager
lnrpc/routerrpc: update failure code rpc comment
e65a7be 
The original intention of this failure code enum was to match the
integer values as defined in the spec. Unfortunately this wasn't done
correctly.

Changing the assigned numbers would be a breaking change. For now we
just update the comment.
@wpaulino wpaulino removed the v0.8.0 label Sep 17, 2019
@wpaulino wpaulino modified the milestones: 0.8.0, 0.9 Sep 17, 2019
Roasbeef
Roasbeef requested changes Oct 23, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Change reads well, one final comment re migrations (new suggested policy, and possibly starting to migrate mission control after all).

Comment thread channeldb/migration_12_mc_final_cltv.go
// which final cltv delta values are stored too. Because mission control data is
// not critical we opt for simply clearing the existing data rather than a
// read/update/write migration.
func migrateMcFinalCltv(tx *bbolt.Tx) error {
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef Oct 23, 2019

Choose a reason for hiding this comment

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

Shall we commence the new migration package proposal with this PR? As it's the first target migration for 0.9, and the migration itself is pretty light.

More on topic for this PR: perhaps we should just start migrating this data? It's not critical, but certain services could see degradation in service quality after updating beyond this PR. We currently don't have a way of loading external data, so there's no way for them to preserve the mission control data if they consider it precious enough to do so.

@Roasbeef Roasbeef modified the milestones: 0.9.0, 0.10.0 Jan 7, 2020
@Roasbeef Roasbeef added limbo and removed v0.9.0 labels Jan 14, 2020
@joostjager
Copy link
Copy Markdown
Contributor Author

joostjager commented Jan 17, 2020

I need to figure out how to continue with this PR. Now that we have the payment secret, it may again be possible to return more failure details to the sender.

@joostjager
Copy link
Copy Markdown
Contributor Author

joostjager commented Jan 31, 2020

Closing this for now. The approach in this PR isn't the easiest. The mission control store needs to be updated and for SendToRoute calls we don't even know the intended final cltv delta, making it hard to interpret the failure result.

Now that we have the payment address to block probing, it seems like a better option to revive FinalExpiryTooSoon again.

@joostjager joostjager closed this Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Roasbeef Roasbeef Roasbeef requested changes

+1 more reviewer

@cfromknecht cfromknecht cfromknecht approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2 should be fixed if one has time privacy General label for issues/PRs related to the privacy implications of using the software routing

Projects

None yet

Milestone

0.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@joostjager @Roasbeef @cfromknecht @wpaulino