Hold peer lock when pushing to decode_update_adds
What the developer wrote
Authored by Valentine Wallace
Hold peer lock when pushing to decode_update_adds
This avoids race conditions where we're unable to properly update an HTLC's
state because we need to update its state in the ChannelManager, but the HTLC
is stuck in transit from the Channel to
ChannelManager::decode_update_add_htlcs. Now the HTLC will atomically go from
the Channel to the ChannelManager decode queue under the same lock.
What changed, and why it matters
This change fixes a timing issue in how the Lightning node hands off payment packets (HTLCs) between two internal components. Previously, an HTLC could be in an in-between state where one part of the code thought it had been forwarded while another part had not yet processed it, potentially leading to inconsistent bookkeeping. The fix moves the handoff so it happens while the same lock is held, making the transition atomic.
Review the lock ordering around peer_state, decode_update_add_htlcs, and claimable_payments/pending_outbound_payments to ensure no deadlock is introduced. Add regression tests that exercise concurrent monitor update completion and HTLC forwarding. Consider whether the serialization lock reordering needs documentation.
Security signals we found
Race condition in HTLC state handoff between Channel and ChannelManager
Atomicity fix for decode_update_add_htlcs queueing under peer lock
Potential inconsistent HTLC state update during monitor update completion
Lock ordering adjustment in serialization path
Evidence from the diff
The commit moves the push to decode_update_add_htlcs from finish_monitor_update/monitor_update_completed into the peer-state handling code while the per-peer lock is still held. Previously, decode_update_add_htlcs was passed through PostMonitorUpdateChanResume and later pushed after locks were released, creating a window where HTLC state updates in ChannelManager could race with HTLCs in transit from Channel. The patch removes the field from the resume struct and calls push_decode_update_add_htlcs directly inside the locked peer-state block. It also moves the serialization lock acquisition for decode_update_add_htlcs to occur after the peer-state locks in the persistence path, likely to preserve lock ordering.
Changed components
lightning/src/ln/channelmanager.rsPostMonitorUpdateChanResume structfinish_monitor_update / monitor_update_completedpeer_state handling around monitor update completiondecode_update_add_htlcs queueInspect captured patch +12 / −14
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 2c97e4a..4aad2a5 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1516,7 +1516,6 @@ enum PostMonitorUpdateChanResume {
unbroadcasted_batch_funding_txid: Option<Txid>,
update_actions: Vec<MonitorUpdateCompletionAction>,
htlc_forwards: Vec<PendingAddHTLCInfo>,
- decode_update_add_htlcs: Option<(u64, Vec<msgs::UpdateAddHTLC>)>,
finalized_claimed_htlcs: Vec<(HTLCSource, Option<AttributionData>)>,
failed_htlcs: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
committed_outbound_htlc_sources: Vec<(HTLCPreviousHopData, u64)>,
@@ -10116,7 +10115,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
&self, channel_id: ChannelId, counterparty_node_id: PublicKey, funding_txo: OutPoint,
user_channel_id: u128, unbroadcasted_batch_funding_txid: Option<Txid>,
update_actions: Vec<MonitorUpdateCompletionAction>, htlc_forwards: Vec<PendingAddHTLCInfo>,
- decode_update_add_htlcs: Option<(u64, Vec<msgs::UpdateAddHTLC>)>,
finalized_claimed_htlcs: Vec<(HTLCSource, Option<AttributionData>)>,
failed_htlcs: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
committed_outbound_htlc_sources: Vec<(HTLCPreviousHopData, u64)>,
@@ -10177,9 +10175,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
self.handle_monitor_update_completion_actions(update_actions);
self.forward_htlcs(htlc_forwards);
- if let Some(decode) = decode_update_add_htlcs {
- self.push_decode_update_add_htlcs(decode);
- }
self.finalize_claims(finalized_claimed_htlcs);
for failure in failed_htlcs {
let failure_type = failure.0.failure_type(counterparty_node_id, channel_id);
@@ -10667,6 +10662,10 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
pending_msg_events.push(upd);
}
+ if let Some(update_adds) = decode_update_add_htlcs {
+ self.push_decode_update_add_htlcs(update_adds);
+ }
+
let unbroadcasted_batch_funding_txid =
chan.context.unbroadcasted_batch_funding_txid(&chan.funding);
@@ -10678,7 +10677,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
unbroadcasted_batch_funding_txid,
update_actions,
htlc_forwards,
- decode_update_add_htlcs,
finalized_claimed_htlcs: updates.finalized_claimed_htlcs,
failed_htlcs: updates.failed_htlcs,
committed_outbound_htlc_sources: updates.committed_outbound_htlc_sources,
@@ -10780,7 +10778,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
unbroadcasted_batch_funding_txid,
update_actions,
htlc_forwards,
- decode_update_add_htlcs,
finalized_claimed_htlcs,
failed_htlcs,
committed_outbound_htlc_sources,
@@ -10793,7 +10790,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
unbroadcasted_batch_funding_txid,
update_actions,
htlc_forwards,
- decode_update_add_htlcs,
finalized_claimed_htlcs,
failed_htlcs,
committed_outbound_htlc_sources,
@@ -17920,12 +17916,6 @@ impl<
}
}
- let mut decode_update_add_htlcs_opt = None;
- let decode_update_add_htlcs = self.decode_update_add_htlcs.lock().unwrap();
- if !decode_update_add_htlcs.is_empty() {
- decode_update_add_htlcs_opt = Some(decode_update_add_htlcs);
- }
-
let claimable_payments = self.claimable_payments.lock().unwrap();
let pending_outbound_payments = self.pending_outbound_payments.pending_outbound_payments.lock().unwrap();
@@ -17951,6 +17941,14 @@ impl<
peer_states.push(peer_state_mutex.unsafe_well_ordered_double_lock_self());
}
+ let mut decode_update_add_htlcs_opt = None;
+ {
+ let decode_update_add_htlcs = self.decode_update_add_htlcs.lock().unwrap();
+ if !decode_update_add_htlcs.is_empty() {
+ decode_update_add_htlcs_opt = Some(decode_update_add_htlcs);
+ }
+ }
+
let mut peer_storage_dir: Vec<(&PublicKey, &Vec<u8>)> = Vec::new();
(serializable_peer_count).write(writer)?;
Why this scored 55/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.