Skip to content

Commit a515eb3

Browse files
TheBlueMattTheBlueMatt
committed
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.
1 parent f8450a7 commit a515eb3

File tree

5 files changed

+94
-27
lines changed

5 files changed

+94
-27
lines changed

‎lightning/src/chain/channelmonitor.rs

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -502,9 +502,6 @@ enum OnchainEvent {
502502
},
503503
}
504504

505-
const SERIALIZATION_VERSION: u8 = 1;
506-
const MIN_SERIALIZATION_VERSION: u8 = 1;
507-
508505
#[cfg_attr(any(test, feature = "fuzztarget", feature = "_test_utils"), derive(PartialEq))]
509506
#[derive(Clone)]
510507
pub(crate) enum ChannelMonitorUpdateStep {
@@ -805,17 +802,17 @@ impl<Signer: Sign> PartialEq for ChannelMonitorImpl<Signer> {
805802

806803
impl<Signer: Sign> Writeable for ChannelMonitor<Signer> {
807804
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), Error> {
808-
//TODO: We still write out all the serialization here manually instead of using the fancy
809-
//serialization framework we have, we should migrate things over to it.
810-
writer.write_all(&[SERIALIZATION_VERSION; 1])?;
811-
writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;
812-
813805
self.inner.lock().unwrap().write(writer)
814806
}
815807
}
816808

809+
const SERIALIZATION_VERSION: u8 = 1;
810+
const MIN_SERIALIZATION_VERSION: u8 = 1;
811+
817812
impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
818813
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), Error> {
814+
write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
815+
819816
self.latest_update_id.write(writer)?;
820817

821818
// Set in initial Channel-object creation, so should always be set by now:
@@ -991,6 +988,8 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
991988
self.lockdown_from_offchain.write(writer)?;
992989
self.holder_tx_signed.write(writer)?;
993990

991+
write_tlv_fields!(writer, {});
992+
994993
Ok(())
995994
}
996995
}
@@ -2754,11 +2753,7 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
27542753
}
27552754
}
27562755

2757-
let _ver: u8 = Readable::read(reader)?;
2758-
let min_ver: u8 = Readable::read(reader)?;
2759-
if min_ver > SERIALIZATION_VERSION {
2760-
return Err(DecodeError::UnknownVersion);
2761-
}
2756+
let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
27622757

27632758
let latest_update_id: u64 = Readable::read(reader)?;
27642759
let commitment_transaction_number_obscure_factor = <U48 as Readable>::read(reader)?.0;
@@ -2979,6 +2974,8 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
29792974
let lockdown_from_offchain = Readable::read(reader)?;
29802975
let holder_tx_signed = Readable::read(reader)?;
29812976

2977+
read_tlv_fields!(reader, {}, {});
2978+
29822979
let mut secp_ctx = Secp256k1::new();
29832980
secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
29842981

‎lightning/src/ln/channel.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4375,8 +4375,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
43754375
// Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been
43764376
// called.
43774377

4378-
writer.write_all(&[SERIALIZATION_VERSION; 1])?;
4379-
writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;
4378+
write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
43804379

43814380
self.user_id.write(writer)?;
43824381
self.config.write(writer)?;
@@ -4565,6 +4564,9 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
45654564
self.commitment_secrets.write(writer)?;
45664565

45674566
self.channel_update_status.write(writer)?;
4567+
4568+
write_tlv_fields!(writer, {});
4569+
45684570
Ok(())
45694571
}
45704572
}
@@ -4573,11 +4575,7 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
45734575
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
45744576
where K::Target: KeysInterface<Signer = Signer> {
45754577
fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
4576-
let _ver: u8 = Readable::read(reader)?;
4577-
let min_ver: u8 = Readable::read(reader)?;
4578-
if min_ver > SERIALIZATION_VERSION {
4579-
return Err(DecodeError::UnknownVersion);
4580-
}
4578+
let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
45814579

45824580
let user_id = Readable::read(reader)?;
45834581
let config: ChannelConfig = Readable::read(reader)?;
@@ -4739,6 +4737,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
47394737

47404738
let channel_update_status = Readable::read(reader)?;
47414739

4740+
read_tlv_fields!(reader, {}, {});
4741+
47424742
let mut secp_ctx = Secp256k1::new();
47434743
secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes());
47444744

‎lightning/src/ln/channelmanager.rs

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4568,8 +4568,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
45684568
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
45694569
let _consistency_lock = self.total_consistency_lock.write().unwrap();
45704570

4571-
writer.write_all(&[SERIALIZATION_VERSION; 1])?;
4572-
writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;
4571+
write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
45734572

45744573
self.genesis_hash.write(writer)?;
45754574
{
@@ -4652,6 +4651,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
46524651
session_priv.write(writer)?;
46534652
}
46544653

4654+
write_tlv_fields!(writer, {});
4655+
46554656
Ok(())
46564657
}
46574658
}
@@ -4775,11 +4776,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
47754776
L::Target: Logger,
47764777
{
47774778
fn read<R: ::std::io::Read>(reader: &mut R, mut args: ChannelManagerReadArgs<'a, Signer, M, T, K, F, L>) -> Result<Self, DecodeError> {
4778-
let _ver: u8 = Readable::read(reader)?;
4779-
let min_ver: u8 = Readable::read(reader)?;
4780-
if min_ver > SERIALIZATION_VERSION {
4781-
return Err(DecodeError::UnknownVersion);
4782-
}
4779+
let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
47834780

47844781
let genesis_hash: BlockHash = Readable::read(reader)?;
47854782
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>
48994896
}
49004897
}
49014898

