Change Option<T> serialization format to include length

This is a cheap way to fix an error in Router serialization
roundtrip due to us calling read_to_end during the read of
channel/node announcement/updates. During normal message reading,
we only have limited bytes to read (specifically the message buffer)
so this is fine, however when we read them inside Router, we have
more data from other fields of the Router available as well. Thus,
we end up reading the entire rest of the Router into one message
field, and failing to deserialize.

Because such fields are always stored in Option<>s, we can simply
use a LengthLimitingStream in the Option<> serialization format and
make only the correct number of bytes available.

By using a variable-length integer for the new field, we avoid
wasting space compared to the existing serialization format.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -3883,18 +3883,6 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 			}
 		}
 
-		macro_rules! write_option {
-			($thing: expr) => {
-				match &$thing {
-					&None => 0u8.write(writer)?,
-					&Some(ref v) => {
-						1u8.write(writer)?;
-						v.write(writer)?;
-					},
-				}
-			}
-		}
-
 		(self.pending_outbound_htlcs.len() as u64).write(writer)?;
 		for htlc in self.pending_outbound_htlcs.iter() {
 			htlc.htlc_id.write(writer)?;
@@ -3912,15 +3900,15 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 				},
 				&OutboundHTLCState::RemoteRemoved(ref fail_reason) => {
 					2u8.write(writer)?;
-					write_option!(*fail_reason);
+					fail_reason.write(writer)?;
 				},
 				&OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref fail_reason) => {
 					3u8.write(writer)?;
-					write_option!(*fail_reason);
+					fail_reason.write(writer)?;
 				},
 				&OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref fail_reason) => {
 					4u8.write(writer)?;
-					write_option!(*fail_reason);
+					fail_reason.write(writer)?;
 				},
 			}
 		}
@@ -3971,8 +3959,8 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 			fail_reason.write(writer)?;
 		}
 
-		write_option!(self.pending_update_fee);
-		write_option!(self.holding_cell_update_fee);
+		self.pending_update_fee.write(writer)?;
+		self.holding_cell_update_fee.write(writer)?;
 
 		self.next_local_htlc_id.write(writer)?;
 		(self.next_remote_htlc_id - dropped_inbound_htlcs).write(writer)?;
@@ -3989,9 +3977,9 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 			None => 0u8.write(writer)?,
 		}
 
-		write_option!(self.funding_txo);
-		write_option!(self.funding_tx_confirmed_in);
-		write_option!(self.short_channel_id);
+		self.funding_txo.write(writer)?;
+		self.funding_tx_confirmed_in.write(writer)?;
+		self.short_channel_id.write(writer)?;
 
 		self.last_block_connected.write(writer)?;
 		self.funding_tx_confirmations.write(writer)?;
@@ -4007,13 +3995,13 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 		self.their_max_accepted_htlcs.write(writer)?;
 		self.minimum_depth.write(writer)?;
 
-		write_option!(self.their_pubkeys);
-		write_option!(self.their_cur_commitment_point);
+		self.their_pubkeys.write(writer)?;
+		self.their_cur_commitment_point.write(writer)?;
 
-		write_option!(self.their_prev_commitment_point);
+		self.their_prev_commitment_point.write(writer)?;
 		self.their_node_id.write(writer)?;
 
-		write_option!(self.their_shutdown_scriptpubkey);
+		self.their_shutdown_scriptpubkey.write(writer)?;
 
 		self.commitment_secrets.write(writer)?;
 

lightning/src/ln/channelmonitor.rs

