Remove forward_htlcs<>intercepted_htlcs lock dep
What the developer wrote
Authored by Valentine Wallace
Remove forward_htlcs<>intercepted_htlcs lock dep
We previously had a lock order dependency where the forward_htlcs lock had to
be taken before the intercepted_htlcs lock. Here we remove this dependency,
which also prepares for a cleaner commit once we start also intercepting HTLCs
for often-offline recipients.
What changed, and why it matters
This commit restructures how Lightning Dev Kit handles routing of intercepted payments. Previously, the code required holding one internal lock (forward_htlcs) before acquiring another (pending_intercepted_htlcs). The patch removes that lock-order dependency by checking whether an HTLC should be intercepted before, not after, adding it to the forwarding queue. The stated goal is cleaner future code, not a security fix, but removing unnecessary lock nesting can reduce the risk of deadlocks in multi-threaded code.
Treat as a defensive hardening/refactoring change. Review related follow-up commits that add interception for often-offline recipients to ensure the new lock ordering is preserved and no re-introduction of nested locks occurs. No immediate patch deployment is required solely for this commit.
Security signals we found
Lock-order dependency removal in concurrent code
Potential deadlock class reduction
Refactoring of HTLC intercept vs. forward path
No explicit security claim in commit message
Evidence from the diff
The change in channelmanager.rs removes a documented lock-order edge where forward_htlcs had to be locked before pending_intercepted_htlcs. The logic is inverted: instead of always entering forward_htlcs first and then deciding whether to intercept, the code now tests the intercept conditions (is_our_scid, incoming_amt_msat present, fake_scid::is_valid_intercept) and, if true, locks pending_intercepted_htlcs directly. Only non-intercept HTLCs touch forward_htlcs. This eliminates the nested lock acquisition and the associated deadlock potential, though the commit message frames it as a refactoring for future intercept-of-offline-recipients work.
Changed components
lightning/src/ln/channelmanager.rsforward_htlcs mutex/lockpending_intercepted_htlcs mutex/lockHTLC interception and forwarding logicInspect captured patch +66 / −67
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index a4d00cc..8013168 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2506,8 +2506,8 @@ where
// `total_consistency_lock`
// |
// |__`forward_htlcs`
-// | |
-// | |__`pending_intercepted_htlcs`
+// |
+// |__`pending_intercepted_htlcs`
// |
// |__`decode_update_add_htlcs`
// |
@@ -10696,77 +10696,76 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
prev_user_channel_id,
forward_info,
};
- match forward_htlcs.entry(scid) {
- hash_map::Entry::Occupied(mut entry) => {
- entry.get_mut().push(HTLCForwardInfo::AddHTLC(pending_add));
- },
- hash_map::Entry::Vacant(entry) => {
- if !is_our_scid
- && pending_add.forward_info.incoming_amt_msat.is_some()
- && fake_scid::is_valid_intercept(
- &self.fake_scid_rand_bytes,
- scid,
- &self.chain_hash,
- ) {
- let intercept_id = InterceptId(
- Sha256::hash(&pending_add.forward_info.incoming_shared_secret)
- .to_byte_array(),
- );
- let mut pending_intercepts =
- self.pending_intercepted_htlcs.lock().unwrap();
- match pending_intercepts.entry(intercept_id) {
- hash_map::Entry::Vacant(entry) => {
- new_intercept_events.push_back((
- events::Event::HTLCIntercepted {
- requested_next_hop_scid: scid,
- payment_hash,
- inbound_amount_msat: pending_add
- .forward_info
- .incoming_amt_msat
- .unwrap(),
- expected_outbound_amount_msat: pending_add
- .forward_info
- .outgoing_amt_msat,
- intercept_id,
- },
- None,
- ));
- entry.insert(pending_add);
+
+ if !is_our_scid
+ && pending_add.forward_info.incoming_amt_msat.is_some()
+ && fake_scid::is_valid_intercept(
+ &self.fake_scid_rand_bytes,
+ scid,
+ &self.chain_hash,
+ ) {
+ let intercept_id = InterceptId(
+ Sha256::hash(&pending_add.forward_info.incoming_shared_secret)
+ .to_byte_array(),
+ );
+ let mut pending_intercepts = self.pending_intercepted_htlcs.lock().unwrap();
+ match pending_intercepts.entry(intercept_id) {
+ hash_map::Entry::Vacant(entry) => {
+ new_intercept_events.push_back((
+ events::Event::HTLCIntercepted {
+ requested_next_hop_scid: scid,
+ payment_hash,
+ inbound_amount_msat: pending_add
+ .forward_info
+ .incoming_amt_msat
+ .unwrap(),
+ expected_outbound_amount_msat: pending_add
+ .forward_info
+ .outgoing_amt_msat,
+ intercept_id,
},
- hash_map::Entry::Occupied(_) => {
- let logger = WithContext::from(
- &self.logger,
- None,
- Some(prev_channel_id),
- Some(payment_hash),
- );
- log_info!(
+ None,
+ ));
+ entry.insert(pending_add);
+ },
+ hash_map::Entry::Occupied(_) => {
+ let logger = WithContext::from(
+ &self.logger,
+ None,
+ Some(prev_channel_id),
+ Some(payment_hash),
+ );
+ log_info!(
logger,
"Failed to forward incoming HTLC: detected duplicate intercepted payment over short channel id {}",
scid
);
- let htlc_source = HTLCSource::PreviousHopData(
- pending_add.htlc_previous_hop_data(),
- );
- let reason = HTLCFailReason::from_failure_code(
- LocalHTLCFailureReason::UnknownNextPeer,
- );
- let failure_type =
- HTLCHandlingFailureType::InvalidForward {
- requested_forward_scid: scid,
- };
- failed_intercept_forwards.push((
- htlc_source,
- payment_hash,
- reason,
- failure_type,
- ));
- },
- }
- } else {
+ let htlc_source = HTLCSource::PreviousHopData(
+ pending_add.htlc_previous_hop_data(),
+ );
+ let reason = HTLCFailReason::from_failure_code(
+ LocalHTLCFailureReason::UnknownNextPeer,
+ );
+ let failure_type = HTLCHandlingFailureType::InvalidForward {
+ requested_forward_scid: scid,
+ };
+ failed_intercept_forwards.push((
+ htlc_source,
+ payment_hash,
+ reason,
+ failure_type,
+ ));
+ },
+ }
+ } else {
+ match forward_htlcs.entry(scid) {
+ hash_map::Entry::Occupied(mut entry) => {
+ entry.get_mut().push(HTLCForwardInfo::AddHTLC(pending_add));
+ },
+ hash_map::Entry::Vacant(entry) => {
entry.insert(vec![HTLCForwardInfo::AddHTLC(pending_add)]);
- }
- },
+ },
+ }
}
}
}
Why this scored 26/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.