From 16119967df14b47680fae4a338c79168d83d78e5 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Mon, 21 Oct 2024 23:07:26 +0000
Subject: [PATCH 1/5] Add support for mapping old fields to new ones in TLV
 read macros

As we've grown, we regularly face a question of whether to "break
out" of our nice TLV-based struct/enum reading/writing macros in
order to handle mapping legacy fields to new ones, or deal with
keeping the legacy fields and handling at runtime what should be
hanlded at (de-)serialization time.

This attempts to address this tradeoff by adding support for a
"legacy" TLV type. This TLV style allows us to write a a value
which is not in the struct/enum's layout in memory but can be
calculated from its contents at write-time. The field will also be
read and can be use to calculate other fiels defined with
`static_value` or `default_value`.

It takes a type and a `$write` expression.

They are always read as `option`s to retain a future ability to
remove the `legacy` fields.

Sadly, there's two issues with doing this trivially which force us
into `proc-macro` land:

(a) when matching the original struct we want to list the fields
    in the match arm so that we have them available to write.
    Sadly, we can't call a macro to have it write out the field
    name based on the field type, so instead need to pass the whole
    match to a proc-macro and have it walk through to find the
    types and skip fields that are `legacy`.
(b) when building a final struct/enum after reading, we need to
    list a few `$field: $expr`s and cannot decide whether to
    include a field based on a regular macro.

The proc-macros to do so aren't trivial, but they aren't that bad
either. We could instead try to rewrite our TLV stream processing
macros to handle a new set of TLVs which are passed via a separate
argument, but as TLVs are required to in ordered by type this
requires a good chunk of additional generated code in each TLV
write. It also would result in a somewhat less ergonomic callsite
as it would no longer fit into our existing list of TLVs.
---
 lightning-macros/Cargo.toml           |   2 +-
 lightning-macros/src/lib.rs           | 222 +++++++++++++++++++++++++-
 lightning-transaction-sync/Cargo.toml |   2 +-
 lightning/Cargo.toml                  |   1 +
 lightning/src/util/ser_macros.rs      | 115 +++++++------
 5 files changed, 294 insertions(+), 48 deletions(-)

diff --git a/lightning-macros/Cargo.toml b/lightning-macros/Cargo.toml
index 480d8f0f7d8..e6cd14f41bf 100644
--- a/lightning-macros/Cargo.toml
+++ b/lightning-macros/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "lightning-macros"
-version = "0.1.0"
+version = "0.2.0+git"
 authors = ["Elias Rohrer"]
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/lightningdevkit/rust-lightning/"
diff --git a/lightning-macros/src/lib.rs b/lightning-macros/src/lib.rs
index fd4707e5856..deeccf24026 100644
--- a/lightning-macros/src/lib.rs
+++ b/lightning-macros/src/lib.rs
@@ -18,7 +18,10 @@
 #![deny(rustdoc::private_intra_doc_links)]
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
 
-use proc_macro::TokenStream;
+extern crate alloc;
+
+use alloc::string::ToString;
+use proc_macro::{Delimiter, Group, TokenStream, TokenTree};
 use quote::quote;
 use syn::spanned::Spanned;
 use syn::{parse, ImplItemFn, Token};
@@ -74,3 +77,220 @@ pub fn maybe_await(expr: TokenStream) -> TokenStream {
 
 	quoted.into()
 }
