From faaa4d207dd420b0a2a331ddd9c229eeb0ad26f7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 20 Feb 2020 15:12:42 -0500 Subject: [PATCH] Fix incorrect docs around disconnect in peer_handler + rename fns The way PeerHandler was written, it was supposed to remove from self.peers iff the API docs indicate that disconnect_event should NOT be called (and otherwise rely on disconnect_event to do so). Sadly, the implementation was way out of whack with reality - in the implementation, essentially anywhere where PeerHandler originated the disconnection, the peer was removed and no disconnect_event was expected. The docs, however, indicated that disconnect_event should nearly only be called, only not doing so when the initial handshake message never completed. We opt to change the docs, mostly, as well as clean up the ping/pong handling somewhat and rename a few functions to clarify what they actually do. --- fuzz/src/full_stack.rs | 4 +-- lightning-net-tokio/src/lib.rs | 4 +-- lightning/src/ln/peer_handler.rs | 60 +++++++++++++++++++------------- 3 files changed, 39 insertions(+), 29 deletions(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index a5a3f7d8e..bb07d24e8 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -217,7 +217,7 @@ impl<'a> Drop for MoneyLossDetector<'a> { // Disconnect all peers for (idx, peer) in self.peers.borrow().iter().enumerate() { if *peer { - self.handler.disconnect_event(&Peer{id: idx as u8, peers_connected: &self.peers}); + self.handler.socket_disconnected(&Peer{id: idx as u8, peers_connected: &self.peers}); } } @@ -378,7 +378,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { 2 => { let peer_id = get_slice!(1)[0]; if !peers.borrow()[peer_id as usize] { return; } - loss_detector.handler.disconnect_event(&Peer{id: peer_id, peers_connected: &peers}); + loss_detector.handler.socket_disconnected(&Peer{id: peer_id, peers_connected: &peers}); peers.borrow_mut()[peer_id as usize] = false; }, 3 => { diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs index 47e179189..c2bac324b 100644 --- a/lightning-net-tokio/src/lib.rs +++ b/lightning-net-tokio/src/lib.rs @@ -59,7 +59,7 @@ impl Connection { return future::Either::A(blocker.then(|_| { Ok(()) })); } } - //TODO: There's a race where we don't meet the requirements of disconnect_socket if its + //TODO: There's a race where we don't meet the requirements of socket_disconnected if its //called right here, after we release the us_ref lock in the scope above, but before we //call read_event! match peer_manager.read_event(&mut SocketDescriptor::new(us_ref.clone(), peer_manager.clone()), pending_read) { @@ -84,7 +84,7 @@ impl Connection { future::Either::B(future::result(Ok(()))) }).then(move |_| { if us_close_ref.lock().unwrap().need_disconnect { - peer_manager_ref.disconnect_event(&SocketDescriptor::new(us_close_ref, peer_manager_ref.clone())); + peer_manager_ref.socket_disconnected(&SocketDescriptor::new(us_close_ref, peer_manager_ref.clone())); println!("Peer disconnected!"); } else { println!("We disconnected peer!"); diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 6effe5433..ca6b33a77 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -49,12 +49,12 @@ pub struct MessageHandler where CM::Target: msgs::ChannelMessageHandl /// You probably want to just extend an int and put a file descriptor in a struct and implement /// send_data. Note that if you are using a higher-level net library that may close() itself, be /// careful to ensure you don't have races whereby you might register a new connection with an fd -/// the same as a yet-to-be-disconnect_event()-ed. +/// the same as a yet-to-be-socket_disconnected()-ed. pub trait SocketDescriptor : cmp::Eq + hash::Hash + Clone { /// Attempts to send some data from the given slice to the peer. /// /// Returns the amount of data which was sent, possibly 0 if the socket has since disconnected. - /// Note that in the disconnected case, a disconnect_event must still fire and further write + /// Note that in the disconnected case, a socket_disconnected must still fire and further write /// attempts may occur until that time. /// /// If the returned size is smaller than data.len(), a write_available event must @@ -67,17 +67,18 @@ pub trait SocketDescriptor : cmp::Eq + hash::Hash + Clone { /// *not* imply that further read events should be paused. fn send_data(&mut self, data: &[u8], resume_read: bool) -> usize; /// Disconnect the socket pointed to by this SocketDescriptor. Once this function returns, no - /// more calls to write_event, read_event or disconnect_event may be made with this descriptor. - /// No disconnect_event should be generated as a result of this call, though obviously races - /// may occur whereby disconnect_socket is called after a call to disconnect_event but prior to - /// that event completing. + /// more calls to write_buffer_space_avail, read_event or socket_disconnected may be made with + /// this descriptor. No socket_disconnected call should be generated as a result of this call, + /// though obviously races may occur whereby disconnect_socket is called after a call to + /// socket_disconnected but prior to that event completing. fn disconnect_socket(&mut self); } /// Error for PeerManager errors. If you get one of these, you must disconnect the socket and -/// generate no further read/write_events for the descriptor, only triggering a single -/// disconnect_event (unless it was provided in response to a new_*_connection event, in which case -/// no such disconnect_event must be generated and the socket be silently disconencted). +/// generate no further read_event/write_buffer_space_avail calls for the descriptor, only +/// triggering a single socket_disconnected call (unless it was provided in response to a +/// new_*_connection event, in which case no such socket_disconnected() must be generated and the +/// socket be silently disconencted). pub struct PeerHandleError { /// Used to indicate that we probably can't make any future connections to this peer, implying /// we should go ahead and force-close any channels we have with it. @@ -201,7 +202,7 @@ macro_rules! encode_msg { } /// Manages and reacts to connection events. You probably want to use file descriptors as PeerIds. -/// PeerIds may repeat, but only after disconnect_event() has been called. +/// PeerIds may repeat, but only after socket_disconnected() has been called. impl PeerManager where CM::Target: msgs::ChannelMessageHandler { /// Constructs a new PeerManager with the given message handlers and node_id secret key /// ephemeral_random_data is used to derive per-connection ephemeral keys and must be @@ -254,13 +255,13 @@ impl PeerManager where } /// Indicates a new outbound connection has been established to a node with the given node_id. - /// Note that if an Err is returned here you MUST NOT call disconnect_event for the new + /// Note that if an Err is returned here you MUST NOT call socket_disconnected for the new /// descriptor but must disconnect the connection immediately. /// /// Returns a small number of bytes to send to the remote node (currently always 50). /// - /// Panics if descriptor is duplicative with some other descriptor which has not yet has a - /// disconnect_event. + /// Panics if descriptor is duplicative with some other descriptor which has not yet had a + /// socket_disconnected(). pub fn new_outbound_connection(&self, their_node_id: PublicKey, descriptor: Descriptor) -> Result, PeerHandleError> { let mut peer_encryptor = PeerChannelEncryptor::new_outbound(their_node_id.clone(), self.get_ephemeral_key()); let res = peer_encryptor.get_act_one().to_vec(); @@ -294,11 +295,11 @@ impl PeerManager where /// /// May refuse the connection by returning an Err, but will never write bytes to the remote end /// (outbound connector always speaks first). Note that if an Err is returned here you MUST NOT - /// call disconnect_event for the new descriptor but must disconnect the connection + /// call socket_disconnected for the new descriptor but must disconnect the connection /// immediately. /// - /// Panics if descriptor is duplicative with some other descriptor which has not yet has a - /// disconnect_event. + /// Panics if descriptor is duplicative with some other descriptor which has not yet had + /// socket_disconnected called. pub fn new_inbound_connection(&self, descriptor: Descriptor) -> Result<(), PeerHandleError> { let peer_encryptor = PeerChannelEncryptor::new_inbound(&self.our_node_secret); let pending_read_buffer = [0; 50].to_vec(); // Noise act one is 50 bytes @@ -406,10 +407,11 @@ impl PeerManager where /// /// Will most likely call send_data on the descriptor passed in (or the descriptor handed into /// new_*\_connection) before returning. Thus, be very careful with reentrancy issues! The - /// invariants around calling write_event in case a write did not fully complete must still - /// hold - be ready to call write_event again if a write call generated here isn't sufficient! - /// Panics if the descriptor was not previously registered in a new_\*_connection event. - pub fn write_event(&self, descriptor: &mut Descriptor) -> Result<(), PeerHandleError> { + /// invariants around calling write_buffer_space_avail in case a write did not fully complete + /// must still hold - be ready to call write_buffer_space_avail again if a write call generated + /// here isn't sufficient! Panics if the descriptor was not previously registered in a + /// new_\*_connection event. + pub fn write_buffer_space_avail(&self, descriptor: &mut Descriptor) -> Result<(), PeerHandleError> { let mut peers = self.peers.lock().unwrap(); match peers.peers.get_mut(descriptor) { None => panic!("Descriptor for write_event is not already known to PeerManager"), @@ -429,8 +431,8 @@ impl PeerManager where /// Thus, however, you almost certainly want to call process_events() after any read_event to /// generate send_data calls to handle responses. /// - /// If Ok(true) is returned, further read_events should not be triggered until a write_event on - /// this file descriptor has resume_read set (preventing DoS issues in the send buffer). + /// If Ok(true) is returned, further read_events should not be triggered until a send_data call + /// on this file descriptor has resume_read set (preventing DoS issues in the send buffer). /// /// Panics if the descriptor was not previously registered in a new_*_connection event. pub fn read_event(&self, peer_descriptor: &mut Descriptor, data: Vec) -> Result { @@ -1036,11 +1038,13 @@ impl PeerManager where /// Indicates that the given socket descriptor's connection is now closed. /// - /// This must be called even if a PeerHandleError was given for a read_event or write_event, - /// but must NOT be called if a PeerHandleError was provided out of a new_\*\_connection event! + /// This must only be called if the socket has been disconnected by the peer or your own + /// decision to disconnect it and must NOT be called in any case where other parts of this + /// library (eg PeerHandleError, explicit disconnect_socket calls) instruct you to disconnect + /// the peer. /// /// Panics if the descriptor was not previously registered in a successful new_*_connection event. - pub fn disconnect_event(&self, descriptor: &Descriptor) { + pub fn socket_disconnected(&self, descriptor: &Descriptor) { self.disconnect_event_internal(descriptor, false); } @@ -1073,10 +1077,12 @@ impl PeerManager where let peers_needing_send = &mut peers.peers_needing_send; let node_id_to_descriptor = &mut peers.node_id_to_descriptor; let peers = &mut peers.peers; + let mut descriptors_needing_disconnect = Vec::new(); peers.retain(|descriptor, peer| { if peer.awaiting_pong { peers_needing_send.remove(descriptor); + descriptors_needing_disconnect.push(descriptor.clone()); match peer.their_node_id { Some(node_id) => { node_id_to_descriptor.remove(&node_id); @@ -1104,6 +1110,10 @@ impl PeerManager where peer.awaiting_pong = true; true }); + + for mut descriptor in descriptors_needing_disconnect.drain(..) { + descriptor.disconnect_socket(); + } } } } -- 2.39.5