respond to more of Jeff's comments

Author: arik-so

lightning/src/ln/peer_handler.rs

@@ -542,7 +542,8 @@ impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where
 								}
 							}
 
-							let message_option = conduit.decrypt_single_message(Some(&peer.pending_read_buffer));
+							let message_option = conduit.decrypt_single_message(Some(&peer.pending_read_buffer.clone()));
+							peer.pending_read_buffer = Vec::new(); // empty the pending read buffer
 							let msg_data = if let Some(message) = message_option {
 								message
 							} else {

lightning/src/ln/peers/conduit.rs

@@ -3,7 +3,7 @@
 use ln::peers::{chacha, hkdf};
 use util::byte_utils;
 
-type SymmetricKey = [u8; 32];
+pub(super) type SymmetricKey = [u8; 32];
 const MESSAGE_LENGTH_HEADER_SIZE: usize = 2;
 const TAGGED_MESSAGE_LENGTH_HEADER_SIZE: usize = MESSAGE_LENGTH_HEADER_SIZE + chacha::TAG_SIZE;
 
@@ -62,15 +62,17 @@ impl Conduit {
 		}
 
 		let (current_message, offset) = self.decrypt(&read_buffer[..]);
+		read_buffer.drain(0..offset); // drain the read buffer
+		self.read_buffer = Some(read_buffer); // assign the new value to the built-in buffer
+
 		if offset == 0 {
 			return None;
 		}
 
-		read_buffer.drain(0..offset);
 		current_message
 	}
 
