Change Option<T>::read 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.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 3dc5dcd832d4a7f99c1d4590a31a7acb79faf670..f155a0bd295051f87c8b69a308774e4e13e53eb0 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/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)?;
 

diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs
index c5f4fea5de66cff4cd2b2df278a98df4a6f0a350..b1e8428bd0ac553ef3d2320527fe5aa2d9754b19 100644 (file)
--- a/lightning/src/ln/channelmonitor.rs
+++ b/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)?;
                                }
                        }
                }

diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs
index 38133d59670dd5c717a3d1dc850fe3296fc316c6..60d5329d781c8ed247a36eef791470e224878d48 100644 (file)
--- a/lightning/src/util/ser.rs
+++ b/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)?))
+                       }
                }
        }
 }