@@ -489,11 +489,7 @@ impl Writeable for InputMaterial  {
 				script.write(writer)?;
 				pubkey.write(writer)?;
 				writer.write_all(&key[..])?;
-				if *is_htlc {
-					writer.write_all(&[0; 1])?;
-				} else {
-					writer.write_all(&[1; 1])?;
-				}
+				is_htlc.write(writer)?;
 				writer.write_all(&byte_utils::be64_to_array(*amount))?;
 			},
 			&InputMaterial::RemoteHTLC { ref script, ref key, ref preimage, ref amount, ref locktime } => {
@@ -524,11 +520,7 @@ impl Readable for InputMaterial {
 				let script = Readable::read(reader)?;
 				let pubkey = Readable::read(reader)?;
 				let key = Readable::read(reader)?;
-				let is_htlc = match <u8 as Readable>::read(reader)? {
-					0 => true,
-					1 => false,
-					_ => return Err(DecodeError::InvalidValue),
-				};
+				let is_htlc = Readable::read(reader)?;
 				let amount = Readable::read(reader)?;
 				InputMaterial::Revoked {
 					script,
@@ -698,13 +690,7 @@ impl Writeable for ChannelMonitorUpdateStep {
 				(htlc_outputs.len() as u64).write(w)?;
 				for &(ref output, ref source) in htlc_outputs.iter() {
 					output.write(w)?;
-					match source {
-						&None => 0u8.write(w)?,
-						&Some(ref s) => {
-							1u8.write(w)?;
-							s.write(w)?;
-						},
-					}
+					source.as_ref().map(|b| b.as_ref()).write(w)?;
 				}
 			},
 			&ChannelMonitorUpdateStep::PaymentPreimage { ref payment_preimage } => {
@@ -973,18 +959,6 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
 		// Set in initial Channel-object creation, so should always be set by now:
 		U48(self.commitment_transaction_number_obscure_factor).write(writer)?;
 
-		macro_rules! write_option {
-			($thing: expr) => {
-				match $thing {
-					&Some(ref t) => {
-						1u8.write(writer)?;
-						t.write(writer)?;
-					},
-					&None => 0u8.write(writer)?,
-				}
-			}
-		}
-
 		match self.key_storage {
 			Storage::Local { ref keys, ref funding_key, ref revocation_base_key, ref htlc_base_key, ref delayed_payment_base_key, ref payment_base_key, ref shutdown_pubkey, ref funding_info, ref current_remote_commitment_txid, ref prev_remote_commitment_txid } => {
 				writer.write_all(&[0; 1])?;
@@ -1055,7 +1029,7 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
 			writer.write_all(&byte_utils::be64_to_array(htlc_infos.len() as u64))?;
 			for &(ref htlc_output, ref htlc_source) in htlc_infos.iter() {
 				serialize_htlc_in_commitment!(htlc_output);
-				write_option!(htlc_source);
+				htlc_source.as_ref().map(|b| b.as_ref()).write(writer)?;
 			}
 		}
 
@@ -1098,7 +1072,7 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
 					} else {
 						0u8.write(writer)?;
 					}
-					write_option!(htlc_source);
+					htlc_source.write(writer)?;
 				}
 			}
 		}

lightning/src/util/ser.rs

@@ -172,6 +172,10 @@ pub trait Writeable {
 	}
 }
 
+impl<'a, T: Writeable> Writeable for &'a T {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> { (*self).write(writer) }
+}
+
 /// A trait that various rust-lightning types implement allowing them to be read in from a Read
 pub trait Readable
 	where Self: Sized
@@ -592,7 +596,9 @@ impl<T: Writeable> Writeable for Option<T> {
 		match *self {
 			None => 0u8.write(w)?,
 			Some(ref data) => {
-				1u8.write(w)?;
+				let mut len_calc = LengthCalculatingWriter(0);
+				data.write(&mut len_calc).expect("No in-memory data may fail to serialize");
+				BigSize(len_calc.0 as u64 + 1).write(w)?;
 				data.write(w)?;
 			}
 		}
@@ -603,10 +609,12 @@ impl<T: Writeable> Writeable for Option<T> {
 impl<T: Readable> Readable for Option<T>
 {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		match <u8 as Readable>::read(r)? {
+		match BigSize::read(r)?.0 {
 			0 => Ok(None),
-			1 => Ok(Some(Readable::read(r)?)),
-			_ => return Err(DecodeError::InvalidValue),
+			len => {
+				let mut reader = FixedLengthReader::new(r, len - 1);
+				Ok(Some(Readable::read(&mut reader)?))
+			}
 		}
 	}
 }