Move checking of specific require peer feature bits to handlers
authorMatt Corallo <git@bluematt.me>
Mon, 12 Sep 2022 19:06:17 +0000 (19:06 +0000)
committerMatt Corallo <git@bluematt.me>
Tue, 13 Sep 2022 16:59:30 +0000 (16:59 +0000)
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
lightning/src/ln/channelmanager.rs
lightning/src/ln/msgs.rs
lightning/src/ln/onion_route_tests.rs
lightning/src/ln/peer_handler.rs
lightning/src/onion_message/messenger.rs
lightning/src/routing/gossip.rs
lightning/src/util/test_utils.rs

index d5feb9936527ce156b2bec62ccec6c2f241245eb..6a0ca285ba57698ad862edc06824042a3cee8c2b 100644 (file)
@@ -577,7 +577,7 @@ mod tests {
                fn handle_channel_update(&self, _msg: &ChannelUpdate) -> Result<bool, LightningError> { Ok(false) }
                fn get_next_channel_announcement(&self, _starting_point: u64) -> Option<(ChannelAnnouncement, Option<ChannelUpdate>, Option<ChannelUpdate>)> { None }
                fn get_next_node_announcement(&self, _starting_point: Option<&PublicKey>) -> Option<NodeAnnouncement> { 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) {}
index a51eacc9d3f0a69f64882d19161ae90334aae61e..5a699baf5719902c067fe015ca604d54ebf324c4 100644 (file)
@@ -6034,7 +6034,12 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
                }
        }
 
-       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<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
                        retain
                });
                //TODO: Also re-broadcast announcement_signatures
+               Ok(())
        }
 
        fn handle_error(&self, counterparty_node_id: &PublicKey, msg: &msgs::ErrorMessage) {
index 98831137b303b6a9edaae8e393bd32fae2ec48ec..6f2d25a2b70e35a66d7c5054068920b5bb042648 100644 (file)
@@ -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.
        ///
index 4223b4cba06e10b105b914c9fb7714674c424dc0..27f529ee9a9bf81821f49bda9b177bf0bf9b9a88 100644 (file)
@@ -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);
index 838927cb2367ac0e0af1fab435edb7a1170e27da..e6ebe8e34bdd41ded5b090308a08b5d131075219 100644 (file)
@@ -72,7 +72,7 @@ impl RoutingMessageHandler for IgnoringMessageHandler {
        fn get_next_channel_announcement(&self, _starting_point: u64) ->
                Option<(msgs::ChannelAnnouncement, Option<msgs::ChannelUpdate>, Option<msgs::ChannelUpdate>)> { None }
        fn get_next_node_announcement(&self, _starting_point: Option<&PublicKey>) -> Option<msgs::NodeAnnouncement> { 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<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
                                peer_lock.sync_status = InitSyncTracker::ChannelsSyncing(0);
                        }
 
-                       if !msg.features.supports_static_remote_key() {
-                               log_debug!(self.logger, "Peer {} does not support static remote key, disconnecting with no_connection_possible", log_pubkey!(their_node_id));
+                       if let Err(()) = self.message_handler.route_handler.peer_connected(&their_node_id, &msg) {
+                               log_debug!(self.logger, "Route Handler decided we couldn't communicate with peer {}", log_pubkey!(their_node_id));
+                               return Err(PeerHandleError{ no_connection_possible: true }.into());
+                       }
+                       if let Err(()) = self.message_handler.chan_handler.peer_connected(&their_node_id, &msg) {
+                               log_debug!(self.logger, "Channel Handler decided we couldn't communicate with peer {}", log_pubkey!(their_node_id));
+                               return Err(PeerHandleError{ no_connection_possible: true }.into());
+                       }
+                       if let Err(()) = self.message_handler.onion_message_handler.peer_connected(&their_node_id, &msg) {
+                               log_debug!(self.logger, "Onion Message Handler decided we couldn't communicate with peer {}", log_pubkey!(their_node_id));
                                return Err(PeerHandleError{ no_connection_possible: true }.into());
                        }
-
-                       self.message_handler.route_handler.peer_connected(&their_node_id, &msg);
-                       self.message_handler.chan_handler.peer_connected(&their_node_id, &msg);
-                       self.message_handler.onion_message_handler.peer_connected(&their_node_id, &msg);
 
                        peer_lock.their_features = Some(msg.features);
                        return Ok(None);
index 2df01c2ba103dc118d644779aabbcf45489d6ba2..312878110cec299b26c8ce650f7bbb6da94efa08 100644 (file)
@@ -335,11 +335,12 @@ impl<Signer: Sign, K: Deref, L: Deref> OnionMessageHandler for OnionMessenger<Si
                };
        }
 
-       fn peer_connected(&self, their_node_id: &PublicKey, init: &msgs::Init) {
+       fn peer_connected(&self, their_node_id: &PublicKey, init: &msgs::Init) -> 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) {
index e265c6322ec56ad1cb3aee3bae696596d5ff02a5..f973130480d2fd2e832e32f008f43af09dcd10d3 100644 (file)
@@ -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> {
index f0e264b4dd19ba9ad129167bc35f8349e18aaf54..2f772443dd07872f34e56ff687c8d6a6e6fa6062 100644 (file)
@@ -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> {