Make channels public first after 6 confirmations #411

halseth · 2017-11-14T02:00:06Z

This PR builds on top of #332 by @Crypt-iQ, making channels public to the greater network first after 6 confirmations of the funding transaction. This also allows channels to stay "private", by not announcing them at all if the announce flag is not set during initial channel opening.

For now, default behavior is kept the same, by making all channels public by default. Later this will probably be changed to private-by-default.

The separation of "local channel announcement" (ChannelAnnouncement and ChannelUpdate) from the "public announcement" (AnnounceSignatures) creates another state in the internal funding manager state machine. A followup PR will address the now quite fragmented logic this leads to.

halseth · 2017-11-15T19:41:53Z

I realized we don't actually send the ChannelAnnouncement and ChannelUpdate message to the peer before we have a valid AuthProof, meaning we will never do that if the channel is private. This must be sent for the two peers to know the ChannelEdgePolicies to use the private channel for routing.
Only ChannelUpdate needs to be sent directly, this is added.

Roasbeef · 2017-12-03T23:18:38Z

lnrpc/rpc.proto

Why the change? So with this modification, the default will be private? That's a break from the current default behavior, and many confuse several current users.

The default is set announce=true in lncli, but maybe it is still confusing for people utilizing grpc that their default will be false? Anyway, if we want it to default to private eventually, then we might as well start :P

Changed back to private

Roasbeef · 2017-12-03T23:19:15Z

cmd/lncli/commands.go

See my comment on the prior commit. If the default is to be set to true, why not just revert back to the private modifier?

Note that you can also set the default directly by using the Value: default attribute directly on the command's definition.

I made it Announce since we want it to default to false eventually. And I guess we should have the invoice routing hints supported before we do that?

And it doesn't look like BoolFlag has a Value attribute...

Well we'd set it to false only in the desktop app or mobile app.

Ok, I'll change the flag back to private, and make it false by default.

Roasbeef · 2017-12-03T23:27:03Z

discovery/gossiper.go

Roasbeef · 2017-12-03T23:31:34Z

discovery/gossiper.go

If this has just been added to the channel router (by the local party), then it will already have a nil AuthProof. The AuthProof is only set once we receive a AnnouncementSignatures message from the remote party.

This is only the case if the node that sent us the update is also one of the nodes in the OG ChannelAnnouncement.

So what you are saying is that the AuthProof will always be nil if it is coming from the local node? And this would be enough:

if !nMsg.isRemote { ...send... }

?

Yep! Although I guess the extra check can't hurt...may avoid a nil panic in a weird scenario.

Ok, will keep as-is then :)

Roasbeef · 2017-12-03T23:32:15Z

lnwallet/reservation.go

Ewww, a setter!

This is known ahead of time. As a result, it can simply be passed into the main constructor of the reservation.

See other comment/quesition.

Roasbeef · 2017-12-03T23:55:28Z

fundingmanager.go

From my reading, this'll directly announce the channel. Shouldn't it instead use an "un rolled" verion of handleFundingConfirmation as done when we're the initiator of the channel?

handleFundingConfirmation is altered to also announce first after 6 confs. I should really make the state machine to centralize this logic...

Yeah the state machine could really use a refactoring at this point. It sprawls far too much with this additional set of changes.

I think I'll push the state machine refactoring to a new PR once this one is merged. This should work (even though it's messy), and that PR should not change any functionality. Adding the state machine cleanup to this PR looks like it might become a bit messy.

Roasbeef · 2017-12-03T23:58:10Z

fundingmanager.go

Nit: extra new line.

Roasbeef · 2017-12-03T23:59:41Z

fundingmanager.go

Nit: extra space after be.

Roasbeef · 2017-12-04T00:21:11Z

discovery/gossiper.go

This'll need to be update to parse it as a lnwire.ChanUpateFlag.

Roasbeef · 2017-12-04T00:27:26Z

fundingmanager.go

This will need to check the returned error. As atm, it may be rejected if this function partially succeeded. It should be looking for routing.ErrOutdated or routing.ErrIgnored.

The other instance (where we actually try to announce), should also be modified to check the error. Otherwise, if we partially process the state, then we may never actually update the state in the database.

