← Watch feed
Add a read closure to the `legacy` TLV variant
Public commit record
73/100 · Adequate
What the developer wrote
Authored by Jeffrey Czyz
Add a read closure to the `legacy` TLV variant
Update the `legacy` TLV read/write variant signature from
`(legacy, $fieldty, $write)` to `(legacy, $fieldty, $read, $write)`,
adding a read closure parameter matching the `custom` variant's
signature.
The read closure is applied in `_check_missing_tlv!` after all TLV
fields are read but before `static_value` fields consume legacy
values. This preserves backwards compatibility with `static_value` and
`default_value` expressions that reference legacy field variables as
`Option<$fieldty>` during TLV reading.
The read closure signature differs from `custom`:
`FnOnce(Option<&$fieldty>) -> Result<(), DecodeError>`. All existing
usage sites return `Ok(())` as their read closure (no-op).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Names security-relevant behavior explicitly
What changed, and why it matters
This commit is a small, internal refactoring of a Rust serialization macro used in the Lightning Dev Kit. It adds an optional read-time callback to the 'legacy' type-length-value (TLV) field variant so future code can validate or reject decoded legacy values. All existing usage sites pass a no-op callback that always succeeds, so the change does not alter current behavior. It is a defensive/extensibility improvement, not a fix for an active security bug.
Recommended action
No immediate action required. Treat as routine maintenance. If deploying a release containing this commit, verify that downstream serialization tests still pass and that any future `legacy` read closures are reviewed for correctness, since they can now return `DecodeError` and abort deserialization.
Security signals we found
01
Adds a read-time validation hook for legacy TLV fields
02
All existing read closures are no-ops (`|_| Ok(())`)
03
Preserves backward compatibility with `static_value` and `default_value` expressions
04
No new bounds checks, cryptographic operations, or network parsing changes
05
No vendor security advisory or CVE referenced in commit
Evidence from the diff
The patch updates impl_writeable_tlv_based! and related macros in lightning/src/util/ser_macros.rs so the legacy field descriptor takes a new FnOnce(Option<&$fieldty>) -> Result<(), DecodeError> closure. The closure is invoked in _check_missing_tlv! after TLV decoding but before static_value/default_value expressions consume the legacy value. Existing call sites in package.rs, channel_state.rs, onion_utils.rs, and outbound_payment.rs were updated to supply |_| Ok(()), preserving exact prior behavior. The change aligns the legacy signature with the existing custom variant and enables future read-time validation of legacy fields without breaking references to them as Option<$fieldty>.
Changed components
lightning/src/util/ser_macros.rslightning/src/chain/package.rslightning/src/ln/channel_state.rslightning/src/ln/onion_utils.rslightning/src/ln/outbound_payment.rsInspect captured patch +29 / −25
diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs
index 0abe353..0ef8855 100644
--- a/lightning/src/chain/package.rs
+++ b/lightning/src/chain/package.rs
@@ -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.
});
diff --git a/lightning/src/ln/channel_state.rs b/lightning/src/ln/channel_state.rs
index 86e53ba..eda79e0 100644
--- a/lightning/src/ln/channel_state.rs
+++ b/lightning/src/ln/channel_state.rs
@@ -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),
@@ -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),
diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs
index d48fcb2..605f27e 100644
--- a/lightning/src/ln/onion_utils.rs
+++ b/lightning/src/ln/onion_utils.rs
@@ -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 {
@@ -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())
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ea33bb5..170e4e1 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -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,
@@ -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,
@@ -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,
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index bd2b5d1..cc95fe6 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -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)]
{
@@ -64,7 +64,7 @@ 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, $write) $(, $self)?);
+ $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() {
@@ -232,11 +232,11 @@ 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, $write) $(, $self)?);
+ $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() {
@@ -320,7 +320,7 @@ 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)?) => {{
@@ -398,8 +398,10 @@ 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
@@ -463,7 +465,7 @@ 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));
}};
($outer_reader: expr, $reader: expr, $field: ident, (custom, $fieldty: ty, $read: expr, $write: expr)) => {{
@@ -858,7 +860,7 @@ 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)) => {
@@ -927,7 +929,7 @@ 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)) => {
@@ -1012,10 +1014,12 @@ 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
@@ -1039,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))
/// });
/// ```
@@ -1931,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))),
});
Risk score
Our methodology →Why this scored 18/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.