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

payment failure message with attached channel_update not validated #1738

Merged
merged 7 commits into from Sep 18, 2018

Conversation

Projects
None yet
3 participants
@joostjager
Collaborator

joostjager commented Aug 16, 2018

The intention of this PR is to fix #1707.

@Roasbeef Roasbeef added this to the 0.5.1 milestone Aug 17, 2018

@joostjager joostjager force-pushed the joostjager:validation branch 7 times, most recently from 7f0f50b to 8877d51 Aug 18, 2018

Show resolved Hide resolved routing/router_test.go Outdated
Show resolved Hide resolved routing/router_test.go Outdated
Show resolved Hide resolved routing/router_test.go Outdated
Show resolved Hide resolved routing/router_test.go
Show resolved Hide resolved discovery/gossiper.go
@@ -2003,13 +2009,18 @@ func pruneEdgeFailure(paySession *paymentSession, route *Route,
// applyChannelUpdate applies a channel update directly to the database,
// skipping preliminary validation.
func (r *ChannelRouter) applyChannelUpdate(msg *lnwire.ChannelUpdate) error {
func (r *ChannelRouter) applyChannelUpdate(msg *lnwire.ChannelUpdate,

This comment has been minimized.

@halseth

halseth Sep 6, 2018

Collaborator

godoc mus be updated.

@joostjager joostjager force-pushed the joostjager:validation branch from 8877d51 to 481bfbc Sep 6, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Sep 6, 2018

Review comments processed.

@halseth

Awesome job on the commit structure in this PR! Each commit changes as little as possible, individually making changes that are decoupled from the others. The commit messages explain the changes sufficiently. This makes it easy to review, as each commit can be reviewed independently almost as a mini-PR 😁

I don't have much to pick on with the current changes, only a few nits and I think this PR should be ready for merge.

@@ -329,26 +328,36 @@ type testChannel struct {
Capacity btcutil.Amount
}
// createTestGraph returns a fully populated ChannelGraph based on a set of
type testGraphInstance struct {

This comment has been minimized.

@halseth

halseth Sep 11, 2018

Collaborator

👍

return &testGraphInstance{
graph: graph,
cleanUp: cleanUp,
aliasMap: aliasMap}, nil

This comment has been minimized.

@halseth

halseth Sep 11, 2018

Collaborator

style nit: newline before closing bracket

// should be attempted and the channel update should be received by
// router and ignored because it is missing a valid signature.
_, _, err = ctx.router.SendToRoute([]*Route{route}, payment)
if err == nil {

This comment has been minimized.

@halseth

halseth Sep 11, 2018

Collaborator

can you check that the expected error is returned?

This comment has been minimized.

@joostjager

joostjager Sep 11, 2018

Collaborator

do you want me to check on strings?

This comment has been minimized.

@halseth

halseth Sep 17, 2018

Collaborator

Yes!

// Retry the payment using the same route as before.
_, _, err = ctx.router.SendToRoute([]*Route{route}, payment)
if err == nil {

This comment has been minimized.

@halseth

halseth Sep 11, 2018

Collaborator

same

@halseth

This comment has been minimized.

Collaborator

halseth commented Sep 11, 2018

Also needs a rebase.

joostjager added some commits Aug 16, 2018

discovery+routing: move validation logic to routing package
Previously, gossiper was the only object that validated channel
updates. Because updates can also be received as part of a
failed payment session in the routing package, validation logic
needs to be available there too. Gossiper already depends on
routing and having routing call the validation logic inside
gossiper would be a circular dependency. Therefore the validation
was moved to routing.

@joostjager joostjager force-pushed the joostjager:validation branch from 481bfbc to a7fec82 Sep 11, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Sep 11, 2018

Rebased

@Roasbeef

LGTM 🐄

@@ -463,7 +458,7 @@ func createTestGraph(testChannels []*testChannel) (*channeldb.ChannelGraph, func
SigBytes: testSig.Serialize(),
Flags: lnwire.ChanUpdateFlag(lnwire.ChanUpdateDirection),
ChannelID: channelID,
LastUpdate: time.Now(),
LastUpdate: testTime,

This comment has been minimized.

@Roasbeef

Roasbeef Sep 18, 2018

Member

👍 on making the tests deterministic

@Roasbeef Roasbeef merged commit 25145ac into lightningnetwork:master Sep 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0003%) to 53.802%
Details

@joostjager joostjager deleted the joostjager:validation branch Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment