From b80d3d9d292007e9646aed1a150345a5b7896d26 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 23 Feb 2020 23:25:43 -0500 Subject: [PATCH] Change Option 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. --- lightning/src/ln/channel.rs | 36 ++++++++++-------------------- lightning/src/ln/channelmonitor.rs | 36 +++++------------------------- lightning/src/util/ser.rs | 16 +++++++++---- 3 files changed, 29 insertions(+), 59 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 3dc5dcd8..f155a0bd 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3883,18 +3883,6 @@ impl Writeable for Channel { } } - 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 Writeable for Channel { }, &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 Writeable for Channel { 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 Writeable for Channel { 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 Writeable for Channel { 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 c5f4fea5..b1e8428b 100644 --- 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 ::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 ChannelMonitor { // 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 ChannelMonitor { 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 ChannelMonitor { } 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 38133d59..60d5329d 100644 --- 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(&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 Writeable for Option { 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 Writeable for Option { impl Readable for Option { fn read(r: &mut R) -> Result { - match ::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)?)) + } } } } -- 2.30.2