From a515eb3ba6bf9a73db0a8a26774c83561153fb5a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 5 May 2021 22:56:42 +0000 Subject: [PATCH] Append backwards-compat TLVs to serialization of larger structs Currently our serialization is very compact, and contains version numbers to indicate which versions the code can read a given serialized struct. However, if you want to add a new field without needlessly breaking the ability of previous versions of the code to read the struct, there is not a good way to do so. This adds dummy, currently empty, TLVs to the major structs we serialize out for users, providing an easy place to put new optional fields without breaking previous versions. --- lightning/src/chain/channelmonitor.rs | 23 +++++----- lightning/src/ln/channel.rs | 14 +++---- lightning/src/ln/channelmanager.rs | 13 +++--- lightning/src/ln/onchaintx.rs | 11 +++++ lightning/src/util/ser_macros.rs | 60 +++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 27 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 81981115..e7d5e322 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -502,9 +502,6 @@ enum OnchainEvent { }, } -const SERIALIZATION_VERSION: u8 = 1; -const MIN_SERIALIZATION_VERSION: u8 = 1; - #[cfg_attr(any(test, feature = "fuzztarget", feature = "_test_utils"), derive(PartialEq))] #[derive(Clone)] pub(crate) enum ChannelMonitorUpdateStep { @@ -805,17 +802,17 @@ impl PartialEq for ChannelMonitorImpl { impl Writeable for ChannelMonitor { fn write(&self, writer: &mut W) -> Result<(), Error> { - //TODO: We still write out all the serialization here manually instead of using the fancy - //serialization framework we have, we should migrate things over to it. - writer.write_all(&[SERIALIZATION_VERSION; 1])?; - writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?; - self.inner.lock().unwrap().write(writer) } } +const SERIALIZATION_VERSION: u8 = 1; +const MIN_SERIALIZATION_VERSION: u8 = 1; + impl Writeable for ChannelMonitorImpl { fn write(&self, writer: &mut W) -> Result<(), Error> { + write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION); + self.latest_update_id.write(writer)?; // Set in initial Channel-object creation, so should always be set by now: @@ -991,6 +988,8 @@ impl Writeable for ChannelMonitorImpl { self.lockdown_from_offchain.write(writer)?; self.holder_tx_signed.write(writer)?; + write_tlv_fields!(writer, {}); + Ok(()) } } @@ -2754,11 +2753,7 @@ impl<'a, Signer: Sign, K: KeysInterface> ReadableArgs<&'a K> } } - let _ver: u8 = Readable::read(reader)?; - let min_ver: u8 = Readable::read(reader)?; - if min_ver > SERIALIZATION_VERSION { - return Err(DecodeError::UnknownVersion); - } + let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION); let latest_update_id: u64 = Readable::read(reader)?; let commitment_transaction_number_obscure_factor = ::read(reader)?.0; @@ -2979,6 +2974,8 @@ impl<'a, Signer: Sign, K: KeysInterface> ReadableArgs<&'a K> let lockdown_from_offchain = Readable::read(reader)?; let holder_tx_signed = Readable::read(reader)?; + read_tlv_fields!(reader, {}, {}); + let mut secp_ctx = Secp256k1::new(); secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes()); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index dbc63c95..0098f7a9 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4375,8 +4375,7 @@ impl Writeable for Channel { // Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been // called. - writer.write_all(&[SERIALIZATION_VERSION; 1])?; - writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?; + write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION); self.user_id.write(writer)?; self.config.write(writer)?; @@ -4565,6 +4564,9 @@ impl Writeable for Channel { self.commitment_secrets.write(writer)?; self.channel_update_status.write(writer)?; + + write_tlv_fields!(writer, {}); + Ok(()) } } @@ -4573,11 +4575,7 @@ const MAX_ALLOC_SIZE: usize = 64*1024; impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel where K::Target: KeysInterface { fn read(reader: &mut R, keys_source: &'a K) -> Result { - let _ver: u8 = Readable::read(reader)?; - let min_ver: u8 = Readable::read(reader)?; - if min_ver > SERIALIZATION_VERSION { - return Err(DecodeError::UnknownVersion); - } + let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION); let user_id = Readable::read(reader)?; let config: ChannelConfig = Readable::read(reader)?; @@ -4739,6 +4737,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel let channel_update_status = Readable::read(reader)?; + read_tlv_fields!(reader, {}, {}); + let mut secp_ctx = Secp256k1::new(); secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes()); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0662264e..402398d5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4568,8 +4568,7 @@ impl Writeable f fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { let _consistency_lock = self.total_consistency_lock.write().unwrap(); - writer.write_all(&[SERIALIZATION_VERSION; 1])?; - writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?; + write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION); self.genesis_hash.write(writer)?; { @@ -4652,6 +4651,8 @@ impl Writeable f session_priv.write(writer)?; } + write_tlv_fields!(writer, {}); + Ok(()) } } @@ -4775,11 +4776,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> L::Target: Logger, { fn read(reader: &mut R, mut args: ChannelManagerReadArgs<'a, Signer, M, T, K, F, L>) -> Result { - let _ver: u8 = Readable::read(reader)?; - let min_ver: u8 = Readable::read(reader)?; - if min_ver > SERIALIZATION_VERSION { - return Err(DecodeError::UnknownVersion); - } + let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION); let genesis_hash: BlockHash = Readable::read(reader)?; let best_block_height: u32 = Readable::read(reader)?; @@ -4899,6 +4896,8 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> } } + read_tlv_fields!(reader, {}, {}); + let mut secp_ctx = Secp256k1::new(); secp_ctx.seeded_randomize(&args.keys_manager.get_secure_random_bytes()); diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index a51f2651..f18f2d40 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -308,8 +308,13 @@ pub struct OnchainTxHandler { secp_ctx: Secp256k1, } +const SERIALIZATION_VERSION: u8 = 1; +const MIN_SERIALIZATION_VERSION: u8 = 1; + impl OnchainTxHandler { pub(crate) fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { + write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION); + self.destination_script.write(writer)?; self.holder_commitment.write(writer)?; self.holder_htlc_sigs.write(writer)?; @@ -355,12 +360,16 @@ impl OnchainTxHandler { } } self.latest_height.write(writer)?; + + write_tlv_fields!(writer, {}); Ok(()) } } impl<'a, K: KeysInterface> ReadableArgs<&'a K> for OnchainTxHandler { fn read(reader: &mut R, keys_manager: &'a K) -> Result { + let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION); + let destination_script = Readable::read(reader)?; let holder_commitment = Readable::read(reader)?; @@ -421,6 +430,8 @@ impl<'a, K: KeysInterface> ReadableArgs<&'a K> for OnchainTxHandler { } let latest_height = Readable::read(reader)?; + read_tlv_fields!(reader, {}, {}); + let mut secp_ctx = Secp256k1::new(); secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes()); diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 5062bae7..1e81dd65 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -9,6 +9,7 @@ macro_rules! encode_tlv { ($stream: expr, {$(($type: expr, $field: expr)),*}) => { { + #[allow(unused_imports)] use util::ser::{BigSize, LengthCalculatingWriter}; $( BigSize($type).write($stream)?; @@ -23,6 +24,7 @@ macro_rules! encode_tlv { macro_rules! encode_varint_length_prefixed_tlv { ($stream: expr, {$(($type: expr, $field: expr)),*}) => { { use util::ser::{BigSize, LengthCalculatingWriter}; + #[allow(unused_mut)] let mut len = LengthCalculatingWriter(0); { $( @@ -178,6 +180,64 @@ macro_rules! impl_writeable_len_match { } } +/// Write out two bytes to indicate the version of an object. +/// $this_version represents a unique version of a type. Incremented whenever the type's +/// serialization format has changed or has a new interpretation. Used by a type's +/// reader to determine how to interpret fields or if it can understand a serialized +/// object. +/// $min_version_that_can_read_this is the minimum reader version which can understand this +/// serialized object. Previous versions will simply err with a +/// DecodeError::UnknownVersion. +/// +/// Updates to either $this_version or $min_version_that_can_read_this should be included in +/// release notes. +/// +/// Both version fields can be specific to this type of object. +macro_rules! write_ver_prefix { + ($stream: expr, $this_version: expr, $min_version_that_can_read_this: expr) => { + $stream.write_all(&[$this_version; 1])?; + $stream.write_all(&[$min_version_that_can_read_this; 1])?; + } +} + +/// Writes out a suffix to an object which contains potentially backwards-compatible, optional +/// fields which old nodes can happily ignore. +/// +/// It is written out in TLV format and, as with all TLV fields, unknown even fields cause a +/// DecodeError::UnknownRequiredFeature error, with unknown odd fields ignored. +/// +/// This is the preferred method of adding new fields that old nodes can ignore and still function +/// correctly. +macro_rules! write_tlv_fields { + ($stream: expr, {$(($type: expr, $field: expr)),*}) => { + encode_varint_length_prefixed_tlv!($stream, {$(($type, $field)),*}); + } +} + +/// Reads a prefix added by write_ver_prefix!(), above. Takes the current version of the +/// serialization logic for this object. This is compared against the +/// $min_version_that_can_read_this added by write_ver_prefix!(). +macro_rules! read_ver_prefix { + ($stream: expr, $this_version: expr) => { { + let ver: u8 = Readable::read($stream)?; + let min_ver: u8 = Readable::read($stream)?; + if min_ver > $this_version { + return Err(DecodeError::UnknownVersion); + } + ver + } } +} + +/// Reads a suffix added by write_tlv_fields. +macro_rules! read_tlv_fields { + ($stream: expr, {$(($reqtype: expr, $reqfield: ident)),*}, {$(($type: expr, $field: ident)),*}) => { { + let tlv_len = ::util::ser::BigSize::read($stream)?; + let mut rd = ::util::ser::FixedLengthReader::new($stream, tlv_len.0); + decode_tlv!(&mut rd, {$(($reqtype, $reqfield)),*}, {$(($type, $field)),*}); + rd.eat_remaining().map_err(|_| DecodeError::ShortRead)?; + } } +} + #[cfg(test)] mod tests { use std::io::{Cursor, Read}; -- 2.30.2