Speed up TLV serialization length accounting#4642
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
I've completed a thorough re-review of all changes in this PR, including the truncated portions of the diff. I verified:
No issues found. |
joostjager
force-pushed
the
ser-tlv-length-fastpath
branch
from
7afe504 to
084d0ac
Compare
|
👋 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. |
joostjager
force-pushed
the
ser-tlv-length-fastpath
branch
from
084d0ac to
b0e52bd
Compare
| 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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Open to other suggestions for a convenient way to feed strings without intermediate files, while being able to switch logging on or off.
There was a problem hiding this comment.
For the time being, moved to #4646 to unblock this PR.
joostjager
force-pushed
the
ser-tlv-length-fastpath
branch
from
b0e52bd to
78817b6
Compare
joostjager
force-pushed
the
ser-tlv-length-fastpath
branch
from
78817b6 to
2060cff
Compare
|
Dropped commit, so no clean diff is possible anymore. |
joostjager
force-pushed
the
ser-tlv-length-fastpath
branch
4 times, most recently
from
70b69e4 to
18a137a
Compare
| /// [`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 { |
There was a problem hiding this comment.
Should we rename the existing macro(s) to impl_ser_tlv_based so that writeable_only_tlv_fields isn't so awkward?
There was a problem hiding this comment.
Wilmer also suggested this in #4642 (comment)
The delta is quite large, but made the change.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
Rename TLV macros that generate both Readable and Writeable impls to use the impl_ser_tlv_based prefix. Keep the MaybeReadable upgradable enum helpers and shared write-only enum helper under writeable naming so macro names match the traits they generate.
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 writeable TLV helper macro that emits both write and serialized_length from the same field list. Reuse the shared TLV length helper from impl_ser_tlv_based so the generated read/write 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.
joostjager
force-pushed
the
ser-tlv-length-fastpath
branch
from
18a137a to
eb47742
Compare
6 participants
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:
a1a0a3ffa2a1ffffa2a1ff00a2a1ffa2a1a0a3ffa2a1ff19ffffffffffffffacFor that byte string, median real time over 5 runs with target output suppressed: