From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com> Date: Fri, 21 Feb 2020 19:02:21 +0000 (+0000) Subject: Merge pull request #512 from TheBlueMatt/2020-02-peer_handler-docs X-Git-Tag: v0.0.12~122 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=2be0810e782b858f5a3c6fd0fdff52e235ac92be;hp=697b47974fbad82d05cb56a5064feac86e1672b9;p=rust-lightning Merge pull request #512 from TheBlueMatt/2020-02-peer_handler-docs Fix incorrect docs/disconnect handling in peer_handler --- diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index a5a3f7d8..bb07d24e 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 47e17918..c2bac324 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 6effe543..ca6b33a7 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(); + } } } }