Roasbeef · 2017-12-08T23:51:54Z

Needs a rebase to master.

Roasbeef

Found one last little thing when testing locally. You'll find the comment inline. Aside, from that I've found this works as advertised. Yayy private channels!

Also left a comment about modifying the description on the command line. After these are addressed, we can finally get this thing merged!

You can also go ahead and squash down the set of commits. Thanks.

Roasbeef · 2017-12-14T00:10:36Z

fundingmanager.go

I think this logging statement should be moved into the method above. As otherwise, we get conflicting logs like this (the channel was set to private):

2017-12-13 16:08:45.514 [DBG] FNDG: Channel with ShortChanID 1830686860312576 added to router graph 2017-12-13 16:08:45.514 [DBG] PEER: Received FundingLocked(chan_id=32d05b577966bb3280a58c8f2d7cbd2e0a916c4bdce67d8c024ffb693ccf2d88, next_point=033201ae22cbf6fa8cb07d0f2187ab764f33d8378bf830c1bc9b6e21063f5b7be6) from 127.0.0.1:10019 2017-12-13 16:08:45.516 [TRC] PEER: readMessage from 127.0.0.1:10019: (*lnwire.FundingLocked)(0xc4205981e0)({ ChanID: (lnwire.ChannelID) (len=32 cap=32) 32d05b577966bb3280a58c8f2d7cbd2e0a916c4bdce67d8c024ffb693ccf2d88, NextPerCommitmentPoint: (*btcec.PublicKey)(0xc420174840)({ Curve: (elliptic.Curve) <nil>, X: (*big.Int)(0xc420174860)(22618611118210622400841714471844468386370929431841625480347924787655014579174), Y: (*big.Int)(0xc420174880)(5784334520023794081739208847968927079355214281329805474126131064251764117719) }) }) 2017-12-13 16:08:45.516 [DBG] FNDG: Received FundingLocked for ChannelID(32d05b577966bb3280a58c8f2d7cbd2e0a916c4bdce67d8c024ffb693ccf2d88) from peer 02cc27b8ef708b13a6832f1c4f3d8d1207851cda53f5be041f5b1e2ec1afeb755c 2017-12-13 16:08:45.515 [TRC] PEER: writeMessage to 127.0.0.1:10019: (*lnwire.ChannelUpdate)(0xc420082cd0)({ Signature: (*btcec.Signature)(0xc42026a0e0)({ R: (*big.Int)(0xc42023c1c0)(42098235645066207283475299495848826319528238987901551247437816008865568379280), S: (*big.Int)(0xc42023c220)(36309963118044616166236141227015067109229028731851401167576737900038110028029) }), ChainHash: (chainhash.Hash) (len=32 cap=32) 683e86bd5c6d110d91b94b97137ba6bfe02dbbdb8e3dff722a669b5d69d77af6, ShortChannelID: (lnwire.ShortChannelID) { BlockHeight: (uint32) 1665, TxIndex: (uint32) 1, TxPosition: (uint16) 0 }, Timestamp: (uint32) 1513210125, Flags: (lnwire.ChanUpdateFlag) 1, TimeLockDelta: (uint16) 144, HtlcMinimumMsat: (lnwire.MilliSatoshi) 0 mSAT, BaseFee: (uint32) 1000, FeeRate: (uint32) 1 }) 2017-12-13 16:08:45.516 [DBG] FNDG: Will not announce private channel 1830686860312576.

Roasbeef · 2017-12-14T00:17:53Z

discovery/gossiper.go

Atm with this, if we send just the ChannelUpdate message, then the other node will reject it as there's a race condition where we might receive the FundingLocked then this message before we ever send our own chann ann to our internal gossiper:

2017-12-13 16:16:18.052 [DBG] PEER: Received ChannelUpdate(chain_hash=683e86bd5c6d110d91b94b97137ba6bfe02dbbdb8e3dff722a669b5d69d77af6, short_chan_id=1836184418451456, flag=0, update_time=2017-12-13 16:16:18 -0800 PST) from 127.0.0.1:10019 2017-12-13 16:16:18.052 [TRC] PEER: readMessage from 127.0.0.1:10019: (*lnwire.ChannelUpdate)(0xc42048e9b0)({ Signature: (*btcec.Signature)(0xc420011140)({ R: (*big.Int)(0xc420118ea0)(24495463756957618076099430289234328219521735099568467680541926667560037384405), S: (*big.Int)(0xc420118ec0)(41256671207089118935243535172289480169661446552925936953485820156560683126184) }), ChainHash: (chainhash.Hash) (len=32 cap=32) 683e86bd5c6d110d91b94b97137ba6bfe02dbbdb8e3dff722a669b5d69d77af6, ShortChannelID: (lnwire.ShortChannelID) { BlockHeight: (uint32) 1670, TxIndex: (uint32) 1, TxPosition: (uint16) 0 }, Timestamp: (uint32) 1513210578, Flags: (lnwire.ChanUpdateFlag) 0, TimeLockDelta: (uint16) 144, HtlcMinimumMsat: (lnwire.MilliSatoshi) 0 mSAT, BaseFee: (uint32) 1000, FeeRate: (uint32) 1 }) 2017-12-13 16:16:18.053 [ERR] DISC: unable to validate channel update short_chan_id=1836184418451456: edge not found

So one side doesn't see the other's edge:

⛰i lncli describegraph { "nodes": [ { "last_update": 0, "pub_key": "02cc27b8ef708b13a6832f1c4f3d8d1207851cda53f5be041f5b1e2ec1afeb755c", "alias": "", "addresses": [ ], "color": "#000000" }, { "last_update": 1513209865, "pub_key": "03d381f8cd62e79e40f2a0f96caa0f2e9ee1f6526e80a05c4907dcdf8627f0974e", "alias": "03d381f8cd62e79e40f2\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000", "addresses": [ ], "color": "#3399ff" } ], "edges": [ { "channel_id": "1836184418451456", "chan_point": "1592c661827f01270f3e309e82770150eb40fc1df4d4357c1d89c7bc5cf71cef:0", "last_update": 1513210579, "node1_pub": "02cc27b8ef708b13a6832f1c4f3d8d1207851cda53f5be041f5b1e2ec1afeb755c", "node2_pub": "03d381f8cd62e79e40f2a0f96caa0f2e9ee1f6526e80a05c4907dcdf8627f0974e", "capacity": "1500000", "node1_policy": null, "node2_policy": { "time_lock_delta": 144, "min_htlc": "0", "fee_base_msat": "1000", "fee_rate_milli_msat": "1" } } ] }

This means that they wouldn't be able to craft a routing hint to route to the private channel.

woops, this was a big oversight on my part!

I pushed a commit + tests now, that takes care of this case. It stores the ChannelUpdate for the unknown channel in a map, and will reprocess it later, after the local ChannelAnnouncement is processed. At the moment this is just a map in memory, not sure if it is needed/worth it at this point to make it persistent. @aakselrod

If this looks okay, I will squash and rebase this branch, getting it ready for merge 💪

Roasbeef · 2017-12-14T00:58:10Z

cmd/lncli/commands.go

This isn't quite accurate, as we'll be using the payreqs to give routing hints in the future. Instead, we can say something like: "other nodes won't be able to route to this channel unassisted".

This commit introduces some new interdependent functionality. As soon as the fundingLocked message is sent, the channel is immediately added to the ChannelRouter's internal topology. Finally, channels are now only broadcasted to the greater network after six confirmations on the funding transaction has been reached.

Tests in fundingmanager_test.go that assert the privacy guarantees of non-broadcasting.

This make lncli openchannel take a --private parameter, set to false by default.

This commit adds the ChannelFlags field, of type lnwire.FundingFlags, to the OpenChannel struct, including serialization for database storage. This is done to preserve the flags that were sent during channel opening, currently used to determine whether a channel should be made public or not after opening.

This commit adds some comments and does some cleanup of the logic that makes sure non-public channels (channels with no AuthProof) are not broadcasted to the network.

This commits slightly rewrites the newly introduced logic for private channels. Instead of keeping the channel announce preference in a database within fundingManager, it is stored as part of the OpenChannel struct. In addition, the ChanOpenStatus_Open update is now sent after the channel is added to the router, instead of waiting until the 6 blocks confirmation has passed.

