Implement DisconnectPeer event + test #43

ariard · 2018-07-10T01:15:59Z

At first, I intended to clean the TODO in funding_transaction_generated, and then I ended implementing DisconnectPeer event into peer_handler and events.
It contains only node_id and an HandleError, the latter holding differents states to adapt Disconnect processing and make the sending of an error message optionnal.
Add a basic module test into peer_handler, pulling from peer_handler_encryptor to push the peers into NoiseState::Finished and try the disconnection.
I would like also to test funding_transaction_generated but I didn't find a way to provide invalid PubKeys to build_remote_transaction_keys, does bitcoin crates API give us a way to generate invalid PubKeys for testing ?
Waiting your feedbacks!

TheBlueMatt

Started to review and then stepped back cause I'm not sure about the approach. Generally, the error handling stuff is all a bit of a partial-implement right now - my high-level thinking was the msg in HandleError would be renamed to something like action and moved from an Option<> to just an ErrorAction. Then, when a HandleError comes up the stack, you do what it says in the action field, and there would be a SendErrorMessage enum for ErrorAction, instead of making HandleError something you can serialize into an error message and send.

TheBlueMatt · 2018-07-10T01:43:46Z

src/ln/peer_channel_encryptor.rs

@@ -242,7 +242,7 @@ impl PeerChannelEncryptor {
 	}

 	// Separated for testing:
