review: ActBuilder::fill() returns the number of bytes consumed

Instead of returning a subslice of input for the caller to process, just
return the number of bytes consume and let them do the accounting.

Author: julianknutsen

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

@@ -82,8 +82,8 @@ impl ActBuilder {
 		}
 	}
 
-	/// Fills the Act with bytes from input and returns the unprocessed bytes
-	pub(super) fn fill<'a>(&mut self, input: &'a [u8]) -> &'a [u8] {
+	/// Fills the Act with bytes from input and returns the number of bytes consumed from input.
+	pub(super) fn fill(&mut self, input: &[u8]) -> usize {
 		// Simple fill implementation for both almost-identical structs to deduplicate code
 		// $act: Act[One|Two|Three], $input: &[u8]; returns &[u8] of remaining input that was not processed
 		macro_rules! fill_act_content {
@@ -93,7 +93,7 @@ impl ActBuilder {
 				$act[$write_pos..$write_pos + fill_amount].copy_from_slice(&$input[..fill_amount]);
 
 				$write_pos += fill_amount;
-				&$input[fill_amount..]
+				fill_amount
 			}}
 		}
 
@@ -125,40 +125,41 @@ mod tests {
 	fn partial_fill() {
 		let mut builder = ActBuilder::new(Act::One(EMPTY_ACT_ONE));
 
-		let remaining = builder.fill(&[1, 2, 3]);
+		let input = [1, 2, 3];
+		let bytes_read = builder.fill(&input);
 		assert_eq!(builder.partial_act.len(), ACT_ONE_LENGTH);
 		assert_eq!(builder.write_pos, 3);
 		assert!(!builder.is_finished());
-		assert_eq!(remaining, &[]);
+		assert_eq!(bytes_read, input.len());
 	}
 
 	// Test bookkeeping of exact fill
 	#[test]
 	fn exact_fill() {
 		let mut builder = ActBuilder::new(Act::One(EMPTY_ACT_ONE));
 
-		let input = [0; 50];
-		let remaining = builder.fill(&input);
+		let input = [0; ACT_ONE_LENGTH];
+		let bytes_read = builder.fill(&input);
 		assert_eq!(builder.partial_act.len(), ACT_ONE_LENGTH);
 		assert_eq!(builder.write_pos, ACT_ONE_LENGTH);
 		assert!(builder.is_finished());
 		assert_eq!(Act::from(builder).as_ref(), &input[..]);
-		assert_eq!(remaining, &[]);
+		assert_eq!(bytes_read, input.len());
 	}
 
 	// Test bookkeeping of overfill
 	#[test]
 	fn over_fill() {
 		let mut builder = ActBuilder::new(Act::One(EMPTY_ACT_ONE));
 
-		let input = [0; 51];
-		let remaining = builder.fill(&input);
+		let input = [0; ACT_ONE_LENGTH + 1];
+		let bytes_read = builder.fill(&input);
 
 		assert_eq!(builder.partial_act.len(), ACT_ONE_LENGTH);
 		assert_eq!(builder.write_pos, ACT_ONE_LENGTH);
 		assert!(builder.is_finished());
-		assert_eq!(Act::from(builder).as_ref(), &input[..50]);
-		assert_eq!(remaining, &[0]);
+		assert_eq!(Act::from(builder).as_ref(), &input[..ACT_ONE_LENGTH]);
+		assert_eq!(bytes_read, ACT_ONE_LENGTH);
 	}
 
 	// Converting an unfinished ActBuilder panics

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

@@ -181,17 +181,17 @@ impl IHandshakeState for ResponderAwaitingActOneState {
 	// https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#act-two (sender)
 	fn next(self, input: &[u8]) -> Result<(Option<Act>, HandshakeState), String> {
 		let mut act_one_builder = self.act_one_builder;
-		let remaining = act_one_builder.fill(input);
+		let bytes_read = act_one_builder.fill(input);
 
-		// Any payload larger than ACT_ONE_LENGTH indicates a bad peer since initiator data
-		// is required to generate act3 (so it can't come before we transition)
-		if remaining.len() != 0 {
+		// Any payload that is not fully consumed by the builder indicates a bad peer since responder
+		// data is required to generate act3 (so it can't come before we transition)
+		if bytes_read < input.len() {
 			return Err("Act One too large".to_string());
 		}
 
 		// In the event of a partial fill, stay in the same state and wait for more data
 		if !act_one_builder.is_finished() {
-			assert_eq!(remaining.len(), 0);
+			assert_eq!(bytes_read, input.len());
 			return Ok((
 				None,
 				HandshakeState::ResponderAwaitingActOne(Self {
@@ -247,17 +247,17 @@ impl IHandshakeState for InitiatorAwaitingActTwoState {
 	// https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#act-three (sender)
 	fn next(self, input: &[u8]) -> Result<(Option<Act>, HandshakeState), String> {
 		let mut act_two_builder = self.act_two_builder;
-		let remaining = act_two_builder.fill(input);
+		let bytes_read = act_two_builder.fill(input);
 
-		// Any payload larger than ACT_TWO_LENGTH indicates a bad peer since responder data
+		// Any payload that is not fully consumed by the builder indicates a bad peer since responder data
 		// is required to generate post-authentication messages (so it can't come before we transition)
-		if remaining.len() != 0 {
+		if bytes_read < input.len() {
 			return Err("Act Two too large".to_string());
 		}
 
 		// In the event of a partial fill, stay in the same state and wait for more data
 		if !act_two_builder.is_finished() {
-			assert_eq!(remaining.len(), 0);
+			assert_eq!(bytes_read, input.len());
 			return Ok((
 				None,
 				HandshakeState::InitiatorAwaitingActTwo(Self {
@@ -325,11 +325,11 @@ impl IHandshakeState for ResponderAwaitingActThreeState {
 	// https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#act-three (receiver)
 	fn next(self, input: &[u8]) -> Result<(Option<Act>, HandshakeState), String> {
 		let mut act_three_builder = self.act_three_builder;
-		let remaining = act_three_builder.fill(input);
+		let bytes_read = act_three_builder.fill(input);
 
 		// In the event of a partial fill, stay in the same state and wait for more data
 		if !act_three_builder.is_finished() {
-			assert_eq!(remaining.len(), 0);
+			assert_eq!(bytes_read, input.len());
 			return Ok((
 				None,
 				HandshakeState::ResponderAwaitingActThree(Self {
@@ -392,7 +392,7 @@ impl IHandshakeState for ResponderAwaitingActThreeState {
 
 		// Any remaining data in the read buffer would be encrypted, so transfer ownership
 		// to the Conduit for future use.
-		conduit.read(remaining);
+		conduit.read(&input[bytes_read..]);
 
 		Ok((
 			None,