From 5a6eea0644362cbf4396815bfe5767f33a134916 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 8526ce517..bbb55fc22 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 b940b45f7..7da38b7ae 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4395,8 +4395,7 @@ impl Writeable for Channel { // Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been // called but include holding cell updates (and obviously we don't modify self). - 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)?; @@ -4585,6 +4584,9 @@ impl Writeable for Channel { self.commitment_secrets.write(writer)?; self.channel_update_status.write(writer)?; + + write_tlv_fields!(writer, {}); + Ok(()) } } @@ -4593,11 +4595,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)?; @@ -4759,6 +4757,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 fe3be2aec..4919a225a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4431,8 +4431,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)?; { @@ -4515,6 +4514,8 @@ impl Writeable f session_priv.write(writer)?; } + write_tlv_fields!(writer, {}); + Ok(()) } } @@ -4638,11 +4639,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)?; @@ -4762,6 +4759,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 a51f26513..f18f2d406 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 5062bae76..f510e0f17 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 this object. +/// $this_version represents a unique version of this object, incremented whenever the reader may +/// wish to interpret fields differently based on the version or when we wish to +/// communicate to previous versions that they can no longer reasonably understand +/// this serialized object. +/// $min_version_that_can_read_this is the minimum client version which can reasonably 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 which 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.39.5