Fetch InitFeatures from both Channel and Routing Message Handlers #1701
Conversation
|
|
||
| fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures { | ||
| let mut features = InitFeatures::empty(); | ||
| features.set_gossip_queries_optional(); |
There was a problem hiding this comment.
Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
There was a problem hiding this comment.
No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
There was a problem hiding this comment.
Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
There was a problem hiding this comment.
Sure added a note. We will no longer ever set it, no.
There was a problem hiding this comment.
Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
There was a problem hiding this comment.
I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
There was a problem hiding this comment.
Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
TheBlueMatt
force-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dc
Compare
|
|
||
| fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures { | ||
| let mut features = InitFeatures::empty(); | ||
| features.set_gossip_queries_optional(); |
There was a problem hiding this comment.
Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
TheBlueMatt
force-pushed
the
2022-09-feature-or
branch
from
ff3c0dc to
6945f28
Compare
TheBlueMatt
force-pushed
the
2022-09-feature-or
branch
from
6945f28 to
65c3eb9
Compare
|
Rebased on #1699. |
TheBlueMatt
force-pushed
the
2022-09-feature-or
branch
from
65c3eb9 to
9bb722b
Compare
Codecov Report
@@ Coverage Diff @@
## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87%
=======================================
Files 86 86
Lines 46378 46378
Branches 46378 46378
=======================================
Hits 42146 42146
Misses 4232 4232 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
lightning/src/ln/channelmanager.rs
Outdated
| /// Panics if `addresses` is absurdly large (more than 100). | ||
| /// | ||
| /// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events | ||
| pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) { |
There was a problem hiding this comment.
Unused import for NetAddress.
There was a problem hiding this comment.
Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
TheBlueMatt
force-pushed
the
2022-09-feature-or
branch
from
9bb722b to
7d18667
Compare
|
LGTM after squash |
Like we now do for `NodeFeatures`, this converts to asking our registered `ChannelMessageHandler` for our `InitFeatures` instead of hard-coding them to the global LDK known set. This allows handlers to set different feature bits based on what our configuration actually supports rather than what LDK supports in aggregate.
When we go to send an Init message to new peers, 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 InitFeatures across both our Channel and Routing handlers. Note that this also disables setting the `initial_routing_sync` flag in init messages, as was intended in e742894, per the comment added on `clear_initial_routing_sync`, though this should not be a behavior change in practice as nodes which support gossip queries ignore the initial routing sync flag.
When `ChannelMessageHandler` implementations wish to return an `InitFeatures` 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 an InitFeatures with all known flags and then clearing the ones they dont want. Instead of spreading this logic out across the codebase, this consolidates such construction to one place in features.rs.
|
Squashed without further change. |
TheBlueMatt
force-pushed
the
2022-09-feature-or
branch
from
7d18667 to
1b67b0b
Compare
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.