Wipe empty entries from `actions_blocking_raa_monitor_updates`
What changed, and why it matters
This patch fixes a bug in the Lightning Dev Kit's channel manager where empty entries in a tracking map were not being removed. In a rare sequence of events—when an HTLC is claimed both off-chain and then on-chain—the leftover empty entry could trigger a debug assertion failure (a crash in debug builds) due to what appears to the code as a 'double-claim.' The fix simply deletes the empty map entry after cleaning it up. It was found through fuzzing, not a real-world exploit, and affects only debug builds because it relies on a debug_assert.
Apply the patch. It is a low-risk correctness fix with an included regression test. No immediate security response is required beyond normal update cadence, as the issue is a debug-only assertion failure found via fuzzing with no demonstrated production impact.
Security signals we found
Debug assertion failure (denial-of-service in debug builds)
State cleanup bug in channel monitor update blocker tracking
Double-claim HTLC edge case in on-chain/off-chain interaction
Fuzzing-found regression with included test
Evidence from the diff
The commit modifies ChannelManager to remove empty BTreeMap entries from actions_blocking_raa_monitor_updates after retaining only non-matching blockers. Previously, two code paths used retain on a mutable reference obtained via get_mut, leaving an empty Vec in the map. The patch switches to entry(channel_id) and uses btree_map::Entry::Occupied, removing the entry when the inner Vec becomes empty. A regression test reproduces a specific ordering: an HTLC is claimed off-chain via update_fulfill_htlc, then the counterparty commitment and HTLC-success transactions are mined on-chain, causing claim_funds_internal to be invoked a second time and hitting a debug assertion. The fix prevents the stale empty entry from causing the assertion failure.
Changed components
lightning/src/ln/channelmanager.rslightning/src/ln/functional_tests.rsInspect captured patch +79 / −8
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 2e78270..5737c49 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -9818,12 +9818,12 @@ impl<
{
if let Some(peer_state_mtx) = per_peer_state.get(&node_id) {
let mut peer_state = peer_state_mtx.lock().unwrap();
- if let Some(blockers) = peer_state
+ let entry = peer_state
.actions_blocking_raa_monitor_updates
- .get_mut(&channel_id)
- {
+ .entry(channel_id);
+ if let btree_map::Entry::Occupied(mut entry) = entry {
let mut found_blocker = false;
- blockers.retain(|iter| {
+ entry.get_mut().retain(|iter| {
// Note that we could actually be blocked, in
// which case we need to only remove the one
// blocker which was added duplicatively.
@@ -9833,6 +9833,9 @@ impl<
}
*iter != blocker || !first_blocker
});
+ if entry.get().is_empty() {
+ entry.remove();
+ }
debug_assert!(found_blocker);
}
} else {
@@ -15251,10 +15254,12 @@ impl<
let peer_state = &mut *peer_state_lck;
if let Some(blocker) = completed_blocker.take() {
// Only do this on the first iteration of the loop.
- if let Some(blockers) = peer_state.actions_blocking_raa_monitor_updates
- .get_mut(&channel_id)
- {
- blockers.retain(|iter| iter != &blocker);
+ let entry = peer_state.actions_blocking_raa_monitor_updates.entry(channel_id);
+ if let btree_map::Entry::Occupied(mut entry) = entry {
+ entry.get_mut().retain(|iter| iter != &blocker);
+ if entry.get().is_empty() {
+ entry.remove();
+ }
}
}
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 7ed4692..1fd3daf 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -10168,3 +10168,69 @@ pub fn test_dust_exposure_holding_cell_assertion() {
// Now that everything has settled, make sure the channels still work with a simple claim.
claim_payment(&nodes[2], &[&nodes[1]], payment_preimage_cb);
}
+
+#[test]
+fn test_dup_htlc_claim_onchain_and_offchain() {
+ // Tests what happens if we receive a claim first offchain, then see a counterparty broadcast
+ // their commitment transaction and re-claim the same HTLC on-chain. This was never broken, but
+ // the very specific ordering in this test did hit a debug assertion failure.
+ let chanmon_cfgs = create_chanmon_cfgs(3);
+ let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+ let legacy_cfg = test_legacy_channel_config();
+ let node_chanmgrs = create_node_chanmgrs(
+ 3,
+ &node_cfgs,
+ &[Some(legacy_cfg.clone()), Some(legacy_cfg.clone()), Some(legacy_cfg)],
+ );
+ let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+ let node_b_id = nodes[1].node.get_our_node_id();
+ let node_c_id = nodes[2].node.get_our_node_id();
+
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+ let chan_bc = create_announced_chan_between_nodes(&nodes, 1, 2);
+
+ // Route payment A -> B -> C.
+ let (payment_preimage, payment_hash, _, _) =
+ route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000);
+
+ // C claims the payment.
+ nodes[2].node.claim_funds(payment_preimage);
+ expect_payment_claimed!(nodes[2], payment_hash, 1_000_000);
+ check_added_monitors(&nodes[2], 1);
+
+ // Deliver only C's update_fulfill_htlc to B (NOT the commitment_signed). B learns
+ // the preimage and claims from A (adding an RAA blocker on B-C via
+ // internal_update_fulfill_htlc, then removing it when the A-B monitor update completes
+ // and the EmitEventOptionAndFreeOtherChannel action runs).
+ let cs_updates = get_htlc_update_msgs(&nodes[2], &node_b_id);
+ nodes[1].node.handle_update_fulfill_htlc(node_c_id, cs_updates.update_fulfill_htlcs[0].clone());
+ check_added_monitors(&nodes[1], 1);
+
+ // Ignore B's attempts to claim the HTLC from A.
+ nodes[1].node.get_and_clear_pending_msg_events();
+
+ // Get C's commitment transactions. C's commitment includes the HTLC and C has
+ // an HTLC-success transaction (claiming with preimage). Mine both on B.
+ let cs_txn = get_local_commitment_txn!(nodes[2], chan_bc.2);
+ assert!(cs_txn.len() >= 2, "Expected commitment + HTLC-success tx, got {}", cs_txn.len());
+
+ // Mine C's commitment on B. B sees the counterparty commitment on-chain.
+ mine_transaction(&nodes[1], &cs_txn[0]);
+ check_closed_broadcast(&nodes[1], 1, true);
+ check_added_monitors(&nodes[1], 1);
+ let events = nodes[1].node.get_and_clear_pending_events();
+ assert!(
+ events.iter().any(|e| matches!(e, Event::ChannelClosed { .. })),
+ "Expected ChannelClosed event"
+ );
+
+ // Mine C's HTLC-success transaction. B's monitor sees the preimage being used on-chain
+ // and generates an HTLCEvent with the preimage.
+ mine_transaction(&nodes[1], &cs_txn[1]);
+
+ // Advance past ANTI_REORG_DELAY so the on-chain HTLC resolution matures. This triggers
+ // the monitor to generate an HTLCEvent with the preimage via process_pending_monitor_events,
+ // which calls claim_funds_internal a second time.
+ connect_blocks(&nodes[1], ANTI_REORG_DELAY);
+}
Why this scored 34/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.