Correct gossip forwarding criteria while doing background sync
What the developer wrote
Authored by Matt Corallo
Correct gossip forwarding criteria while doing background sync
If we're doing gossip backfill to a peer, we first forward all our
`channel_announcement`s and `channel_update`s in SCID-order. While
doing so, we don't forward any fresh `channel_update` messages for
any channels which we haven't yet backfilled (as we'll eventually
send the new update anyway, and it might get rejected without the
corresponding `channel_announcement`).
Sadly, our comparison for this was the wrong way, so we actually
*only* forwarded updates which were for channels we haven't yet
backfilled, and dropped updates for channels we already had
backfilled.
What changed, and why it matters
This commit fixes a flipped comparison in Lightning Dev Kit's gossip message forwarding. During a background sync, nodes share old channel announcements in order. To avoid sending updates before their matching announcements, the code was supposed to temporarily hold back fresh updates for channels not yet synced. Instead, the comparison was backwards, so it held back updates for already-synced channels and forwarded only updates for not-yet-synced channels. The fix changes the comparison direction and adds a test to confirm the correct behavior.
Apply the patch to correct the comparison and run the new regression test. Monitor for any related gossip synchronization issues in deployed nodes, particularly those doing full sync with peers.
Security signals we found
Logic error in gossip forwarding filter
Inverted comparison operator in sync-state guard
New regression test added for forwarding behavior
Test helper added to expose encoded message contents
Evidence from the diff
In Peer::should_forward_channel_update_with_scid, the InitSyncTracker::ChannelsSyncing(i) branch used i < channel_id, which inverted the intended filter. The intended logic is to forward a channel_update only if its SCID is less than the current sync cursor (i.e., already backfilled), so the comparison is corrected to channel_id < i. The patch also adds test infrastructure to inspect encrypted message buffers and a new test, test_forward_while_syncing, verifying that an update for SCID 100 (already backfilled) is forwarded while an update for SCID 5000 (not yet backfilled) is buffered.
Changed components
lightning/src/ln/peer_handler.rslightning/src/ln/peer_channel_encryptor.rslightning/src/util/test_utils.rsInspect captured patch +81 / −2
diff --git a/lightning/src/ln/peer_channel_encryptor.rs b/lightning/src/ln/peer_channel_encryptor.rs
index 3103088..c7a7ba5 100644
--- a/lightning/src/ln/peer_channel_encryptor.rs
+++ b/lightning/src/ln/peer_channel_encryptor.rs
@@ -656,6 +656,11 @@ impl MessageBuf {
res[16 + 2..].copy_from_slice(&encoded_msg);
Self(res)
}
+
+ #[cfg(test)]
+ pub(crate) fn fetch_encoded_msg_with_type_pfx(&self) -> Vec<u8> {
+ self.0.clone().split_off(16 + 2)
+ }
}
#[cfg(test)]
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index c1809fd..c41fe1f 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -828,7 +828,7 @@ impl Peer {
}
match self.sync_status {
InitSyncTracker::NoSyncRequested => true,
- InitSyncTracker::ChannelsSyncing(i) => i < channel_id,
+ InitSyncTracker::ChannelsSyncing(i) => channel_id < i,
InitSyncTracker::NodesSyncing(_) => true,
}
}
@@ -4403,6 +4403,80 @@ mod tests {
assert_eq!(cfgs[1].routing_handler.chan_anns_recvd.load(Ordering::Acquire), 54);
}
+ #[test]
+ fn test_forward_while_syncing() {
+ use crate::ln::peer_handler::tests::test_utils::get_dummy_channel_update;
+
+ // Test forwarding new channel announcements while we're doing syncing.
+ let cfgs = create_peermgr_cfgs(2);
+ cfgs[0].routing_handler.request_full_sync.store(true, Ordering::Release);
+ cfgs[1].routing_handler.request_full_sync.store(true, Ordering::Release);
+ cfgs[0].routing_handler.announcement_available_for_sync.store(true, Ordering::Release);
+ cfgs[1].routing_handler.announcement_available_for_sync.store(true, Ordering::Release);
+ let peers = create_network(2, &cfgs);
+
+ let (mut fd_a, mut fd_b) = establish_connection(&peers[0], &peers[1]);
+
+ // Iterate a handful of times to exchange some messages
+ for _ in 0..150 {
+ peers[1].process_events();
+ let a_read_data = fd_b.outbound_data.lock().unwrap().split_off(0);
+ assert!(!a_read_data.is_empty());
+
+ peers[0].read_event(&mut fd_a, &a_read_data).unwrap();
+ peers[0].process_events();
+
+ let b_read_data = fd_a.outbound_data.lock().unwrap().split_off(0);
+ assert!(!b_read_data.is_empty());
+ peers[1].read_event(&mut fd_b, &b_read_data).unwrap();
+
+ peers[0].process_events();
+ assert_eq!(
+ fd_a.outbound_data.lock().unwrap().len(),
+ 0,
+ "Until A receives data, it shouldn't send more messages"
+ );
+ }
+
+ // Forward one more gossip backfill message but don't flush it so that we can examine the
+ // unencrypted message for broadcasts.
+ fd_b.hang_writes.store(true, Ordering::Relaxed);
+ peers[1].process_events();
+
+ {
+ let peer_lock = peers[1].peers.read().unwrap();
+ let peer = peer_lock.get(&fd_b).unwrap().lock().unwrap();
+ assert_eq!(peer.pending_outbound_buffer.len(), 1);
+ assert_eq!(peer.gossip_broadcast_buffer.len(), 0);
+ }
+
+ // At this point we should have sent channel announcements up to roughly SCID 150. Now
+ // build an updated update for SCID 100 and SCID 5000 and make sure only the one for SCID
+ // 100 gets forwarded
+ let msg_100 = get_dummy_channel_update(100);
+ let msg_ev_100 = MessageSendEvent::BroadcastChannelUpdate { msg: msg_100.clone() };
+
+ let msg_5000 = get_dummy_channel_update(5000);
+ let msg_ev_5000 = MessageSendEvent::BroadcastChannelUpdate { msg: msg_5000 };
+
+ fd_a.hang_writes.store(true, Ordering::Relaxed);
+
+ cfgs[1].routing_handler.pending_events.lock().unwrap().push(msg_ev_100);
+ cfgs[1].routing_handler.pending_events.lock().unwrap().push(msg_ev_5000);
+ peers[1].process_events();
+
+ {
+ let peer_lock = peers[1].peers.read().unwrap();
+ let peer = peer_lock.get(&fd_b).unwrap().lock().unwrap();
+ assert_eq!(peer.pending_outbound_buffer.len(), 1);
+ assert_eq!(peer.gossip_broadcast_buffer.len(), 1);
+
+ let pending_msg = &peer.gossip_broadcast_buffer[0];
+ let expected = encode_msg!(&msg_100);
+ assert_eq!(expected, pending_msg.fetch_encoded_msg_with_type_pfx());
+ }
+ }
+
#[test]
fn test_handshake_timeout() {
// Tests that we time out a peer still waiting on handshake completion after a full timer
diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs
index 769c2a3..16eb5da 100644
--- a/lightning/src/util/test_utils.rs
+++ b/lightning/src/util/test_utils.rs
@@ -1355,7 +1355,7 @@ fn get_dummy_channel_announcement(short_chan_id: u64) -> msgs::ChannelAnnounceme
}
}
-fn get_dummy_channel_update(short_chan_id: u64) -> msgs::ChannelUpdate {
+pub fn get_dummy_channel_update(short_chan_id: u64) -> msgs::ChannelUpdate {
use bitcoin::secp256k1::ffi::Signature as FFISignature;
let network = Network::Testnet;
msgs::ChannelUpdate {
Why this scored 36/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.