+
+fn expect_ident(token: &TokenTree, expected_name: Option<&str>) {
+	if let TokenTree::Ident(id) = &token {
+		if let Some(exp) = expected_name {
+			assert_eq!(id.to_string(), exp, "Expected ident {}, got {:?}", exp, token);
+		}
+	} else {
+		panic!("Expected ident {:?}, got {:?}", expected_name, token);
+	}
+}
+
+fn expect_punct(token: &TokenTree, expected: char) {
+	if let TokenTree::Punct(p) = &token {
+		assert_eq!(p.as_char(), expected, "Expected punctuation {}, got {}", expected, p);
+	} else {
+		panic!("Expected punctuation {}, got {:?}", expected, token);
+	}
+}
+
+fn token_to_stream(token: TokenTree) -> proc_macro::TokenStream {
+	proc_macro::TokenStream::from(token)
+}
+
+/// Processes a list of fields in a variant definition (see the docs for [`skip_legacy_fields!`])
+fn process_fields(group: Group) -> proc_macro::TokenStream {
+	let mut computed_fields = proc_macro::TokenStream::new();
+	if group.delimiter() == Delimiter::Brace {
+		let mut fields_stream = group.stream().into_iter().peekable();
+
+		let mut new_fields = proc_macro::TokenStream::new();
+		loop {
+			// The field list should end with .., at which point we break
+			let next_tok = fields_stream.peek();
+			if let Some(TokenTree::Punct(_)) = next_tok {
+				let dot1 = fields_stream.next().unwrap();
+				expect_punct(&dot1, '.');
+				let dot2 = fields_stream.next().expect("Missing second trailing .");
+				expect_punct(&dot2, '.');
+				let trailing_dots = [dot1, dot2];
+				new_fields.extend(trailing_dots.into_iter().map(token_to_stream));
+				assert!(fields_stream.peek().is_none());
+				break;
+			}
+
+			// Fields should take the form `ref field_name: ty_info` where `ty_info`
+			// may be a single ident or may be a group. We skip the field if `ty_info`
+			// is a group where the first token is the ident `legacy`.
+			let ref_ident = fields_stream.next().unwrap();
+			expect_ident(&ref_ident, Some("ref"));
+			let field_name_ident = fields_stream.next().unwrap();
+			let co = fields_stream.next().unwrap();
+			expect_punct(&co, ':');
+			let ty_info = fields_stream.next().unwrap();
+			let com = fields_stream.next().unwrap();
+			expect_punct(&com, ',');
+
+			if let TokenTree::Group(group) = ty_info {
+				let first_group_tok = group.stream().into_iter().next().unwrap();
+				if let TokenTree::Ident(ident) = first_group_tok {
+					if ident.to_string() == "legacy" {
+						continue;
+					}
+				}
+			}
+
+			let field = [ref_ident, field_name_ident, com];
+			new_fields.extend(field.into_iter().map(token_to_stream));
+		}
+		let fields_group = Group::new(Delimiter::Brace, new_fields);
+		computed_fields.extend(token_to_stream(TokenTree::Group(fields_group)));
+	} else {
+		computed_fields.extend(token_to_stream(TokenTree::Group(group)));
+	}
+	computed_fields
+}
+
+/// Scans a match statement for legacy fields which should be skipped.
+///
+/// This is used internally in LDK's TLV serialization logic and is not expected to be used by
+/// other crates.
+///
+/// Wraps a `match self {..}` statement and scans the fields in the match patterns (in the form
+/// `ref $field_name: $field_ty`) for types marked `legacy`, skipping those fields.
+///
+/// Specifically, it expects input like the following, simply dropping `field3` and the
+/// `: $field_ty` after each field name.
+/// ```ignore
+/// match self {
+///		Enum::Variant {
+///			ref field1: option,
+///			ref field2: (option, explicit_type: u64),
+///			ref field3: (legacy, u64, {}, {}), // will be skipped
+///			..
+///		} => expression
+///	}
+/// ```
+#[proc_macro]
+pub fn skip_legacy_fields(expr: TokenStream) -> TokenStream {
+	let mut stream = expr.into_iter();
+	let mut res = TokenStream::new();
+
+	// First expect `match self` followed by a `{}` group...
+	let match_ident = stream.next().unwrap();
+	expect_ident(&match_ident, Some("match"));
+	res.extend(proc_macro::TokenStream::from(match_ident));
+
+	let self_ident = stream.next().unwrap();
+	expect_ident(&self_ident, Some("self"));
+	res.extend(proc_macro::TokenStream::from(self_ident));
+
+	let arms = stream.next().unwrap();
+	if let TokenTree::Group(group) = arms {
+		let mut new_arms = TokenStream::new();
+
+		let mut arm_stream = group.stream().into_iter().peekable();
+		while arm_stream.peek().is_some() {
+			// Each arm should contain Enum::Variant { fields } => init
+			// We explicitly check the :s, =, and >, as well as an optional trailing ,
+			let enum_ident = arm_stream.next().unwrap();
+			let co1 = arm_stream.next().unwrap();
+			expect_punct(&co1, ':');
+			let co2 = arm_stream.next().unwrap();
+			expect_punct(&co2, ':');
+			let variant_ident = arm_stream.next().unwrap();
+			let fields = arm_stream.next().unwrap();
+			let eq = arm_stream.next().unwrap();
+			expect_punct(&eq, '=');
+			let gt = arm_stream.next().unwrap();
+			expect_punct(&gt, '>');
+			let init = arm_stream.next().unwrap();
+
+			let next_tok = arm_stream.peek();
+			if let Some(TokenTree::Punct(_)) = next_tok {
+				expect_punct(next_tok.unwrap(), ',');
+				arm_stream.next();
+			}
+
+			let computed_fields = if let TokenTree::Group(group) = fields {
+				process_fields(group)
+			} else {
+				panic!("Expected a group for the fields in a match arm");
+			};
+
+			let arm_pfx = [enum_ident, co1, co2, variant_ident];
+			new_arms.extend(arm_pfx.into_iter().map(token_to_stream));
+			new_arms.extend(computed_fields);
+			let arm_sfx = [eq, gt, init];
+			new_arms.extend(arm_sfx.into_iter().map(token_to_stream));
+		}
+
+		let new_arm_group = Group::new(Delimiter::Brace, new_arms);
+		res.extend(token_to_stream(TokenTree::Group(new_arm_group)));
+	} else {
+		panic!("Expected `match self {{..}}` and nothing else");
+	}
+
+	assert!(stream.next().is_none(), "Expected `match self {{..}}` and nothing else");
+
+	res
+}
+
+/// Scans an enum definition for fields initialized with `legacy` types and drops them.
+///
+/// This is used internally in LDK's TLV serialization logic and is not expected to be used by
+/// other crates.
+///
+/// Is expected to wrap a struct definition like
+/// ```ignore
+/// drop_legacy_field_definition!(Self {
+/// 	field1: $crate::_ignore_arg!(field1, option),
+/// 	field2: $crate::_ignore_arg!(field2, (legacy, u64, {})),
+/// })
+/// ```
+/// and will drop fields defined like `field2` with a type starting with `legacy`.
+#[proc_macro]
+pub fn drop_legacy_field_definition(expr: TokenStream) -> TokenStream {
+	let mut st = if let Ok(parsed) = parse::<syn::Expr>(expr) {
+		if let syn::Expr::Struct(st) = parsed {
+			st
+		} else {
+			return (quote! {
+				compile_error!("drop_legacy_field_definition!() can only be used on struct expressions")
+			})
+			.into();
+		}
+	} else {
+		return (quote! {
+			compile_error!("drop_legacy_field_definition!() can only be used on expressions")
+		})
+		.into();
+	};
+	assert!(st.attrs.is_empty());
+	assert!(st.qself.is_none());
+	assert!(st.dot2_token.is_none());
+	assert!(st.rest.is_none());
+	let mut new_fields = syn::punctuated::Punctuated::new();
+	core::mem::swap(&mut new_fields, &mut st.fields);
+	for field in new_fields {
+		if let syn::Expr::Macro(syn::ExprMacro { mac, .. }) = &field.expr {
+			let macro_name = mac.path.segments.last().unwrap().ident.to_string();
+			let is_init = macro_name == "_ignore_arg";
+			// Skip `field_name` and `:`, giving us just the type's group
+			let ty_tokens = mac.tokens.clone().into_iter().skip(2).next();
+			if let Some(proc_macro2::TokenTree::Group(group)) = ty_tokens {
+				let first_token = group.stream().into_iter().next();
+				if let Some(proc_macro2::TokenTree::Ident(ident)) = first_token {
+					if is_init && ident == "legacy" {
+						continue;
+					}
+				}
+			}
+		}
+		st.fields.push(field);
+	}
+	let out = syn::Expr::Struct(st);
+	quote! { #out }.into()
+}
diff --git a/lightning-transaction-sync/Cargo.toml b/lightning-transaction-sync/Cargo.toml
index 39d7bb7a038..5e473d31266 100644
--- a/lightning-transaction-sync/Cargo.toml
+++ b/lightning-transaction-sync/Cargo.toml
@@ -24,7 +24,7 @@ async-interface = []
 
 [dependencies]
 lightning = { version = "0.2.0", path = "../lightning", default-features = false, features = ["std"] }
-lightning-macros = { version = "0.1", path = "../lightning-macros", default-features = false }
+lightning-macros = { version = "0.2", path = "../lightning-macros", default-features = false }
 bitcoin = { version = "0.32.2", default-features = false }
 futures = { version = "0.3", optional = true }
 esplora-client = { version = "0.11", default-features = false, optional = true }
diff --git a/lightning/Cargo.toml b/lightning/Cargo.toml
index f0bb2eb32e5..da0961f25d2 100644
--- a/lightning/Cargo.toml
+++ b/lightning/Cargo.toml
@@ -35,6 +35,7 @@ default = ["std", "grind_signatures"]
 [dependencies]
 lightning-types = { version = "0.3.0", path = "../lightning-types", default-features = false }
 lightning-invoice = { version = "0.34.0", path = "../lightning-invoice", default-features = false }
+lightning-macros = { version = "0.2", path = "../lightning-macros" }
 
 bech32 = { version = "0.11.0", default-features = false }
 bitcoin = { version = "0.32.2", default-features = false, features = ["secp-recovery"] }
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index d046a28e7d8..ecdd10dd3e3 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -54,6 +54,9 @@ macro_rules! _encode_tlv {
 			field.write($stream)?;
 		}
 	};