-	fn decrypt(&mut self, buffer: &[u8]) -> (Option<Vec<u8>>, usize) { // the response slice should have the same lifetime as the argument. It's the slice data is read from
+	pub(crate) fn decrypt(&mut self, buffer: &[u8]) -> (Option<Vec<u8>>, usize) { // the response slice should have the same lifetime as the argument. It's the slice data is read from
 		if buffer.len() < TAGGED_MESSAGE_LENGTH_HEADER_SIZE {
 			return (None, 0);
 		}
@@ -81,7 +83,7 @@ impl Conduit {
 		length_bytes.copy_from_slice(length_vec.as_slice());
 		let message_length = byte_utils::slice_to_be16(&length_bytes) as usize;
 
-		let message_end_index = message_length + TAGGED_MESSAGE_LENGTH_HEADER_SIZE + chacha::TAG_SIZE; // todo: abort if too short
+		let message_end_index = TAGGED_MESSAGE_LENGTH_HEADER_SIZE + message_length + chacha::TAG_SIZE; // todo: abort if too short
 		if buffer.len() < message_end_index {
 			return (None, 0);
 		}
@@ -175,9 +177,18 @@ mod tests {
 		assert_eq!(encrypted_messages[1000], hex::decode("4a2f3cc3b5e78ddb83dcb426d9863d9d9a723b0337c89dd0b005d89f8d3c05c52b76b29b740f09").unwrap());
 		assert_eq!(encrypted_messages[1001], hex::decode("2ecd8c8a5629d0d02ab457a0fdd0f7b90a192cd46be5ecb6ca570bfc5e268338b1a16cf4ef2d36").unwrap());
 
-		for _ in 0..1002 {
-			let encrypted_message = encrypted_messages.remove(0);
-			let decrypted_message = remote_peer.decrypt_single_message(Some(&encrypted_message)).unwrap();
+		for _ in 0..501 {
+			// read two messages at once, filling buffer
+			let mut current_encrypted_message = encrypted_messages.remove(0);
+			let mut next_encrypted_message = encrypted_messages.remove(0);
+			current_encrypted_message.extend_from_slice(&next_encrypted_message);
+			let decrypted_message = remote_peer.decrypt_single_message(Some(&current_encrypted_message)).unwrap();
+			assert_eq!(decrypted_message, hex::decode("68656c6c6f").unwrap());
+		}
+
+		for _ in 0..501 {
+			// decrypt messages directly from buffer without adding to it
+			let decrypted_message = remote_peer.decrypt_single_message(None).unwrap();
 			assert_eq!(decrypted_message, hex::decode("68656c6c6f").unwrap());
 		}
 	}

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

@@ -1,16 +1,20 @@
+pub const ACT_ONE_LENGTH: usize = 50;
+pub const ACT_TWO_LENGTH: usize = 50;
+pub const ACT_THREE_LENGTH: usize = 66;
+
 /// Wrapper for the first act message
 pub struct ActOne(
-	pub(super) [u8; 50]
+	pub(super) [u8; ACT_ONE_LENGTH]
 );
 
 /// Wrapper for the second act message
 pub struct ActTwo(
-	pub(super) [u8; 50]
+	pub(super) [u8; ACT_TWO_LENGTH]
 );
 
 /// Wrapper for the third act message
 pub struct ActThree(
-	pub(super) [u8; 66]
+	pub(super) [u8; ACT_THREE_LENGTH]
 );
 
 /// Wrapper for any act message

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

@@ -9,8 +9,8 @@ use bitcoin_hashes::sha256::Hash as Sha256;
 use secp256k1::{PublicKey, SecretKey};
 
 use ln::peers::{chacha, hkdf};
-use ln::peers::conduit::Conduit;
-use ln::peers::handshake::acts::{ActOne, ActThree, ActTwo};
+use ln::peers::conduit::{Conduit, SymmetricKey};
+use ln::peers::handshake::acts::{ActOne, ActThree, ActTwo, ACT_ONE_LENGTH, ACT_TWO_LENGTH, ACT_THREE_LENGTH};
 use ln::peers::handshake::hash::HandshakeHash;
 use ln::peers::handshake::states::{ActOneExpectation, ActThreeExpectation, ActTwoExpectation, HandshakeState};
 
@@ -33,7 +33,7 @@ impl PeerHandshake {
 	/// Instantiate a new handshake with a node identity secret key and an ephemeral private key
 	pub fn new(private_key: &SecretKey, ephemeral_private_key: &SecretKey) -> Self {
 		Self {
-			state: Some(HandshakeState::Blank),
+			state: Some(HandshakeState::Uninitiated),
 			private_key: (*private_key).clone(),
 			ephemeral_private_key: (*ephemeral_private_key).clone(),
 			read_buffer: Vec::new(),
@@ -68,7 +68,15 @@ impl PeerHandshake {
 	}
 
 	/// Process act dynamically
-	/// The role must be set before this method can be called
+	/// # Arguments
+	/// `input`: Byte slice received from peer as part of the handshake protocol
+	/// `remote_public_key`: If outbound, the peer's static identity public key needs is required for the handshake initiation
+	///
+	/// # 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`: Conduit option if the handshake was just processed to completion and messages can now be encrypted and decrypted
+	/// `.2`: Public key option if the handshake was inbound and the peer's static identity pubkey was just learned
 	pub fn process_act(&mut self, input: &[u8], remote_public_key: Option<&PublicKey>) -> Result<(Vec<u8>, Option<Conduit>, Option<PublicKey>), String> {
 		let mut response: Vec<u8> = Vec::new();
 		let mut connected_peer = None;
@@ -78,7 +86,7 @@ impl PeerHandshake {
 		let read_buffer_length = self.read_buffer.len();
 
 		match &self.state {
-			&Some(HandshakeState::Blank) => {
+			&Some(HandshakeState::Uninitiated) => {
 				let remote_public_key = remote_public_key.ok_or("remote_public_key must be Some for outbound connections")?;
 				let act_one = self.initiate(&remote_public_key)?;
 				response = act_one.0.to_vec();
@@ -89,22 +97,21 @@ impl PeerHandshake {
 					return Err("need at least 50 bytes".to_string());
 				}
 
-				let mut act_one_buffer = [0u8; 50];
-				act_one_buffer.copy_from_slice(&self.read_buffer[..act_length]);
-				self.read_buffer.drain(..act_length);
+				let mut act_one_buffer = [0u8; ACT_ONE_LENGTH];
+				act_one_buffer.copy_from_slice(&self.read_buffer[..ACT_ONE_LENGTH]);
+				self.read_buffer.drain(..ACT_ONE_LENGTH);
 
 				let act_two = self.process_act_one(ActOne(act_one_buffer))?;
 				response = act_two.0.to_vec();
 			}
 			&Some(HandshakeState::AwaitingActTwo(_)) => {
-				let act_length = 50;
-				if read_buffer_length < act_length {
+				if read_buffer_length < ACT_TWO_LENGTH {
 					return Err("need at least 50 bytes".to_string());
 				}
 
-				let mut act_two_buffer = [0u8; 50];
-				act_two_buffer.copy_from_slice(&self.read_buffer[..act_length]);
-				self.read_buffer.drain(..act_length);
+				let mut act_two_buffer = [0u8; ACT_TWO_LENGTH];
+				act_two_buffer.copy_from_slice(&self.read_buffer[..ACT_TWO_LENGTH]);
+				self.read_buffer.drain(..ACT_TWO_LENGTH);
 
 				let (act_three, mut conduit) = self.process_act_two(ActTwo(act_two_buffer))?;
 
@@ -117,14 +124,13 @@ impl PeerHandshake {
 				connected_peer = Some(conduit);
 			}
 			&Some(HandshakeState::AwaitingActThree(_)) => {
-				let act_length = 66;
-				if read_buffer_length < act_length {
-					return Err("need at least 50 bytes".to_string());
+				if read_buffer_length < ACT_THREE_LENGTH {
+					return Err("need at least 66 bytes".to_string());
 				}
 
-				let mut act_three_buffer = [0u8; 66];
-				act_three_buffer.copy_from_slice(&self.read_buffer[..act_length]);
-				self.read_buffer.drain(..act_length);
+				let mut act_three_buffer = [0u8; ACT_THREE_LENGTH];
+				act_three_buffer.copy_from_slice(&self.read_buffer[..ACT_THREE_LENGTH]);
+				self.read_buffer.drain(..ACT_THREE_LENGTH);
 
 				let (public_key, mut conduit) = self.process_act_three(ActThree(act_three_buffer))?;
 
@@ -145,7 +151,7 @@ impl PeerHandshake {
 
 	/// Initiate the handshake with a peer and return the first act
 	pub fn initiate(&mut self, remote_public_key: &PublicKey) -> Result<ActOne, String> {
-		if let &Some(HandshakeState::Blank) = &self.state {} else {
+		if let &Some(HandshakeState::Uninitiated) = &self.state {} else {
 			return Err("incorrect state".to_string());
 		}
 
@@ -174,9 +180,8 @@ impl PeerHandshake {
 		let state = self.state.take();
 		let act_one_expectation = match state {
 			Some(HandshakeState::AwaitingActOne(act_state)) => act_state,
-			Some(HandshakeState::Blank) => {
+			Some(HandshakeState::Uninitiated) => {
 				// this can also be initiated from a blank state
-				// public key
 				let public_key = Self::private_key_to_public_key(&self.private_key);
 				let (hash, chaining_key) = Self::initialize_state(&public_key);
 				ActOneExpectation {
@@ -315,7 +320,7 @@ impl PeerHandshake {
 		Ok((remote_pubkey, connected_peer))
 	}
 
-	fn calculate_act_message(local_private_key: &SecretKey, remote_public_key: &PublicKey, chaining_key: [u8; 32], hash: &mut HandshakeHash) -> ([u8; 50], [u8; 32], [u8; 32]) {
+	fn calculate_act_message(local_private_key: &SecretKey, remote_public_key: &PublicKey, chaining_key: [u8; 32], hash: &mut HandshakeHash) -> ([u8; 50], SymmetricKey, SymmetricKey) {
 		let local_public_key = Self::private_key_to_public_key(local_private_key);
 
 		hash.update(&local_public_key.serialize());
@@ -333,7 +338,7 @@ impl PeerHandshake {
 		(act, chaining_key, temporary_key)
 	}
 
-	fn process_act_message(act_bytes: [u8; 50], local_private_key: &SecretKey, chaining_key: [u8; 32], hash: &mut HandshakeHash) -> Result<(PublicKey, [u8; 32], [u8; 32]), String> {
+	fn process_act_message(act_bytes: [u8; 50], local_private_key: &SecretKey, chaining_key: [u8; 32], hash: &mut HandshakeHash) -> Result<(PublicKey, SymmetricKey, SymmetricKey), String> {
 		let version = act_bytes[0];
 		if version != 0 {
 			return Err("unexpected version".to_string());
@@ -358,7 +363,7 @@ impl PeerHandshake {
 		// HKDF(chaining key, ECDH) -> chaining key' + next temporary key
 		let (chaining_key, temporary_key) = hkdf::derive(&chaining_key, &ecdh);
 
-		// Validate chacha tag (temporary key, 0, self.hash, chacha_tag)
+		// Validate chacha tag (temporary key, 0, hash, chacha_tag)
 		let _tag_check = chacha::decrypt(&temporary_key, 0, &hash.value, &chacha_tag)?;
 
 		hash.update(&chacha_tag);
@@ -372,7 +377,7 @@ impl PeerHandshake {
 		pk_object
 	}
 
-	fn ecdh(private_key: &SecretKey, public_key: &PublicKey) -> [u8; 32] {
+	fn ecdh(private_key: &SecretKey, public_key: &PublicKey) -> SymmetricKey {
 		let curve = secp256k1::Secp256k1::new();
 		let mut pk_object = public_key.clone();
 		pk_object.mul_assign(&curve, &private_key[..]).expect("invalid multiplication");

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

@@ -2,7 +2,7 @@ use ln::peers::handshake::hash::HandshakeHash;
 use secp256k1::{SecretKey, PublicKey};
 
 pub enum HandshakeState {
-	Blank,
+	Uninitiated,
 	AwaitingActOne(ActOneExpectation),
 	AwaitingActTwo(ActTwoExpectation),
 	AwaitingActThree(ActThreeExpectation),