Onion messages: flip feature bit 🎉 #1688

valentinewallace · 2022-08-31T19:12:42Z

Based on #1650

codecov-commenter · 2022-08-31T19:33:00Z

Codecov Report

Merging #1688 (5d9dddd) into main (3fb3218) will increase coverage by 0.01%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
+ Coverage   90.84%   90.85%   +0.01%     
==========================================
  Files          86       86              
  Lines       46399    46422      +23     
  Branches    46399    46422      +23     
==========================================
+ Hits        42150    42178      +28     
+ Misses       4249     4244       -5

Impacted Files	Coverage Δ
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.24%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <ø> (ø)`
lightning/src/ln/msgs.rs	`86.24% <ø> (ø)`
lightning/src/onion_message/messenger.rs	`89.50% <ø> (ø)`
lightning/src/routing/gossip.rs	`91.43% <ø> (ø)`
lightning/src/util/test_utils.rs	`76.85% <0.00%> (-0.70%)`	⬇️
lightning/src/ln/peer_handler.rs	`56.87% <77.77%> (+0.08%)`	⬆️
lightning/src/ln/features.rs	`98.97% <89.47%> (-0.50%)`	⬇️
lightning/src/ln/monitor_tests.rs	`99.44% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (+0.16%)`	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dunxen · 2022-09-02T09:24:25Z

lightning/src/ln/features.rs

 			// - option_channel_type | option_scid_alias
 			// - option_zeroconf
 			assert_eq!(node_features.flags.len(), 7);
 			assert_eq!(node_features.flags[0], 0b00000010);
 			assert_eq!(node_features.flags[1], 0b01010001);
 			assert_eq!(node_features.flags[2], 0b00001010);
 			assert_eq!(node_features.flags[3], 0b00001000);
-			assert_eq!(node_features.flags[4], 0b00000000);
+			assert_eq!(node_features.flags[4], 0b10000000);


I think I gotta spend some time figuring out how we handle feature bits internally lol

TheBlueMatt

Basically LGTM, but we need to change the Init message we send our peers based on if we have an onion messenger hooked up or not as well.

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2022-09-02T20:51:38Z

LGTM, will need rebase after 1650.

TheBlueMatt · 2022-09-02T22:23:05Z

Needs rebase 🚀

lightning/src/ln/features.rs

lightning/src/ln/peer_handler.rs

jkczyz · 2022-09-02T22:41:26Z

lightning/src/ln/msgs.rs

+	/// Advertise onion message forwarding support in broadcasted node announcements.
+	fn advertise_onion_message_support(&self);


I get that we're trying to only advertise the feature if we have a handler that will support it, but this seems like it should be more of a configuration parameter. Otherwise, we're shoehorning it into a trait meant for handling messages even though this only affects message broadcasting.

Trying to think if there is a better way of accomplishing this...

Yea, I'd really love a cleaner way of accomplishing this, definitely agree, but its not clear to me there is one, at least not without exposing this stuff to the user and making them set yet another config knob correctly, which I'd strongly rather avoid :(

Some questions / ideas to bounce of you both:

Do we expect the user to ever need to write their own OnionMessageHandler? Or will they always configure OnionMessenger with individual handlers for the different types of onion message payloads? If the latter, instead of using IgnoringMessageHandler , maybe we could have it be Option<OnionMessenger> and use its presence to signal if onion message forwarding is supported? Might be worth doing this regardless instead of having two levels of configuration.

For broadcast_node_announcement, could this also take InitFeatures as a parameter? I guess we'd want only PeerManager to determine which features to pass to it. So could have a broadcast_node_announcement method on PeerManager (without the parameter), which delegates to ChannelManager (with the parameter). Though that means having a somewhat unrelated broadcast_node_announcement on ChannelMessageHandler or MessageSendEventsProvider. Alternatively, have BackgroundProcessor be responsible for calling broadcast_node_announcement on ChannelManager with features obtained from PeerManager.

Do we expect the user to ever need to write their own OnionMessageHandler? Or will they always configure OnionMessenger with individual handlers for the different types of onion message payloads? If the latter, instead of using IgnoringMessageHandler , maybe we could have it be Option and use its presence to signal if onion message forwarding is supported? Might be worth doing this regardless instead of having two levels of configuration.

Yea, I'd be fine with that, I don't have a super strong idea of when a user would want a custom onion message handler vs just handling onion messages in a custom way. It doesn't seem to clean the interface up all that much, though, having a "do you forward messages" call on the handler interface seems fine, its the calling back through to ChannelManager that's awkward.

For broadcast_node_announcement, could this also take InitFeatures as a parameter? I guess we'd want only PeerManager to determine which features to pass to it.

I don't think so, there's plenty of things the ChannelManager is responsible for and needs to select the features of :(.

I don't think so, there's plenty of things the ChannelManager is responsible for and needs to select the features of :(.

Not sure I understand here. Looks like broadcast_node_announcement is the only place it needs our features.

Hmmm... I see . Though that would also mean the features advertised by Init may not be accurate if a custom channel message handler were used? Seems each handler may want to declare which features they support (i.e., implement), and the PeerManager would want to take the intersection of these for what is actually used in either Init or ChannelAnnouncement?

er... s/intersection/union

Though that would also mean the features advertised by Init may not be accurate if a custom channel message handler were used?

Hmm, good point.

the PeerManager would want to take the intersection of these for what is actually used in either Init or ChannelAnnouncement?

Yea, maybe that's the way we want to go. One annoying part is that we can't make a node announcement until we've sent at least one public channel announcement. Thus, if we make a new peer connection we'll really have to ask the ChannelManager to rebroadcast the channel announcements (and updates, I guess) from the PeerManager. We maybe should do this anyway and do it when we connect to a peer.

Ok, take a look at #1699 - it basically does this, though doesn't implement the feature OR'ing part yet.

TheBlueMatt

Still LGTM, Feel free to squash once @jkczyz is fine.

TheBlueMatt · 2022-09-06T16:19:51Z

lightning/src/ln/peer_handler.rs

@@ -506,6 +508,7 @@ pub struct PeerManager<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: D
 	/// when an event process call is waiting.
 	blocked_event_processors: AtomicBool,
 	our_node_secret: SecretKey,
+	our_features: InitFeatures,


Oops, lol, I commented wrong, I meant "stack/local variable", not "member variable". It doesn't really matter all that much, though.

Oh lol. @jkczyz do you have a preference?

Possibly would depend on the resolution of #1688 (comment).

lightning/src/ln/features.rs

valentinewallace · 2022-09-09T17:42:57Z

Oops, mis-push. Updating

ariard

Light Code Review ACK 43ae31b

ariard · 2022-09-09T19:39:55Z

lightning/src/ln/features.rs

@@ -39,8 +39,13 @@
 //!     (see [BOLT-4](https://github.com/lightning/bolts/blob/master/04-onion-routing.md) for more information).
 //! - `BasicMPP` - requires/supports that a node can receive basic multi-part payments
 //!     (see [BOLT-4](https://github.com/lightning/bolts/blob/master/04-onion-routing.md#basic-multi-part-payments) for more information).
+//! - `Wumbo` - requires/supports that a node create large channels. Called `option_support_large_channel` in the spec.


Could say "large channels (superior to MAX_FUNDING_SATOSHIS_NO_WUMBO)"

Ah, that const isn't public

lightning/src/ln/msgs.rs

When we broadcast a node announcement, the features we support are really a combination of all the various features our different handlers support. This commit captures this concept by OR'ing our NodeFeatures across both our channel and routing message handlers.

When ChannelMessageHandler implementations wish to return a NodeFeatures which contain all the known flags that are relevant to channel handling, but not gossip handling, they currently need to do so by manually constructing a NodeFeatures with all known flags and then clearing the ones they don't want. Instead of spreading this logic across the codebase, this consolidates such construction into one place in features.rs.

In upcoming commit(s), onion message support will be advertised conditionally based on the OnionMessageProvider provided to PeerManager.

Similar to how we OR our InitFeaures and NodeFeatures across both our channel and routing message handlers, we also want to OR the features of our onion message handler.

To be uniform with the other handler provided_node_features docs

ariard

ACK 5d9dddd

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch from 81a18d2 to c5503ee Compare August 31, 2022 19:14

valentinewallace added Seeking Code Review blocked on dependent pr labels Aug 31, 2022

jkczyz self-requested a review September 1, 2022 19:54

dunxen reviewed Sep 2, 2022

View reviewed changes

TheBlueMatt reviewed Sep 2, 2022

View reviewed changes

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

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch from c5503ee to f64aea0 Compare September 2, 2022 19:41

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch 2 times, most recently from d923d54 to 4f2a842 Compare September 2, 2022 22:22

valentinewallace removed the blocked on dependent pr label Sep 2, 2022

jkczyz reviewed Sep 2, 2022

View reviewed changes

TheBlueMatt previously approved these changes Sep 2, 2022

View reviewed changes

valentinewallace dismissed TheBlueMatt’s stale review via 3ed9262 September 6, 2022 15:49

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch 3 times, most recently from ee576ca to 1fee894 Compare September 6, 2022 15:57

valentinewallace removed the Seeking Code Review label Sep 6, 2022

valentinewallace assigned TheBlueMatt and jkczyz Sep 6, 2022

TheBlueMatt reviewed Sep 6, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Sep 7, 2022

Fetch InitFeatures from both Channel and Routing Message Handlers #1701

Merged

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch from 1fee894 to 50444ea Compare September 9, 2022 16:45

Enable all feature sets to OR with another set of the same type

fe8b4c7

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch 2 times, most recently from 08ed7d0 to 4805961 Compare September 9, 2022 16:52

TheBlueMatt previously approved these changes Sep 9, 2022

View reviewed changes

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

TheBlueMatt added this to the 0.0.111 milestone Sep 9, 2022

valentinewallace dismissed TheBlueMatt’s stale review via d757585 September 9, 2022 17:42

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch from 4805961 to d757585 Compare September 9, 2022 17:42

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch 2 times, most recently from 5a2350b to 43ae31b Compare September 9, 2022 17:56

TheBlueMatt previously approved these changes Sep 9, 2022

View reviewed changes

jkczyz previously approved these changes Sep 9, 2022

View reviewed changes

ariard reviewed Sep 9, 2022

View reviewed changes

ariard previously approved these changes Sep 9, 2022

View reviewed changes

valentinewallace added 7 commits September 9, 2022 15:58

Add missing wumbo feature bit docs

f5e1255

Support forwarding onion messages in advertised features

1ac3fcc

In upcoming commit(s), onion message support will be advertised conditionally based on the OnionMessageProvider provided to PeerManager.

OR InitFeatures and NodeFeatures from onion message handler

90f5906

Similar to how we OR our InitFeaures and NodeFeatures across both our channel and routing message handlers, we also want to OR the features of our onion message handler.

Don't advertise onion messages in known channel features

9c3b6d2

Update ChannelMessageHandler::provided_node_features docs

5d9dddd

To be uniform with the other handler provided_node_features docs

valentinewallace dismissed stale reviews from ariard, jkczyz, and TheBlueMatt via 5d9dddd September 9, 2022 20:01

valentinewallace force-pushed the 2022-08-flip-om-feature-bit branch from 43ae31b to 5d9dddd Compare September 9, 2022 20:01

jkczyz approved these changes Sep 9, 2022

View reviewed changes

ariard approved these changes Sep 9, 2022

View reviewed changes

TheBlueMatt approved these changes Sep 9, 2022

View reviewed changes

TheBlueMatt merged commit 877a5fc into lightningdevkit:main Sep 9, 2022

jkczyz mentioned this pull request May 10, 2023

BOLT12 Offers Tracking Issue #1970

Open

60 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Onion messages: flip feature bit 🎉 #1688

Onion messages: flip feature bit 🎉 #1688

valentinewallace commented Aug 31, 2022

codecov-commenter commented Aug 31, 2022 •

edited

Loading

dunxen Sep 2, 2022

TheBlueMatt left a comment

TheBlueMatt commented Sep 2, 2022

TheBlueMatt commented Sep 2, 2022

jkczyz Sep 2, 2022

TheBlueMatt Sep 2, 2022

jkczyz Sep 6, 2022

TheBlueMatt Sep 6, 2022

jkczyz Sep 6, 2022

jkczyz Sep 6, 2022

jkczyz Sep 6, 2022

TheBlueMatt Sep 6, 2022 •

edited

Loading

TheBlueMatt Sep 6, 2022

TheBlueMatt Sep 6, 2022

TheBlueMatt left a comment

TheBlueMatt Sep 6, 2022

valentinewallace Sep 6, 2022

jkczyz Sep 6, 2022

valentinewallace commented Sep 9, 2022

ariard left a comment

ariard Sep 9, 2022

valentinewallace Sep 9, 2022

ariard left a comment

		/// Advertise onion message forwarding support in broadcasted node announcements.
		fn advertise_onion_message_support(&self);

Onion messages: flip feature bit 🎉 #1688

Onion messages: flip feature bit 🎉 #1688

Conversation

valentinewallace commented Aug 31, 2022

codecov-commenter commented Aug 31, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 2, 2022

TheBlueMatt commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Sep 9, 2022

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 31, 2022 •

edited

Loading

TheBlueMatt Sep 6, 2022 •

edited

Loading