Encrypt ping messages before sending them #506

arik-so · 2020-02-18T22:17:36Z

This fixes #504.

TheBlueMatt · 2020-02-18T23:54:14Z

lightning/src/ln/peer_handler.rs

@@ -1209,7 +1216,8 @@ mod tests {
 		assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 1);

 		// Since timer_tick_occured() is called again when awaiting_pong is true, all Peers are disconnected
-		peers[0].timer_tick_occured();
-		assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 0);
+		// TODO: simulate handshake completion to fully support ping exchange simulations


Why not just exchange the handshake? Shouldn't be that hard?

I don't think it's super trivial because the test needs to manually call the read events, but I'll give it a try. I originally thought it might be best reserved for a separate diff.

This is proving to be mutex mess. I think it will be easier once one of (or preferably both) the modular handshake and the peer handler refactor into individual peer holders commits land. CC: @jkczyz

I don't have strong feelings about maintaining this test. It should be a few tests covering the different behaviors. Currently, it tests a subset of the behaviors and so will test a smaller subset when removing this part. It would probably be easier to test after refactoring, as you said.

Hmm, should be doable. Can you try building on the top commit on https://github.com/TheBlueMatt/rust-lightning/commits/2020-02-peer_handler-handshake

wow, I had no idea you could use the descriptors for that

Descriptors, ultimately, are an API for the PeerHandler to send data to a given socket. So that is, in fact, the only thing you can use the descriptors for :p

Interesting. The word descriptor suggested to me that it's just something that describes the socket, aka to be used as a key to find it in the map.

jkczyz · 2020-02-18T23:23:34Z

lightning/src/ln/peer_handler.rs

+				// check if the peer is ready for encryption
+				if !peer.channel_encryptor.is_ready_for_encryption() {
+					// let's wait for the peer to complete the handshake


The first comment doesn't add much; it simply restates the if expression. Could you replace it with the inner comment?

jkczyz · 2020-02-19T19:03:54Z

lightning/src/ln/peer_handler.rs

@@ -1209,7 +1216,8 @@ mod tests {
 		assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 1);

 		// Since timer_tick_occured() is called again when awaiting_pong is true, all Peers are disconnected
-		peers[0].timer_tick_occured();
-		assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 0);
+		// TODO: simulate handshake completion to fully support ping exchange simulations


I don't have strong feelings about maintaining this test. It should be a few tests covering the different behaviors. Currently, it tests a subset of the behaviors and so will test a smaller subset when removing this part. It would probably be easier to test after refactoring, as you said.

jkczyz · 2020-02-19T19:06:53Z

lightning/src/ln/peer_handler.rs

-				// check if the peer is ready for encryption
+				// The peer needs to complete its handshake before we can exchange messages
 				if !peer.channel_encryptor.is_ready_for_encryption() {
-					// let's wait for the peer to complete the handshake
 					return true;
 				}


I'd consider moving this before line 1078. The rationale being that this is a guard clause so what comes before does not need to be checked if this condition is true. That said, the above condition should be mutually exclusive with this one, so reordering should not affect the behavior.

TheBlueMatt · 2020-02-19T20:48:15Z

It’s intended to reference a file descriptor, which is, in fact, an index in a map, but also a token to be able to send data to a socket (which, of course, for us, has to be generic).

…

On Feb 19, 2020, at 12:44, Arik Sosman ***@***.***> wrote: @arik-so commented on this pull request. In lightning/src/ln/peer_handler.rs: > @@ -1209,7 +1216,8 @@ mod tests { assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 1); // Since timer_tick_occured() is called again when awaiting_pong is true, all Peers are disconnected - peers[0].timer_tick_occured(); - assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 0); + // TODO: simulate handshake completion to fully support ping exchange simulations Interesting. The word descriptor suggested to me that it's just something that describes the socket, aka to be used as a key to find it in the map. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

TheBlueMatt

Looks good. One minor comment and can you squash the fixup commits?

TheBlueMatt · 2020-02-19T21:27:47Z

lightning/src/ln/peer_handler.rs

-					peer.awaiting_pong = true;
-					true
-				}
+				peer.awaiting_pong = !peer.awaiting_pong; // flip the condition


This is pretty hard to read, can you just drop the peer at the top of this fn instead of waiting to the end and returning the awaiting_pong var?

TheBlueMatt · 2020-02-19T21:45:34Z

Just a nit, but can you update the commit message? Just including the log of temp fixups that you pushed isn't really a useful commit message.

arik-so · 2020-02-19T21:46:29Z

I thought I had fixed it, let me check.

…

On Feb 19, 2020, at 1:45 PM, Matt Corallo ***@***.***> wrote: Just a nit, but can you update the commit message? Just including the log of temp fixups that you pushed isn't really a useful commit message. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#506>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHCOLVYTEOICMA5W3MTNPDRDWR75ANCNFSM4KXNQNIQ>.

jkczyz · 2020-02-19T22:05:11Z

Yeah, PSA on commit messages:

https://chris.beams.io/posts/git-commit/

…decrypt it instead of shutting down the connection.

jkczyz · 2020-02-19T23:04:30Z

lightning/src/ln/peer_handler.rs

-		peer_a.new_inbound_connection(fd.clone()).unwrap();
-		peer_a.peers.lock().unwrap().node_id_to_descriptor.insert(their_id, fd.clone());
+		let a_id = PublicKey::from_secret_key(&secp_ctx, &peer_a.our_node_secret);
+		//let b_id = PublicKey::from_secret_key(&secp_ctx, &peer_b.our_node_secret);


Eh, remove unused code.

this code is actually directly taken from Matt's commit. If I remove it, will it cause you merging headaches, @TheBlueMatt?

Oops, sorry, that was cause I was just playing and testing, you can remove it :).

arik-so requested a review from TheBlueMatt February 18, 2020 22:25

TheBlueMatt reviewed Feb 18, 2020

View reviewed changes

jkczyz reviewed Feb 19, 2020

View reviewed changes

TheBlueMatt added this to the 0.0.10 milestone Feb 19, 2020

jkczyz approved these changes Feb 19, 2020

View reviewed changes

TheBlueMatt reviewed Feb 19, 2020

View reviewed changes

arik-so force-pushed the ping_encryption_fix branch from beb0be0 to d93bc3d Compare February 19, 2020 21:39

Fix ping message sent on timer ticks to be encrypted so the peer can …

b317f52

…decrypt it instead of shutting down the connection.

arik-so force-pushed the ping_encryption_fix branch from d93bc3d to b317f52 Compare February 19, 2020 22:09

jkczyz approved these changes Feb 19, 2020

View reviewed changes

arik-so self-assigned this Feb 19, 2020

Remove unused code in peer handler test utility method.

d9e1f98

TheBlueMatt merged commit 3e726c4 into lightningdevkit:master Feb 20, 2020

TheBlueMatt modified the milestones: 0.0.10, 0.0.11 Feb 26, 2020

Encrypt ping messages before sending them #506

Encrypt ping messages before sending them #506

Uh oh!

Conversation

arik-so commented Feb 18, 2020

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!

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!

TheBlueMatt commented Feb 19, 2020 via email

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 19, 2020

Uh oh!

arik-so commented Feb 19, 2020 via email

Uh oh!

jkczyz commented Feb 19, 2020

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!

Uh oh!