Skip to content

Add custom TLV read/write #4390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jkczyz wants to merge 2 commits into
base: main
Choose a base branch
from
+73 −26
Open

Add custom TLV read/write #4390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05c9a03
Add a `custom` TLV read/write variant
TheBlueMatt Feb 1, 2026
e71ad81
Add a read closure to the `legacy` TLV variant
jkczyz Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lightning-macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ fn process_fields(group: Group) -> proc_macro::TokenStream {
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" {
if ident.to_string() == "legacy" || ident.to_string() == "custom" {
continue;
}
}
Expand All @@ -155,13 +155,13 @@ fn process_fields(group: Group) -> proc_macro::TokenStream {
computed_fields
}

/// Scans a match statement for legacy fields which should be skipped.
/// Scans a match statement for legacy or custom 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.
/// `ref $field_name: $field_ty`) for types marked `legacy` or `custom`, skipping those fields.
///
/// Specifically, it expects input like the following, simply dropping `field3` and the
/// `: $field_ty` after each field name.
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/chain/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ impl_writeable_tlv_based!(RevokedOutput, {
(12, on_counterparty_tx_csv, required),
// Unused since 0.1, this setting causes downgrades to before 0.1 to refuse to
// aggregate `RevokedOutput` claims, which is the more conservative stance.
(14, is_counterparty_balance_on_anchors, (legacy, (), |_| Some(()))),
(14, is_counterparty_balance_on_anchors, (legacy, (), |_| Ok(()), |_| Some(()))),
(15, channel_parameters, (option: ReadableArgs, None)), // Added in 0.2.
});

Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/channel_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,9 @@ impl_writeable_tlv_based!(ChannelDetails, {
(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,
(14, _user_channel_id_low, (legacy, u64, |_| Ok(()),
|us: &ChannelDetails| Some(us.user_channel_id as u64))),
(16, _balance_msat, (legacy, u64, |us: &ChannelDetails| Some(us.next_outbound_htlc_limit_msat))),
(16, _balance_msat, (legacy, u64, |_| Ok(()), |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),
Expand All @@ -623,7 +623,7 @@ impl_writeable_tlv_based!(ChannelDetails, {
(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,
(37, _user_channel_id_high, (legacy, u64, |_| Ok(()),
|us: &ChannelDetails| Some((us.user_channel_id >> 64) as u64))),
(39, feerate_sat_per_1000_weight, option),
(41, channel_shutdown_state, option),
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1943,14 +1943,14 @@ impl Readable for HTLCFailReason {

impl_writeable_tlv_based_enum!(HTLCFailReasonRepr,
(0, LightningError) => {
(0, data, (legacy, Vec<u8>, |us|
(0, data, (legacy, Vec<u8>, |_| Ok(()), |us|
if let &HTLCFailReasonRepr::LightningError { err: msgs::OnionErrorPacket { ref data, .. }, .. } = us {
Some(data)
} else {
None
})
),
(1, attribution_data, (legacy, AttributionData, |us|
(1, attribution_data, (legacy, AttributionData, |_| Ok(()), |us|
if let &HTLCFailReasonRepr::LightningError { err: msgs::OnionErrorPacket { ref attribution_data, .. }, .. } = us {
attribution_data.as_ref()
} else {
Expand All @@ -1961,7 +1961,7 @@ impl_writeable_tlv_based_enum!(HTLCFailReasonRepr,
(_unused, err, (static_value, msgs::OnionErrorPacket { data: data.ok_or(DecodeError::InvalidValue)?, attribution_data })),
},
(1, Reason) => {
(0, _failure_code, (legacy, u16,
(0, _failure_code, (legacy, u16, |_| Ok(()),
|r: &HTLCFailReasonRepr| match r {
HTLCFailReasonRepr::LightningError{ .. } => None,
HTLCFailReasonRepr::Reason{ failure_reason, .. } => Some(failure_reason.failure_code())
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2731,7 +2731,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
(5, AwaitingInvoice) => {
(0, expiration, required),
(2, retry_strategy, required),
(4, _max_total_routing_fee_msat, (legacy, u64,
(4, _max_total_routing_fee_msat, (legacy, u64, |_| Ok(()),
|us: &PendingOutboundPayment| match us {
PendingOutboundPayment::AwaitingInvoice { route_params_config, .. } => route_params_config.max_total_routing_fee_msat,
_ => None,
Expand All @@ -2748,7 +2748,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
(7, InvoiceReceived) => {
(0, payment_hash, required),
(2, retry_strategy, required),
(4, _max_total_routing_fee_msat, (legacy, u64,
(4, _max_total_routing_fee_msat, (legacy, u64, |_| Ok(()),
|us: &PendingOutboundPayment| match us {
PendingOutboundPayment::InvoiceReceived { route_params_config, .. } => route_params_config.max_total_routing_fee_msat,
_ => None,
Expand Down Expand Up @@ -2779,7 +2779,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
(11, AwaitingOffer) => {
(0, expiration, required),
(2, retry_strategy, required),
(4, _max_total_routing_fee_msat, (legacy, u64,
(4, _max_total_routing_fee_msat, (legacy, u64, |_| Ok(()),
|us: &PendingOutboundPayment| match us {
PendingOutboundPayment::AwaitingOffer { route_params_config, .. } => route_params_config.max_total_routing_fee_msat,
_ => None,
Expand Down
73 changes: 60 additions & 13 deletions lightning/src/util/ser_macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ macro_rules! _encode_tlv {
field.write($stream)?;
}
};
($stream: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $write: expr) $(, $self: ident)?) => { {
($stream: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $read: expr, $write: expr) $(, $self: ident)?) => { {
let value: Option<_> = $write($($self)?);
#[cfg(debug_assertions)]
{
Expand All @@ -63,6 +63,9 @@ macro_rules! _encode_tlv {
}
$crate::_encode_tlv!($stream, $optional_type, value, option);
} };
($stream: expr, $optional_type: expr, $optional_field: expr, (custom, $fieldty: ty, $read: expr, $write: expr) $(, $self: ident)?) => { {
$crate::_encode_tlv!($stream, $optional_type, $optional_field, (legacy, $fieldty, $read, $write) $(, $self)?);
} };
($stream: expr, $type: expr, $field: expr, optional_vec $(, $self: ident)?) => {
if !$field.is_empty() {
$crate::_encode_tlv!($stream, $type, $field, required_vec);
Expand Down Expand Up @@ -229,9 +232,12 @@ 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) $(, $self: ident)?) => {
($len: expr, $optional_type: expr, $optional_field: expr, (legacy, $fieldty: ty, $read: expr, $write: expr) $(, $self: ident)?) => {
$crate::_get_varint_length_prefixed_tlv_length!($len, $optional_type, $write($($self)?), option);
};
($len: expr, $optional_type: expr, $optional_field: expr, (custom, $fieldty: ty, $read: expr, $write: expr) $(, $self: ident)?) => {
$crate::_get_varint_length_prefixed_tlv_length!($len, $optional_type, $optional_field, (legacy, $fieldty, $read, $write) $(, $self)?);
};
($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);
Expand Down Expand Up @@ -314,9 +320,19 @@ 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)) => {{
($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (legacy, $fieldty: ty, $read: expr, $write: expr)) => {{
// no-op
}};
($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (custom, $fieldty: ty, $read: expr, $write: expr) $(, $self: ident)?) => {{
// Note that $type may be 0 making the second comparison always false
#[allow(unused_comparisons)]
let invalid_order =
($last_seen_type.is_none() || $last_seen_type.unwrap() < $type) && $typ.0 > $type;
if invalid_order {
let read_result: Result<_, DecodeError> = $read(None);
$field = read_result?.into();
}
}};
($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);
}};
Expand Down Expand Up @@ -382,8 +398,19 @@ 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, (legacy, $fieldty: ty, $read: expr, $write: expr)) => {{
use $crate::ln::msgs::DecodeError;
let read_result: Result<(), DecodeError> = $read($field.as_ref());
read_result?;
}};
($last_seen_type: expr, $type: expr, $field: ident, (custom, $fieldty: ty, $read: expr, $write: expr)) => {{
// Note that $type may be 0 making the second comparison always false
#[allow(unused_comparisons)]
let missing_req_type = $last_seen_type.is_none() || $last_seen_type.unwrap() < $type;
if missing_req_type {
let read_result: Result<_, DecodeError> = $read(None);
$field = read_result?.into();
}
}};
($last_seen_type: expr, $type: expr, $field: ident, (required, explicit_type: $fieldty: ty)) => {{
_check_missing_tlv!($last_seen_type, $type, $field, required);
Expand Down Expand Up @@ -438,9 +465,15 @@ macro_rules! _decode_tlv {
let _field: &Option<$fieldty> = &$field;
$crate::_decode_tlv!($outer_reader, $reader, $field, option);
}};
($outer_reader: expr, $reader: expr, $field: ident, (legacy, $fieldty: ty, $write: expr)) => {{
($outer_reader: expr, $reader: expr, $field: ident, (legacy, $fieldty: ty, $read: expr, $write: expr)) => {{
$crate::_decode_tlv!($outer_reader, $reader, $field, (option, explicit_type: $fieldty));
}};
Comment on lines -441 to 470
Copy link
Contributor Author

@jkczyz jkczyz Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why Claude didn't update _decode_tlv to use $read like for custom. Below is what it gave me.

@TheBlueMatt Do we want to use $read in _decode_tlv or in _check_missing_tlv? If we use it in _check_missing_tlv, would we have all non-legacy fields set whereas in _decode_tlv we only have the earlier TLVs set?

❯ I'm looking through the places where $read is used in ser_macros.rs for legacy TLV variant. Could you explain how each use (or lack
   thereof) compares to the custom TLV variant?

⏺ Let me read the current state of the macros to give an accurate comparison.

⏺ Explore(Compare legacy vs custom $read usage)
  ⎿  Done (10 tool uses · 28.7k tokens · 41s)

⏺ Here's the comparison across all 7 macros:

  Write-side macros (no $read usage in either variant)
  ┌─────────────────────────────────────────┬─────────────────────────────┬─────────────────────────┐
  │                  Macro                  │           legacy            │         custom          │
  ├─────────────────────────────────────────┼─────────────────────────────┼─────────────────────────┤
  │ _encode_tlv!                            │ $read ignored; calls $write │ Delegates to legacy arm │
  ├─────────────────────────────────────────┼─────────────────────────────┼─────────────────────────┤
  │ _get_varint_length_prefixed_tlv_length! │ $read ignored; calls $write │ Delegates to legacy arm │
  └─────────────────────────────────────────┴─────────────────────────────┴─────────────────────────┘
  These are purely write-path — neither variant uses $read.

  Init macros (no $read usage in either variant)
  Macro: _init_tlv_field_var!
  legacy: Delegates to (option, explicit_type: $fieldty) → Option<$fieldty>
  custom: Delegates to required → RequiredWrapper<_>
  ────────────────────────────────────────
  Macro: _init_tlv_based_struct_field!
  legacy: Delegates to option → returns $field as-is
  custom: Delegates to required → returns $field.0.unwrap()
  The key structural difference: legacy fields are Option<$fieldty> all the way through, while custom fields are RequiredWrapper that get
  unwrapped during struct construction.

  Read-side macros (where $read matters)

  _decode_tlv! — when a TLV is present in the stream:
  - legacy: Reads as (option, explicit_type: $fieldty), setting $field = Some(value). $read is not called.
  - custom: Reads as required, then calls $read(Some(read_field)) and assigns $field = read_result?.into(). $read is called with Some.

  _check_decoded_tlv_order! — when a higher-numbered TLV appears before this one (gap detection mid-stream):
  - legacy: No-op. $read is not called.
  - custom: Calls $read(None) and assigns $field = read_result?.into(). $read is called with None.

  _check_missing_tlv! — after all TLVs are read, for fields never seen:
  - legacy: Calls $read($field) (passing the Option<$fieldty>, which is None if absent or Some if present). $read is called
  unconditionally with the field value.
  - custom: Calls $read(None) only if the TLV was missing. $read is called conditionally with None.

  The key behavioral difference

  For custom, $read is called in three places that cover all cases:
  1. TLV present → _decode_tlv! calls $read(Some(value))
  2. TLV missing (gap mid-stream) → _check_decoded_tlv_order! calls $read(None)
  3. TLV missing (past end) → _check_missing_tlv! calls $read(None)

  For legacy, $read is called in only one place:
  1. _check_missing_tlv! calls $read($field) unconditionally — $field is Some(value) if the TLV was present (set by _decode_tlv!), or None
   if absent.

  This means legacy's $read always runs exactly once at the end, with the full Option<$fieldty> result of TLV reading. The custom
  variant's $read also runs exactly once, but from whichever of the three call sites fires first, and its return value goes through
  .into() to convert from Option into the field's actual type (since custom fields aren't necessarily Option).

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's an interesting point. The tradeoff is we do actually have to have a $field to write to, which kinda defeats the point of legacy (I mean it works fine in the struct impl macros, but not the low level ones). Maybe for now let's do the ordered version that avoids needing a field, its easy to relax that in the future if we need (whereas going the other way is harder - we'd have to audit every use).

($outer_reader: expr, $reader: expr, $field: ident, (custom, $fieldty: ty, $read: expr, $write: expr)) => {{
let read_field: $fieldty;
$crate::_decode_tlv!($outer_reader, $reader, read_field, required);
let read_result: Result<_, DecodeError> = $read(Some(read_field));
$field = read_result?.into();
}};
($outer_reader: expr, $reader: expr, $field: ident, (required, explicit_type: $fieldty: ty)) => {{
let _field: &$fieldty = &$field;
_decode_tlv!($outer_reader, $reader, $field, required);
Expand Down Expand Up @@ -827,9 +860,12 @@ macro_rules! _init_tlv_based_struct_field {
($field: ident, option) => {
$field
};
($field: ident, (legacy, $fieldty: ty, $write: expr)) => {
($field: ident, (legacy, $fieldty: ty, $read: expr, $write: expr)) => {
$crate::_init_tlv_based_struct_field!($field, option)
};
($field: ident, (custom, $fieldty: ty, $read: expr, $write: expr)) => {
$crate::_init_tlv_based_struct_field!($field, required)
};
($field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {
$crate::_init_tlv_based_struct_field!($field, option)
};
Expand Down Expand Up @@ -893,9 +929,12 @@ 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)) => {
($field: ident, (legacy, $fieldty: ty, $read: expr, $write: expr)) => {
$crate::_init_tlv_field_var!($field, (option, explicit_type: $fieldty));
};
($field: ident, (custom, $fieldty: ty, $read: expr, $write: expr)) => {
$crate::_init_tlv_field_var!($field, required);
};
($field: ident, (required, explicit_type: $fieldty: ty)) => {
let mut $field = $crate::util::ser::RequiredWrapper::<$fieldty>(None);
};
Expand Down Expand Up @@ -975,10 +1014,18 @@ macro_rules! _decode_and_build {
/// [`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
/// If `$fieldty` is `(legacy, $ty, $read, $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 by referring to the value by name).
/// `Some`. When reading, an optional field of type `$ty` is read, and after all TLV fields are
/// read, the `$read` closure is called with the `Option<&$ty>` value. The `$read` closure should
/// return a `Result<(), DecodeError>`. Legacy field values can be used in later
/// `default_value` or `static_value` fields by referring to the value by name.
/// If `$fieldty` is `(custom, $ty, $read, $write)` then, when writing, the same behavior as
/// `legacy`, above is used. When reading, if a TLV is present, it is read as `$ty` and the
/// `$read` method is called with `Some(decoded_$ty_object)`. If no TLV is present, the field
/// will be initialized by calling `$read(None)`. `$read` should return a
/// `Result<field type, DecodeError>` (note that the processed field type may differ from `$ty`;
/// `$ty` is the type as de/serialized, not necessarily the actual field type).
///
/// For example,
/// ```
Expand All @@ -996,7 +1043,7 @@ 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))),
/// (4, unwritten_type, (legacy, u32, |_| Ok(()), |us: &LightningMessage| Some(us.tlv_integer))),
/// (_unused, tlv_upgraded_integer, (static_value, unwritten_type.unwrap_or(0) * 2))
/// });
/// ```
Expand Down Expand Up @@ -1888,7 +1935,7 @@ mod tests {
new_field: (u8, u8),
}
impl_writeable_tlv_based!(ExpandedField, {
(0, old_field, (legacy, u8, |us: &ExpandedField| Some(us.new_field.0))),
(0, old_field, (legacy, u8, |_| Ok(()), |us: &ExpandedField| Some(us.new_field.0))),
(1, new_field, (default_value, (old_field.ok_or(DecodeError::InvalidValue)?, 0))),
});

Expand Down
Loading