This commit makes the gossiper store received ChannelUpdates that is not for any known channel in a map, such that they can be reprocessed when the ChannelAnnouncement arrives. This is done to handle the case where we receive a ChannelUpdate from our channel counterparty before we have been able to process our own local ChannelAnnouncement.

This commit adds a test that ensures that if we receive a ChannelUpdate for a channel we don't know about, it will be reprocessed after we receive a ChannelAnnouncement for that channel.

Roasbeef

LGTM 🚨

halseth mentioned this pull request Nov 14, 2017

funding: adding to topology after fundinglocked, private channels, chanAnn after 6 confs #332

Closed

halseth force-pushed the channelgraph-try-split-commits branch from baaa9cd to c369b6d Compare November 23, 2017 11:41

Roasbeef requested changes Dec 4, 2017

View reviewed changes

halseth force-pushed the channelgraph-try-split-commits branch from c369b6d to dd6cd6c Compare December 5, 2017 20:04

halseth force-pushed the channelgraph-try-split-commits branch 3 times, most recently from 83b3789 to 59f854f Compare December 12, 2017 20:07

Roasbeef previously approved these changes Dec 14, 2017

View reviewed changes

halseth dismissed Roasbeef’s stale review via e54d8f1 December 14, 2017 16:59

Crypt-iQ and others added 15 commits December 17, 2017 23:32

lnrpc: add --private arg to OpenChanRequest

2a14958

server: use private parameter when opening channel

fa8a0b0

pilot: pass private=false to OpenChannel

1c4a13b

rpcserver: forward private parameter to OpenChannel

7ad57f7

gossiper: Only send channel updates for edges with AuthProof.

056c637

funding test: test private channels.

6cbb12d

Tests in fundingmanager_test.go that assert the privacy guarantees of non-broadcasting.

lnrpc: update godoc for 'private' flag

3a319a8

lncli: add 'private' flag to openchannel command

657f9ba

This make lncli openchannel take a --private parameter, set to false by default.

gossiper: private channel cleanup + comments

9a89d48

This commit adds some comments and does some cleanup of the logic that makes sure non-public channels (channels with no AuthProof) are not broadcasted to the network.

lnwallet: add FundingFlag parameter to InitChannelReservation

0d72153

funding tests: update tests for the 6 block and private channel changes

c06a246

lntest/harness: set Private=false in OpenChannel

0523959

halseth force-pushed the channelgraph-try-split-commits branch from e54d8f1 to 2b77756 Compare December 17, 2017 22:38

halseth added 2 commits December 17, 2017 23:41

lnd_test: update integration tests for 6 block channel announcements

0d26e6d

lnwallet test: add FFAnnounceChan flag to reservation init

7264224

halseth force-pushed the channelgraph-try-split-commits branch from 2b77756 to 7af8b9c Compare December 17, 2017 22:41

halseth force-pushed the channelgraph-try-split-commits branch from 7af8b9c to a9ae8cc Compare December 17, 2017 23:13

halseth added 2 commits December 18, 2017 00:24

discovery test: test for remote ChannelUpdate for unknown channel

702f2a6

This commit adds a test that ensures that if we receive a ChannelUpdate for a channel we don't know about, it will be reprocessed after we receive a ChannelAnnouncement for that channel.

halseth force-pushed the channelgraph-try-split-commits branch from a9ae8cc to 702f2a6 Compare December 17, 2017 23:24

Roasbeef approved these changes Dec 18, 2017

View reviewed changes

Roasbeef merged commit b74a281 into lightningnetwork:master Dec 18, 2017

This was referenced Dec 18, 2017

Observe channel announcement preference during channel funding workflow #306

Closed

Implement block-based grace period for channel announcements #305

Closed

halseth deleted the channelgraph-try-split-commits branch July 12, 2018 13:39

Make channels public first after 6 confirmations #411

Make channels public first after 6 confirmations #411

Uh oh!

Conversation

halseth commented Nov 14, 2017

Uh oh!

halseth commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halseth Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Dec 8, 2017

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halseth commented Nov 15, 2017 •

edited

Loading

halseth Dec 4, 2017 •

edited

Loading