refactor: Use a real struct instead of conduit_and_remote_pubkey tuple

This makes the process_act() interface a bit cleaner on a successfully
completed handshake.

Author: julianknutsen

fuzz/src/peer_crypt.rs

@@ -177,15 +177,15 @@ fn do_completed_handshake_test(generator: &mut FuzzGen) -> Result<(), GeneratorF
 	// The handshake should complete with any valid private keys
 	let act2 = test_ctx.responder_handshake.process_act(&test_ctx.act1).unwrap().0.unwrap();
 	let (act3, (mut initiator_conduit, responder_pubkey)) = match test_ctx.initiator_handshake.process_act(&act2) {
-		Ok((Some(act3), Some((conduit, remote_pubkey)))) => {
-			(act3, (conduit, remote_pubkey))
+		Ok((Some(act3), Some(completed_handshake_info))) => {
+			(act3, (completed_handshake_info.conduit, completed_handshake_info.their_node_id))
 		}
 		_ => panic!("handshake failed")
 	};
 
 	let (mut responder_conduit, initiator_pubkey) = match test_ctx.responder_handshake.process_act(&act3) {
-		Ok((None, Some((conduit, remote_pubkey)))) => {
-			(conduit, remote_pubkey)
+		Ok((None, Some(completed_handshake_info))) => {
+			(completed_handshake_info.conduit, completed_handshake_info.their_node_id)
 		}
 		_ => panic!("handshake failed")
 	};
@@ -276,12 +276,12 @@ fn do_handshake_test(generator: &mut FuzzGen) -> Result<(), GeneratorFinishedErr
 	let act2_chunks = split_vec(generator, &act2)?;
 
 	let mut act3_option = None;
-	let mut conduit_and_remote_pubkey_option = None;
+	let mut initiator_completed_handshake_info_option = None;
 	for partial_act2 in act2_chunks {
 		match test_ctx.initiator_handshake.process_act(&partial_act2) {
-			Ok((Some(act3), Some(conduit_and_remote_pubkey_option_inner))) => {
+			Ok((Some(act3), Some(completed_handshake_info_option_inner))) => {
 				act3_option = Some(act3);
-				conduit_and_remote_pubkey_option = Some(conduit_and_remote_pubkey_option_inner);
+				initiator_completed_handshake_info_option = Some(completed_handshake_info_option_inner);
 
 				// Valid conduit and pubkey indicates handshake is over
 				break;
@@ -299,7 +299,7 @@ fn do_handshake_test(generator: &mut FuzzGen) -> Result<(), GeneratorFinishedErr
 
 	// Ensure we actually received act3 bytes, conduit, and remote pubkey from process_act()
 	let act3 = act3_option.unwrap();
-	let (mut initiator_conduit, responder_pubkey) = conduit_and_remote_pubkey_option.unwrap();
+	let mut initiator_completed_handshake_info = initiator_completed_handshake_info_option.unwrap();
 
 	// Possibly generate bad data for act3
 	let (mut act3, is_bad_data) = maybe_generate_bad_act(generator, act3)?;
@@ -313,11 +313,11 @@ fn do_handshake_test(generator: &mut FuzzGen) -> Result<(), GeneratorFinishedErr
 	// Split act3 into between 1..7 chunks
 	let act3_chunks = split_vec(generator, &act3)?;
 
-	let mut conduit_and_remote_pubkey_option = None;
+	let mut responder_completed_handshake_info = None;
 	for partial_act3 in act3_chunks {
 		match test_ctx.responder_handshake.process_act(&partial_act3) {
-			Ok((None, Some(conduit_and_remote_pubkey_option_inner))) => {
-				conduit_and_remote_pubkey_option = Some(conduit_and_remote_pubkey_option_inner);
+			Ok((None, Some(completed_handshake_info_inner))) => {
+				responder_completed_handshake_info = Some(completed_handshake_info_inner);
 
 				// Valid conduit and pubkey indicates handshake is over
 				break;
@@ -333,20 +333,20 @@ fn do_handshake_test(generator: &mut FuzzGen) -> Result<(), GeneratorFinishedErr
 		};
 	}
 	// Ensure we actually received conduit and remote pubkey from process_act()
-	let (mut responder_conduit, initiator_pubkey) = conduit_and_remote_pubkey_option.unwrap();
+	let mut responder_completed_handshake_info = responder_completed_handshake_info.unwrap();
 
 	// The handshake should complete with each peer knowing the static_public_key of the remote peer
-	if initiator_pubkey != test_ctx.initiator_static_public_key {
+	if responder_completed_handshake_info.their_node_id != test_ctx.initiator_static_public_key {
 		assert!(used_generated_data);
 		return Ok(());
 	}
-	if responder_pubkey != test_ctx.responder_static_public_key {
+	if initiator_completed_handshake_info.their_node_id != test_ctx.responder_static_public_key {
 		assert!(used_generated_data);
 		return Ok(());
 	}
 
 	// The nodes should be able to communicate over the conduit
-	do_conduit_tests(generator, &mut initiator_conduit, &mut responder_conduit, used_generated_data)
+	do_conduit_tests(generator, &mut initiator_completed_handshake_info.conduit, &mut responder_completed_handshake_info.conduit, used_generated_data)
 }
 
 #[inline]

lightning/src/ln/peers/handshake/mod.rs

@@ -27,6 +27,12 @@ pub struct PeerHandshake {
 	ready_to_process: bool,
 }
 
+/// Container for the information returned from a successfully completed handshake
+pub struct CompletedHandshakeInfo {
+	pub conduit: Conduit,
+	pub their_node_id: PublicKey,
+}
+
 impl IPeerHandshake for PeerHandshake {
 	/// Instantiate a handshake given the peer's static public key. The ephemeral private key MUST
 	/// generate a new session with strong cryptographic randomness.
@@ -66,8 +72,8 @@ impl IPeerHandshake for PeerHandshake {
 	/// # Return values
 	/// Returns a tuple with the following components:
 	/// `.0`: Byte vector containing the next act to send back to the peer per the handshake protocol
-	/// `.1`: Some(Conduit, PublicKey) if the handshake was just processed to completion and messages can now be encrypted and decrypted
-	fn process_act(&mut self, input: &[u8]) -> Result<(Option<Vec<u8>>, Option<(Conduit, PublicKey)>), String> {
+	/// `.1`: Some(CompleteHandshakeInfo) if the handshake was just processed to completion and messages can now be encrypted and decrypted
+	fn process_act(&mut self, input: &[u8]) -> Result<(Option<Vec<u8>>, Option<CompletedHandshakeInfo>), String> {
 		assert!(self.ready_to_process);
 		let cur_state = self.state.take().unwrap();
 
@@ -77,8 +83,8 @@ impl IPeerHandshake for PeerHandshake {
 		};
 
 		let result = match next_state {
-			HandshakeState::Complete(ref mut conduit_and_pubkey) => {
-				Ok((act_option, conduit_and_pubkey.take()))
+			HandshakeState::Complete(ref mut complete_handshake_info) => {
+				Ok((act_option, complete_handshake_info.take()))
 			},
 			_ => { Ok((act_option, None)) }
 		};
@@ -188,14 +194,14 @@ mod test {
 		let mut test_ctx = TestCtx::new();
 		let act2 = do_process_act_or_panic!(test_ctx.inbound_handshake, &test_ctx.act1);
 
-		let (act3, inbound_remote_pubkey) = if let (Some(act3), Some((_, remote_pubkey))) = test_ctx.outbound_handshake.process_act(&act2).unwrap() {
-			(act3, remote_pubkey)
+		let (act3, inbound_remote_pubkey) = if let (Some(act3), Some(completed_handshake_info)) = test_ctx.outbound_handshake.process_act(&act2).unwrap() {
+			(act3, completed_handshake_info.their_node_id)
 		} else {
 			panic!();
 		};
 
-		let outbound_remote_pubkey = if let (None, Some((_, remote_pubkey))) = test_ctx.inbound_handshake.process_act(&act3).unwrap() {
-			remote_pubkey
+		let outbound_remote_pubkey = if let (None, Some(completed_handshake_info)) = test_ctx.inbound_handshake.process_act(&act3).unwrap() {
+			completed_handshake_info.their_node_id
 		} else {
 			panic!();
 		};

lightning/src/ln/peers/handshake/states.rs

@@ -7,7 +7,7 @@ use bitcoin::secp256k1::{SecretKey, PublicKey};
 use ln::peers::{chacha, hkdf5869rfc};
 use ln::peers::conduit::{Conduit, SymmetricKey};
 use ln::peers::handshake::acts::{Act, ActBuilder, ACT_ONE_LENGTH, ACT_TWO_LENGTH, ACT_THREE_LENGTH, EMPTY_ACT_ONE, EMPTY_ACT_TWO, EMPTY_ACT_THREE};
-use ln::peers::handshake::IHandshakeState;
+use ln::peers::handshake::{CompletedHandshakeInfo, IHandshakeState};
 
 // Alias type to help differentiate between temporary key and chaining key when passing bytes around
 type ChainingKey = [u8; 32];
@@ -28,7 +28,7 @@ pub(super) enum HandshakeState {
 	ResponderAwaitingActOne(ResponderAwaitingActOneState),
 	InitiatorAwaitingActTwo(InitiatorAwaitingActTwoState),
 	ResponderAwaitingActThree(ResponderAwaitingActThreeState),
-	Complete(Option<(Conduit, PublicKey)>),
+	Complete(Option<CompletedHandshakeInfo>),
 }
 
 // Enum dispatch for state machine. Single public interface can statically dispatch to all states
@@ -48,7 +48,7 @@ impl IHandshakeState for HandshakeState {
 			HandshakeState::ResponderAwaitingActOne(state) => { state.next(input) },
 			HandshakeState::InitiatorAwaitingActTwo(state) => { state.next(input) },
 			HandshakeState::ResponderAwaitingActThree(state) => { state.next(input) },
-			HandshakeState::Complete(_conduit) => { panic!("no acts left to process") }
+			HandshakeState::Complete(_completed_handshake_info) => { panic!("no acts left to process") }
 		}
 	}
 }
@@ -310,7 +310,12 @@ impl IHandshakeState for InitiatorAwaitingActTwoState {
 		act_three[0] = 0;
 		Ok((
 			Some(Act::Three(act_three)),
-			HandshakeState::Complete(Some((conduit, responder_static_public_key)))
+			HandshakeState::Complete(Some(
+				CompletedHandshakeInfo {
+					conduit,
+					their_node_id: responder_static_public_key
+				}
+			))
 		))
 	}
 }
@@ -390,7 +395,12 @@ impl IHandshakeState for ResponderAwaitingActThreeState {
 
 		Ok((
 			None,
-			HandshakeState::Complete(Some((conduit, initiator_pubkey)))
+			HandshakeState::Complete(Some(
+				CompletedHandshakeInfo {
+					conduit,
+					their_node_id: initiator_pubkey
+				}
+			))
 		))
 	}
 }
@@ -684,15 +694,15 @@ mod test {
 		let (_act1, awaiting_act_two_state) = do_next_or_panic!(test_ctx.initiator, &[]);
 		let (act3, complete_state) = do_next_or_panic!(awaiting_act_two_state, &test_ctx.valid_act2);
 
-		let (conduit, remote_pubkey) = if let Complete(Some((conduit, remote_pubkey))) = complete_state {
-			(conduit, remote_pubkey)
+		let completed_handshake_info = if let Complete(Some(completed_handshake_info)) = complete_state {
+			completed_handshake_info
 		} else {
 			panic!();
 		};
 
 		assert_eq!(act3.as_ref(), test_ctx.valid_act3.as_slice());
-		assert_eq!(remote_pubkey, test_ctx.responder_static_public_key);
-		assert_eq!(0, conduit.decryptor.read_buffer_length());
+		assert_eq!(completed_handshake_info.their_node_id, test_ctx.responder_static_public_key);
+		assert_eq!(0, completed_handshake_info.conduit.decryptor.read_buffer_length());
 	}
 
 	// Initiator::AwaitingActTwo -> Complete (segmented calls)
@@ -751,14 +761,14 @@ mod test {
 		let test_ctx = TestCtx::new();
 		let (_act2, awaiting_act_three_state) = do_next_or_panic!(test_ctx.responder, &test_ctx.valid_act1);
 
-		let (conduit, remote_pubkey) = if let (None, Complete(Some((conduit, remote_pubkey)))) = awaiting_act_three_state.next(&test_ctx.valid_act3).unwrap() {
-			(conduit, remote_pubkey)
+		let completed_handshake_info = if let (None, Complete(Some(completed_handshake_info))) = awaiting_act_three_state.next(&test_ctx.valid_act3).unwrap() {
+			completed_handshake_info
 		} else {
 			panic!();
 		};
 
-		assert_eq!(remote_pubkey, test_ctx.initiator_public_key);
-		assert_eq!(0, conduit.decryptor.read_buffer_length());
+		assert_eq!(completed_handshake_info.their_node_id, test_ctx.initiator_public_key);
+		assert_eq!(0, completed_handshake_info.conduit.decryptor.read_buffer_length());
 	}
 
 	// Responder::AwaitingActThree -> None (with extra bytes)
@@ -771,14 +781,14 @@ mod test {
 		let mut act3 = test_ctx.valid_act3;
 		act3.extend_from_slice(&[2; 16]);
 
-		let (conduit, remote_pubkey) = if let (None, Complete(Some((conduit, remote_pubkey)))) = awaiting_act_three_state.next(&act3).unwrap() {
-			(conduit, remote_pubkey)
+		let completed_handshake_info = if let (None, Complete(Some(completed_handshake_info))) = awaiting_act_three_state.next(&act3).unwrap() {
+			completed_handshake_info
 		} else {
 			panic!();
 		};
 
-		assert_eq!(remote_pubkey, test_ctx.initiator_public_key);
-		assert_eq!(16, conduit.decryptor.read_buffer_length());
+		assert_eq!(completed_handshake_info.their_node_id, test_ctx.initiator_public_key);
+		assert_eq!(16, completed_handshake_info.conduit.decryptor.read_buffer_length());
 	}
 
 	// Responder::AwaitingActThree -> Error (bad version bytes)

lightning/src/ln/peers/test_util.rs

@@ -5,6 +5,7 @@ use bitcoin::secp256k1::key::{PublicKey, SecretKey};
 
 use ln::peers::conduit::Conduit;
 use ln::peers::handler::{SocketDescriptor, ITransport, PeerHandleError, MessageQueuer};
+use ln::peers::handshake::CompletedHandshakeInfo;
 use ln::peers::transport::{IPeerHandshake, PayloadQueuer};
 
 use std::rc::Rc;
@@ -42,7 +43,7 @@ impl IPeerHandshake for PeerHandshakeTestStubFail {
 		PeerHandshakeTestStubFail { }
 	}
 
-	fn process_act(&mut self, _input: &[u8]) -> Result<(Option<Vec<u8>>, Option<(Conduit, PublicKey)>), String> {
+	fn process_act(&mut self, _input: &[u8]) -> Result<(Option<Vec<u8>>, Option<CompletedHandshakeInfo>), String> {
 		Err("Oh no!".to_string())
 	}
 }
@@ -68,7 +69,7 @@ impl IPeerHandshake for PeerHandshakeTestStubBytes {
 		PeerHandshakeTestStubBytes { }
 	}
 
-	fn process_act(&mut self, _input: &[u8]) -> Result<(Option<Vec<u8>>, Option<(Conduit, PublicKey)>), String> {
+	fn process_act(&mut self, _input: &[u8]) -> Result<(Option<Vec<u8>>, Option<CompletedHandshakeInfo>), String> {
 		Ok((Some(Self::RETURNED_BYTES[..].to_vec()), None))
 	}
 }
@@ -89,13 +90,18 @@ impl IPeerHandshake for PeerHandshakeTestStubComplete {
 		PeerHandshakeTestStubComplete { }
 	}
 
-	fn process_act(&mut self, _input: &[u8]) -> Result<(Option<Vec<u8>>, Option<(Conduit, PublicKey)>), String> {
+	fn process_act(&mut self, _input: &[u8]) -> Result<(Option<Vec<u8>>, Option<CompletedHandshakeInfo>), String> {
 		let curve = secp256k1::Secp256k1::new();
 		let private_key = SecretKey::from_slice(&[0x_21_u8; 32]).unwrap();
 		let public_key = PublicKey::from_secret_key(&curve, &private_key);
 		let conduit = Conduit::new([0;32], [0;32], [0;32]);
 
-		Ok((None, Some((conduit, public_key))))
+		Ok((None, Some(
+			CompletedHandshakeInfo {
+				conduit,
+				their_node_id: public_key
+			}
+		)))
 	}
 }
 

lightning/src/ln/peers/transport.rs

@@ -4,7 +4,7 @@ use bitcoin::secp256k1::{SecretKey, PublicKey};
 
 use ln::peers::conduit::Conduit;
 use ln::peers::handler::{ITransport, PeerHandleError, MessageQueuer};
-use ln::peers::handshake::PeerHandshake;
+use ln::peers::handshake::{CompletedHandshakeInfo, PeerHandshake};
 use ln::{wire, msgs};
 use ln::wire::{Encode, Message};
 
@@ -25,7 +25,7 @@ pub trait IPeerHandshake {
 
 	/// Progress the handshake given bytes received from the peer. Returns Some(Conduit, PublicKey) when the handshake
 	/// is complete.
-	fn process_act(&mut self, input: &[u8]) -> Result<(Option<Vec<u8>>, Option<(Conduit, PublicKey)>), String>;
+	fn process_act(&mut self, input: &[u8]) -> Result<(Option<Vec<u8>>, Option<CompletedHandshakeInfo>), String>;
 }
 
 /// Trait representing a container that allows enqueuing of Vec<[u8]>. Used by Transport to enqueue
@@ -74,17 +74,17 @@ impl<PeerHandshakeImpl: IPeerHandshake> ITransport for Transport<PeerHandshakeIm
 		match self.conduit {
 			// Continue handshake
 			None => {
-				let (response_option, conduit_and_remote_pubkey_option) = self.handshake.process_act(input)?;
+				let (response_option, completed_handshake_info_option) = self.handshake.process_act(input)?;
 
 				// Any response generated by the handshake sequence is put into the response buffer
 				if let Some(response) = response_option {
 					output_buffer.push_back(response.to_vec());
 				}
 
 				// If handshake is complete change the state
-				if let Some((conduit, remote_pubkey)) = conduit_and_remote_pubkey_option {
-					self.conduit = Some(conduit);
-					self.their_node_id = Some(remote_pubkey);
+				if let Some(completed_handshake_info) = completed_handshake_info_option {
+					self.conduit = Some(completed_handshake_info.conduit);
+					self.their_node_id = Some(completed_handshake_info.their_node_id);
 					Ok(true) // newly connected
 				} else {
 					Ok(false) // newly connected