4899+
read_tlv_fields!(reader, {}, {});
4900+
49024901
let mut secp_ctx = Secp256k1::new();
49034902
secp_ctx.seeded_randomize(&args.keys_manager.get_secure_random_bytes());
49044903

‎lightning/src/ln/onchaintx.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,8 +308,13 @@ pub struct OnchainTxHandler<ChannelSigner: Sign> {
308308
secp_ctx: Secp256k1<secp256k1::All>,
309309
}
310310

311+
const SERIALIZATION_VERSION: u8 = 1;
312+
const MIN_SERIALIZATION_VERSION: u8 = 1;
313+
311314
impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
312315
pub(crate) fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
316+
write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
317+
313318
self.destination_script.write(writer)?;
314319
self.holder_commitment.write(writer)?;
315320
self.holder_htlc_sigs.write(writer)?;
@@ -355,12 +360,16 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
355360
}
356361
}
357362
self.latest_height.write(writer)?;
363+
364+
write_tlv_fields!(writer, {});
358365
Ok(())
359366
}
360367
}
361368

362369
impl<'a, K: KeysInterface> ReadableArgs<&'a K> for OnchainTxHandler<K::Signer> {
363370
fn read<R: ::std::io::Read>(reader: &mut R, keys_manager: &'a K) -> Result<Self, DecodeError> {
371+
let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
372+
364373
let destination_script = Readable::read(reader)?;
365374

366375
let holder_commitment = Readable::read(reader)?;
@@ -421,6 +430,8 @@ impl<'a, K: KeysInterface> ReadableArgs<&'a K> for OnchainTxHandler<K::Signer> {
421430
}
422431
let latest_height = Readable::read(reader)?;
423432

433+
read_tlv_fields!(reader, {}, {});
434+
424435
let mut secp_ctx = Secp256k1::new();
425436
secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
426437

‎lightning/src/util/ser_macros.rs

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
macro_rules! encode_tlv {
1111
($stream: expr, {$(($type: expr, $field: expr)),*}) => { {
12+
#[allow(unused_imports)]
1213
use util::ser::{BigSize, LengthCalculatingWriter};
1314
$(
1415
BigSize($type).write($stream)?;
@@ -23,6 +24,7 @@ macro_rules! encode_tlv {
2324
macro_rules! encode_varint_length_prefixed_tlv {
2425
($stream: expr, {$(($type: expr, $field: expr)),*}) => { {
2526
use util::ser::{BigSize, LengthCalculatingWriter};
27+
#[allow(unused_mut)]
2628
let mut len = LengthCalculatingWriter(0);
2729
{
2830
$(
@@ -178,6 +180,64 @@ macro_rules! impl_writeable_len_match {
178180
}
179181
}
180182

183+
/// Write out two bytes to indicate the version of an object.
184+
/// $this_version represents a unique version of a type. Incremented whenever the type's
185+
/// serialization format has changed or has a new interpretation. Used by a type's
186+
/// reader to determine how to interpret fields or if it can understand a serialized
187+
/// object.
188+
/// $min_version_that_can_read_this is the minimum reader version which can understand this
189+
/// serialized object. Previous versions will simply err with a
190+
/// DecodeError::UnknownVersion.
191+
///
192+
/// Updates to either $this_version or $min_version_that_can_read_this should be included in
193+
/// release notes.
194+
///
195+
/// Both version fields can be specific to this type of object.
196+
macro_rules! write_ver_prefix {
197+
($stream: expr, $this_version: expr, $min_version_that_can_read_this: expr) => {
198+
$stream.write_all(&[$this_version; 1])?;
199+
$stream.write_all(&[$min_version_that_can_read_this; 1])?;
200+
}
201+
}
202+
203+
/// Writes out a suffix to an object which contains potentially backwards-compatible, optional
204+
/// fields which old nodes can happily ignore.
205+
///
206+
/// It is written out in TLV format and, as with all TLV fields, unknown even fields cause a
207+
/// DecodeError::UnknownRequiredFeature error, with unknown odd fields ignored.
208+
///
209+
/// This is the preferred method of adding new fields that old nodes can ignore and still function
210+
/// correctly.
211+
macro_rules! write_tlv_fields {
212+
($stream: expr, {$(($type: expr, $field: expr)),*}) => {
213+
encode_varint_length_prefixed_tlv!($stream, {$(($type, $field)),*});
214+
}
215+
}
216+
217+
/// Reads a prefix added by write_ver_prefix!(), above. Takes the current version of the
218+
/// serialization logic for this object. This is compared against the
219+
/// $min_version_that_can_read_this added by write_ver_prefix!().
220+
macro_rules! read_ver_prefix {
221+
($stream: expr, $this_version: expr) => { {
222+
let ver: u8 = Readable::read($stream)?;
223+
let min_ver: u8 = Readable::read($stream)?;
224+
if min_ver > $this_version {
225+
return Err(DecodeError::UnknownVersion);
226+
}
227+
ver
228+
} }
229+
}
230+
231+
/// Reads a suffix added by write_tlv_fields.
232+
macro_rules! read_tlv_fields {
233+
($stream: expr, {$(($reqtype: expr, $reqfield: ident)),*}, {$(($type: expr, $field: ident)),*}) => { {
234+
let tlv_len = ::util::ser::BigSize::read($stream)?;
235+
let mut rd = ::util::ser::FixedLengthReader::new($stream, tlv_len.0);
236+
decode_tlv!(&mut rd, {$(($reqtype, $reqfield)),*}, {$(($type, $field)),*});
237+
rd.eat_remaining().map_err(|_| DecodeError::ShortRead)?;
238+
} }
239+
}
240+
181241
#[cfg(test)]
182242
mod tests {
183243
use std::io::{Cursor, Read};

0 commit comments

Comments
 (0)