Skip to content

htlcswitch+routing+routerrpc: error source as index and expose decrypt failure#3188

Merged
cfromknecht merged 7 commits into
lightningnetwork:masterfrom
joostjager:error-source-idx
Jul 11, 2019
Merged

htlcswitch+routing+routerrpc: error source as index and expose decrypt failure#3188
cfromknecht merged 7 commits into
lightningnetwork:masterfrom
joostjager:error-source-idx

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Jun 11, 2019

This PR modifies the switch behavior to return an integer indicating the error source instead of a pub key. This allows for a node occurring in the path multiple times. It also changes routerrpc.SendToRoute to return the failure source as an integer.

A second change introduces the ErrUnreadableFailure result for payments. This is a preparation for this case to be processed properly in router. Also an unknown failure (this is different from an unreadable failure) is now distinguishable.

Builds onto lightningnetwork/lightning-onion#37

This PR prepares for persistent mission control #3164

@joostjager joostjager requested a review from halseth June 11, 2019 08:38
@joostjager joostjager force-pushed the error-source-idx branch 2 times, most recently from 7a74e8b to a10b382 Compare June 11, 2019 15:11
@Roasbeef Roasbeef added error messages P3 might get fixed, nice to have routing rpc Related to the RPC interface labels Jun 18, 2019
@joostjager joostjager force-pushed the error-source-idx branch 3 times, most recently from 6a4854a to 3f007d5 Compare June 19, 2019 09:50
@joostjager joostjager changed the title htlcswitch+routing+routerrpc: return error source as index htlcswitch+routing+routerrpc: error source as index and expose decrypt failure Jun 19, 2019
@joostjager joostjager force-pushed the error-source-idx branch 6 times, most recently from 5ba8ff1 to 3408d85 Compare June 20, 2019 12:15
Comment thread htlcswitch/switch.go Outdated
Comment thread lnrpc/routerrpc/router.proto Outdated
Comment thread htlcswitch/failure.go Outdated
Comment thread htlcswitch/switch.go Outdated
Comment thread htlcswitch/failure.go Outdated
@joostjager joostjager force-pushed the error-source-idx branch 3 times, most recently from 7e719c5 to e24614e Compare June 20, 2019 13:53
@joostjager
Copy link
Copy Markdown
Contributor Author

@halseth ptal

@joostjager joostjager force-pushed the error-source-idx branch 2 times, most recently from ed0767f to 59e021c Compare June 28, 2019 07:48
Comment thread routing/router.go Outdated
Comment thread routing/router.go Outdated
Comment thread htlcswitch/failure.go Outdated
Comment thread htlcswitch/failure.go Outdated
Comment thread routing/payment_lifecycle.go Outdated
@joostjager joostjager force-pushed the error-source-idx branch 2 times, most recently from 3b3fc2b to cd36ba2 Compare July 1, 2019 07:42
@joostjager
Copy link
Copy Markdown
Contributor Author

The itest single_hop_invoice was failing because of a subtle bug caused by mixing values and pointers for failure messages. Added commit to fix that.

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.

Latest version LGTM ✅

Comment thread go.sum Outdated
Comment thread routing/router.go Outdated
@joostjager joostjager requested a review from Roasbeef July 4, 2019 06:43
Comment thread routing/router.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can be initialized with the other vars

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, but not sure if it is an improvement.

Comment thread routing/router.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

Comment thread routing/router.go Outdated
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.

LGTM 🐍

@joostjager joostjager requested a review from halseth July 10, 2019 21:12
@joostjager
Copy link
Copy Markdown
Contributor Author

PR rebased after failure reason changes were merged in #3156

@cfromknecht cfromknecht added this to the 0.7.1 milestone Jul 11, 2019
Comment thread lnwire/onion_error_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: take reference when var is declared instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Also in the reflect.DeepEqual below, the same objects were compared.

@joostjager joostjager force-pushed the error-source-idx branch 2 times, most recently from 24d88b9 to 41b1e7f Compare July 11, 2019 17:48
Methods on failure message types used to be defined on value receivers.
This allowed assignment of a failure message to ForwardingError both as
a value and as a pointer. This is error-prone, especially when using a
type switch.

In this commit the failure message methods are changed so that they
target pointer receivers.

Two instances where a value was assigned instead of a reference are
fixed.
@cfromknecht cfromknecht merged commit a4f4ff0 into lightningnetwork:master Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

error messages P3 might get fixed, nice to have routing rpc Related to the RPC interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants