Send channel_update messages to direct peers on private channels #949

TheBlueMatt · 2021-06-12T22:02:21Z

If we are a public node and have a private channel, our
counterparty needs to know the fees which we will charge to forward
payments to them. Without sending them a channel_update, they have
no way to learn that information, resulting in the channel being
effectively useless for outbound-from-us payments.

This commit fixes our lack of channel_update messages to private
channel counterparties, ensuring we always send them a
channel_update after the channel funding is confirmed.

Still need to fix existing tests/add new tests for the new behavior.

lightning/src/ln/channelmanager.rs

devrandom · 2021-06-13T16:59:42Z

Tested - correctly sends a channel_update right after funding_locked

TheBlueMatt · 2021-06-17T18:30:50Z

Fixed tests after rebasing on #954.

codecov · 2021-06-17T18:50:53Z

Codecov Report

Merging #949 (ba600db) into main (0c57018) will increase coverage by 0.96%.
The diff coverage is 87.59%.

@@            Coverage Diff             @@
##             main     #949      +/-   ##
==========================================
+ Coverage   90.72%   91.68%   +0.96%     
==========================================
  Files          60       60              
  Lines       30640    35745    +5105     
==========================================
+ Hits        27798    32774    +4976     
- Misses       2842     2971     +129

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`94.83% <ø> (ø)`
lightning/src/ln/peer_handler.rs	`46.44% <0.00%> (-0.29%)`	⬇️
lightning/src/util/events.rs	`17.16% <0.00%> (-0.27%)`	⬇️
lightning/src/ln/channelmanager.rs	`88.72% <91.07%> (+4.09%)`	⬆️
lightning-background-processor/src/lib.rs	`95.43% <100.00%> (+0.55%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.94% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.84% <100.00%> (-0.44%)`	⬇️
lightning/src/routing/router.rs	`97.18% <0.00%> (+1.21%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c57018...ba600db. Read the comment docs.

lightning/src/ln/channelmanager.rs

ariard

Overall SGTM

lightning/src/ln/channel.rs

lightning/src/ln/chanmon_update_fail_tests.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-06-24T18:58:26Z

Rebased now that #948 is merged, note that this still depends on # #954.

TheBlueMatt · 2021-06-30T00:33:46Z

Rebased on #954 and latest upstream, squashing the commits in this PR and fixing conflicts, also rebased on #972 and added a new commit which extends the test from it and adds a new bugfix.

TheBlueMatt · 2021-07-01T03:30:36Z

Rebased after all dependent PRs were merged.

TheBlueMatt · 2021-07-02T15:15:00Z

Fixed bench + fuzz failures due to the new messages which they weren't expecting.

Currently we always generate a `MessageSendEvent::BroadcastChannelUpdate` when a channel is closed even if the channel is private. Our immediate peers should ignore such messages as they haven't seen a corresponding `channel_announcement`, but we are still giving up some privacy by informing our immediate peers of which channels were ours. Here we split `ChannelManager::get_channel_update` into a `get_channel_update_for_broadcast` and `get_channel_update_for_unicast`. The first is used when we are broadcasting a `channel_update`, allowing us to refuse to do so for private channels. The second is used when failing a payment (in which case the recipient has already shown that they are aware of the channel so no such privacy concerns exist).

lightning/src/ln/channelmanager.rs

lightning/src/util/events.rs

valentinewallace · 2021-07-05T17:45:46Z

lightning/src/ln/channelmanager.rs

@@ -3408,7 +3408,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					}
 					return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a channel_update for a channel from the wrong node - it shouldn't know about our private channels!".to_owned(), chan_id));
 				}
-				try_chan_entry!(self, chan.get_mut().channel_update(&msg), channel_state, chan);
+				let were_node_one = self.get_our_node_id().serialize()[..] < chan.get().get_counterparty_node_id().serialize()[..];
+				let msg_from_node_one = msg.contents.flags & 1 == 0;


Might be nicer to make ChannelFlags into its own struct and have methods to check who's node_one, etc. Not necessary for this PR though

Yea, that's probably a good idea for the channel_state in Channel too, but we can do it later.

devrandom · 2021-07-06T09:58:33Z

lightning/src/ln/channelmanager.rs

+	/// [`MessageSendEvent::BroadcastChannelUpdate`] event.
+	///
+	/// May be called with channel_state already locked!
+	fn get_channel_update_for_broadcast(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {


nit: the fn signature is interchangeable with the unicast version, so there's no way to tell if the caller used the wrong one. but there might not be a way to meaningfully improve.

Hmm, yea, happy to do something if you have a specific suggestion but I'm not sure what you really could do there. At least the broadcast/unicast naming makes it pretty clear IMO, especially when you're shoving it in a Broadcast... event

Could have two thin wrappers around msg::ChannelUpdate for unicast/broadcast, but I haven't looked in detail whether it really makes sense.

I dunno, its very explicit at the callsite already, making it more explicit doesn't seem like it'll solve anything IMO.

devrandom · 2021-07-06T10:12:44Z

lightning/src/ln/channelmanager.rs

@@ -2947,6 +2981,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						node_id: counterparty_node_id.clone(),
 						msg: announcement_sigs,
 					});
+				} else if chan.get().is_usable() {
+					channel_state.pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate {


is it appropriate to send this message if we haven't seen the locking block?

Hmm, good point. The spec is mum on it (it only mentions that we have to wait until we receive a funding_locked) see https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#requirements-3 so I think its fine. There isn't a ton of logical reason to wait, either, IMO, its just some fields the counterparty stores locally, so who cares when its received.

OK, something to keep in mind if we see interop issues in this area.

If we are a public node and have a private channel, our counterparty needs to know the fees which we will charge to forward payments to them. Without sending them a channel_update, they have no way to learn that information, resulting in the channel being effectively useless for outbound-from-us payments. This commit fixes our lack of channel_update messages to private channel counterparties, ensuring we always send them a channel_update after the channel funding is confirmed.

If our channel party sends us our own channel_update message, we'll erroneously use the information in that message to update our view of the forwarding parameters our counterparty requires of us, ultimately generating invoices with bogus forwarding information. This fixes that behavior by checking the channel_update's directionality before handling it.

TheBlueMatt · 2021-07-07T19:46:57Z

Squashed without diff, will merge after CI:

$ git diff-tree -U1 6cf22aa2 ba600db7
$

TheBlueMatt marked this pull request as draft June 12, 2021 22:04

devrandom reviewed Jun 13, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

devrandom reviewed Jun 13, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

devrandom reviewed Jun 13, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt mentioned this pull request Jun 17, 2021

Consider channels "live" even if they are awaiting a monitor update #954

Merged

TheBlueMatt force-pushed the 2021-06-send-priv-update branch from cd10b7f to 586d437 Compare June 17, 2021 18:30

devrandom reviewed Jun 18, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

TheBlueMatt added this to the 0.0.99 milestone Jun 20, 2021

ariard reviewed Jun 23, 2021

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/chanmon_update_fail_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-06-send-priv-update branch 2 times, most recently from a8093ee to f00c67d Compare June 24, 2021 18:54

TheBlueMatt force-pushed the 2021-06-send-priv-update branch 2 times, most recently from 7d053b7 to 5b78674 Compare June 30, 2021 00:32

TheBlueMatt force-pushed the 2021-06-send-priv-update branch from 5b78674 to 314acb5 Compare July 1, 2021 03:30

TheBlueMatt marked this pull request as ready for review July 1, 2021 03:30

TheBlueMatt force-pushed the 2021-06-send-priv-update branch 2 times, most recently from 2119ccd to 4d4aeca Compare July 2, 2021 15:14

TheBlueMatt force-pushed the 2021-06-send-priv-update branch 2 times, most recently from e2c7163 to 4d4aeca Compare July 2, 2021 22:22

valentinewallace reviewed Jul 4, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

lightning/src/util/events.rs Show resolved Hide resolved

lightning/src/util/events.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-06-send-priv-update branch from 4d4aeca to 6cf22aa Compare July 4, 2021 23:52

valentinewallace approved these changes Jul 5, 2021

View reviewed changes

devrandom reviewed Jul 6, 2021

View reviewed changes

devrandom approved these changes Jul 7, 2021

View reviewed changes

TheBlueMatt added 3 commits July 7, 2021 19:45

Fix spelling in ChannelManager comment

11594c3

TheBlueMatt force-pushed the 2021-06-send-priv-update branch from 6cf22aa to ba600db Compare July 7, 2021 19:45

TheBlueMatt merged commit 9993845 into lightningdevkit:main Jul 7, 2021

Send channel_update messages to direct peers on private channels #949

Send channel_update messages to direct peers on private channels #949

Uh oh!

Conversation

TheBlueMatt commented Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devrandom commented Jun 13, 2021

Uh oh!

TheBlueMatt commented Jun 17, 2021

Uh oh!

codecov bot commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Jun 30, 2021

Uh oh!

TheBlueMatt commented Jul 1, 2021

Uh oh!

TheBlueMatt commented Jul 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom Jul 6, 2021 • 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!

TheBlueMatt commented Jul 7, 2021

Uh oh!

Uh oh!

TheBlueMatt commented Jun 12, 2021 •

edited

Loading

codecov bot commented Jun 17, 2021 •

edited

Loading

TheBlueMatt commented Jun 24, 2021 •

edited

Loading

devrandom Jul 6, 2021 •

edited

Loading