Make impl_writeable_tlv_based_enum* actually upgradable

In cc78b77c71 it was discovered that
`impl_writeable_tlv_based_enum_upgradable` wasn't actually
upgradable - tuple variants weren't written with length-prefixes,
causing downgrades with new tuple variants to be unreadable by
older clients as they wouldn't know where to stop reading.

This was fixed by simply assuming that any new variants will be
non-tuple variants with a length prefix, but no code write-side
changes were made, allowing new code to freely continue to use the
broken tuple-variant serialization.

Here we address this be defining yet more serialization macros
which aren't broken, and convert existing usage of the existing
macros using non-length-prefixed tuple variants to renamed
`*_legacy` macros.

Note that this changes the serialization format of
`impl_writeable_tlv_based_enum[_upgradable]` when tuple fields are
written, and as such deliberately changes the call semantics for
such tuples.

Only the serialization format of `MessageContext` is changed here
which is fine as it has not yet reached a release of LDK.
This commit is contained in:
Matt Corallo 2024-07-08 19:21:19 +00:00
parent 6035c83a1d
commit 72f883e0a1
15 changed files with 223 additions and 53 deletions

View file

@ -119,9 +119,9 @@ pub enum OffersContext {
},
}
impl_writeable_tlv_based_enum!(MessageContext, ;
(0, Offers),
(1, Custom),
impl_writeable_tlv_based_enum!(MessageContext,
{0, Offers} => (),
{1, Custom} => (),
);
impl_writeable_tlv_based_enum!(OffersContext,
@ -129,7 +129,7 @@ impl_writeable_tlv_based_enum!(OffersContext,
(1, OutboundPayment) => {
(0, payment_id, required),
},
;);
);
/// Construct blinded onion message hops for the given `intermediate_nodes` and `recipient_node_id`.
pub(super) fn blinded_hops<T: secp256k1::Signing + secp256k1::Verification>(

View file

@ -431,7 +431,7 @@ impl Readable for PaymentConstraints {
}
}
impl_writeable_tlv_based_enum!(PaymentContext,
impl_writeable_tlv_based_enum_legacy!(PaymentContext,
;
(0, Unknown),
(1, Bolt12Offer),

View file

@ -189,7 +189,7 @@ pub enum MonitorEvent {
monitor_update_id: u64,
},
}
impl_writeable_tlv_based_enum_upgradable!(MonitorEvent,
impl_writeable_tlv_based_enum_upgradable_legacy!(MonitorEvent,
// Note that Completed is currently never serialized to disk as it is generated only in
// ChainMonitor.
(0, Completed) => {

View file

@ -689,7 +689,7 @@ impl PackageSolvingData {
}
}
impl_writeable_tlv_based_enum!(PackageSolvingData, ;
impl_writeable_tlv_based_enum_legacy!(PackageSolvingData, ;
(0, RevokedOutput),
(1, RevokedHTLCOutput),
(2, CounterpartyOfferedHTLCOutput),

View file

@ -171,7 +171,7 @@ impl PaymentPurpose {
}
}
impl_writeable_tlv_based_enum!(PaymentPurpose,
impl_writeable_tlv_based_enum_legacy!(PaymentPurpose,
(0, Bolt11InvoicePayment) => {
(0, payment_preimage, option),
(2, payment_secret, required),
@ -494,7 +494,7 @@ enum InterceptNextHop {
impl_writeable_tlv_based_enum!(InterceptNextHop,
(0, FakeScid) => {
(0, requested_next_hop_scid, required),
};
},
);
/// The reason the payment failed. Used in [`Event::PaymentFailed`].
@ -535,7 +535,7 @@ impl_writeable_tlv_based_enum!(PaymentFailureReason,
(4, RetriesExhausted) => {},
(6, PaymentExpired) => {},
(8, RouteNotFound) => {},
(10, UnexpectedError) => {}, ;
(10, UnexpectedError) => {},
);
/// An Event which you should probably take some action in response to.

View file

@ -130,7 +130,7 @@ impl_writeable_tlv_based_enum!(InboundHTLCResolution,
},
(2, Pending) => {
(0, update_add_htlc, required),
};
},
);
enum InboundHTLCState {
@ -8390,7 +8390,7 @@ fn get_initial_channel_type(config: &UserConfig, their_features: &InitFeatures)
const SERIALIZATION_VERSION: u8 = 4;
const MIN_SERIALIZATION_VERSION: u8 = 3;
impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
impl_writeable_tlv_based_enum_legacy!(InboundHTLCRemovalReason,;
(0, FailRelay),
(1, FailMalformed),
(2, Fulfill),

View file

@ -69,7 +69,7 @@ impl_writeable_tlv_based_enum_upgradable!(InboundHTLCStateDetails,
(0, AwaitingRemoteRevokeToAdd) => {},
(2, Committed) => {},
(4, AwaitingRemoteRevokeToRemoveFulfill) => {},
(6, AwaitingRemoteRevokeToRemoveFail) => {};
(6, AwaitingRemoteRevokeToRemoveFail) => {},
);
/// Exposes details around pending inbound HTLCs.
@ -159,7 +159,7 @@ impl_writeable_tlv_based_enum_upgradable!(OutboundHTLCStateDetails,
(0, AwaitingRemoteRevokeToAdd) => {},
(2, Committed) => {},
(4, AwaitingRemoteRevokeToRemoveSuccess) => {},
(6, AwaitingRemoteRevokeToRemoveFailure) => {};
(6, AwaitingRemoteRevokeToRemoveFailure) => {},
);
/// Exposes details around pending outbound HTLCs.
@ -701,5 +701,5 @@ impl_writeable_tlv_based_enum!(ChannelShutdownState,
(2, ShutdownInitiated) => {},
(4, ResolvingHTLCs) => {},
(6, NegotiatingClosingFee) => {},
(8, ShutdownComplete) => {}, ;
(8, ShutdownComplete) => {},
);

View file

@ -476,7 +476,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,
},
(2, OutboundRoute) => {
(0, session_priv, required),
};
},
);
@ -881,7 +881,7 @@ impl_writeable_tlv_based_enum!(EventCompletionAction,
// Note that by the time we get past the required read above, channel_funding_outpoint will be
// filled in, so we can safely unwrap it here.
(3, channel_id, (default_value, ChannelId::v1_from_funding_outpoint(channel_funding_outpoint.0.unwrap()))),
};
}
);
#[derive(Debug)]
@ -10808,7 +10808,7 @@ impl_writeable_tlv_based_enum!(PendingHTLCRouting,
(4, payment_data, option), // Added in 0.0.116
(5, custom_tlvs, optional_vec),
},
;);
);
impl_writeable_tlv_based!(PendingHTLCInfo, {
(0, routing, required),
@ -10888,14 +10888,14 @@ impl Readable for HTLCFailureMsg {
}
}
impl_writeable_tlv_based_enum!(PendingHTLCStatus, ;
impl_writeable_tlv_based_enum_legacy!(PendingHTLCStatus, ;
(0, Forward),
(1, Fail),
);
impl_writeable_tlv_based_enum!(BlindedFailure,
(0, FromIntroductionNode) => {},
(2, FromBlindedNode) => {}, ;
(2, FromBlindedNode) => {},
);
impl_writeable_tlv_based!(HTLCPreviousHopData, {

View file

@ -953,7 +953,7 @@ impl_writeable_tlv_based_enum!(HTLCFailReasonRepr,
(0, failure_code, required),
(2, data, required_vec),
},
;);
);
impl HTLCFailReason {
#[rustfmt::skip]

View file

@ -300,13 +300,13 @@ pub enum Retry {
}
#[cfg(not(feature = "std"))]
impl_writeable_tlv_based_enum!(Retry,
impl_writeable_tlv_based_enum_legacy!(Retry,
;
(0, Attempts)
);
#[cfg(feature = "std")]
impl_writeable_tlv_based_enum!(Retry,
impl_writeable_tlv_based_enum_legacy!(Retry,
;
(0, Attempts),
(2, Timeout)
@ -397,7 +397,7 @@ pub(crate) enum StaleExpiration {
AbsoluteTimeout(core::time::Duration),
}
impl_writeable_tlv_based_enum!(StaleExpiration,
impl_writeable_tlv_based_enum_legacy!(StaleExpiration,
;
(0, TimerTicks),
(2, AbsoluteTimeout)

View file

@ -53,7 +53,7 @@ impl Readable for ShutdownScript {
}
}
impl_writeable_tlv_based_enum!(ShutdownScriptImpl, ;
impl_writeable_tlv_based_enum_legacy!(ShutdownScriptImpl, ;
(0, Legacy),
(1, Bolt2),
);

View file

@ -301,7 +301,7 @@ pub enum SpendableOutputDescriptor {
StaticPaymentOutput(StaticPaymentOutputDescriptor),
}
impl_writeable_tlv_based_enum!(SpendableOutputDescriptor,
impl_writeable_tlv_based_enum_legacy!(SpendableOutputDescriptor,
(0, StaticOutput) => {
(0, outpoint, required),
(1, channel_keys_id, option),

View file

@ -410,7 +410,7 @@ pub enum MaxDustHTLCExposure {
FeeRateMultiplier(u64),
}
impl_writeable_tlv_based_enum!(MaxDustHTLCExposure, ;
impl_writeable_tlv_based_enum_legacy!(MaxDustHTLCExposure, ;
(1, FixedLimitMsat),
(3, FeeRateMultiplier),
);

View file

@ -996,8 +996,11 @@ macro_rules! tlv_record_ref_type {
macro_rules! _impl_writeable_tlv_based_enum_common {
($st: ident, $(($variant_id: expr, $variant_name: ident) =>
{$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
),* $(,)*;
$(($tuple_variant_id: expr, $tuple_variant_name: ident)),* $(,)*) => {
),* $(,)?;
// $tuple_variant_* are only passed from `impl_writeable_tlv_based_enum_*_legacy`
$(($tuple_variant_id: expr, $tuple_variant_name: ident)),* $(,)?;
// $length_prefixed_* are only passed from `impl_writeable_tlv_based_enum_*` non-`legacy`
$(($length_prefixed_tuple_variant_id: expr, $length_prefixed_tuple_variant_name: ident)),* $(,)?) => {
impl $crate::util::ser::Writeable for $st {
fn write<W: $crate::util::ser::Writer>(&self, writer: &mut W) -> Result<(), $crate::io::Error> {
match self {
@ -1013,6 +1016,12 @@ macro_rules! _impl_writeable_tlv_based_enum_common {
id.write(writer)?;
field.write(writer)?;
}),*
$($st::$length_prefixed_tuple_variant_name (ref field) => {
let id: u8 = $length_prefixed_tuple_variant_id;
id.write(writer)?;
$crate::util::ser::BigSize(field.serialized_length() as u64).write(writer)?;
field.write(writer)?;
}),*
}
Ok(())
}
@ -1022,29 +1031,104 @@ macro_rules! _impl_writeable_tlv_based_enum_common {
/// Implement [`Readable`] and [`Writeable`] for an enum, with struct variants stored as TLVs and tuple
/// variants stored directly.
/// The format is, for example
/// ```ignore
///
/// The format is, for example,
/// ```
/// enum EnumName {
/// StructVariantA {
/// required_variant_field: u64,
/// optional_variant_field: Option<u8>,
/// },
/// StructVariantB {
/// variant_field_a: bool,
/// variant_field_b: u32,
/// variant_vec_field: Vec<u32>,
/// },
/// TupleVariantA(),
/// TupleVariantB(Vec<u8>),
/// }
/// # use lightning::impl_writeable_tlv_based_enum;
/// impl_writeable_tlv_based_enum!(EnumName,
/// (0, StructVariantA) => {(0, required_variant_field, required), (1, optional_variant_field, option)},
/// (1, StructVariantB) => {(0, variant_field_a, required), (1, variant_field_b, required), (2, variant_vec_field, optional_vec)};
/// (2, TupleVariantA), (3, TupleVariantB),
/// (1, StructVariantB) => {(0, variant_field_a, required), (1, variant_field_b, required), (2, variant_vec_field, optional_vec)},
/// (2, TupleVariantA) => {}, // Note that empty tuple variants have to use the struct syntax due to rust limitations
/// {3, TupleVariantB} => (),
/// );
/// ```
/// The type is written as a single byte, followed by any variant data.
///
/// The type is written as a single byte, followed by length-prefixed variant data.
///
/// Attempts to read an unknown type byte result in [`DecodeError::UnknownRequiredFeature`].
///
/// Note that the serialization for tuple variants (as well as the call format) was changed in LDK
/// 0.0.124.
///
/// [`Readable`]: crate::util::ser::Readable
/// [`Writeable`]: crate::util::ser::Writeable
/// [`DecodeError::UnknownRequiredFeature`]: crate::ln::msgs::DecodeError::UnknownRequiredFeature
#[macro_export]
macro_rules! impl_writeable_tlv_based_enum {
($st: ident,
$(($variant_id: expr, $variant_name: ident) =>
{$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
),*
$($(,)? {$tuple_variant_id: expr, $tuple_variant_name: ident} => ()),*
$(,)?
) => {
$crate::_impl_writeable_tlv_based_enum_common!($st,
$(($variant_id, $variant_name) => {$(($type, $field, $fieldty)),*}),*
;;
$(($tuple_variant_id, $tuple_variant_name)),*);
impl $crate::util::ser::Readable for $st {
#[allow(unused_mut)]
fn read<R: $crate::io::Read>(mut reader: &mut R) -> Result<Self, $crate::ln::msgs::DecodeError> {
let id: u8 = $crate::util::ser::Readable::read(reader)?;
match id {
$($variant_id => {
// Because read_tlv_fields creates a labeled loop, we cannot call it twice
// in the same function body. Instead, we define a closure and call it.
let mut f = || {
$crate::_init_and_read_len_prefixed_tlv_fields!(reader, {
$(($type, $field, $fieldty)),*
});
Ok($st::$variant_name {
$(
$field: $crate::_init_tlv_based_struct_field!($field, $fieldty)
),*
})
};
f()
}),*
$($tuple_variant_id => {
let length: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?;
let mut s = $crate::util::ser::FixedLengthReader::new(&mut reader, length.0);
let res = $crate::util::ser::Readable::read(&mut s)?;
if s.bytes_remain() {
s.eat_remaining()?; // Return ShortRead if there's actually not enough bytes
return Err($crate::ln::msgs::DecodeError::InvalidValue);
}
Ok($st::$tuple_variant_name(res))
}),*
_ => {
Err($crate::ln::msgs::DecodeError::UnknownRequiredFeature)
},
}
}
}
}
}
/// See [`impl_writeable_tlv_based_enum`] and use that unless backwards-compatibility with tuple
/// variants is required.
macro_rules! impl_writeable_tlv_based_enum_legacy {
($st: ident, $(($variant_id: expr, $variant_name: ident) =>
{$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
),* $(,)*;
$(($tuple_variant_id: expr, $tuple_variant_name: ident)),* $(,)*) => {
$(($tuple_variant_id: expr, $tuple_variant_name: ident)),+ $(,)?) => {
$crate::_impl_writeable_tlv_based_enum_common!($st,
$(($variant_id, $variant_name) => {$(($type, $field, $fieldty)),*}),*;
$(($tuple_variant_id, $tuple_variant_name)),*);
$(($tuple_variant_id, $tuple_variant_name)),+;);
impl $crate::util::ser::Readable for $st {
fn read<R: $crate::io::Read>(reader: &mut R) -> Result<Self, $crate::ln::msgs::DecodeError> {
@ -1067,7 +1151,7 @@ macro_rules! impl_writeable_tlv_based_enum {
}),*
$($tuple_variant_id => {
Ok($st::$tuple_variant_name($crate::util::ser::Readable::read(reader)?))
}),*
}),+
_ => {
Err($crate::ln::msgs::DecodeError::UnknownRequiredFeature)
},
@ -1085,9 +1169,8 @@ macro_rules! impl_writeable_tlv_based_enum {
/// when [`MaybeReadable`] is practical instead of just [`Readable`] as it provides an upgrade path for
/// new variants to be added which are simply ignored by existing clients.
///
/// Note that only struct and unit variants (not tuple variants) will support downgrading, thus any
/// new odd variants MUST be non-tuple (i.e. described using `$variant_id` and `$variant_name` not
/// `$tuple_variant_id` and `$tuple_variant_name`).
/// Note that the serialization for tuple variants (as well as the call format) was changed in LDK
/// 0.0.124.
///
/// [`MaybeReadable`]: crate::util::ser::MaybeReadable
/// [`Writeable`]: crate::util::ser::Writeable
@ -1095,16 +1178,76 @@ macro_rules! impl_writeable_tlv_based_enum {
/// [`Readable`]: crate::util::ser::Readable
#[macro_export]
macro_rules! impl_writeable_tlv_based_enum_upgradable {
($st: ident, $(($variant_id: expr, $variant_name: ident) =>
{$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
),* $(,)*
$(;
$(($tuple_variant_id: expr, $tuple_variant_name: ident)),* $(,)*)?
$(unread_variants: $($unread_variant: ident),*)?) => {
($st: ident,
$(($variant_id: expr, $variant_name: ident) =>
{$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
),*
$(, {$tuple_variant_id: expr, $tuple_variant_name: ident} => ())*
$(, unread_variants: $($unread_variant: ident),*)?
$(,)?
) => {
$crate::_impl_writeable_tlv_based_enum_common!($st,
$(($variant_id, $variant_name) => {$(($type, $field, $fieldty)),*}),*
$(, $((255, $unread_variant) => {}),*)?
; $($(($tuple_variant_id, $tuple_variant_name)),*)?);
;;
$(($tuple_variant_id, $tuple_variant_name)),*);
impl $crate::util::ser::MaybeReadable for $st {
#[allow(unused_mut)]
fn read<R: $crate::io::Read>(mut reader: &mut R) -> Result<Option<Self>, $crate::ln::msgs::DecodeError> {
let id: u8 = $crate::util::ser::Readable::read(reader)?;
match id {
$($variant_id => {
// Because read_tlv_fields creates a labeled loop, we cannot call it twice
// in the same function body. Instead, we define a closure and call it.
let mut f = || {
$crate::_init_and_read_len_prefixed_tlv_fields!(reader, {
$(($type, $field, $fieldty)),*
});
Ok(Some($st::$variant_name {
$(
$field: $crate::_init_tlv_based_struct_field!($field, $fieldty)
),*
}))
};
f()
}),*
$($tuple_variant_id => {
let length: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?;
let mut s = $crate::util::ser::FixedLengthReader::new(&mut reader, length.0);
let res = $crate::util::ser::Readable::read(&mut s)?;
if s.bytes_remain() {
s.eat_remaining()?; // Return ShortRead if there's actually not enough bytes
return Err($crate::ln::msgs::DecodeError::InvalidValue);
}
Ok(Some($st::$tuple_variant_name(res)))
}),*
// Note that we explicitly match 255 here to reserve it for use in
// `unread_variants`.
255|_ if id % 2 == 1 => {
let tlv_len: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?;
let mut rd = $crate::util::ser::FixedLengthReader::new(reader, tlv_len.0);
rd.eat_remaining().map_err(|_| $crate::ln::msgs::DecodeError::ShortRead)?;
Ok(None)
},
_ => Err($crate::ln::msgs::DecodeError::UnknownRequiredFeature),
}
}
}
}
}
/// See [`impl_writeable_tlv_based_enum_upgradable`] and use that unless backwards-compatibility
/// with tuple variants is required.
macro_rules! impl_writeable_tlv_based_enum_upgradable_legacy {
($st: ident, $(($variant_id: expr, $variant_name: ident) =>
{$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
),* $(,)?
;
$(($tuple_variant_id: expr, $tuple_variant_name: ident)),+ $(,)?) => {
$crate::_impl_writeable_tlv_based_enum_common!($st,
$(($variant_id, $variant_name) => {$(($type, $field, $fieldty)),*}),*;
$(($tuple_variant_id, $tuple_variant_name)),+;);
impl $crate::util::ser::MaybeReadable for $st {
fn read<R: $crate::io::Read>(reader: &mut R) -> Result<Option<Self>, $crate::ln::msgs::DecodeError> {
@ -1125,12 +1268,10 @@ macro_rules! impl_writeable_tlv_based_enum_upgradable {
};
f()
}),*
$($($tuple_variant_id => {
$($tuple_variant_id => {
Ok(Some($st::$tuple_variant_name(Readable::read(reader)?)))
}),*)*
// Note that we explicitly match 255 here to reserve it for use in
// `unread_variants`.
255|_ if id % 2 == 1 => {
}),+
_ if id % 2 == 1 => {
// Assume that a $variant_id was written, not a $tuple_variant_id, and read
// the length prefix and discard the correct number of bytes.
let tlv_len: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?;
@ -1549,4 +1690,33 @@ mod tests {
let decoded_msg: EmptyMsg = Readable::read(&mut encoded_msg_stream).unwrap();
assert_eq!(msg, decoded_msg);
}
#[derive(Debug, PartialEq, Eq)]
enum TuplesOnly {
A(),
B(u64),
}
impl_writeable_tlv_based_enum_upgradable!(TuplesOnly, (2, A) => {}, {3, B} => ());
#[test]
fn test_impl_writeable_enum() {
let a = TuplesOnly::A().encode();
assert_eq!(TuplesOnly::read(&mut Cursor::new(&a)).unwrap(), Some(TuplesOnly::A()));
let b42 = TuplesOnly::B(42).encode();
assert_eq!(TuplesOnly::read(&mut Cursor::new(&b42)).unwrap(), Some(TuplesOnly::B(42)));
// Test unknown variants with 0-length data
let unknown_variant = vec![41, 0];
let mut none_read = Cursor::new(&unknown_variant);
assert_eq!(TuplesOnly::read(&mut none_read).unwrap(), None);
assert_eq!(none_read.position(), unknown_variant.len() as u64);
TuplesOnly::read(&mut Cursor::new(&vec![42, 0])).unwrap_err();
// Test unknown variants with data
let unknown_data_variant = vec![41, 3, 42, 52, 62];
let mut none_data_read = Cursor::new(&unknown_data_variant);
assert_eq!(TuplesOnly::read(&mut none_data_read).unwrap(), None);
assert_eq!(none_data_read.position(), unknown_data_variant.len() as u64);
}
}

View file

@ -315,7 +315,7 @@ impl_writeable_tlv_based_enum!(OutputSpendStatus,
(4, latest_spending_tx, required),
(6, confirmation_height, required),
(8, confirmation_hash, required),
};
},
);
/// A utility that keeps track of [`SpendableOutputDescriptor`]s, persists them in a given