Implement Readable for LengthReadable #3579

arik-so · 2025-01-30T17:45:15Z

Inbound Trampoline entrypoint packets will contain inner onions, which only implement LengthReadable. Until now, we never had to decode LengthReadable-only structs in TLV streams, so this issue never surfaced before, but going forward, this will allow us to parse inbound Trampoline data.

arik-so · 2025-01-30T17:45:52Z

lightning/src/util/ser_macros.rs

@@ -395,7 +395,7 @@ macro_rules! _decode_tlv {
 		$field = f.0;
 	}};
 	($outer_reader: expr, $reader: expr, $field: ident, option) => {{
-		$field = Some($crate::util::ser::Readable::read(&mut $reader)?);
+		$field = Some($crate::util::ser::LengthReadable::read(&mut $reader)?);


it might be prudent to change the one line line 388, too

Yea, probably should use it for all TLV value reads.

codecov · 2025-01-30T21:31:38Z

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.84%. Comparing base (31b32c5) to head (c92c81b).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/msgs.rs	60.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3579      +/-   ##
==========================================
+ Coverage   88.53%   88.84%   +0.31%     
==========================================
  Files         149      149              
  Lines      114567   116432    +1865     
  Branches   114567   116432    +1865     
==========================================
+ Hits       101428   103448    +2020     
+ Misses      10645    10514     -131     
+ Partials     2494     2470      -24

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

TheBlueMatt · 2025-01-31T20:56:10Z

lightning/src/util/ser.rs

@@ -363,14 +363,21 @@ where

 /// A trait that various higher-level LDK types implement allowing them to be read in
 /// from a [`Read`], requiring the implementer to provide the total length of the read.
-pub(crate) trait LengthReadable


Probably could update the docs since its now pub

TheBlueMatt · 2025-01-31T21:57:12Z

Feel free to squash.

shaavan · 2025-02-04T14:56:40Z

lightning/src/util/ser_macros.rs

Hello!
I was reviewing this and noticed that the rule at line 454 doesn’t use LengthReadable. I was curious—why is that the case?

($outer_reader: expr, $reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident))) => {{ $field = { let field: $encoding<$fieldty> = ser::Readable::read(&mut $reader)?; Some(field.0) }; }};

Would love to understand it. Thanks!

Yeah, I think this is the last missing one

CI is currently running with it added

shaavan

LGTM 🚀

TheBlueMatt · 2025-02-05T15:50:34Z

lightning/src/util/ser.rs

-/// A trait that various higher-level LDK types implement allowing them to be read in
-/// from a [`Read`], requiring the implementer to provide the total length of the read.
-pub(crate) trait LengthReadable
+/// A trait that various LDK types implement allowing them to be read in from a [`Read`], requiring


Were you gonna remove the weird reference to "various LDK types" implementing this? Its a public trait - anything can implement it now (and only a few LDK types actually implement it)

no more weird references

TheBlueMatt · 2025-02-05T15:51:14Z

lightning/src/util/ser.rs

-/// from a [`Read`], requiring the implementer to provide the total length of the read.
-pub(crate) trait LengthReadable
+/// A trait that various LDK types implement allowing them to be read in from a [`Read`], requiring
+/// the implementer to provide the total length of the read.


The implementer isn't providing the total length of the read, the implementer is relying on having the length of the buffer available when being read - the implementer is the struct being deserialized.

lightning/src/util/ser.rs

lightning/src/ln/msgs.rs

TheBlueMatt · 2025-02-05T17:10:56Z

lightning/src/util/ser.rs

