From bbb590b5518eb6e76ddd642187163c487c67dd5c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 12 Sep 2022 19:06:17 +0000 Subject: [PATCH] Move checking of specific require peer feature bits to handlers As we remove the concept of a global "known/supported" feature set in LDK, we should also remove the concept of a global "required" feature set. This does so by moving the checks for specific required features into handlers. Specifically, it allows the handler `peer_connected` method to return an `Err` if the peer should be disconnected. Only one such required feature bit is currently set - `static_remote_key`, which is required in `ChannelManager`. --- lightning-net-tokio/src/lib.rs | 5 +++-- lightning/src/ln/channelmanager.rs | 8 +++++++- lightning/src/ln/msgs.rs | 18 +++++++++++++++--- lightning/src/ln/onion_route_tests.rs | 6 ++++-- lightning/src/ln/peer_handler.rs | 22 +++++++++++++--------- lightning/src/onion_message/messenger.rs | 3 ++- lightning/src/routing/gossip.rs | 7 +++++-- lightning/src/util/test_utils.rs | 8 +++++--- 8 files changed, 54 insertions(+), 23 deletions(-) diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs index d5feb9936..6a0ca285b 100644 --- a/lightning-net-tokio/src/lib.rs +++ b/lightning-net-tokio/src/lib.rs @@ -577,7 +577,7 @@ mod tests { fn handle_channel_update(&self, _msg: &ChannelUpdate) -> Result { Ok(false) } fn get_next_channel_announcement(&self, _starting_point: u64) -> Option<(ChannelAnnouncement, Option, Option)> { None } fn get_next_node_announcement(&self, _starting_point: Option<&PublicKey>) -> Option { None } - fn peer_connected(&self, _their_node_id: &PublicKey, _init_msg: &Init) { } + fn peer_connected(&self, _their_node_id: &PublicKey, _init_msg: &Init) -> Result<(), ()> { Ok(()) } fn handle_reply_channel_range(&self, _their_node_id: &PublicKey, _msg: ReplyChannelRange) -> Result<(), LightningError> { Ok(()) } fn handle_reply_short_channel_ids_end(&self, _their_node_id: &PublicKey, _msg: ReplyShortChannelIdsEnd) -> Result<(), LightningError> { Ok(()) } fn handle_query_channel_range(&self, _their_node_id: &PublicKey, _msg: QueryChannelRange) -> Result<(), LightningError> { Ok(()) } @@ -608,10 +608,11 @@ mod tests { self.pubkey_disconnected.clone().try_send(()).unwrap(); } } - fn peer_connected(&self, their_node_id: &PublicKey, _msg: &Init) { + fn peer_connected(&self, their_node_id: &PublicKey, _init_msg: &Init) -> Result<(), ()> { if *their_node_id == self.expected_pubkey { self.pubkey_connected.clone().try_send(()).unwrap(); } + Ok(()) } fn handle_channel_reestablish(&self, _their_node_id: &PublicKey, _msg: &ChannelReestablish) {} fn handle_error(&self, _their_node_id: &PublicKey, _msg: &ErrorMessage) {} diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a51eacc9d..5a699baf5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6034,7 +6034,12 @@ impl } } - fn peer_connected(&self, counterparty_node_id: &PublicKey, init_msg: &msgs::Init) { + fn peer_connected(&self, counterparty_node_id: &PublicKey, init_msg: &msgs::Init) -> Result<(), ()> { + if !init_msg.features.supports_static_remote_key() { + log_debug!(self.logger, "Peer {} does not support static remote key, disconnecting with no_connection_possible", log_pubkey!(counterparty_node_id)); + return Err(()); + } + log_debug!(self.logger, "Generating channel_reestablish events for {}", log_pubkey!(counterparty_node_id)); let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); @@ -6085,6 +6090,7 @@ impl retain }); //TODO: Also re-broadcast announcement_signatures + Ok(()) } fn handle_error(&self, counterparty_node_id: &PublicKey, msg: &msgs::ErrorMessage) { diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 98831137b..6f2d25a2b 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -889,7 +889,11 @@ pub trait ChannelMessageHandler : MessageSendEventsProvider { fn peer_disconnected(&self, their_node_id: &PublicKey, no_connection_possible: bool); /// Handle a peer reconnecting, possibly generating channel_reestablish message(s). - fn peer_connected(&self, their_node_id: &PublicKey, msg: &Init); + /// + /// May return an `Err(())` if the features the peer supports are not sufficient to communicate + /// with us. Implementors should be somewhat conservative about doing so, however, as other + /// message handlers may still wish to communicate with this peer. + fn peer_connected(&self, their_node_id: &PublicKey, msg: &Init) -> Result<(), ()>; /// Handle an incoming channel_reestablish message from the given peer. fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &ChannelReestablish); @@ -943,7 +947,11 @@ pub trait RoutingMessageHandler : MessageSendEventsProvider { /// Called when a connection is established with a peer. This can be used to /// perform routing table synchronization using a strategy defined by the /// implementor. - fn peer_connected(&self, their_node_id: &PublicKey, init: &Init); + /// + /// May return an `Err(())` if the features the peer supports are not sufficient to communicate + /// with us. Implementors should be somewhat conservative about doing so, however, as other + /// message handlers may still wish to communicate with this peer. + fn peer_connected(&self, their_node_id: &PublicKey, init: &Init) -> Result<(), ()>; /// Handles the reply of a query we initiated to learn about channels /// for a given range of blocks. We can expect to receive one or more /// replies to a single query. @@ -979,7 +987,11 @@ pub trait OnionMessageHandler : OnionMessageProvider { fn handle_onion_message(&self, peer_node_id: &PublicKey, msg: &OnionMessage); /// Called when a connection is established with a peer. Can be used to track which peers /// advertise onion message support and are online. - fn peer_connected(&self, their_node_id: &PublicKey, init: &Init); + /// + /// May return an `Err(())` if the features the peer supports are not sufficient to communicate + /// with us. Implementors should be somewhat conservative about doing so, however, as other + /// message handlers may still wish to communicate with this peer. + fn peer_connected(&self, their_node_id: &PublicKey, init: &Init) -> Result<(), ()>; /// Indicates a connection to the peer failed/an existing connection was lost. Allows handlers to /// drop and refuse to forward onion messages to this peer. /// diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 4223b4cba..27f529ee9 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -887,10 +887,12 @@ fn test_do_not_default_to_onion_payload_tlv_format_when_unsupported() { let chanmon_cfgs = create_chanmon_cfgs(4); let mut node_cfgs = create_node_cfgs(4, &chanmon_cfgs); - // Set `node[1]` config to `InitFeatures::empty()` which return `false` for - // `supports_variable_length_onion()` + // Set `node[1]` config to `InitFeatures::empty()` + `static_remote_key` which implies + // `!supports_variable_length_onion()` but still supports the required static-remote-key + // feature. let mut node_1_cfg = &mut node_cfgs[1]; node_1_cfg.features = InitFeatures::empty(); + node_1_cfg.features.set_static_remote_key_required(); let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs); diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 838927cb2..e6ebe8e34 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -72,7 +72,7 @@ impl RoutingMessageHandler for IgnoringMessageHandler { fn get_next_channel_announcement(&self, _starting_point: u64) -> Option<(msgs::ChannelAnnouncement, Option, Option)> { None } fn get_next_node_announcement(&self, _starting_point: Option<&PublicKey>) -> Option { None } - fn peer_connected(&self, _their_node_id: &PublicKey, _init: &msgs::Init) {} + fn peer_connected(&self, _their_node_id: &PublicKey, _init: &msgs::Init) -> Result<(), ()> { Ok(()) } fn handle_reply_channel_range(&self, _their_node_id: &PublicKey, _msg: msgs::ReplyChannelRange) -> Result<(), LightningError> { Ok(()) } fn handle_reply_short_channel_ids_end(&self, _their_node_id: &PublicKey, _msg: msgs::ReplyShortChannelIdsEnd) -> Result<(), LightningError> { Ok(()) } fn handle_query_channel_range(&self, _their_node_id: &PublicKey, _msg: msgs::QueryChannelRange) -> Result<(), LightningError> { Ok(()) } @@ -87,7 +87,7 @@ impl OnionMessageProvider for IgnoringMessageHandler { } impl OnionMessageHandler for IgnoringMessageHandler { fn handle_onion_message(&self, _their_node_id: &PublicKey, _msg: &msgs::OnionMessage) {} - fn peer_connected(&self, _their_node_id: &PublicKey, _init: &msgs::Init) {} + fn peer_connected(&self, _their_node_id: &PublicKey, _init: &msgs::Init) -> Result<(), ()> { Ok(()) } fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {} fn provided_node_features(&self) -> NodeFeatures { NodeFeatures::empty() } fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures { @@ -208,7 +208,7 @@ impl ChannelMessageHandler for ErroringMessageHandler { // msgs::ChannelUpdate does not contain the channel_id field, so we just drop them. fn handle_channel_update(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelUpdate) {} fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {} - fn peer_connected(&self, _their_node_id: &PublicKey, _msg: &msgs::Init) {} + fn peer_connected(&self, _their_node_id: &PublicKey, _init: &msgs::Init) -> Result<(), ()> { Ok(()) } fn handle_error(&self, _their_node_id: &PublicKey, _msg: &msgs::ErrorMessage) {} fn provided_node_features(&self) -> NodeFeatures { NodeFeatures::empty() } fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures { @@ -1210,14 +1210,18 @@ impl OnionMessageHandler for OnionMessenger Result<(), ()> { if init.features.supports_onion_messages() { let mut peers = self.pending_messages.lock().unwrap(); peers.insert(their_node_id.clone(), VecDeque::new()); } + Ok(()) } fn peer_disconnected(&self, their_node_id: &PublicKey, _no_connection_possible: bool) { diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index e265c6322..f97313048 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -368,10 +368,12 @@ where C::Target: chain::Access, L::Target: Logger /// to request gossip messages for each channel. The sync is considered complete /// when the final reply_scids_end message is received, though we are not /// tracking this directly. - fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &Init) { + fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &Init) -> Result<(), ()> { // We will only perform a sync with peers that support gossip_queries. if !init_msg.features.supports_gossip_queries() { - return (); + // Don't disconnect peers for not supporting gossip queries. We may wish to have + // channels with peers even without being able to exchange gossip. + return Ok(()); } // The lightning network's gossip sync system is completely broken in numerous ways. @@ -445,6 +447,7 @@ where C::Target: chain::Access, L::Target: Logger timestamp_range: u32::max_value(), }, }); + Ok(()) } fn handle_reply_channel_range(&self, _their_node_id: &PublicKey, _msg: ReplyChannelRange) -> Result<(), LightningError> { diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index f0e264b4d..2f772443d 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -350,9 +350,10 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler { self.received_msg(wire::Message::ChannelReestablish(msg.clone())); } fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {} - fn peer_connected(&self, _their_node_id: &PublicKey, _msg: &msgs::Init) { + fn peer_connected(&self, _their_node_id: &PublicKey, _msg: &msgs::Init) -> Result<(), ()> { // Don't bother with `received_msg` for Init as its auto-generated and we don't want to // bother re-generating the expected Init message in all tests. + Ok(()) } fn handle_error(&self, _their_node_id: &PublicKey, msg: &msgs::ErrorMessage) { self.received_msg(wire::Message::Error(msg.clone())); @@ -465,9 +466,9 @@ impl msgs::RoutingMessageHandler for TestRoutingMessageHandler { None } - fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) { + fn peer_connected(&self, their_node_id: &PublicKey, init_msg: &msgs::Init) -> Result<(), ()> { if !init_msg.features.supports_gossip_queries() { - return (); + return Ok(()); } #[allow(unused_mut, unused_assignments)] @@ -491,6 +492,7 @@ impl msgs::RoutingMessageHandler for TestRoutingMessageHandler { timestamp_range: u32::max_value(), }, }); + Ok(()) } fn handle_reply_channel_range(&self, _their_node_id: &PublicKey, _msg: msgs::ReplyChannelRange) -> Result<(), msgs::LightningError> { -- 2.39.5