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

Add --noprivate flag to describegraph rpc to filter out private channels #1389

Merged
merged 4 commits into from Oct 17, 2018

Conversation

Projects
None yet
5 participants
@Bluetegu
Contributor

Bluetegu commented Jun 14, 2018

Add --noprivate flag to describegraph rpc to filter out private channels per #1037

Initially I named the flag private as suggested in the issue, but setting private to filter private seems wrong to me, and I didn't want to change the behaviour of describegraph that returns private channels by default, so I changed the flag name to noprivate

Note: this additional parameter needs to be added to the describegraph cache issue #1232

Testing done on simnet:
Public channel between Alice and Bob
Private channel between Bob and Charlie

On Charlie's side, run describegraph and describegraph --noprivate and compare outputs. Only difference is that when --noprivate flag is on, the private channel was not included in the output.

Fixes #1037.

@halseth

This comment has been minimized.

Collaborator

halseth commented Jun 18, 2018

I would rather make private channels not appear by default, as that might make users posting their private channels by mistake. Instead --private should be provided to include private channels.

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Jun 18, 2018

@halseth, I agree this makes more sense. Switched flag from 'noprivate' to 'private'.

@wpaulino

Thanks for the PR! These changes read well to me, I just have some minor nits to point out. Other than that, could you restructure your commits according to the contribution guidelines?

}
req := &lnrpc.ChannelGraphRequest{
Private: bool(private),

This comment has been minimized.

@wpaulino

wpaulino Jun 25, 2018

Collaborator

As it's a boolean value we're attempting to retrieve, we can simply do Private: ctx.Bool("private"), and remove the code above.

@@ -2709,8 +2710,13 @@ func (r *rpcServer) DescribeGraph(ctx context.Context,
err = graph.ForEachChannel(func(edgeInfo *channeldb.ChannelEdgeInfo,
c1, c2 *channeldb.ChannelEdgePolicy) error {
edge := marshalDbEdge(edgeInfo, c1, c2)
resp.Edges = append(resp.Edges, edge)
// A private channel does not need an authentication proof, while an

This comment has been minimized.

@wpaulino

wpaulino Jun 25, 2018

Collaborator

nit: comment greater than 80 char limit.

Show resolved Hide resolved rpcserver.go
@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Jun 26, 2018

@wpaulino , I made the changes per your review comments. Let me know if further changes need to be made. Thanks!

@halseth

This comment has been minimized.

Collaborator

halseth commented Jul 10, 2018

Commits that revert/changes a previous commit should be squashed together :)

Also needs a rebase and a proto-regen!

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from a200362 to 66dc347 Jul 10, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Jul 10, 2018

@halseth
I squashed the commits and rebased; my first squash ever, its a learning experience :-). Hope its ok now.

I've redone the test above to verify correct operation of describegraph with and without the private flag.

I try to make sure to do a proto gen upon each commit. I'm using the versions for the generator specified for lnd. It does though introduce a small number of changes in other places in code in rpc.pb.go and rpc.pb.gw.go. I patch those back, using the diff of running the proto gen on the master branch. Most of the changes seem trivial, i.e. one liner function instead of 3:

-type isOpenStatusUpdate_Update interface{ isOpenStatusUpdate_Update() }
+type isOpenStatusUpdate_Update interface {
+       isOpenStatusUpdate_Update()
+}

But also:

 func RegisterLightningHandler(ctx context.Context, mux *runtime.ServeMux, conn *grpc.ClientConn) error {
-       client := NewLightningClient(conn)
+       return RegisterLightningHandlerClient(ctx, mux, NewLightningClient(conn))
+}

I double checked the versions on golang/protobuf and grpc-ecosystem/grpc-gateway.
And my protobuf version seems fine too:
ubuntu@btc:~$ protoc --version
libprotoc 3.4.0

I'm not sure what is causing this. I don't think this has consequences on this PR, but it would be nice to know how to fix it such that when I regen proto on master it will not generate any changes at all, as it should.

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Jul 11, 2018

@halseth , sorry, just realized that I'm pulling from my master, so indeed I need to rebase...
I'll submit an updated pull request asap.

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from 66dc347 to b6ad2b8 Jul 11, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Jul 11, 2018

@halseth, I rebased, re-gen proto, didn't do any weird patches this time, did the test mentioned above again to verify that it's working. Hope its now ready. Sorry for the noob-ness.

@halseth

It looks all good now, nice work! 😀

@@ -2306,6 +2306,10 @@ var describeGraphCommand = cli.Command{
Name: "render",
Usage: "If set, then an image of graph will be generated and displayed. The generated image is stored within the current directory with a file name of 'graph.svg'",
},
cli.BoolFlag{
Name: "private",

This comment has been minimized.

@halseth

halseth Jul 12, 2018

Collaborator

Awesome, look like you were able to squash 👍

Ideally, you would make one commit for all the changes in lnrpc/, one for changes in cmd/ and one commit for the changes to rpcserver.go. Not a blocker though!

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from b6ad2b8 to b6cbf9d Jul 24, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Jul 24, 2018

@halseth, I rebased again, as master moved forward and a conflict was detected in rpc.bp.go. I rerun gen_proto, rebuild, go test, and run the specific tests above.
But I see that this update dismissed you ok review.
Was I wrong to rebase? What is the process here, should you re-ok?
Thanks!

@halseth

This comment has been minimized.

Collaborator

halseth commented Jul 24, 2018

No, that was correct! 👍 Will re-approve when we have been able to get Travis to run.

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Jul 24, 2018

Thanks! Travis support un-flagged my account, but pull requests that were submitted before the unflagging will need to be re-triggered manually or by an updated pull request :-(

@halseth

This comment has been minimized.

Collaborator

halseth commented Jul 24, 2018

@Bluetegu I think you should be able to trigger a build by either rebasing, or altering your commits, then force pushing :)

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch 3 times, most recently from e61026c to 73646c8 Jul 26, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Aug 12, 2018

@halseth I need some help here :-)

The code I added assumes that non-private channels, or more accurately their edgeInfo, always have a non-nill authProof. This works when I tested with lncli. However this check/assumption fails for non-private channels in the integration tests. I am not sure why though.

Looking at other code in rpcserver.go that checks for private channels, e.g. like ListChannels, the code checks whether the channel is private by comparing the channel flags against lnwire.FFAnnounceChannel

What I'm thinking of doing, is instead of checking if authProof is empty or not, add a function that gets all private channels, and somehow check whether the edgeInfo being iterated upon is private or not by checking whether it is/reflects one of those channels.

Is it the right direction to go?

Thanks!

@halseth

This comment has been minimized.

Collaborator

halseth commented Aug 12, 2018

Non-private channel should have an authproof after the funding transaction has received 6 confirmations. However there can be cases where this doesn't get created, if one of the channel parties goes offline for instance before those 6 confirmations. In such cases the channel won't be active though, so it shouldn't be very interesting.

Using FFAnnounceChannel to filter out private channels is a possibility, but note that we only have these flags for our own channels. So in that case we would need to determine which channels we own, and query the database for the flags.

What integration test is failing because of this?

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Aug 13, 2018

@halseth The following tests fail:

'open_channel_reorg_test' fails at:
*errors.errorString expected to find one edge in the graph, found 0
/home/ubuntu/go/src/github.com/lightningnetwork/lnd/lnd_test.go:1166 (0xb6b4f9)

'channel_force_closure' fails at:
*errors.errorString expected force closed channel to have 6 pending htlcs, found 0 instead
/home/ubuntu/go/src/github.com/lightningnetwork/lnd/lnd_test.go:1615 (0xb69807)

'private_channels' fails at:
lnd_test.go:74: Failed: (private channels): exited with error:
*errors.errorString expected Alice to know 4 edges, had 3
/home/ubuntu/go/src/github.com/lightningnetwork/lnd/lnd_test.go:3876 (0xb7216a)

'test_multi-hop_htlc_local_chain_claim' fails too. I stopped there...

Regarding 'using FFAnnounceChannel to filter out private channels is a possibility, but note that we only have these flags for our own channels.', yes, but per my understanding we are not aware of private channels (edgeInfo) that are not ours, right? Otherwise, if we got those through gossip they are public.

Thanks!

@halseth

This comment has been minimized.

Collaborator

halseth commented Aug 14, 2018

@Bluetegu I don't think your changes should affect the first two tests you mentioned, so that is probably a different bug... I think I have PRs up that fixes both these two flakes.

For the test private_channels, try to determine if it is a flake (fails just sometimes) or if it can be reproed constantly.

Regarding 'using FFAnnounceChannel to filter out private channels is a possibility, but note that we only have these flags for our own channels.', yes, but per my understanding we are not aware of private channels (edgeInfo) that are not ours, right? Otherwise, if we got those through gossip they are public.

You are absolutely right, edgeInfos for private channels won't propagate in the network :) What I'm saying is that ForEachChannel will return all channels, whether they are ours or not, and to check the ChannelFlags we must query the channeldb (instead of just the graph). Using the authProof directly lets us avoid that.

@halseth halseth added this to the 0.5.1 milestone Sep 18, 2018

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from 654a0a3 to 6c5c655 Sep 20, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Sep 26, 2018

Changes made. Ready for inspection :-)

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch 2 times, most recently from 576cd3c to 3a89f51 Sep 27, 2018

@halseth

Awesome work on this one! Thanks for sticking it out with my pickiness, now it has gotten to a state where I am happy with the overall changes 😄

The comments are mostly style suggestions and nits, otherwise this looks good. Also thank you for splitting up into separate commits, makes reviewing much easier!

@@ -1498,6 +1498,13 @@ message ChannelEdge {
}
message ChannelGraphRequest {
/**
Whether unannounced channels should be returned or not. If set,

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

suggestions for text: returned included in the response

/**
Whether unannounced channels should be returned or not. If set,
unannounced channels are returned.
Unannoucned channels are both private channels and public

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

typo: Unannoucned

Whether unannounced channels should be returned or not. If set,
unannounced channels are returned.
Unannoucned channels are both private channels and public
channels whose authentication proof has not been confirmed yet.

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

suggestion: whose authentication proof has not been confirmed yet that are not yet announced to the network

Show resolved Hide resolved channeldb/db_test.go
if err := cdb.Wipe(); err != nil {
t.Fatalf("unable to wipe channeldb: %v", err)
}
// check errors are returned

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

Comments should be full sentences.

lnd_test.go Outdated
mineBlocks(t, net, 4)
// Give the network a chance to learn that auth proof is confirmed
time.Sleep(time.Millisecond * 50)

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

Instead of sleeping here, you should be using a WaitPredicate. Check other tests for how this is done :)

Show resolved Hide resolved lnd_test.go
edge := marshalDbEdge(edgeInfo, c1, c2)
resp.Edges = append(resp.Edges, edge)
// Do not include unannounced channels unless specifically
// requested.

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

nit: no need for new line.

// Do not include unannounced channels unless specifically
// requested.
// Unannounced channels include both private channels as well as
// public channels whose authentication proof were not confirmed

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

Good comment!

// Unannounced channels include both private channels as well as
// public channels whose authentication proof were not confirmed
// yet, hence were not advertized.
if includeUnannounced || edgeInfo.AuthProof != nil {

This comment has been minimized.

@halseth

halseth Sep 27, 2018

Collaborator

style nit: To avoid the indentation you can do:

if !includeUnannounced && edgeInfo.AuthProof == nil {
      return nil
}
....append edge...
return nil

This comment has been minimized.

@Bluetegu

Bluetegu Sep 27, 2018

Contributor

I missed this one, sorry. Why do you think its better to avoid the indentation?

This comment has been minimized.

@halseth

halseth Sep 28, 2018

Collaborator

I generally like to use "exit early" in case of non-happy path. It's just a style preference though, so if you prefer doing it like this, I don't mind.

In this case it would also have made the diff smaller, which is also a plus ;)

This comment has been minimized.

@halseth

halseth Sep 28, 2018

Collaborator

On indentation: here it is not that bad, but imagine we had nested conditions we wanted to be satisfied before adding the edge:

if condition1 {
      calculation....
      if condtiondion2 {
            ....more caluclation....
            if condition3 {
                   edges = append(edges, edge)
            }
      }
}
return nil

while we could be doing

if !condition1 {
     return nil
}

calculation....
if !condtiondion2 {
     return nil
}

....more caluclation....
if !condition3 {
    return nil
}
edges = append(edges, edge)
return nil

This comment has been minimized.

@Bluetegu

Bluetegu Sep 28, 2018

Contributor

Sounds reasonable. The condition is also a bit more clear this way.

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch 3 times, most recently from 4b8100d to 519a410 Sep 27, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Sep 27, 2018

Thanks for the thorough review. I believe I fixed all of the issues, spare the last. I actually somehow missed it while doing the mods. Let me know if you want it done too, and if you find any other issue. Thanks!

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from 519a410 to 6c86315 Sep 28, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Sep 28, 2018

Rebased and took the opportunity to fix the last nit.

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from 6c86315 to ffbe013 Sep 29, 2018

@halseth

Almost there!

@@ -1488,6 +1488,13 @@ message ChannelEdge {
}
message ChannelGraphRequest {
/**
Whether unannounced channels are included in the response or
not. If set, unannounced channels are included.

This comment has been minimized.

@halseth

halseth Oct 2, 2018

Collaborator

nit: unnecessary line break.

lnd_test.go Outdated
// Give the network a chance to learn that auth proof is confirmed.
var predErr error
err = lntest.WaitPredicate(func() bool {
// The channel should now be announced. Check that Alice has 1 announced edge.

This comment has been minimized.

@halseth

halseth Oct 2, 2018

Collaborator

nit: line looks a bit long

Show resolved Hide resolved lnd_test.go
lnd_test.go Outdated
return false
}
return true
}, time.Millisecond*50)

This comment has been minimized.

@halseth

halseth Oct 2, 2018

Collaborator

timeouts for WaitPredicates should be of the order of ~10s

lnd_test.go Outdated
t.Fatalf("expected to find 1 announced edge in the graph, found %d", numEdges)
}
// Close channel

This comment has been minimized.

@halseth

halseth Oct 2, 2018

Collaborator

☝️

cli.BoolFlag{
Name: "include_unannounced",
Usage: "If set, unannounced channels will be included in the graph. " +
"Unannounced channels include both private channels, and public " +

This comment has been minimized.

@halseth

halseth Oct 2, 2018

Collaborator

Update comment to reflect the change you did to the proto comment

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from ffbe013 to 98292ef Oct 2, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Oct 2, 2018

I think I got them all :-)

@halseth

This comment has been minimized.

Collaborator

halseth commented Oct 4, 2018

There's still this one unresolved comment about making comment full sentence.

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Oct 4, 2018

I'm not a native English speaker so please bear with me on this.

The current comment is "Close channel"
When you say a full sentence you mean something like:
"Close the channel."
This is similar to the "Close all channels." comment on another test.
Or are you referring to something more than that?

BTW, do you want me to switch to closeChannelAndAssert?

@halseth

This comment has been minimized.

Collaborator

halseth commented Oct 4, 2018

No worries, sorry I didn't explain. "Close the channel." is a full sentence, even better would be "Close the channel used during the test.".

And yes, closeChannelAndAssert is preferred, good catch!

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from 98292ef to 0fd3da0 Oct 4, 2018

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch 2 times, most recently from 2690bd9 to 0f1cf2c Oct 4, 2018

Bluetegu added some commits Sep 26, 2018

Add --include_unannounced flag to describegraph lncli command.
Private and public channels which weren't announced yet are returned only
if the flag is set.
Note: this changes the default output of describegraph which used to
return all channels known to node.

@Bluetegu Bluetegu force-pushed the Bluetegu:noprivate-describegraph-1037 branch from 0f1cf2c to 2f3e2f9 Oct 4, 2018

@Bluetegu

This comment has been minimized.

Contributor

Bluetegu commented Oct 4, 2018

Changes made.

BTW, I'm not sure how the 'resolve conversation' here in the discussion of the pull request is supposed to work: do I need to press the 'resolve conversation' once I fix the issue, or is the reviewer (you) supposed to do that once the issue is indeed fixed, or we should simply ignore this github feature altogether.

@halseth

halseth approved these changes Oct 5, 2018

Changes made.

Awesome, LGTM! 😀

I'm not sure how the 'resolve conversation' here in the discussion of the pull request is supposed to work.

It is a new feature, so I'm not totally sure either 😛 I think it makes sense for the author of the conversation to resolve it when he's satisfied with the fix.

@Roasbeef

LGTM 🐉

@Roasbeef Roasbeef merged commit 4c0ca37 into lightningnetwork:master Oct 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 55.237%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment