Skip to content

Commit 455a733

Browse files
committed
Clean up docs on peer_handler significantly.
There are various typo and grammatical fixes here, as well as concrete updates to correctness.
1 parent 8708f5d commit 455a733

File tree

1 file changed

+78
-44
lines changed

1 file changed

+78
-44
lines changed

lightning/src/ln/peer_handler.rs

Lines changed: 78 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,15 @@ pub struct MessageHandler<CM: Deref, RM: Deref> where
161161
CM::Target: ChannelMessageHandler,
162162
RM::Target: RoutingMessageHandler {
163163
/// A message handler which handles messages specific to channels. Usually this is just a
164-
/// ChannelManager object or a ErroringMessageHandler.
164+
/// [`ChannelManager`] object or a [`ErroringMessageHandler`].
165+
///
166+
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
165167
pub chan_handler: CM,
166168
/// A message handler which handles messages updating our knowledge of the network channel
167-
/// graph. Usually this is just a NetGraphMsgHandlerMonitor object or an IgnoringMessageHandler.
169+
/// graph. Usually this is just a [`NetGraphMsgHandler`] object or an
170+
/// [`IgnoringMessageHandler`].
171+
///
172+
/// [`NetGraphMsgHandler`]: crate::routing::network_graph::NetGraphMsgHandler
168173
pub route_handler: RM,
169174
}
170175

@@ -174,26 +179,34 @@ pub struct MessageHandler<CM: Deref, RM: Deref> where
174179
///
175180
/// For efficiency, Clone should be relatively cheap for this type.
176181
///
177-
/// You probably want to just extend an int and put a file descriptor in a struct and implement
178-
/// send_data. Note that if you are using a higher-level net library that may call close() itself,
179-
/// be careful to ensure you don't have races whereby you might register a new connection with an
180-
/// fd which is the same as a previous one which has yet to be removed via
181-
/// PeerManager::socket_disconnected().
182+
/// If applicable in your language, you probably want to just extend an int and put a file
183+
/// descriptor in a struct and implement this trait on it. Note, of course, that if you do so and
184+
/// have multiple threads interacting with the [`PeerManager`], you must not call `close()` on the
185+
/// file descriptor until all threads are guaranteed to not call any [`PeerManager`] functions with
186+
/// the same descriptor. Otherwise, file descriptor re-use could imply that you call a
187+
/// [`PeerManager`] function with a file descriptor representing a different connection.
188+
///
189+
/// Note that if you are using a higher-level net library that may call close() itself when you
190+
/// close a socket, be careful to ensure you don't directly use the file descriptor for your [`Eq`]
191+
/// and [`Hash`] implementations.
182192
pub trait SocketDescriptor : cmp::Eq + hash::Hash + Clone {
183193
/// Attempts to send some data from the given slice to the peer.
184194
///
185195
/// Returns the amount of data which was sent, possibly 0 if the socket has since disconnected.
186-
/// Note that in the disconnected case, socket_disconnected must still fire and further write
187-
/// attempts may occur until that time.
196+
/// Note that in the disconnected case, [`PeerManager::socket_disconnected`] must still be
197+
/// called and further write attempts may occur until that time.
188198
///
189-
/// If the returned size is smaller than data.len(), a write_available event must
190-
/// trigger the next time more data can be written. Additionally, until the a send_data event
191-
/// completes fully, no further read_events should trigger on the same peer!
199+
/// If the returned size is smaller than data.len(), a
200+
/// [`PeerManager::write_buffer_space_avail`] call must be made the next time more data can be
201+
/// written. Additionally, until a send_data event completes fully, no further
202+
/// [`PeerManager::read_event`] calls should be made for the same peer! Because this is to
203+
/// prevent denial-of-service issues, you should not read or buffer any data from the socket
204+
/// until then.
192205
///
193-
/// If a read_event on this descriptor had previously returned true (indicating that read
194-
/// events should be paused to prevent DoS in the send buffer), resume_read may be set
195-
/// indicating that read events on this descriptor should resume. A resume_read of false does
196-
/// *not* imply that further read events should be paused.
206+
/// If a [`PeerManager::read_event`] call on this descriptor had previously returned true
207+
/// (indicating that read events should be paused to prevent DoS in the send buffer),
208+
/// resume_read may be set indicating that read events on this descriptor should resume. A
209+
/// resume_read of false does *not* imply on its own that further read events should be paused.
197210
fn send_data(&mut self, data: &[u8], resume_read: bool) -> usize;
198211
/// Disconnect the socket pointed to by this SocketDescriptor.
199212
/// No [`PeerManager::socket_disconnected`] call need be generated as a result of this call.
@@ -310,8 +323,17 @@ pub type SimpleArcPeerManager<SD, M, T, F, C, L> = PeerManager<SD, Arc<SimpleArc
310323
/// helps with issues such as long function definitions.
311324
pub type SimpleRefPeerManager<'a, 'b, 'c, 'd, 'e, 'f, 'g, SD, M, T, F, C, L> = PeerManager<SD, SimpleRefChannelManager<'a, 'b, 'c, 'd, 'e, M, T, F, L>, &'e NetGraphMsgHandler<&'g C, &'f L>, &'f L>;
312325

313-
/// A PeerManager manages a set of peers, described by their SocketDescriptor and marshalls socket
314-
/// events into messages which it passes on to its MessageHandlers.
326+
/// A PeerManager manages a set of peers, described by their [`SocketDescriptor`] and marshalls
327+
/// socket events into messages which it passes on to its MessageHandlers.
328+
///
329+
/// Locks are taken internally, so you must never assume that reentrancy from a
330+
/// [`SocketDescriptor`] call back into [`PeerManager`] methods will not deadlock.
331+
///
332+
/// Calls to [`read_event`] will decode relevant messages and pass them to the
333+
/// [`ChannelMessageHandler`], likely doing message processing in-line. Thus, the primary form of
334+
/// parallelism in Rust-Lightning is parallelism in calls to [`read_event`]. Note, however, that
335+
/// calls to any [`PeerManager`] functions related to the same connection must occur only in
336+
/// serial, making new calls only after previous ones have returned.
315337
///
316338
/// Rather than using a plain PeerManager, it is preferable to use either a SimpleArcPeerManager
317339
/// a SimpleRefPeerManager, for conciseness. See their documentation for more details, but
@@ -398,8 +420,6 @@ impl<Descriptor: SocketDescriptor, RM: Deref, L: Deref> PeerManager<Descriptor,
398420
}
399421
}
400422

401-
/// Manages and reacts to connection events. You probably want to use file descriptors as PeerIds.
402-
/// PeerIds may repeat, but only after socket_disconnected() has been called.
403423
impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<Descriptor, CM, RM, L> where
404424
CM::Target: ChannelMessageHandler,
405425
RM::Target: RoutingMessageHandler,
@@ -459,8 +479,10 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
459479
///
460480
/// Returns a small number of bytes to send to the remote node (currently always 50).
461481
///
462-
/// Panics if descriptor is duplicative with some other descriptor which has not yet had a
463-
/// socket_disconnected().
482+
/// Panics if descriptor is duplicative with some other descriptor which has not yet been
483+
/// [`socket_disconnected()`].
484+
///
485+
/// [`socket_disconnected()`]: PeerManager::socket_disconnected
464486
pub fn new_outbound_connection(&self, their_node_id: PublicKey, descriptor: Descriptor) -> Result<Vec<u8>, PeerHandleError> {
465487
let mut peer_encryptor = PeerChannelEncryptor::new_outbound(their_node_id.clone(), self.get_ephemeral_key());
466488
let res = peer_encryptor.get_act_one().to_vec();
@@ -496,8 +518,10 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
496518
/// call socket_disconnected for the new descriptor but must disconnect the connection
497519
/// immediately.
498520
///
499-
/// Panics if descriptor is duplicative with some other descriptor which has not yet had
500-
/// socket_disconnected called.
521+
/// Panics if descriptor is duplicative with some other descriptor which has not yet been
522+
/// [`socket_disconnected()`].
523+
///
524+
/// [`socket_disconnected()`]: PeerManager::socket_disconnected
501525
pub fn new_inbound_connection(&self, descriptor: Descriptor) -> Result<(), PeerHandleError> {
502526
let peer_encryptor = PeerChannelEncryptor::new_inbound(&self.our_node_secret);
503527
let pending_read_buffer = [0; 50].to_vec(); // Noise act one is 50 bytes
@@ -605,12 +629,14 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
605629
///
606630
/// May return an Err to indicate that the connection should be closed.
607631
///
608-
/// Will most likely call send_data on the descriptor passed in (or the descriptor handed into
609-
/// new_*\_connection) before returning. Thus, be very careful with reentrancy issues! The
610-
/// invariants around calling write_buffer_space_avail in case a write did not fully complete
611-
/// must still hold - be ready to call write_buffer_space_avail again if a write call generated
612-
/// here isn't sufficient! Panics if the descriptor was not previously registered in a
613-
/// new_\*_connection event.
632+
/// Will most likely call [`send_data`] on the descriptor passed in (or the descriptor handed
633+
/// into new_*\_connection) before returning. Thus, be very careful with reentrancy issues! The
634+
/// invariants around calling [`write_buffer_space_avail`] in case a write did not fully
635+
/// complete must still hold - be ready to call `[write_buffer_space_avail`] again if a write
636+
/// call generated here isn't sufficient!
637+
///
638+
/// [`send_data`]: SocketDescriptor::send_data
639+
/// [`write_buffer_space_avail`]: PeerManager::write_buffer_space_avail
614640
pub fn write_buffer_space_avail(&self, descriptor: &mut Descriptor) -> Result<(), PeerHandleError> {
615641
let mut peers = self.peers.lock().unwrap();
616642
match peers.peers.get_mut(descriptor) {
@@ -632,13 +658,15 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
632658
///
633659
/// May return an Err to indicate that the connection should be closed.
634660
///
635-
/// Will *not* call back into send_data on any descriptors to avoid reentrancy complexity.
636-
/// Thus, however, you almost certainly want to call process_events() after any read_event to
637-
/// generate send_data calls to handle responses.
661+
/// Will *not* call back into [`send_data`] on any descriptors to avoid reentrancy complexity.
662+
/// Thus, however, you almost certainly want to call [`process_events`] after any read_event
663+
/// to generate [`send_data`] calls to handle responses.
638664
///
639-
/// If Ok(true) is returned, further read_events should not be triggered until a send_data call
640-
/// on this file descriptor has resume_read set (preventing DoS issues in the send buffer).
665+
/// If Ok(true) is returned, further read_events should not be triggered until a [`send_data`]
666+
/// call on this descriptor has resume_read set (preventing DoS issues in the send buffer).
641667
///
668+
/// [`send_data`]: SocketDescriptor::send_data
669+
/// [`process_events`]: PeerManager::process_events
642670
pub fn read_event(&self, peer_descriptor: &mut Descriptor, data: &[u8]) -> Result<bool, PeerHandleError> {
643671
match self.do_read_event(peer_descriptor, data) {
644672
Ok(res) => Ok(res),
@@ -1086,7 +1114,14 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
10861114

10871115
/// Checks for any events generated by our handlers and processes them. Includes sending most
10881116
/// response messages as well as messages generated by calls to handler functions directly (eg
1089-
/// functions like ChannelManager::process_pending_htlc_forward or send_payment).
1117+
/// functions like [`ChannelManager::process_pending_htlc_forward`] or [`send_payment`]).
1118+
///
1119+
/// Will most likely call [`send_data`] on descriptors previously provided to
1120+
/// new_*\_connection. Thus, be very careful with reentrancy issues!
1121+
///
1122+
/// [`send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment
1123+
/// [`ChannelManager::process_pending_htlc_forward`]: crate::ln::channelmanager::ChannelManager::process_pending_htlc_forward
1124+
/// [`send_data`]: SocketDescriptor::send_data
10901125
pub fn process_events(&self) {
10911126
{
10921127
// TODO: There are some DoS attacks here where you can flood someone's outbound send
@@ -1299,10 +1334,6 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
12991334
}
13001335

13011336
/// Indicates that the given socket descriptor's connection is now closed.
1302-
///
1303-
/// This need only be called if the socket has been disconnected by the peer or your own
1304-
/// decision to disconnect it and may be skipped in any case where other parts of this library
1305-
/// (eg PeerHandleError, explicit disconnect_socket calls) instruct you to disconnect the peer.
13061337
pub fn socket_disconnected(&self, descriptor: &Descriptor) {
13071338
self.disconnect_event_internal(descriptor, false);
13081339
}
@@ -1333,8 +1364,10 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
13331364
/// Set no_connection_possible to true to prevent any further connection with this peer,
13341365
/// force-closing any channels we have with it.
13351366
///
1336-
/// If a peer is connected, this will call `disconnect_socket` on the descriptor for the peer,
1337-
/// so be careful about reentrancy issues.
1367+
/// If a peer is connected, this will call [`disconnect_socket`] on the descriptor for the
1368+
/// peer. Thus, be very careful about reentrancy issues.
1369+
///
1370+
/// [`disconnect_socket`]: SocketDescriptor::disconnect_socket
13381371
pub fn disconnect_by_node_id(&self, node_id: PublicKey, no_connection_possible: bool) {
13391372
let mut peers_lock = self.peers.lock().unwrap();
13401373
if let Some(mut descriptor) = peers_lock.node_id_to_descriptor.remove(&node_id) {
@@ -1347,8 +1380,9 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
13471380

13481381
/// This function should be called roughly once every 30 seconds.
13491382
/// It will send pings to each peer and disconnect those which did not respond to the last round of pings.
1350-
1351-
/// Will most likely call send_data on all of the registered descriptors, thus, be very careful with reentrancy issues!
1383+
///
1384+
/// Will most likely call [`send_data`] all descriptors previously provided to
1385+
/// new_*\_connection. Thus, be very careful with reentrancy issues!
13521386
pub fn timer_tick_occurred(&self) {
13531387
let mut peers_lock = self.peers.lock().unwrap();
13541388
{

0 commit comments

Comments
 (0)