+impl<T: Readable> LengthReadable for T {
+	#[inline]
+	fn read<R: Read>(reader: &mut R) -> Result<T, DecodeError> {
+		Readable::read(reader)


Should this fail if the expected length wasn't read?

given that without resorting to assert_eq, it would produce pretty ubiquitous overhead, we agreed not to

In this commit we rename this method to `read_from_fixed_length_buffer` to avoid ambiguities with `Readable`.

valentinewallace · 2025-02-06T19:34:12Z

lightning/src/ln/msgs.rs

@@ -2854,7 +2854,7 @@ impl<NS: Deref> ReadableArgs<(Option<PublicKey>, NS)> for InboundOnionPayload wh
 		let mut keysend_preimage: Option<PaymentPreimage> = None;
 		let mut custom_tlvs = Vec::new();

-		let tlv_len = BigSize::read(r)?;
+		let tlv_len = <BigSize as Readable>::read(r)?;


Looks like this can be reverted since the trait method was renamed, some other changes in this commit may also be unnecessary now?

fair point. Reverted

valentinewallace · 2025-02-06T20:16:33Z

lightning/src/ln/msgs.rs

@@ -1964,7 +1964,7 @@ impl LengthReadable for TrampolineOnionPacket {

 		let hop_data_len = r.total_bytes().saturating_sub(66); // 1 (version) + 33 (pubkey) + 32 (HMAC) = 66
 		let mut rd = FixedLengthReader::new(r, hop_data_len);
-		let hop_data = WithoutLength::<Vec<u8>>::read(&mut rd)?.0;
+		let hop_data = WithoutLength::<Vec<u8>>::read_from_fixed_length_buffer(&mut rd)?.0;


This should either go in a separate commit or the commit message should be updated for why we need this

it's actually not even necessary any longer, dropped

Inbound Trampoline entrypoint packets will contain inner onions, which only implement LengthReadable. Until now, we never had to decode LengthReadable-only structs in TLV streams, so this issue never surfaced before, but going forward, this will allow us to parse inbound Trampoline data.

A followup commit will require that LengthReadable be accessible from outside the lightning crate, as it will be the default trait referenced for TLV stream decoding.

Previously, our deserialization macros used the `Readable` trait by default. Now that `LengthReadable` is public and all `Readable` types also implement it, we can use `LengthReadable` instead.

arik-so commented Jan 30, 2025

View reviewed changes

arik-so force-pushed the length_readable_serde branch 2 times, most recently from 58c565c to c92c81b Compare January 30, 2025 21:23

TheBlueMatt reviewed Jan 31, 2025

View reviewed changes

arik-so force-pushed the length_readable_serde branch from c92c81b to 21acd71 Compare January 31, 2025 21:48

arik-so force-pushed the length_readable_serde branch from 21acd71 to e302a12 Compare January 31, 2025 22:14

arik-so mentioned this pull request Feb 3, 2025

WIP: Decrypt inbound Trampoline onions #3585

Draft

shaavan reviewed Feb 4, 2025

View reviewed changes

arik-so force-pushed the length_readable_serde branch from 4fc2a9d to 9189640 Compare February 4, 2025 21:42

valentinewallace previously approved these changes Feb 4, 2025

View reviewed changes

arik-so requested a review from TheBlueMatt February 5, 2025 01:03

shaavan approved these changes Feb 5, 2025

View reviewed changes

TheBlueMatt reviewed Feb 5, 2025

View reviewed changes

lightning/src/util/ser.rs Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Feb 5, 2025

View reviewed changes

Rename LengthReadable's read method

9ae25a8

In this commit we rename this method to `read_from_fixed_length_buffer` to avoid ambiguities with `Readable`.

arik-so dismissed valentinewallace’s stale review via 8f21227 February 5, 2025 18:00

arik-so force-pushed the length_readable_serde branch from 9189640 to 8f21227 Compare February 5, 2025 18:00

valentinewallace reviewed Feb 6, 2025

View reviewed changes

arik-so force-pushed the length_readable_serde branch from 8f21227 to a86e58e Compare February 6, 2025 19:41

valentinewallace reviewed Feb 6, 2025

View reviewed changes

arik-so added 3 commits February 6, 2025 12:32

Make LengthRead[able] public

ed4c52c

A followup commit will require that LengthReadable be accessible from outside the lightning crate, as it will be the default trait referenced for TLV stream decoding.

Use LengthReadable trait for deserialization

9dbe358

Previously, our deserialization macros used the `Readable` trait by default. Now that `LengthReadable` is public and all `Readable` types also implement it, we can use `LengthReadable` instead.

arik-so force-pushed the length_readable_serde branch from a86e58e to 9dbe358 Compare February 6, 2025 20:45

valentinewallace approved these changes Feb 6, 2025

View reviewed changes

TheBlueMatt approved these changes Feb 6, 2025

View reviewed changes

arik-so merged commit 8e90841 into lightningdevkit:main Feb 7, 2025
24 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Readable for LengthReadable #3579

Implement Readable for LengthReadable #3579

arik-so commented Jan 30, 2025

arik-so Jan 30, 2025

TheBlueMatt Jan 31, 2025

codecov bot commented Jan 30, 2025 •

edited

Loading

TheBlueMatt Jan 31, 2025

TheBlueMatt commented Jan 31, 2025

shaavan Feb 4, 2025

valentinewallace Feb 4, 2025

arik-so Feb 4, 2025

arik-so Feb 4, 2025

shaavan left a comment

TheBlueMatt Feb 5, 2025

arik-so Feb 5, 2025

TheBlueMatt Feb 5, 2025

arik-so Feb 5, 2025

TheBlueMatt Feb 5, 2025

arik-so Feb 5, 2025

valentinewallace Feb 6, 2025

arik-so Feb 6, 2025

valentinewallace Feb 6, 2025

arik-so Feb 6, 2025

Implement Readable for LengthReadable #3579

Implement Readable for LengthReadable #3579

Conversation

arik-so commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

TheBlueMatt commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2025 •

edited

Loading