fuzz: Add upgrade/downgrade simulation to chanmon_consistency and fix chacha20 build#4499
fuzz: Add upgrade/downgrade simulation to chanmon_consistency and fix chacha20 build#4499Atishyy27 wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Hi! This PR is now in draft status. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4499 +/- ##
==========================================
+ Coverage 86.18% 86.21% +0.03%
==========================================
Files 160 160
Lines 107441 107441
Branches 107441 107441
==========================================
+ Hits 92593 92634 +41
+ Misses 12229 12189 -40
+ Partials 2619 2618 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
requested review from
joostjager
and removed request for
valentinewallace
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Huh? In order to do an upgrade/downgrade we'll need to actually depend on a previous version of LDK.
|
Ah, I see! I was initially aiming for forward-compatibility coverage by simulating unknown odd TLVs. If the goal is actual cross-version testing, should I add a previous version (e.g., |
|
See the upgrade tests in |
|
Got it. I checked Before I go too deep into the implementation for the fuzzer: since Shall I go ahead and duplicate those test harness implementations for the 0.2 dependency, or is there a lighter-weight serialization round-trip pattern you had in mind for the fuzzer? |
|
I was hoping we'd be able to keep the existing test harnesses but just implement both traits on them - moving common logic to util methods to avoid duplicate code. |
| let mut modified_ser = ser.clone(); | ||
| if use_old_mons % 2 == 0 { | ||
| modified_ser = append_dummy_tlv(modified_ser, 0xBADF00D1); | ||
| } | ||
|
|
||
| let manager = | ||
| <(BlockHash, ChanMan)>::read(&mut &ser[..], read_args).expect("Failed to read manager"); | ||
| <(BlockHash, ChanMan)>::read(&mut &modified_ser[..], read_args).expect("Failed to read manager"); |
There was a problem hiding this comment.
Bug: Appended TLV is never parsed — this test is a no-op.
ChannelManager deserialization uses read_tlv_fields! (see channelmanager.rs:18488), which reads a BigSize length prefix and then creates a FixedLengthReader bounded to exactly that many bytes (ser_macros.rs:841-842):
let tlv_len: BigSize = Readable::read($stream)?;
let mut rd = FixedLengthReader::new($stream, tlv_len.0);All TLV decoding happens within this bounded reader. After the TLV region, the reader returns to the outer stream, and ChannelManagerData::read() returns without consuming any further bytes.
Appending a dummy TLV to the end of the serialized byte vector places it outside the length-prefixed TLV region. The FixedLengthReader will never see these bytes — they are simply left unconsumed in the &[u8] slice.
This means the fuzzer is not testing upgrade/downgrade resilience to unknown odd TLVs. The appended bytes are completely invisible to the deserializer.
To properly test this, the dummy TLV must be injected inside the TLV region, which requires:
- Locating the TLV length prefix in the byte stream
- Inserting the dummy TLV bytes after the last existing TLV (to maintain type ordering —
0xBADF00D1is large enough to come last) - Updating the length prefix to account for the additional bytes
| }; | ||
|
|
||
| let mut modified_ser = ser.clone(); | ||
| if use_old_mons % 2 == 0 { |
There was a problem hiding this comment.
Design issue: use_old_mons has already been modified by the monitor selection loop.
By line 1047, use_old_mons has been divided by 3 once per channel monitor in the loop at line 1011 (use_old_mons /= 3). For example, if the original fuzzer byte is 170 and there are 2 monitors, use_old_mons is 170 / 3 / 3 = 18 by this point.
This means the TLV-injection decision is coupled to the monitor selection logic rather than being independently controllable. It would be cleaner to consume a separate fuzzer input byte for this decision, so the fuzzer can independently explore the TLV injection path without it being a side effect of the monitor selection state.
|
|
Atishyy27
force-pushed
the
feature/fuzzer-upgrade-downgrade
branch
from
0891df8 to
90980cc
Compare
Atishyy27
force-pushed
the
feature/fuzzer-upgrade-downgrade
branch
from
90980cc to
2358ca7
Compare
|
|
||
| pub fn seek_to_block(&mut self, _block_offset: u32) {} |
There was a problem hiding this comment.
Formatting: mixed whitespace and trailing space.
Line 337 uses spaces for a blank line while the rest of the file uses tabs. Line 338 has a trailing space after {}. Run cargo fmt to fix.
| pub fn seek_to_block(&mut self, _block_offset: u32) {} | |
| pub fn seek_to_block(&mut self, _block_offset: u32) {} |
Also: the PR description claims this fixes a compile error for fuzzer binaries, but seek_to_block is only called from a #[cfg(test)] function (line 592). Fuzz binaries don't compile test code. Could you confirm which build configuration actually fails without this? (It may be needed when running cargo test with cfg(fuzzing) set, but that's different from the fuzzer binaries themselves.)
4 participants
Closes #4452
This PR introduces upgrade/downgrade serialization coverage to the
chanmon_consistencyfuzzer as discussed.Changes:
reload_nodeclosure inchanmon_consistency.rs. Based on a fuzzer input byte, it randomly appends a dummy/unknown odd-type TLV record (0xBADF00D1) to the serializedChannelManagerbyte stream before reloading. This simulates loading data written by a newer version, ensuring we don't drop channels on unknown odd TLVs.seek_to_blockdummy method to the#[cfg(fuzzing)]block ofChaCha20inlightning/src/crypto/chacha20.rs. Without this, the fuzzer binaries currently fail to compile onmain(Exit Code 1).