Speed up TLV serialization length accounting by joostjager · Pull Request #4642 · lightningdevkit/rust-lightning

joostjager · 2026-05-28T13:34:54Z

Recent splice fuzzing coverage made it clear that some chanmon consistency inputs spend a surprising amount of time encoding snapshots. This PR tightens the serialization length path used by those snapshots.

Slow fuzz byte string used while checking this:

a1a0a3ffa2a1ffffa2a1ff00a2a1ffa2a1a0a3ffa2a1ff19ffffffffffffffac

For that byte string, median real time over 5 runs with target output suppressed:

Profile	Pre changes	Post changes
Debug	17.17s	3.61s
Release	0.60s	0.24s

ldk-reviews-bot · 2026-05-28T13:34:57Z

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

ldk-claude-review-bot · 2026-05-28T14:43:20Z

I've completed a thorough review of all changes in this PR. Here's my analysis:

Verification points checked:

_tlv_fields_serialized_length! macro: Computes len + BigSize(len).serialized_length(), matching exactly what _encode_varint_length_prefixed_tlv! writes (BigSize prefix + TLV stream body).
impl_writeable_tlv_based! refactoring: The old inlined length code and the new _tlv_fields_serialized_length! delegation are equivalent — both pass &self.field_name to _get_varint_length_prefixed_tlv_length! (the old code passed &self.$field directly, the new code passes self.$field and the macro adds &).
impl_writeable_only_tlv_fields!: Write path uses write_tlv_fields! and length path uses _tlv_fields_serialized_length!. Both receive the same field expressions. The $self ident is not forwarded to inner macros, which is consistent — the write path (write_tlv_fields!) also doesn't forward it. Current usages (required, option, required_vec, (required: ReadableArgs, ...)) don't need it.
Option<T>::serialized_length: None → 1 (matching 0u8.write()). Some(data) → BigSize(data_len + 1).serialized_length() + data_len (matching BigSize(data_len + 1).write() + data.write()).
WithoutLength<S>::serialized_length: Iterates elements calling serialized_length(), matching the write path that iterates calling write().
&T and Box<T> overrides: Both delegate to inner type, matching their write implementations.
debug_assert_eq! safety net in encode() (ser.rs:289) validates at test time that any serialized_length() override matches actual write() output.
legacy_deserialization_prevention_marker inlining: Previously a local variable (evaluated once), now an expression (evaluated twice in write path — once for length calc, once for encoding). The function is pure and deterministic, so this is safe.

No issues found.

ldk-reviews-bot · 2026-05-28T17:33:28Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

TheBlueMatt · 2026-05-29T09:20:53Z

 	let mut data = Vec::with_capacity(8192);
 	std::io::stdin().read_to_end(&mut data).unwrap();
-	zbase32_test(&data, test_logger::Stdout {});
+	if std::env::var_os("LDK_FUZZ_SUPPRESS_LOGS").is_some() {


Bleh. Tell claude env variables for runtime decisions is gross and not the clean solution it thinks it is. If the point of stdin-fuzz is to be used to replicate tests, we should leave stdout, if its purpose is as a fuzz target, it should be DevNull.

I wanted this, not Claude 😅 I use stdin_fuzz also to run big sets of test cases using a runner that parallelizes and also continues on failure to examine the rest. That wouldn't work using the cargo test and test_cases dir approach.

Open to other suggestions for a convenient way to feed strings without intermediate files, while being able to switch logging on or off.

For the time being, moved to #4646 to unblock this PR.

joostjager · 2026-05-29T10:46:15Z

~~Review comments addressed: https://github.com/lightningdevkit/rust-lightning/compare/b0e52bd..7dd5bca~~

Dropped commit, so no clean diff is possible anymore.

Add direct serialized length implementations for common serialization wrappers. This avoids routing field payload length calculations through in-memory writers for common nested serialization paths used by the existing TLV length helpers.

Add a write-only TLV helper macro that emits both write and serialized_length from the same field list. Reuse the shared TLV length helper from impl_writeable_tlv_based so the existing generated writer path and the new custom-read path stay aligned. Use the new helper for the hot channel funding and commitment transaction TLV writers while leaving their custom read implementations unchanged.

TheBlueMatt · 2026-05-30T09:03:08Z

+
+	#[inline]
+	fn serialized_length(&self) -> usize {
+		(**self).serialized_length()


nit: can we use the same syntax as the write?

TheBlueMatt · 2026-05-31T08:14:52Z

+/// [`Writeable`]: crate::util::ser::Writeable
+/// [`impl_writeable_tlv_based`]: crate::impl_writeable_tlv_based
+/// [`write_tlv_fields`]: crate::write_tlv_fields
+macro_rules! impl_writeable_only_tlv_fields {


Should we rename the existing macro(s) to impl_ser_tlv_based so that writeable_only_tlv_fields isn't so awkward?

joostjager requested a review from wpaulino May 28, 2026 14:28

joostjager marked this pull request as ready for review May 28, 2026 14:28

joostjager force-pushed the ser-tlv-length-fastpath branch from 7afe504 to 084d0ac Compare May 28, 2026 15:21

joostjager added this to Weekly Goals May 28, 2026

joostjager self-assigned this May 28, 2026

wpaulino reviewed May 28, 2026

View reviewed changes

Comment thread lightning/src/util/ser_macros.rs Outdated

joostjager force-pushed the ser-tlv-length-fastpath branch from 084d0ac to b0e52bd Compare May 28, 2026 17:41

joostjager requested a review from wpaulino May 28, 2026 18:01

tnull reviewed May 29, 2026

View reviewed changes

Comment thread lightning/src/util/ser_macros.rs

TheBlueMatt reviewed May 29, 2026

View reviewed changes

joostjager force-pushed the ser-tlv-length-fastpath branch from b0e52bd to 78817b6 Compare May 29, 2026 10:36

joostjager requested a review from TheBlueMatt May 29, 2026 10:40

joostjager force-pushed the ser-tlv-length-fastpath branch from 78817b6 to 2060cff Compare May 29, 2026 10:45

joostjager removed the request for review from TheBlueMatt May 29, 2026 10:48

joostjager force-pushed the ser-tlv-length-fastpath branch 3 times, most recently from 7dd5bca to 70b69e4 Compare May 29, 2026 13:35

joostjager added 2 commits May 29, 2026 15:35

Avoid nested TLV length counting writes

f188957

Add direct serialized length implementations for common serialization wrappers. This avoids routing field payload length calculations through in-memory writers for common nested serialization paths used by the existing TLV length helpers.

joostjager force-pushed the ser-tlv-length-fastpath branch from 70b69e4 to 18a137a Compare May 29, 2026 13:36

joostjager requested a review from TheBlueMatt May 29, 2026 13:45

TheBlueMatt reviewed May 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up TLV serialization length accounting#4642

Speed up TLV serialization length accounting#4642
joostjager wants to merge 2 commits into
lightningdevkit:mainfrom
joostjager:ser-tlv-length-fastpath

joostjager commented May 28, 2026

Uh oh!

ldk-reviews-bot commented May 28, 2026 •

edited

Loading

Uh oh!

ldk-claude-review-bot commented May 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

ldk-reviews-bot commented May 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt May 29, 2026

Uh oh!

joostjager May 29, 2026

Uh oh!

joostjager May 29, 2026

Uh oh!

joostjager May 29, 2026

Uh oh!

joostjager commented May 29, 2026 •

edited

Loading

Uh oh!

TheBlueMatt May 30, 2026

Uh oh!

TheBlueMatt May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

joostjager commented May 28, 2026

Uh oh!

ldk-reviews-bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-claude-review-bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented May 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt May 29, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager May 29, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager May 29, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager May 29, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt May 30, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ldk-reviews-bot commented May 28, 2026 •

edited

Loading

ldk-claude-review-bot commented May 28, 2026 •

edited

Loading

joostjager commented May 29, 2026 •

edited

Loading