fuzz: Add cross-version ChannelMonitor roundtrip coverage#4622
fuzz: Add cross-version ChannelMonitor roundtrip coverage#4622Atishyy27 wants to merge 1 commit into
Conversation
|
👋 Thanks for assigning @ldk-claude-review-bot as a reviewer! |
| let mut monitor_bytes = serialized_mon.clone(); | ||
| if use_old_mons % 3 == 2 { | ||
| let keys_manager_0_2 = lightning_0_2::sign::KeysManager::new(&[0u8; 32], 0, 0, false); | ||
| let read_res: Result<( | ||
| bitcoin::BlockHash, | ||
| lightning_0_2::chain::channelmonitor::ChannelMonitor<lightning_0_2::sign::InMemorySigner> | ||
| ), _> = lightning_0_2::util::ser::ReadableArgs::read( | ||
| &mut &monitor_bytes[..], | ||
| (&keys_manager_0_2, &keys_manager_0_2) | ||
| ); | ||
|
|
||
| if let Ok((block_hash_0_2, old_monitor)) = read_res { | ||
| let mut reserialized = Vec::new(); | ||
| lightning_0_2::util::ser::Writeable::write(&(block_hash_0_2, &old_monitor), &mut reserialized).unwrap(); | ||
| monitor_bytes = reserialized; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: The use_old_mons /= 3; line that was here before was deleted and not re-added. This line is essential — it shifts the base-3 value so that when node B has two monitors (one for the A-B channel, one for the B-C channel), each monitor can use a different reload strategy.
Without this line, every iteration of the for (channel_id, mut prev_state) in old_monitors.drain() loop uses the same use_old_mons % 3 value, so both monitors always get the same treatment. This is a regression in fuzz coverage.
The comment at lines 972-973 still describes the intended behavior ("shifting use_old_mons one in base-3") but the actual shift was removed.
| let mut monitor_bytes = serialized_mon.clone(); | |
| if use_old_mons % 3 == 2 { | |
| let keys_manager_0_2 = lightning_0_2::sign::KeysManager::new(&[0u8; 32], 0, 0, false); | |
| let read_res: Result<( | |
| bitcoin::BlockHash, | |
| lightning_0_2::chain::channelmonitor::ChannelMonitor<lightning_0_2::sign::InMemorySigner> | |
| ), _> = lightning_0_2::util::ser::ReadableArgs::read( | |
| &mut &monitor_bytes[..], | |
| (&keys_manager_0_2, &keys_manager_0_2) | |
| ); | |
| if let Ok((block_hash_0_2, old_monitor)) = read_res { | |
| let mut reserialized = Vec::new(); | |
| lightning_0_2::util::ser::Writeable::write(&(block_hash_0_2, &old_monitor), &mut reserialized).unwrap(); | |
| monitor_bytes = reserialized; | |
| } | |
| } | |
| let mut monitor_bytes = serialized_mon.clone(); | |
| if use_old_mons % 3 == 2 { | |
| let keys_manager_0_2 = lightning_0_2::sign::KeysManager::new(&[0u8; 32], 0, 0, false); | |
| let read_res: Result<( | |
| bitcoin::BlockHash, | |
| lightning_0_2::chain::channelmonitor::ChannelMonitor<lightning_0_2::sign::InMemorySigner> | |
| ), _> = lightning_0_2::util::ser::ReadableArgs::read( | |
| &mut &monitor_bytes[..], | |
| (&keys_manager_0_2, &keys_manager_0_2) | |
| ); | |
| if let Ok((block_hash_0_2, old_monitor)) = read_res { | |
| let mut reserialized = Vec::new(); | |
| lightning_0_2::util::ser::Writeable::write(&(block_hash_0_2, &old_monitor), &mut reserialized).unwrap(); | |
| monitor_bytes = reserialized; | |
| } | |
| } | |
| use_old_mons /= 3; |
| use_old_mons /= 3; | ||
| let mut monitor_bytes = serialized_mon.clone(); | ||
| if use_old_mons % 3 == 2 { | ||
| let keys_manager_0_2 = lightning_0_2::sign::KeysManager::new(&[0u8; 32], 0, 0, false); |
There was a problem hiding this comment.
The KeysManager is initialized with an all-zero seed [0u8; 32], while the actual node uses a KeyProvider with secret key [0..0, 1, node_id]. Since read_chan_signer reads the signer data from the serialized bytes this likely works, but it would be more correct to derive the seed from the node context (or at minimum add a comment explaining why all-zeros is acceptable here).
| lightning-liquidity = { path = "../lightning-liquidity" } | ||
| lightning-rapid-gossip-sync = { path = "../lightning-rapid-gossip-sync" } | ||
| lightning-persister = { path = "../lightning-persister", features = ["tokio"]} | ||
| lightning_0_2 = { package = "lightning", version = "0.2.0", features = ["_test_utils"] } |
There was a problem hiding this comment.
Adding lightning v0.2.0 from crates.io as a dependency with the _test_utils feature. Be aware that _test_utils is a semver-exempt internal feature — if it changes or is removed in a point release, this will break. Also, pulling a published crate alongside the local path version of the same crate may cause type/trait incompatibilities (e.g., bitcoin::BlockHash from the crates.io dependency's transitive deps vs. the workspace's bitcoin). Please verify that the bitcoin types from both dependency trees are the same version and actually unify.
|
After thorough re-review of the entire diff, I find that the issues raised in my prior review have been addressed by the current code:
The remaining concerns from my prior review (all-zero No new issues found beyond what was already flagged. |
Atishyy27
force-pushed
the
feature/fuzz-chanmon-cross-version-roundtrip
branch
from
e0cb071 to
b8db756
Compare
3 participants
Addresses #4452.
This extends
chanmon_consistencywith optional cross-versionChannelMonitordeserialize/reserialize roundtrip coverage duringreload flows.
When selected by the fuzz input, serialized monitor bytes are:
lightning_0_2lightning_0_2This exercises cross-version monitor persistence/reload paths during
fuzzing while keeping version-specific runtime types isolated.
While developing this path, fuzzing also reproduced the
persistence/reload asymmetry behavior tracked in #4518 and fixed in
#4520.
This builds on the upgrade/downgrade fuzzing direction discussed in
#4452.