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

Private Channels #2147

Merged
merged 3 commits into from Dec 8, 2018

Conversation

Projects
None yet
3 participants
@niftynei
Copy link
Collaborator

niftynei commented Dec 7, 2018

Allows users to create private channels, solves #2125

@niftynei niftynei requested review from cdecker and rustyrussell Dec 7, 2018

@rustyrussell
Copy link
Contributor

rustyrussell left a comment

Great work! I picked many nits, and colored multiple bikesheds :)

@@ -780,6 +781,7 @@ static void json_fund_channel(struct command *cmd,
p_req("id", json_tok_pubkey, &id),
p_req("satoshi", json_tok_tok, &sattok),
p_opt("feerate", json_tok_feerate, &feerate_per_kw),
p_opt_def("private", json_tok_bool, &private_channel, false),

This comment has been minimized.

@rustyrussell

rustyrussell Dec 7, 2018

Contributor

The spec calls this bit announce_channel; should we invert this and call it "announce", default=true?

This comment has been minimized.

@niftynei

niftynei Dec 7, 2018

Collaborator

works for me. 👍

*/
if (!(channel->channel_flags & CHANNEL_FLAGS_ANNOUNCE_CHANNEL))
return DELETE_WATCH;

This comment has been minimized.

@rustyrussell

rustyrussell Dec 7, 2018

Contributor

This change is probably OK, but I'd like something in the commit msg justifying its deletion; it seems orthogonal?

This comment has been minimized.

@niftynei

niftynei Dec 7, 2018

Collaborator

reverted. I was under the mistaken assumption that this would prevent private channels from being locked. Turns out that check is the line above...

@@ -791,6 +791,21 @@ def test_channel_persistence(node_factory, bitcoind, executor):
l1.daemon.wait_for_log(' to ONCHAIN')


def test_private_channel(node_factory):
l1, l2 = node_factory.line_graph(2, fundchannel=True, announce=True, privatechannels=True)
l3, l4 = node_factory.line_graph(2, fundchannel=True, announce=True, privatechannels=False)

This comment has been minimized.

@rustyrussell

rustyrussell Dec 7, 2018

Contributor

fundchannel defaults True already AFAICT? We should probably rename the 'announce' parameter to 'wait_for_announce'?

for n in nodes:
for end in (nodes[0], nodes[-1]):
wait_for(lambda: 'alias' in only_one(end.rpc.listnodes(n.info['id'])['nodes']))
if not privatechannels:

This comment has been minimized.

@rustyrussell

rustyrussell Dec 7, 2018

Contributor

Hmm announce=True and privatechannels=True is an invalid combination (which kinda reinforces my preference to call them 'wait_for_announce' and 'announce' BTW) Just assert() above that they're not asking for it?

This comment has been minimized.

@niftynei

niftynei Dec 7, 2018

Collaborator

Great point. Updated.

@@ -31,6 +31,9 @@ than 16777215 satoshi.
'urgent', 'normal' or 'slow' to use lightningd's internal estimates:
'normal' is the default.

'private' is an optional flag to keep this channel private, i.e. not
broadcast a `channel_announcement` message.

This comment has been minimized.

@rustyrussell

rustyrussell Dec 7, 2018

Contributor

s/don't offer to create a channel_announcement message. Note that we won't create a channel_announcement message unless the peer agrees to as well./?

This comment has been minimized.

@niftynei

niftynei Dec 7, 2018

Collaborator

channel_flags (and thus the announce_channel bit) is only sent in open_channel. afaict if the channel opener asks to keep the channel private, then the other side follows suit.

This comment has been minimized.

@rustyrussell

rustyrussell Dec 8, 2018

Contributor

Damn, you're right! Oops, I don't know how I got that idea then. Thanks!

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- JSON API: `getinfo` now returns `num_peers` `num_pending_channels`,
`num_active_channels` and `num_inactive_channels` fields.
- JSON API: use `\n\n` to terminate responses, for simplified parsing (pylightning now relies on this)
- JSON API: `fundchannel` now includes option to keep channel private

This comment has been minimized.

@rustyrussell

rustyrussell Dec 7, 2018

Contributor

Name of option, and note default is public?

This comment has been minimized.

@niftynei

niftynei Dec 7, 2018

Collaborator

Updated.

py-tests: rename 'announce' to 'wait_for_announce'
Better description of what the option actually does -- if true
waits for the announcement messages to be generated and exchanged.

@niftynei niftynei force-pushed the niftynei:nifty/private-chans branch 3 times, most recently from f00d4da to d66b381 Dec 7, 2018

@rustyrussell
Copy link
Contributor

rustyrussell left a comment

Ack d66b381

One hunk I'd like removed, but feel free to self-Ack and merge once you've done that...

if not announce_channels:
for n in nodes:
for end in (nodes[0], nodes[-1]):
wait_for(lambda: 'alias' in only_one(end.rpc.listnodes(n.info['id'])['nodes']))

This comment has been minimized.

@rustyrussell

rustyrussell Dec 8, 2018

Contributor

Can't happen? We have if not wait_for_announce: return above

This comment has been minimized.

@niftynei

niftynei Dec 8, 2018

Collaborator

ack.

@niftynei niftynei force-pushed the niftynei:nifty/private-chans branch from d66b381 to 8a1fc7b Dec 8, 2018

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Dec 8, 2018

self ACK 8a1fc7b

niftynei added some commits Dec 7, 2018

channeld: support private channel creation, fixes #2125
Adds a new 'announce' field for `fundchannel`, which if false
won't broadcast a `channel_announcement`.

@niftynei niftynei force-pushed the niftynei:nifty/private-chans branch from 8a1fc7b to 7129e99 Dec 8, 2018

ack'd above

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Dec 8, 2018

ACK 7129e99

@niftynei niftynei merged commit c497bad into ElementsProject:master Dec 8, 2018

2 checks passed

ackbot PR ack'd by niftynei
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shesek

This comment has been minimized.

Copy link
Contributor

shesek commented Jan 7, 2019

Is there a way to tell if an existing channel is private or not? I can't seem to find anything indicating this in listpeers.

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Jan 7, 2019

listchannels has a public boolean which should tell you if a channel is private or not. it'd make sense to add this to the channel obj in listpeers as well.

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