← Watch feed
Stop enqueueing error messages for disconnected peers
Public commit record
73/100 · Adequate
What the developer wrote
Authored by Matt Corallo
Stop enqueueing error messages for disconnected peers
If a channel is failed while a peer is disconnected, we'll still
have a `PeerState` for that peer. Historically, we haven't bothered
to check if a peer is actually connected before we push the `error`
message onto the `PeerState::pending_msg_events` queue, leaving us
sending messages into the void.
This is generally not an issue as
`ChannelManager::get_and_clear_pending_msg_events` should be called
very regularly, removing these messages and then dropping them as
`PeerManager` won't have anything to do with them. Further, when
the the message is an `error`, if a peer happens to connect between
when we push the message and when `get_and_clear_pending_msg_events`
is called the worst that happens is they get the `error` message
we'd end up sending them when they try to reestablish the channel
anyway.
Still, its awkward to leave the `error`s lying around in a message
queue for a disconnected peer, so we remove them here.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This change stops a Lightning node from queuing 'error' messages for peers that are currently disconnected. Previously, when a channel failed while a peer was offline, the node would still put an error message in that peer's outgoing message queue. The message would normally be discarded harmlessly later, but it was untidy and could briefly sit in the queue. The patch simply checks whether the peer is connected before adding the message. It is described by the authors as a cleanup rather than a security fix, with no practical exploit identified.
Recommended action
Treat as a minor hygiene fix. No urgent security action is warranted based on the commit content and committer's own assessment. Users may upgrade at normal cadence.
Security signals we found
01
Behavioral change in error-message queuing for disconnected peers
02
Test expectations changed from expecting broadcast/error message to not expecting one
03
Commit message frames change as cleanup/awkwardness, not security vulnerability
04
No bounds, lifetime, cryptographic, or authorization changes
Evidence from the diff
In rust-lightning’s ChannelManager, the handle_error! macro previously pushed error-message events into PeerState::pending_msg_events without checking peer_state.is_connected. The patch adds an if peer_state.is_connected guard, so messages are only queued for live peers. Test helpers are updated: check_closed_broadcast expectations flip from true (broadcast/error queued) to false (no broadcast/error queued) in cases where the counterparty is disconnected. One reorg test is adjusted to explicitly disconnect/reconnect peers so the expected message behavior remains testable. The commit message explicitly downplays security impact, noting queued messages are cleared by regular event polling and that an error sent on reconnection would be sent anyway during channel reestablishment.
Changed components
lightning/src/ln/channelmanager.rs handle_error! macroPeerState::pending_msg_events queue handlingLN channel failure / force-close error message generationInspect captured patch +28 / −16
diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs
index e0de92c..7b09831 100644
--- a/lightning/src/ln/chanmon_update_fail_tests.rs
+++ b/lightning/src/ln/chanmon_update_fail_tests.rs
@@ -3897,7 +3897,7 @@ fn do_test_durable_preimages_on_closed_channel(
}
}
if !close_chans_before_reload {
- check_closed_broadcast(&nodes[1], 1, true);
+ check_closed_broadcast(&nodes[1], 1, false);
let reason = ClosureReason::CommitmentTxConfirmed;
check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 100000);
}
@@ -3914,7 +3914,7 @@ fn do_test_durable_preimages_on_closed_channel(
check_spends!(bs_preimage_tx, as_closing_tx[0]);
mine_transactions(&nodes[0], &[&as_closing_tx[0], bs_preimage_tx]);
- check_closed_broadcast(&nodes[0], 1, true);
+ check_closed_broadcast(&nodes[0], 1, false);
expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
if !close_chans_before_reload || close_only_a {
@@ -4063,7 +4063,7 @@ fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) {
Event::ChannelClosed { .. } => {},
_ => panic!(),
}
- check_closed_broadcast!(nodes[1], true);
+ check_closed_broadcast(&nodes[1], 1, false);
}
// Once we run event processing the monitor should free, check that it was indeed the B<->C
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 78bf950..b93f289 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3228,7 +3228,9 @@ macro_rules! handle_error {
let per_peer_state = $self.per_peer_state.read().unwrap();
if let Some(peer_state_mutex) = per_peer_state.get(&$counterparty_node_id) {
let mut peer_state = peer_state_mutex.lock().unwrap();
- peer_state.pending_msg_events.push(msg_event);
+ if peer_state.is_connected {
+ peer_state.pending_msg_events.push(msg_event);
+ }
}
}
@@ -18808,7 +18810,7 @@ mod tests {
.node
.force_close_broadcasting_latest_txn(&chan_id, &nodes[1].node.get_our_node_id(), message.clone())
.unwrap();
- check_closed_broadcast(&nodes[0], 1, true);
+ check_closed_broadcast(&nodes[0], 1, false);
check_added_monitors(&nodes[0], 1);
let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
check_closed_event!(nodes[0], 1, reason, [nodes[1].node.get_our_node_id()], 100000);
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index d8f0966..89677db 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -8023,7 +8023,7 @@ fn do_test_tx_confirmed_skipping_blocks_immediate_broadcast(test_height_before_t
.force_close_broadcasting_latest_txn(&channel_id, &node_c_id, message.clone())
.unwrap();
- check_closed_broadcast!(nodes[1], true);
+ check_closed_broadcast(&nodes[1], 1, false);
let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
check_closed_event!(nodes[1], 1, reason, [node_c_id], 100000);
check_added_monitors(&nodes[1], 1);
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index c1cd8f9..ba3312c 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -3044,7 +3044,7 @@ fn do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(confirm_c
let serialized_monitor = get_monitor!(nodes[1], chan_id).encode();
reload_node!(nodes[1], user_config, &nodes[1].node.encode(), &[&serialized_monitor], persister, chain_monitor, node_deserialized);
let commitment_tx_conf_height = block_from_scid(mine_transaction(&nodes[1], &commitment_tx));
- check_closed_broadcast(&nodes[1], 1, true);
+ check_closed_broadcast(&nodes[1], 1, false);
check_added_monitors(&nodes[1], 1);
commitment_tx_conf_height
};
@@ -3407,7 +3407,7 @@ fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool) {
check_added_monitors(&nodes[2], 1);
let c_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
check_closed_event!(nodes[2], 1, c_reason, [node_b_id], 1_000_000);
- check_closed_broadcast!(nodes[2], true);
+ check_closed_broadcast(&nodes[2], 1, false);
handle_bump_events(&nodes[2], true, 0);
let cs_commit_tx = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
@@ -3421,7 +3421,7 @@ fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool) {
check_added_monitors(&nodes[1], 1);
let b_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
check_closed_event!(nodes[1], 1, b_reason, [node_c_id], 1_000_000);
- check_closed_broadcast!(nodes[1], true);
+ check_closed_broadcast(&nodes[1], 1, false);
handle_bump_events(&nodes[1], true, 0);
let bs_commit_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
@@ -3618,7 +3618,7 @@ fn do_test_lost_timeout_monitor_events(confirm_tx: CommitmentType, dust_htlcs: b
check_added_monitors(&nodes[2], 1);
let c_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
check_closed_event!(nodes[2], 1, c_reason, [node_b_id], 1_000_000);
- check_closed_broadcast!(nodes[2], true);
+ check_closed_broadcast(&nodes[2], 1, false);
handle_bump_events(&nodes[2], true, 0);
let cs_commit_tx = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
@@ -3632,7 +3632,7 @@ fn do_test_lost_timeout_monitor_events(confirm_tx: CommitmentType, dust_htlcs: b
check_added_monitors(&nodes[1], 1);
let b_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
check_closed_event!(nodes[1], 1, b_reason, [node_c_id], 1_000_000);
- check_closed_broadcast!(nodes[1], true);
+ check_closed_broadcast(&nodes[1], 1, false);
handle_bump_events(&nodes[1], true, 0);
let bs_commit_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 18fb33a..60e2153 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -1287,7 +1287,7 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(
expect_payment_claimed!(nodes[1], payment_hash, 10_000_000);
mine_transaction(&nodes[1], &commitment_tx);
- check_closed_broadcast!(nodes[1], true);
+ check_closed_broadcast(&nodes[1], 1, false);
check_added_monitors!(nodes[1], 1);
check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [node_a_id], 100000);
let htlc_success_tx = {
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 579c3ae..9c99337 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -325,6 +325,16 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
let chan_0_monitor_serialized = get_monitor!(nodes[0], chan.2).encode();
reload_node!(nodes[0], nodes[0].node.get_current_config(), &nodes_0_serialized, &[&chan_0_monitor_serialized], persister, new_chain_monitor, nodes_0_deserialized);
+
+ nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
+
+ if reorg_after_reload {
+ // If we haven't yet closed the channel, reconnect the peers so that nodes[0] will
+ // generate an error message we can handle below.
+ let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
+ reconnect_args.send_channel_ready = (true, true);
+ reconnect_nodes(reconnect_args);
+ }
}
if reorg_after_reload {
@@ -387,7 +397,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
[nodes[1].node.get_our_node_id()], 100000);
// Now check that we can create a new channel
- if reload_node {
+ if reload_node && !reorg_after_reload {
// If we dropped the channel before reloading the node, nodes[1] was also dropped from
// nodes[0] storage, and hence not connected again on startup. We therefore need to
// reconnect to the node before attempting to create a new channel.
@@ -879,7 +889,7 @@ fn do_test_retries_own_commitment_broadcast_after_reorg(anchors: bool, revoked_c
.node
.force_close_broadcasting_latest_txn(&chan_id, &nodes[0].node.get_our_node_id(), message.clone())
.unwrap();
- check_closed_broadcast(&nodes[1], 1, true);
+ check_closed_broadcast(&nodes[1], 1, !revoked_counterparty_commitment);
check_added_monitors(&nodes[1], 1);
let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
check_closed_event(&nodes[1], 1, reason, false, &[nodes[0].node.get_our_node_id()], 100_000);
@@ -1002,7 +1012,7 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) {
// Force-close and fetch node B's commitment transaction and the transaction claiming the first
// two HTLCs.
nodes[1].node.force_close_broadcasting_latest_txn(&chan_id, &node_a_id, err).unwrap();
- check_closed_broadcast(&nodes[1], 1, true);
+ check_closed_broadcast(&nodes[1], 1, false);
check_added_monitors(&nodes[1], 1);
let message = "Channel force-closed".to_owned();
let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
@@ -1015,7 +1025,7 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) {
check_spends!(commitment_tx, funding_tx);
mine_transaction(&nodes[0], &commitment_tx);
- check_closed_broadcast(&nodes[0], 1, true);
+ check_closed_broadcast(&nodes[0], 1, false);
let reason = ClosureReason::CommitmentTxConfirmed;
check_closed_event(&nodes[0], 1, reason, false, &[node_b_id], 10_000_000);
check_added_monitors(&nodes[0], 1);
Risk score
Our methodology →Why this scored 18/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.