+	($stream: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $write: expr)) => {
+		$crate::_encode_tlv!($stream, $optional_type, $write, option);
+	};
 	($stream: expr, $type: expr, $field: expr, optional_vec) => {
 		if !$field.is_empty() {
 			$crate::_encode_tlv!($stream, $type, $field, required_vec);
@@ -213,6 +216,9 @@ macro_rules! _get_varint_length_prefixed_tlv_length {
 			$len.0 += field_len;
 		}
 	};
+	($len: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $write: expr)) => {
+		$crate::_get_varint_length_prefixed_tlv_length!($len, $optional_type, $write, option);
+	};
 	($len: expr, $type: expr, $field: expr, optional_vec) => {
 		if !$field.is_empty() {
 			$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, required_vec);
@@ -295,6 +301,9 @@ macro_rules! _check_decoded_tlv_order {
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (option, explicit_type: $fieldty: ty)) => {{
 		// no-op
 	}};
+	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (legacy, $fieldty: ty, $write: expr)) => {{
+		// no-op
+	}};
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (required, explicit_type: $fieldty: ty)) => {{
 		_check_decoded_tlv_order!($last_seen_type, $typ, $type, $field, required);
 	}};
@@ -354,6 +363,9 @@ macro_rules! _check_missing_tlv {
 	($last_seen_type: expr, $type: expr, $field: ident, (option, explicit_type: $fieldty: ty)) => {{
 		// no-op
 	}};
+	($last_seen_type: expr, $type: expr, $field: ident, (legacy, $fieldty: ty, $write: expr)) => {{
+		// no-op
+	}};
 	($last_seen_type: expr, $type: expr, $field: ident, (required, explicit_type: $fieldty: ty)) => {{
 		_check_missing_tlv!($last_seen_type, $type, $field, required);
 	}};
@@ -399,7 +411,10 @@ macro_rules! _decode_tlv {
 	}};
 	($outer_reader: expr, $reader: expr, $field: ident, (option, explicit_type: $fieldty: ty)) => {{
 		let _field: &Option<$fieldty> = &$field;
-		_decode_tlv!($outer_reader, $reader, $field, option);
+		$crate::_decode_tlv!($outer_reader, $reader, $field, option);
+	}};
+	($outer_reader: expr, $reader: expr, $field: ident, (legacy, $fieldty: ty, $write: expr)) => {{
+		$crate::_decode_tlv!($outer_reader, $reader, $field, (option, explicit_type: $fieldty));
 	}};
 	($outer_reader: expr, $reader: expr, $field: ident, (required, explicit_type: $fieldty: ty)) => {{
 		let _field: &$fieldty = &$field;
@@ -785,9 +800,13 @@ macro_rules! _init_tlv_based_struct_field {
 	($field: ident, option) => {
 		$field
 	};
+	($field: ident, (legacy, $fieldty: ty, $write: expr)) => {
+		$crate::_init_tlv_based_struct_field!($field, option)
+	};
 	($field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {
 		$crate::_init_tlv_based_struct_field!($field, option)
 	};
+	// Note that legacy TLVs are eaten by `drop_legacy_field_definition`
 	($field: ident, upgradable_required) => {
 		$field.0.unwrap()
 	};
@@ -835,6 +854,9 @@ macro_rules! _init_tlv_field_var {
 	($field: ident, (option, explicit_type: $fieldty: ty)) => {
 		let mut $field: Option<$fieldty> = None;
 	};
+	($field: ident, (legacy, $fieldty: ty, $write: expr)) => {
+		$crate::_init_tlv_field_var!($field, (option, explicit_type: $fieldty));
+	};
 	($field: ident, (required, explicit_type: $fieldty: ty)) => {
 		let mut $field = $crate::util::ser::RequiredWrapper::<$fieldty>(None);
 	};
@@ -881,19 +903,57 @@ macro_rules! _init_and_read_tlv_stream {
 		$(
 			$crate::_init_tlv_field_var!($field, $fieldty);
 		)*
-
 		$crate::decode_tlv_stream!($reader, {
 			$(($type, $field, $fieldty)),*
 		});
 	}
 }
 
+/// Dummy macro that drops the second argument (which is used by
+/// [`lightning_macros::drop_legacy_field_definition`] to match for legacy fields but isn't needed
+/// in the final code we want to generate).
+#[doc(hidden)]
+#[macro_export]
+macro_rules! _ignore_arg {
+	($field: ident, $fieldty: tt) => {
+		$field
+	};
+}
+
+/// Reads a TLV stream with the given fields to build a struct/enum variant of type `$thing`
+#[doc(hidden)]
+#[macro_export]
+macro_rules! _decode_and_build {
+	($stream: ident, $thing: path, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}) => { {
+		$crate::_init_and_read_len_prefixed_tlv_fields!($stream, {
+			$(($type, $field, $fieldty)),*
+		});
+		// rustc is kinda dumb about unused variable warnings when we declare a variable via an
+		// ident in a macro and then use it in an expr also defined in the same macro call. Thus,
+		// it may generate unused variable warnings for variables that are, in fact, very much
+		// used. Instead, we just blanket ignore unused variables here as it may be useful to write
+		// field names without a _ prefix for legacy fields even if we don't care about the read
+		// value.
+		$(
+			#[allow(unused_variables)]
+			let $field = $crate::_init_tlv_based_struct_field!($field, $fieldty);
+		)*
+		::lightning_macros::drop_legacy_field_definition!($thing {
+			$($field: $crate::_ignore_arg!($field, $fieldty)),*
+		})
+	} }
+}
+
 /// Implements [`Readable`]/[`Writeable`] for a struct storing it as a set of TLVs
 /// If `$fieldty` is `required`, then `$field` is a required field that is not an [`Option`] nor a [`Vec`].
 /// If `$fieldty` is `(default_value, $default)`, then `$field` will be set to `$default` if not present.
 /// If `$fieldty` is `option`, then `$field` is optional field.
 /// If `$fieldty` is `optional_vec`, then `$field` is a [`Vec`], which needs to have its individual elements serialized.
 ///    Note that for `optional_vec` no bytes are written if the vec is empty
+/// If `$fieldty` is `(legacy, $ty, $write)` then, when writing, the expression $write will
+///    be called which returns an `Option` and is written as a TLV if `Some`. When reading, an
+///    optional field of type `$ty` is read (which can be used in later `default_value` or
+///    `static_value` fields).
 ///
 /// For example,
 /// ```
@@ -946,14 +1006,7 @@ macro_rules! impl_writeable_tlv_based {
 
 		impl $crate::util::ser::Readable for $st {
 			fn read<R: $crate::io::Read>(reader: &mut R) -> Result<Self, $crate::ln::msgs::DecodeError> {
-				$crate::_init_and_read_len_prefixed_tlv_fields!(reader, {
-					$(($type, $field, $fieldty)),*
-				});
-				Ok(Self {
-					$(
-						$field: $crate::_init_tlv_based_struct_field!($field, $fieldty)
-					),*
-				})
+				Ok($crate::_decode_and_build!(reader, Self, {$(($type, $field, $fieldty)),*}))
 			}
 		}
 	}
@@ -1053,8 +1106,8 @@ macro_rules! _impl_writeable_tlv_based_enum_common {
 	$(($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 {
-					$($st::$variant_name { $(ref $field, )* .. } => {
+				lightning_macros::skip_legacy_fields!(match self {
+					$($st::$variant_name { $(ref $field: $fieldty, )* .. } => {
 						let id: u8 = $variant_id;
 						id.write(writer)?;
 						$crate::write_tlv_fields!(writer, {
@@ -1072,7 +1125,7 @@ macro_rules! _impl_writeable_tlv_based_enum_common {
 						$crate::util::ser::BigSize(field.serialized_length() as u64).write(writer)?;
 						field.write(writer)?;
 					}),*
-				}
+				});
 				Ok(())
 			}
 		}
@@ -1139,14 +1192,7 @@ macro_rules! impl_writeable_tlv_based_enum {
 						// 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)
-								),*
-							})
+							Ok($crate::_decode_and_build!(reader, $st::$variant_name, {$(($type, $field, $fieldty)),*}))
 						};
 						f()
 					}),*
@@ -1188,14 +1234,7 @@ macro_rules! impl_writeable_tlv_based_enum_legacy {
 						// 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)
-								),*
-							})
+							Ok($crate::_decode_and_build!(reader, $st::$variant_name, {$(($type, $field, $fieldty)),*}))
 						};
 						f()
 					}),*
