Add custom TLV read/write

[jkczyz]

The legacy TLV variant allows for custom writing, but it doesn't support custom read logic. This PR adds general custom TLV read/write support along with custom read support for the legacy variant. This is useful as it allows us to update a previously read TLV with the value from a legacy TLV.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[jkczyz]

Need to check Claude's work...

[jkczyz]

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).

[TheBlueMatt]

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).

[TheBlueMatt]

I don't think we should assign a $field here - legacy is all about not having a field to assign anymore.

[TheBlueMatt]

Oh! I guess we were already reading an object lol. Well I take back my earlier feedback, running the $read at the end vs in the loop is probably a good thing!

[jkczyz]

Ok, so the original version that only calling $read in _check_missing_tlv? FWIW, I believe we need the field because it can be used in default_value or static_value.

[TheBlueMatt]

yea, i think so. Claude was right lol

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.01%. Comparing base (817ab5e) to head (579f14f).
⚠️ Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4390      +/-   ##
==========================================
- Coverage   86.01%   86.01%   -0.01%     
==========================================
  Files         156      156              
  Lines      102857   103002     +145     
  Branches   102857   103002     +145     
==========================================
+ Hits        88476    88595     +119     
- Misses      11871    11898      +27     
+ Partials     2510     2509       -1     

Flag	Coverage Δ
tests	86.01% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

Okay one last suggestion sorry. So $read() is called on the $field that we decoded, but having it return $fieldty is a confusing API - its result is actually entirely unused and only exists to allow for error cases. It makes the callsites a bit more annoying (|_| Ok(()) instead of Ok) but IMO this is a cleaner API:

Suggested change

	let read_result: Result<_, DecodeError> = $read($field);
	#[allow(unused_assignments)]
	{
	$field = read_result?;
	}
	let read_result: Result<(), DecodeError> = $read($field);
	read_result?;

[jkczyz]

Hmm... this doesn't seem to work in the presence of static_value:

    Checking lightning v0.3.0+git (/Users/jkczyz/src/rust-lightning/lightning)
error[E0382]: use of moved value: `data`
    --> lightning/src/ln/onion_utils.rs:1961:64
     |
1961 |         (_unused, err, (static_value, msgs::OnionErrorPacket { data: data.ok_or(DecodeError::InvalidValue)?, attribution_data })),
     |                                                                      ^^^^ value used here after move
     |
    ::: lightning/src/util/ser_macros.rs:403:52
     |
403  |         let read_result: Result<(), DecodeError> = $read($field);
     |                                                          ------ value moved here
...
930  |         let mut $field: Option<$fieldty> = None;
     |             ---------- move occurs because `data` has type `Option<std::vec::Vec<u8>>`, which does not implement the `Copy` trait
     |
help: consider cloning the value if the performance cost is acceptable
    -->  lightning/src/util/ser_macros.rs:403:58
     |
403  |         let read_result: Result<(), DecodeError> = $read($field.clone());
     |                                                                ++++++++

error[E0382]: use of moved value: `attribution_data`
    --> lightning/src/ln/onion_utils.rs:1961:104
     |
1961 |         (_unused, err, (static_value, msgs::OnionErrorPacket { data: data.ok_or(DecodeError::InvalidValue)?, attribution_data })),
     |                                                                                                              ^^^^^^^^^^^^^^^^ value used here after move
     |
    ::: lightning/src/util/ser_macros.rs:403:52
     |
403  |         let read_result: Result<(), DecodeError> = $read($field);
     |                                                          ------ value moved here
...
930  |         let mut $field: Option<$fieldty> = None;
     |             ---------- move occurs because `attribution_data` has type `Option<onion_utils::AttributionData>`, which does not implement the `Copy` trait
     |
help: consider cloning the value if the performance cost is acceptable
    -->  lightning/src/util/ser_macros.rs:403:58
     |
403  |         let read_result: Result<(), DecodeError> = $read($field.clone());
     |                                                                ++++++++

For more information about this error, try `rustc --explain E0382`.
error: could not compile `lightning` (lib) due to 2 previous errors

[jkczyz]

I guess passing a reference may work...

[TheBlueMatt]

Discussed offline but, yea, for now maybe the answer is reference.

[jkczyz]

Done