-	fn process_act_one_with_ephemeral_key(&mut self, act_one: &[u8], our_node_secret: &SecretKey, our_ephemeral: SecretKey) -> Result<[u8; 50], HandleError> {
+	pub fn process_act_one_with_ephemeral_key(&mut self, act_one: &[u8], our_node_secret: &SecretKey, our_ephemeral: SecretKey) -> Result<[u8; 50], HandleError> {


Please only use pub(super) not pub for this kind of thing (and maybe note that its also for testing only). Still, I think you can do this testing without exposing a bunch of testing-specific functions - is it possible to hit it with keys that will result in build_remote_transaction_keys failing here?

Yes, in fact dropping the send message part from DisconnectPeer event means that I don't need the peer_channel_encryptor testing function anymore. I'll move the test to channelmanager and hit via build_remote_transaction_keys, but I'm stuck on generating "valid" invalid pubkeys to pass them to build_remote_transaction_keys , as the rust-secp256k1 internals prevent you of doing this.

TheBlueMatt · 2018-07-10T02:07:48Z

src/ln/channelmanager.rs

-							//TODO: Push e to pendingevents
+						Err(e) => {
+							let mut pending_events = self.pending_events.lock().unwrap();
+							pending_events.push(events::Event::DisconnectPeer {


This should be more like HandleError, not DisconnectPeer, and we should handle the error as any other error instead, I think.

ariard · 2018-07-11T01:08:56Z

In funding_transaction_generated, I rewrote with a match to let taking action on every kind of errors. If match, and if needed, push the event to peer_handler. I'm worried that for every HandleError to manage, we'll have a lot of particular code, than we can't simplifiy because you need to pass specific data from current function environnement. I implement also SendErrorMessage as I understand what you describe, even if there is no case for the moment. Maybe all this stuff is too much early and if so close the PR, but I hope it will help clarify thinking on error management.

ariard · 2018-07-11T01:44:17Z

Combining ErrorAction should be also easy, in some case you want SendErrorMessage + DisconnectPeer or BanPeer + DisconnectPeer

TheBlueMatt

Awesome, looking good, few API/general comments, and haven't had a chance to review the new test-cases yet, but thanks for adding some tests for the peer_handling logic!

TheBlueMatt · 2018-07-11T20:18:52Z

src/ln/channelmanager.rs

+							if let Some(action) = e.msg {
+								match action {
+									msgs::ErrorAction::DisconnectPeer {} => {
+										add_pending_event!(events::Event::DisconnectPeer {


Please dont take both the pending_events lock and the channel_stat lock at the same time here. I'm probably overparanoid about deadlocks, but we dont have any automated analysis to detect them so best to take the perf hit of just using a temporary vector to ensure they cant happen.

TheBlueMatt · 2018-07-11T20:20:14Z

src/ln/channelmanager.rs

@@ -1811,6 +1828,12 @@ impl ChannelMessageHandler for ChannelManager {
 			}
 		}
 	}
+
+	//For testing purpose in peer_handler
+	fn push_event(&self, event: events::Event) {


Hmm, I mean you could put a #[cfg(test)] here, actually.

TheBlueMatt · 2018-07-11T20:23:12Z

src/ln/msgs.rs

@@ -435,6 +439,9 @@ pub trait ChannelMessageHandler : events::EventsProvider + Send + Sync {
 	/// understand or indicate they require unknown feature bits), no_connection_possible is set
 	/// and any outstanding channels should be failed.
 	fn peer_disconnected(&self, their_node_id: &PublicKey, no_connection_possible: bool);
+
+	//Testing
+	fn push_event(&self, event: events::Event);


Please at least put a #[cfg(test)] here so that there isn't a testing function in a pub trait.

TheBlueMatt · 2018-07-11T20:45:26Z

src/util/events.rs

+	},
+	// Events indicating the network loop should change the state of peer connection:
+	/// Used to indicate that the peer connection should be closed with a message holding the reason
+	DisconnectPeer {


Why not just make it a bool in SendErrorMessage as to whether to disconnect? Feels like thats usually what you'd want, no?

TheBlueMatt · 2018-07-11T20:49:28Z

src/ln/peer_handler.rs

 				}

 				upstream_events.push(event);
 			}
 		}

+		for descriptor in disconnect_peers {
+			self.disconnect_event(&descriptor);


You also need some fn in the SocketDescriptor trait that allows us to notify the network provider that we want to disconnect the peer. For simplicity of this patch, I think its fine to indicate that if its called, disconnect_event must not be called, nor any future read_events.

ariard · 2018-07-11T23:57:28Z

Implemented comments, will add also test on funding_transaction_generated case in channelmanager I think in another PR to stay simple here. Peer_handler test is very basic for the moment but I hope it will help more test on this part later

ariard · 2018-07-11T23:58:12Z

src/util/events.rs

@@ -100,6 +100,13 @@ pub enum Event {
 	BroadcastChannelUpdate {
 		msg: msgs::ChannelUpdate,
 	},
+	/// Used to tell a peer that something is incorrect
+	SendErrorMessage {


Ok change as recommanded, but add two flags, one for disconnection and another one for a "hard" disconnect, when you want you to disconnect without spoiling ressources on an error message , in case of banned peers or malicious ones.

TheBlueMatt

Note that travis failed as fuzz failed to build, also please check that you're not adding more rustc warnings when they aren't neccessary.

TheBlueMatt · 2018-07-14T04:35:30Z

src/ln/channelmanager.rs

@@ -677,6 +677,15 @@ impl ChannelManager {
 	/// Call this upon creation of a funding transaction for the given channel.
 	/// Panics if a funding transaction has already been provided for this channel.
 	pub fn funding_transaction_generated(&self, temporary_channel_id: &Uint256, funding_txo: OutPoint) {
+
+		macro_rules! add_pending_event {


nit: maybe move down closer to first use?

Might as well fix this while you're fixing other stuff, so the duplicate-lock issue doesn't get re-introduced.

TheBlueMatt · 2018-07-14T04:44:57Z

src/ln/channelmanager.rs

+							println!("Got error handling message: {}!", e.err);
+							if let Some(action) = e.msg {
+								match action {
+									msgs::ErrorAction::DisconnectPeer {} => {


Hmmm, I'm really not a fan of this setup - needing to handle conversion of errors to different types of errors at each place where we can generate outbound messages really sucks. Just having a general HandleOutboundMsgGenerationError event which looks like SendErrorMessage as proposed but instead of msg, disconnect, and hard, it just holds a HandleError should simplify this PR, and more importantly, future PRs. Further, SendErrorMessage would be a HandleError::msg action option (with a disconnect option, or maybe it could go in DisconnectPeer? I'm not picky there, I think).

TheBlueMatt · 2018-07-14T04:52:19Z

src/ln/peer_handler.rs

@@ -37,6 +37,9 @@ pub trait SocketDescriptor : cmp::Eq + hash::Hash + Clone {
 	/// indicating that read events on this descriptor should resume. A resume_read of false does
 	/// *not* imply that further read events should be paused.
 	fn send_data(&mut self, data: &Vec<u8>, write_offset: usize, resume_read: bool) -> usize;
+	///Indicate to the network provider that we want to disconnect this peer, no more allowing any call
+	///to disconnect_event or read_event
+	fn notify_disconnection(&mut self);


Maybe call this "disconnect_socket" instead, and the comment needs flushed out more since its an external API that's easy to mess up:

/// 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.

should be sufficient, no?

ariard · 2018-07-15T03:09:52Z

src/ln/channelmanager.rs

-							//TODO: Push e to pendingevents
+						Err(e) => {
+							mem::drop(channel_state);
+							add_pending_event!(events::Event::HandleOutboundError {


After previous version, was working on others parts on channelmanager on error handling, and yes it's cumbersome to deal with multiple errors each time, doesn't ease the codebase. So I wrote HandleOutboundError event (really not sure about the name, for me HandleOutboundMsgGenerationError was too specific with DisconnectPeer-only case) which contains a vec of ErrorAction to combine SendErrorMessage, DisconnectPeer and futures errors. That's still hacky, ideally that would be HandleError which hold multiple ErrorAction instead of an Option like right now and having wrapper event like HandleOutboundError avoiding a 1:1 mapping ErrorAction-Events. What do you think about if I refactor first HandleError with a Vec of ErrorAction and after going back to this PR ? Open an issue if you want to think on it before.

Hmm, I think I kinda prefer a 1-to-1 mapping for now, on both - a DisconnectPeer ErrorAction could reasonably take an ErrorMessage to be sent pre-disconnect. If we ever want to do more than just send an ErrorMessage we could revisit it, but having the Vec indirection kinda sucks, especially when we really are using it as an Option and never anything more.

ariard · 2018-07-15T03:12:30Z

Yes, sorry for the fuzzing, didn't think about the adding of disconnect_socket.
I implemented your comments but more broadly HandleError is still an open issue.

TheBlueMatt

Sorry it took me a bit to get back to this.

TheBlueMatt · 2018-07-17T18:26:22Z

fuzz/fuzz_targets/full_stack_target.rs

@@ -114,6 +114,7 @@ impl SocketDescriptor for Peer {
 		assert!(write_offset < data.len());
 		data.len() - write_offset
 	}
+	fn disconnect_socket(&mut self) {}


This needs to be implemented - it needs to set the bool in peers appropriately, which I guess is gonna need some kind of Rc<RefCell<[]>>. I think you can skip handling the race condition where a disconnect_socket and a disconnect_event happen just because its all single threaded here.

TheBlueMatt · 2018-07-17T18:30:46Z

src/ln/channelmanager.rs

-							//TODO: Push e to pendingevents
+						Err(e) => {
+							mem::drop(channel_state);
+							add_pending_event!(events::Event::HandleOutboundError {


Hmm, I think I kinda prefer a 1-to-1 mapping for now, on both - a DisconnectPeer ErrorAction could reasonably take an ErrorMessage to be sent pre-disconnect. If we ever want to do more than just send an ErrorMessage we could revisit it, but having the Vec indirection kinda sucks, especially when we really are using it as an Option and never anything more.

TheBlueMatt · 2018-07-17T18:38:28Z

src/ln/msgs.rs

@@ -1534,3 +1546,11 @@ impl MsgEncodable for OnionErrorPacket {
 		res
 	}
 }
+
+impl MsgEncodable for ErrorMessage {


Can you add a TODO for MsgDecodable for ErrorMessage? We should obviously also support receiving them.

TheBlueMatt · 2018-07-17T18:38:39Z

src/ln/msgs.rs

@@ -139,6 +139,10 @@ pub struct Init {
 	pub local_features: LocalFeatures,
 }

+pub struct ErrorMessage {
+	pub err: &'static str,


So that we can both send and receive, I think this should be String.

TheBlueMatt · 2018-07-18T19:54:11Z

src/ln/channelmanager.rs

@@ -677,6 +677,15 @@ impl ChannelManager {
 	/// Call this upon creation of a funding transaction for the given channel.
 	/// Panics if a funding transaction has already been provided for this channel.
 	pub fn funding_transaction_generated(&self, temporary_channel_id: &Uint256, funding_txo: OutPoint) {
+
+		macro_rules! add_pending_event {


Might as well fix this while you're fixing other stuff, so the duplicate-lock issue doesn't get re-introduced.

TheBlueMatt · 2018-07-18T19:56:05Z

src/ln/peer_handler.rs

@@ -710,12 +721,32 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 						}
 						continue;
 					},
+					Event::HandleOutboundError { ref node_id, ref err } => {
+						let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, {
+								//TODO: Do whatever we're gonna do for handling dropped messages


Note duplicate indentation here, and also probably don't need a TODO, just note that we don't have to disconnect them if they're already disconnected.

TheBlueMatt · 2018-07-18T19:58:42Z

src/ln/peer_handler.rs

@@ -576,6 +586,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 	/// calls to ChannelManager::process_pending_htlc_forward.
 	pub fn process_events(&self) {
 		let mut upstream_events = Vec::new();
+		let mut disconnect_peers = Vec::new();


I dont see why you need this - you're doing two map lookups, passing an event to that entry, then doing another two map lookups to remove it, might as well replace the get_peer_for_forwarding with a written out version that removes the entries all at once.

TheBlueMatt · 2018-07-18T20:01:20Z

src/ln/peer_handler.rs

+	}
+
+	#[test]
+	fn test_disconnect_peer() {


This seems way overkill for what you want - instead of starting up Monitors and Broadcasters and ChainWatchers and Routers and ChannelManagers, why not add some dummy ChannelMessageHandler and RoutingMessageHandlers to test_utils (or just put them here). That way you can mock the message handlers without needing to add a push_event to the traits themselves and you can more directly test the peer_handler logic without needing to worry about what's handling the messages themselves.

ariard · 2018-07-22T23:10:39Z

I split the ErrorMessage implementation in a commit appart with encode/decode stuff. Add a lot of things in test_utils for easing the disconnect test case, is this what you had in head ?

ariard · 2018-07-22T23:12:10Z

fuzz/fuzz_targets/full_stack_target.rs

 }
 impl SocketDescriptor for Peer {
 	fn send_data(&mut self, data: &Vec<u8>, write_offset: usize, _resume_read: bool) -> usize {
 		assert!(write_offset < data.len());
 		data.len() - write_offset
 	}
+	fn disconnect_socket(&mut self) {
+		self.connected = false;
+	}


Sorry didn't get you on this one, disconnect_socket is used by the network provider to notify that we want to disconnect the peer, right ? So what do we want here is setting a bool in SocketDescriptor object and let the network provider disconnect peer, block further disconnect_event and remove peer infos. Why do we need RefCell ?

Add test for DisconnectPeer event Update DisconnectPeer with optional ErrorMessage Manage error for funding_transaction_generated Add disconnect_socket to SocketDescriptor trait

ariard · 2018-07-23T01:11:52Z

Updated to solve conflict with #53 and being up to date with #73

TheBlueMatt · 2018-07-23T03:30:46Z

Awesome, will take another look at this tomorrow morning, thanks for doing the rebase.

TheBlueMatt

Nice, thanks for finishing this one. A few one-line comments, but I'm gonna go ahead and fix them, rebase and merge this.

TheBlueMatt · 2018-07-23T17:19:46Z

src/ln/msgs.rs

+		let len = byte_utils::slice_to_be16(&v[33..34]);
+		let mut data = String::new();
+		if len > 0 {
+			data = String::from_utf8(v[35..len as usize].to_vec()).unwrap();


unwrap() here can panic if the message contains non-utf-8 crap.

Also, some of the indexing is off in the decoder here.

TheBlueMatt · 2018-07-23T17:23:02Z

src/ln/channel.rs

-			return Err(HandleError{err: "Peer never wants payout outputs?", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "Peer never wants payout outputs?", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
+		}
+		if msg.max_htlc_value_in_flight_msat > msg.funding_satoshis * 1000 {


Rebase error here - this if was removed on master.

TheBlueMatt · 2018-07-23T17:27:51Z

src/ln/peer_handler.rs

+							}
+							descriptor.disconnect_socket();
+						}
+						self.message_handler.chan_handler.peer_disconnected(&node_id, false);


This should be redundant - if we dont have a descriptor for the node_id we've already disconnected it, and, thus, already called peer_disconnected.

TheBlueMatt · 2018-07-23T17:31:01Z

src/ln/peer_handler.rs

+		peers[0].message_handler.chan_handler = Arc::new(chan_handler);
+
+		peers[0].process_events();
+		assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 0);


Nice, this test looks good, I'm gonna add a few more asserts just to increase coverage but I like the layout here.

TheBlueMatt · 2018-07-23T18:11:34Z

Will take as #78

LDK 0.0.104

TheBlueMatt reviewed Jul 10, 2018

View reviewed changes

ariard force-pushed the add_disconnect_peer_event branch from aa622b9 to fd4524b Compare July 11, 2018 01:07

TheBlueMatt reviewed Jul 11, 2018

View reviewed changes

ariard force-pushed the add_disconnect_peer_event branch from fd4524b to 0df41f4 Compare July 11, 2018 23:56

ariard commented Jul 11, 2018

View reviewed changes

TheBlueMatt reviewed Jul 14, 2018

View reviewed changes

ariard force-pushed the add_disconnect_peer_event branch from 0df41f4 to 53cea1b Compare July 15, 2018 02:48

ariard commented Jul 15, 2018

View reviewed changes

TheBlueMatt reviewed Jul 18, 2018

View reviewed changes

savil mentioned this pull request Jul 20, 2018

[HandleError] rename msg field to action #53

Merged

ariard force-pushed the add_disconnect_peer_event branch from 53cea1b to c26b0fb Compare July 22, 2018 23:08

ariard commented Jul 22, 2018

View reviewed changes

Antoine Riard added 2 commits July 23, 2018 00:59

Implement ErrorMessage msg and ErrorAction::SendErrorMessage + fuzz test

fdfe483

Implement DisconnectPeer and HandleOutboundMsgGenerationError events

f32d1ea

Add test for DisconnectPeer event Update DisconnectPeer with optional ErrorMessage Manage error for funding_transaction_generated Add disconnect_socket to SocketDescriptor trait

ariard force-pushed the add_disconnect_peer_event branch from c26b0fb to f32d1ea Compare July 23, 2018 01:09

TheBlueMatt reviewed Jul 23, 2018

View reviewed changes

TheBlueMatt mentioned this pull request Jul 23, 2018

Add DisconnectPeer event #78

Merged

TheBlueMatt closed this Jul 23, 2018

ariard deleted the add_disconnect_peer_event branch August 27, 2018 22:30

carlaKC pushed a commit to carlaKC/rust-lightning that referenced this pull request Aug 9, 2023

Merge pull request lightningdevkit#43 from TheBlueMatt/2021-12-0.0.104

86a474d

LDK 0.0.104

Implement DisconnectPeer event + test #43

Implement DisconnectPeer event + test #43

Uh oh!

Conversation

ariard commented Jul 10, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 11, 2018

Uh oh!

ariard commented Jul 11, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 15, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 23, 2018

Uh oh!

TheBlueMatt commented Jul 23, 2018

Uh oh!