@@ -1251,14 +1290,7 @@ macro_rules! impl_writeable_tlv_based_enum_upgradable {
 						// 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)
-								),*
-							}))
+							Ok(Some($crate::_decode_and_build!(reader, $st::$variant_name, {$(($type, $field, $fieldty)),*})))
 						};
 						f()
 					}),*
@@ -1307,14 +1339,7 @@ macro_rules! impl_writeable_tlv_based_enum_upgradable_legacy {
 						// 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)
-								),*
-							}))
+							Ok(Some($crate::_decode_and_build!(reader, $st::$variant_name, {$(($type, $field, $fieldty)),*})))
 						};
 						f()
 					}),*

From 931d625292f0104e3d48e08295937d450608458c Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Thu, 23 Jan 2025 16:25:29 +0000
Subject: [PATCH 2/5] Change the legacy write-side expr to a lambda which takes
 self

In the previous commit we added the ability to write legacy fields
without having a corresponding fields in the in-memory encoding of
structs and enums and without breaking out of the high-level
utility macros like `impl_writeable_tlv_based`.

However, the write-side expr was required to be a static expr,
without the ability to access `self`, limiting its ability to write
useful fields.

Here we swap it for a `Fn` which takes the object being serialized.
---
 lightning/src/util/ser_macros.rs | 86 ++++++++++++++++----------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index ecdd10dd3e3..03c4bc069bf 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -33,48 +33,48 @@
 #[doc(hidden)]
 #[macro_export]
 macro_rules! _encode_tlv {
-	($stream: expr, $type: expr, $field: expr, (default_value, $default: expr)) => {
+	($stream: expr, $type: expr, $field: expr, (default_value, $default: expr) $(, $self: ident)?) => {
 		$crate::_encode_tlv!($stream, $type, $field, required)
 	};
-	($stream: expr, $type: expr, $field: expr, (static_value, $value: expr)) => {
+	($stream: expr, $type: expr, $field: expr, (static_value, $value: expr) $(, $self: ident)?) => {
 		let _ = &$field; // Ensure we "use" the $field
 	};
-	($stream: expr, $type: expr, $field: expr, required) => {
+	($stream: expr, $type: expr, $field: expr, required $(, $self: ident)?) => {
 		BigSize($type).write($stream)?;
 		BigSize($field.serialized_length() as u64).write($stream)?;
 		$field.write($stream)?;
 	};
-	($stream: expr, $type: expr, $field: expr, required_vec) => {
+	($stream: expr, $type: expr, $field: expr, required_vec $(, $self: ident)?) => {
 		$crate::_encode_tlv!($stream, $type, $crate::util::ser::WithoutLength(&$field), required);
 	};
-	($stream: expr, $optional_type: expr, $optional_field: expr, option) => {
+	($stream: expr, $optional_type: expr, $optional_field: expr, option $(, $self: ident)?) => {
 		if let Some(ref field) = $optional_field {
 			BigSize($optional_type).write($stream)?;
 			BigSize(field.serialized_length() as u64).write($stream)?;
 			field.write($stream)?;
 		}
 	};
-	($stream: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $write: expr)) => {
-		$crate::_encode_tlv!($stream, $optional_type, $write, option);
+	($stream: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $write: expr) $(, $self: ident)?) => {
+		$crate::_encode_tlv!($stream, $optional_type, $write($($self)?), option);
 	};
-	($stream: expr, $type: expr, $field: expr, optional_vec) => {
+	($stream: expr, $type: expr, $field: expr, optional_vec $(, $self: ident)?) => {
 		if !$field.is_empty() {
 			$crate::_encode_tlv!($stream, $type, $field, required_vec);
 		}
 	};
-	($stream: expr, $type: expr, $field: expr, upgradable_required) => {
+	($stream: expr, $type: expr, $field: expr, upgradable_required $(, $self: ident)?) => {
 		$crate::_encode_tlv!($stream, $type, $field, required);
 	};
-	($stream: expr, $type: expr, $field: expr, upgradable_option) => {
+	($stream: expr, $type: expr, $field: expr, upgradable_option $(, $self: ident)?) => {
 		$crate::_encode_tlv!($stream, $type, $field, option);
 	};
-	($stream: expr, $type: expr, $field: expr, (option, encoding: ($fieldty: ty, $encoding: ident))) => {
+	($stream: expr, $type: expr, $field: expr, (option, encoding: ($fieldty: ty, $encoding: ident) $(, $self: ident)?)) => {
 		$crate::_encode_tlv!($stream, $type, $field.map(|f| $encoding(f)), option);
 	};
-	($stream: expr, $type: expr, $field: expr, (option, encoding: $fieldty: ty)) => {
+	($stream: expr, $type: expr, $field: expr, (option, encoding: $fieldty: ty) $(, $self: ident)?) => {
 		$crate::_encode_tlv!($stream, $type, $field, option);
 	};
-	($stream: expr, $type: expr, $field: expr, (option: $trait: ident $(, $read_arg: expr)?)) => {
+	($stream: expr, $type: expr, $field: expr, (option: $trait: ident $(, $read_arg: expr)?) $(, $self: ident)?) => {
 		// Just a read-mapped type
 		$crate::_encode_tlv!($stream, $type, $field, option);
 	};
@@ -146,10 +146,10 @@ macro_rules! encode_tlv_stream {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! _encode_tlv_stream {
-	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),* $(,)*}) => { {
-		$crate::_encode_tlv_stream!($stream, { $(($type, $field, $fieldty)),* }, &[])
+	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt $(, $self: ident)?)),* $(,)*}) => { {
+		$crate::_encode_tlv_stream!($stream, { $(($type, $field, $fieldty $(, $self)?)),* }, &[])
 	} };
-	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),* $(,)*}, $extra_tlvs: expr) => { {
+	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt $(, $self: ident)?)),* $(,)*}, $extra_tlvs: expr) => { {
 		#[allow(unused_imports)]
 		use $crate::{
 			ln::msgs::DecodeError,
@@ -159,7 +159,7 @@ macro_rules! _encode_tlv_stream {
 		};
 
 		$(
-			$crate::_encode_tlv!($stream, $type, $field, $fieldty);
+			$crate::_encode_tlv!($stream, $type, $field, $fieldty $(, $self)?);
 		)*
 		for tlv in $extra_tlvs {
 			let (typ, value): &(u64, Vec<u8>) = tlv;
@@ -188,11 +188,11 @@ macro_rules! _encode_tlv_stream {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! _get_varint_length_prefixed_tlv_length {
-	($len: expr, $type: expr, $field: expr, (default_value, $default: expr)) => {
+	($len: expr, $type: expr, $field: expr, (default_value, $default: expr) $(, $self: ident)?) => {
 		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, required)
 	};
-	($len: expr, $type: expr, $field: expr, (static_value, $value: expr)) => {};
-	($len: expr, $type: expr, $field: expr, required) => {
+	($len: expr, $type: expr, $field: expr, (static_value, $value: expr) $(, $self: ident)?) => {};
+	($len: expr, $type: expr, $field: expr, required $(, $self: ident)?) => {
 		BigSize($type).write(&mut $len).expect("No in-memory data may fail to serialize");
 		let field_len = $field.serialized_length();
 		BigSize(field_len as u64)
@@ -200,11 +200,11 @@ macro_rules! _get_varint_length_prefixed_tlv_length {
 			.expect("No in-memory data may fail to serialize");
 		$len.0 += field_len;
 	};
-	($len: expr, $type: expr, $field: expr, required_vec) => {
+	($len: expr, $type: expr, $field: expr, required_vec $(, $self: ident)?) => {
 		let field = $crate::util::ser::WithoutLength(&$field);
 		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, field, required);
 	};
-	($len: expr, $optional_type: expr, $optional_field: expr, option) => {
+	($len: expr, $optional_type: expr, $optional_field: expr, option $(, $self: ident)?) => {
 		if let Some(ref field) = $optional_field {
 			BigSize($optional_type)
 				.write(&mut $len)
@@ -216,25 +216,25 @@ macro_rules! _get_varint_length_prefixed_tlv_length {
 			$len.0 += field_len;
 		}
 	};
-	($len: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $write: expr)) => {
-		$crate::_get_varint_length_prefixed_tlv_length!($len, $optional_type, $write, option);
+	($len: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $write: expr) $(, $self: ident)?) => {
+		$crate::_get_varint_length_prefixed_tlv_length!($len, $optional_type, $write($($self)?), option);
 	};
-	($len: expr, $type: expr, $field: expr, optional_vec) => {
+	($len: expr, $type: expr, $field: expr, optional_vec $(, $self: ident)?) => {
 		if !$field.is_empty() {
 			$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, required_vec);
 		}
 	};
-	($len: expr, $type: expr, $field: expr, (option: $trait: ident $(, $read_arg: expr)?)) => {
+	($len: expr, $type: expr, $field: expr, (option: $trait: ident $(, $read_arg: expr)?) $(, $self: ident)?) => {
 		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, option);
 	};
-	($len: expr, $type: expr, $field: expr, (option, encoding: ($fieldty: ty, $encoding: ident))) => {
+	($len: expr, $type: expr, $field: expr, (option, encoding: ($fieldty: ty, $encoding: ident)) $(, $self: ident)?) => {
 		let field = $field.map(|f| $encoding(f));
 		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, field, option);
 	};
-	($len: expr, $type: expr, $field: expr, upgradable_required) => {
+	($len: expr, $type: expr, $field: expr, upgradable_required $(, $self: ident)?) => {
 		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, required);
 	};
-	($len: expr, $type: expr, $field: expr, upgradable_option) => {
+	($len: expr, $type: expr, $field: expr, upgradable_option $(, $self: ident)?) => {
 		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, option);
 	};
 }
@@ -244,10 +244,10 @@ macro_rules! _get_varint_length_prefixed_tlv_length {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! _encode_varint_length_prefixed_tlv {
-	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),*}) => { {
-		$crate::_encode_varint_length_prefixed_tlv!($stream, {$(($type, $field, $fieldty)),*}, &[])
+	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt $(, $self: ident)?)),*}) => { {
+		$crate::_encode_varint_length_prefixed_tlv!($stream, {$(($type, $field, $fieldty $(, $self)?)),*}, &[])
 	} };
-	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),*}, $extra_tlvs: expr) => { {
+	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt $(, $self: ident)?)),*}, $extra_tlvs: expr) => { {
 		extern crate alloc;
 		use $crate::util::ser::BigSize;
 		use alloc::vec::Vec;
@@ -255,7 +255,7 @@ macro_rules! _encode_varint_length_prefixed_tlv {
 			#[allow(unused_mut)]
 			let mut len = $crate::util::ser::LengthCalculatingWriter(0);
 			$(
-				$crate::_get_varint_length_prefixed_tlv_length!(len, $type, $field, $fieldty);
+				$crate::_get_varint_length_prefixed_tlv_length!(len, $type, $field, $fieldty $(, $self)?);
 			)*
 			for tlv in $extra_tlvs {
 				let (typ, value): &(u64, Vec<u8>) = tlv;
@@ -264,7 +264,7 @@ macro_rules! _encode_varint_length_prefixed_tlv {
 			len.0
 		};
 		BigSize(len as u64).write($stream)?;
-		$crate::_encode_tlv_stream!($stream, { $(($type, $field, $fieldty)),* }, $extra_tlvs);
+		$crate::_encode_tlv_stream!($stream, { $(($type, $field, $fieldty $(, $self)?)),* }, $extra_tlvs);
 	} };
 }
 
@@ -950,10 +950,10 @@ macro_rules! _decode_and_build {
 /// If `$fieldty` is `option`, then `$field` is optional field.
 /// If `$fieldty` is `optional_vec`, then `$field` is a [`Vec`], which needs to have its individual elements serialized.
 ///    Note that for `optional_vec` no bytes are written if the vec is empty
-/// If `$fieldty` is `(legacy, $ty, $write)` then, when writing, the expression $write will
-///    be called which returns an `Option` and is written as a TLV if `Some`. When reading, an
-///    optional field of type `$ty` is read (which can be used in later `default_value` or
-///    `static_value` fields).
+/// If `$fieldty` is `(legacy, $ty, $write)` then, when writing, the function $write will be
+///    called with the object being serialized and a returned `Option` and is written as a TLV if
+///    `Some`. When reading, an optional field of type `$ty` is read (which can be used in later
+///    `default_value` or `static_value` fields).
 ///
 /// For example,
 /// ```
@@ -981,8 +981,8 @@ macro_rules! impl_writeable_tlv_based {
 	($st: ident, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}) => {
 		impl $crate::util::ser::Writeable for $st {
 			fn write<W: $crate::util::ser::Writer>(&self, writer: &mut W) -> Result<(), $crate::io::Error> {
-				$crate::write_tlv_fields!(writer, {
-					$(($type, self.$field, $fieldty)),*
+				$crate::_encode_varint_length_prefixed_tlv!(writer, {
+					$(($type, self.$field, $fieldty, self)),*
 				});
 				Ok(())
 			}
@@ -994,7 +994,7 @@ macro_rules! impl_writeable_tlv_based {
 					#[allow(unused_mut)]
 					let mut len = $crate::util::ser::LengthCalculatingWriter(0);
 					$(
-						$crate::_get_varint_length_prefixed_tlv_length!(len, $type, self.$field, $fieldty);
+						$crate::_get_varint_length_prefixed_tlv_length!(len, $type, self.$field, $fieldty, self);
 					)*
 					len.0
 				};
@@ -1110,8 +1110,8 @@ macro_rules! _impl_writeable_tlv_based_enum_common {
 					$($st::$variant_name { $(ref $field: $fieldty, )* .. } => {
 						let id: u8 = $variant_id;
 						id.write(writer)?;
-						$crate::write_tlv_fields!(writer, {
-							$(($type, *$field, $fieldty)),*
+						$crate::_encode_varint_length_prefixed_tlv!(writer, {
+							$(($type, *$field, $fieldty, self)),*
 						});
 					}),*
 					$($st::$tuple_variant_name (ref field) => {

From 7f9c9a4754df31c539200bf30f203ffdac7893d1 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Wed, 22 Jan 2025 20:14:38 +0000
Subject: [PATCH 3/5] Add a test for the `legacy` TLV field logic

---
 lightning/src/util/ser_macros.rs | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 03c4bc069bf..6590fdbb728 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -1850,4 +1850,32 @@ mod tests {
 		assert_eq!(TuplesOnly::read(&mut none_data_read).unwrap(), None);
 		assert_eq!(none_data_read.position(), unknown_data_variant.len() as u64);
 	}
+
+	#[derive(Debug, PartialEq, Eq)]
+	struct ExpandedField {
+		// Old versions of LDK are presumed to have had something like:
+		// old_field: u8,
+		new_field: (u8, u8),
+	}
+	impl_writeable_tlv_based!(ExpandedField, {
+		(0, old_field, (legacy, u8, |us: &ExpandedField| Some(us.new_field.0))),
+		(1, new_field, (default_value, (old_field.ok_or(DecodeError::InvalidValue)?, 0))),
+	});
+
+	#[test]
+	fn test_legacy_conversion() {
+		let mut encoded = ExpandedField { new_field: (43, 42) }.encode();
+		assert_eq!(encoded, <Vec<u8>>::from_hex("0700012b01022b2a").unwrap());
+
+		// On read, we'll read a `new_field` which means we won't bother looking at `old_field`.
+		encoded[3] = 10;
+		let read = <ExpandedField as Readable>::read(&mut &encoded[..]).unwrap();
+		assert_eq!(read, ExpandedField { new_field: (43, 42) });
+
+		// On read, if we read an old `ExpandedField` that just has a type-0 `old_field` entry,
+		// we'll copy that into the first position of `new_field`.
+		let encoded = <Vec<u8>>::from_hex("0300012a").unwrap();
+		let read = <ExpandedField as Readable>::read(&mut &encoded[..]).unwrap();
+		assert_eq!(read, ExpandedField { new_field: (42, 0) });
+	}
 }

From 09a06a8a253d20f2c9af3f2dbae6c97b8b506ab8 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Thu, 30 Jan 2025 01:54:57 +0000
Subject: [PATCH 4/5] Use the new `legacy` en/decoding to de/ser
 `ChannelDetails`

Allowing us to use `impl_writeable_tlv_based!` again rather than
manually writing the de/ser logic.
---
 lightning/src/ln/channel_state.rs | 164 +++++++-----------------------
 lightning/src/util/ser.rs         |   6 ++
 2 files changed, 45 insertions(+), 125 deletions(-)

diff --git a/lightning/src/ln/channel_state.rs b/lightning/src/ln/channel_state.rs
index 9945bba885c..cbea8d54776 100644
--- a/lightning/src/ln/channel_state.rs
+++ b/lightning/src/ln/channel_state.rs
@@ -15,15 +15,12 @@ use bitcoin::secp256k1::PublicKey;
 
 use crate::chain::chaininterface::{FeeEstimator, LowerBoundedFeeEstimator};
 use crate::chain::transaction::OutPoint;
-use crate::io;
 use crate::ln::channel::ChannelContext;
-use crate::ln::msgs::DecodeError;
 use crate::ln::types::ChannelId;
 use crate::sign::SignerProvider;
 use crate::types::features::{ChannelTypeFeatures, InitFeatures};
 use crate::types::payment::PaymentHash;
 use crate::util::config::ChannelConfig;
-use crate::util::ser::{Readable, Writeable, Writer};
 
 use core::ops::Deref;
 
@@ -549,128 +546,45 @@ impl ChannelDetails {
 	}
 }
 
-impl Writeable for ChannelDetails {
-	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
-		// `user_channel_id` used to be a single u64 value. In order to remain backwards compatible with
-		// versions prior to 0.0.113, the u128 is serialized as two separate u64 values.
-		let user_channel_id_low = self.user_channel_id as u64;
-		let user_channel_id_high_opt = Some((self.user_channel_id >> 64) as u64);
-		#[allow(deprecated)] // TODO: Remove once balance_msat is removed.
-		{
-			write_tlv_fields!(writer, {
-				(1, self.inbound_scid_alias, option),
-				(2, self.channel_id, required),
-				(3, self.channel_type, option),
-				(4, self.counterparty, required),
-				(5, self.outbound_scid_alias, option),
-				(6, self.funding_txo, option),
-				(7, self.config, option),
-				(8, self.short_channel_id, option),
-				(9, self.confirmations, option),
-				(10, self.channel_value_satoshis, required),
-				(12, self.unspendable_punishment_reserve, option),
-				(14, user_channel_id_low, required),
-				(16, self.next_outbound_htlc_limit_msat, required), // Forwards compatibility for removed balance_msat field.
-				(18, self.outbound_capacity_msat, required),
-				(19, self.next_outbound_htlc_limit_msat, required),
-				(20, self.inbound_capacity_msat, required),
-				(21, self.next_outbound_htlc_minimum_msat, required),
-				(22, self.confirmations_required, option),
-				(24, self.force_close_spend_delay, option),
-				(26, self.is_outbound, required),
-				(28, self.is_channel_ready, required),
-				(30, self.is_usable, required),
-				(32, self.is_announced, required),
-				(33, self.inbound_htlc_minimum_msat, option),
-				(35, self.inbound_htlc_maximum_msat, option),
-				(37, user_channel_id_high_opt, option),
-				(39, self.feerate_sat_per_1000_weight, option),
-				(41, self.channel_shutdown_state, option),
-				(43, self.pending_inbound_htlcs, optional_vec),
-				(45, self.pending_outbound_htlcs, optional_vec),
-			});
-		}
-		Ok(())
-	}
-}
-
-impl Readable for ChannelDetails {
-	fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
-		_init_and_read_len_prefixed_tlv_fields!(reader, {
-			(1, inbound_scid_alias, option),
-			(2, channel_id, required),
-			(3, channel_type, option),
-			(4, counterparty, required),
-			(5, outbound_scid_alias, option),
-			(6, funding_txo, option),
-			(7, config, option),
-			(8, short_channel_id, option),
-			(9, confirmations, option),
-			(10, channel_value_satoshis, required),
-			(12, unspendable_punishment_reserve, option),
-			(14, user_channel_id_low, required),
-			(16, _balance_msat, option), // Backwards compatibility for removed balance_msat field.
-			(18, outbound_capacity_msat, required),
-			// Note that by the time we get past the required read above, outbound_capacity_msat will be
-			// filled in, so we can safely unwrap it here.
-			(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
-			(20, inbound_capacity_msat, required),
-			(21, next_outbound_htlc_minimum_msat, (default_value, 0)),
-			(22, confirmations_required, option),
-			(24, force_close_spend_delay, option),
-			(26, is_outbound, required),
-			(28, is_channel_ready, required),
-			(30, is_usable, required),
-			(32, is_announced, required),
-			(33, inbound_htlc_minimum_msat, option),
-			(35, inbound_htlc_maximum_msat, option),
-			(37, user_channel_id_high_opt, option),
-			(39, feerate_sat_per_1000_weight, option),
-			(41, channel_shutdown_state, option),
-			(43, pending_inbound_htlcs, optional_vec),
-			(45, pending_outbound_htlcs, optional_vec),
-		});
-
-		// `user_channel_id` used to be a single u64 value. In order to remain backwards compatible with
-		// versions prior to 0.0.113, the u128 is serialized as two separate u64 values.
-		let user_channel_id_low: u64 = user_channel_id_low.0.unwrap();
-		let user_channel_id = user_channel_id_low as u128
-			+ ((user_channel_id_high_opt.unwrap_or(0 as u64) as u128) << 64);
-
-		let _balance_msat: Option<u64> = _balance_msat;
-
-		Ok(Self {
-			inbound_scid_alias,
-			channel_id: channel_id.0.unwrap(),
-			channel_type,
-			counterparty: counterparty.0.unwrap(),
-			outbound_scid_alias,
-			funding_txo,
-			config,
-			short_channel_id,
-			channel_value_satoshis: channel_value_satoshis.0.unwrap(),
-			unspendable_punishment_reserve,
-			user_channel_id,
-			outbound_capacity_msat: outbound_capacity_msat.0.unwrap(),
-			next_outbound_htlc_limit_msat: next_outbound_htlc_limit_msat.0.unwrap(),
-			next_outbound_htlc_minimum_msat: next_outbound_htlc_minimum_msat.0.unwrap(),
-			inbound_capacity_msat: inbound_capacity_msat.0.unwrap(),
-			confirmations_required,
-			confirmations,
-			force_close_spend_delay,
-			is_outbound: is_outbound.0.unwrap(),
-			is_channel_ready: is_channel_ready.0.unwrap(),
-			is_usable: is_usable.0.unwrap(),
-			is_announced: is_announced.0.unwrap(),
-			inbound_htlc_minimum_msat,
-			inbound_htlc_maximum_msat,
-			feerate_sat_per_1000_weight,
-			channel_shutdown_state,
-			pending_inbound_htlcs: pending_inbound_htlcs.unwrap_or(Vec::new()),
-			pending_outbound_htlcs: pending_outbound_htlcs.unwrap_or(Vec::new()),
-		})
-	}
-}
+impl_writeable_tlv_based!(ChannelDetails, {
+	(1, inbound_scid_alias, option),
+	(2, channel_id, required),
+	(3, channel_type, option),
+	(4, counterparty, required),
+	(5, outbound_scid_alias, option),
+	(6, funding_txo, option),
+	(7, config, option),
+	(8, short_channel_id, option),
+	(9, confirmations, option),
+	(10, channel_value_satoshis, required),
+	(12, unspendable_punishment_reserve, option),
+	// Note that _user_channel_id_low is used below, but rustc warns anyway
+	(14, _user_channel_id_low, (legacy, u64,
+		|us: &ChannelDetails| Some(us.user_channel_id as u64))),
+	(16, _balance_msat, (legacy, u64, |us: &ChannelDetails| Some(us.next_outbound_htlc_limit_msat))),
+	(18, outbound_capacity_msat, required),
+	(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat)),
+	(20, inbound_capacity_msat, required),
+	(21, next_outbound_htlc_minimum_msat, (default_value, 0)),
+	(22, confirmations_required, option),
+	(24, force_close_spend_delay, option),
+	(26, is_outbound, required),
+	(28, is_channel_ready, required),
+	(30, is_usable, required),
+	(32, is_announced, required),
+	(33, inbound_htlc_minimum_msat, option),
+	(35, inbound_htlc_maximum_msat, option),
+	// Note that _user_channel_id_high is used below, but rustc warns anyway
+	(37, _user_channel_id_high, (legacy, u64,
+		|us: &ChannelDetails| Some((us.user_channel_id >> 64) as u64))),
+	(39, feerate_sat_per_1000_weight, option),
+	(41, channel_shutdown_state, option),
+	(43, pending_inbound_htlcs, optional_vec),
+	(45, pending_outbound_htlcs, optional_vec),
+	(_unused, user_channel_id, (static_value,
+		_user_channel_id_low.unwrap_or(0) as u128 | ((_user_channel_id_high.unwrap_or(0) as u128) << 64)
+	)),
+});
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 /// Further information on the details of the channel shutdown.
diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs
index 0f8dae0eb8d..6a4d49fcdbe 100644
--- a/lightning/src/util/ser.rs
+++ b/lightning/src/util/ser.rs
@@ -413,6 +413,12 @@ impl<T> From<T> for RequiredWrapper<T> {
 		RequiredWrapper(Some(t))
 	}
 }
+impl<T: Clone> Clone for RequiredWrapper<T> {
+	fn clone(&self) -> Self {
+		Self(self.0.clone())
+	}
+}
+impl<T: Copy> Copy for RequiredWrapper<T> {}
 
 /// Wrapper to read a required (non-optional) TLV record that may have been upgraded without
 /// backwards compat.

From 6ae5b5f07207318ff4f22aad81a2da8488990a72 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Wed, 5 Feb 2025 21:59:16 +0000
Subject: [PATCH 5/5] Document most read/write styles in
 `impl_writeable_tlv_based`

We previously declined to document various read/write styles in
`impl_writeable_tlv_based` to avoid committing to them in the
public API, but they've been quite stable over time, so its not
clear that that was worth anything.

Here we at least document the read/write styles we already had
documented in a comment at the top of the file, which isn't all
of them but its all of the useful ones.
---
 lightning/src/util/ser_macros.rs | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 6590fdbb728..8dbf0635d80 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -13,21 +13,6 @@
 //! [`Readable`]: crate::util::ser::Readable
 //! [`Writeable`]: crate::util::ser::Writeable
 
-// There are quite a few TLV serialization "types" which behave differently. We currently only
-// publicly document the `optional` and `required` types, not supporting anything else publicly and
-// changing them at will.
-//
-// Some of the other types include:
-//  * (default_value, $default) - reads optionally, reading $default if no TLV is present
-//  * (static_value, $value) - ignores any TLVs, always using $value
-//  * required_vec - reads into a Vec without a length prefix, failing if no TLV is present.
-//  * optional_vec - reads into an Option<Vec> without a length prefix, continuing if no TLV is
-//                   present. Writes from a Vec directly, only if any elements are present. Note
-//                   that the struct deserialization macros return a Vec, not an Option.
-//  * upgradable_option - reads via MaybeReadable.
-//  * upgradable_required - reads via MaybeReadable, requiring a TLV be present but may return None
-//                          if MaybeReadable::read() returns None.
-
 /// Implements serialization for a single TLV record.
 /// This is exported for use by other exported macros, do not use directly.
 #[doc(hidden)]
@@ -944,16 +929,23 @@ macro_rules! _decode_and_build {
 	} }
 }
 
-/// Implements [`Readable`]/[`Writeable`] for a struct storing it as a set of TLVs
+/// Implements [`Readable`]/[`Writeable`] for a struct storing it as a set of TLVs. Each TLV is
+/// read/written in the order they appear and contains a type number, a field name, and a
+/// de/serialization method, from the following:
+///
 /// If `$fieldty` is `required`, then `$field` is a required field that is not an [`Option`] nor a [`Vec`].
 /// If `$fieldty` is `(default_value, $default)`, then `$field` will be set to `$default` if not present.
+/// If `$fieldty` is `(static_value, $static)`, then `$field` will be set to `$static`.
 /// If `$fieldty` is `option`, then `$field` is optional field.
+/// If `$fieldty` is `upgradable_option`, then `$field` is optional and read via [`MaybeReadable`].
+/// If `$fieldty` is `upgradable_required`, then `$field` is stored as an [`Option`] and read via
+///    [`MaybeReadable`], requiring the TLV to be present.
 /// If `$fieldty` is `optional_vec`, then `$field` is a [`Vec`], which needs to have its individual elements serialized.
 ///    Note that for `optional_vec` no bytes are written if the vec is empty
 /// If `$fieldty` is `(legacy, $ty, $write)` then, when writing, the function $write will be
 ///    called with the object being serialized and a returned `Option` and is written as a TLV if
 ///    `Some`. When reading, an optional field of type `$ty` is read (which can be used in later
-///    `default_value` or `static_value` fields).
+///    `default_value` or `static_value` fields by referring to the value by name).
 ///
 /// For example,
 /// ```
@@ -963,6 +955,7 @@ macro_rules! _decode_and_build {
 /// 	tlv_default_integer: u32,
 /// 	tlv_optional_integer: Option<u32>,
 /// 	tlv_vec_type_integer: Vec<u32>,
+///		tlv_upgraded_integer: u32,
 /// }
 ///
 /// impl_writeable_tlv_based!(LightningMessage, {
@@ -970,10 +963,13 @@ macro_rules! _decode_and_build {
 /// 	(1, tlv_default_integer, (default_value, 7)),
 /// 	(2, tlv_optional_integer, option),
 /// 	(3, tlv_vec_type_integer, optional_vec),
+/// 	(4, unwritten_type, (legacy, u32, |us: &LightningMessage| Some(us.tlv_integer))),
+/// 	(_unused, tlv_upgraded_integer, (static_value, unwritten_type.unwrap_or(0) * 2))
 /// });
 /// ```
 ///
 /// [`Readable`]: crate::util::ser::Readable
+/// [`MaybeReadable`]: crate::util::ser::MaybeReadable
 /// [`Writeable`]: crate::util::ser::Writeable
 /// [`Vec`]: crate::prelude::Vec
 #[macro_export]