Remove spurious debug assertion added in 0.2
What changed, and why it matters
This commit removes a harmless debug-only assertion in the Lightning Dev Kit's channel handling code. The assertion could be triggered during normal operation when an HTLC (a conditional Bitcoin payment) in a 'holding cell' couldn't be added to a channel due to a dust-exposure limit. Debug assertions only fire in test builds, so production nodes were never affected. The commit also adds a regression test to exercise this edge case.
No immediate action required. This is a code-quality/test fix. Users running debug builds or CI may see fewer assertion failures. Reviewers should verify the new regression test adequately covers the edge case and that no other similar spurious assertions remain.
Security signals we found
Removal of a debug assertion that was reachable during normal protocol operation
Added regression test covering dust-exposure holding-cell HTLC failure path
No production code behavior change; only debug/test behavior changes
Evidence from the diff
The patch removes debug_assert!(htlcs_to_fail.is_empty()); in channel.rs inside maybe_free_holding_cell_htlcs. The assertion incorrectly assumed that if a new commitment couldn’t be generated, there would be no HTLCs to fail. The new test demonstrates a reachable scenario where an HTLC sits in the holding cell, then a counterparty action pushes dust exposure over the limit, so when the holding cell is processed the HTLC must be failed back. The assertion was reachable in debug builds but had no effect in release builds. Two panic messages in test utilities are also improved to print the unexpected events.
Changed components
lightning/src/ln/channel.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rsInspect captured patch +153 / −3
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d223599..659735c 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -8981,7 +8981,6 @@ where
update_fail_htlcs.len() + update_fail_malformed_htlcs.len(),
&self.context.channel_id);
} else {
- debug_assert!(htlcs_to_fail.is_empty());
let reason = if self.context.channel_state.is_local_stfu_sent() {
"exits quiescence"
} else if self.context.channel_state.is_monitor_update_in_progress() {
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index e9cb13d..00fbca2 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -971,7 +971,7 @@ pub fn get_revoke_commit_msgs<CM: AChannelManager, H: NodeHolder<CM = CM>>(
assert_eq!(node_id, recipient);
(*msg).clone()
},
- _ => panic!("Unexpected event"),
+ _ => panic!("Unexpected event: {events:?}"),
},
match events[1] {
MessageSendEvent::UpdateHTLCs { ref node_id, ref channel_id, ref updates } => {
@@ -984,7 +984,7 @@ pub fn get_revoke_commit_msgs<CM: AChannelManager, H: NodeHolder<CM = CM>>(
assert!(updates.commitment_signed.iter().all(|cs| cs.channel_id == *channel_id));
updates.commitment_signed.clone()
},
- _ => panic!("Unexpected event"),
+ _ => panic!("Unexpected event: {events:?}"),
},
)
}
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 58ef44c..fcb348c 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -9900,3 +9900,154 @@ pub fn test_multi_post_event_actions() {
do_test_multi_post_event_actions(true);
do_test_multi_post_event_actions(false);
}
+
+#[xtest(feature = "_externalize_tests")]
+pub fn test_dust_exposure_holding_cell_assertion() {
+ // Test that we properly move forward if we pop an HTLC-add from the holding cell but fail to
+ // add it to the channel. In 0.2 this cause a (harmless in prod) debug assertion failure. We
+ // try to ensure that this won't happen by checking that an HTLC will be able to be added
+ // before we add it to the holding cell, so getting into this state takes a bit of work.
+ //
+ // Here we accomplish this by using the dust exposure limit. This has the unique feature that
+ // node C can increase node B's dust exposure on the B <-> C channel without B doing anything.
+ // To exploit this, we get node B one HTLC away from being over-exposed to dust, give it one
+ // more HTLC in the holding cell, then have node C add an HTLC. By the time the holding-cell
+ // HTLC is released we are at max-dust-exposure and will fail it.
+
+ let chanmon_cfgs = create_chanmon_cfgs(3);
+ let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+
+ // Configure nodes with specific dust limits
+ let mut config = test_default_channel_config();
+ // Use a fixed dust exposure limit to make the test simpler
+ const DUST_HTLC_VALUE_MSAT: u64 = 500_000;
+ config.channel_config.max_dust_htlc_exposure = MaxDustHTLCExposure::FixedLimitMsat(5_000_000);
+ config.channel_handshake_config.max_inbound_htlc_value_in_flight_percent_of_channel = 100;
+
+ let configs = [Some(config.clone()), Some(config.clone()), Some(config.clone())];
+ let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &configs);
+ let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+ let node_a_id = nodes[0].node.get_our_node_id();
+ let node_b_id = nodes[1].node.get_our_node_id();
+ let node_c_id = nodes[2].node.get_our_node_id();
+
+ // Create channels: A <-> B <-> C
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+ let bc_chan_id = create_announced_chan_between_nodes(&nodes, 1, 2).2;
+ send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 10_000_000);
+
+ // Send multiple dust HTLCs from B to C to approach the dust limit (including transaction fees)
+ for _ in 0..4 {
+ route_payment(&nodes[1], &[&nodes[2]], DUST_HTLC_VALUE_MSAT);
+ }
+
+ // At this point we shouldn't be over the dust limit, and should still be able to send HTLCs.
+ let bs_chans = nodes[1].node.list_channels();
+ let bc_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap();
+ assert_eq!(
+ bc_chan.next_outbound_htlc_minimum_msat,
+ config.channel_handshake_config.our_htlc_minimum_msat
+ );
+
+ // Add a further HTLC from B to C, but don't deliver the send messages.
+ // After this we'll only have the ability to add one more HTLC, but by not delivering the send
+ // messages (leaving B waiting on C's RAA) the next HTLC will go into B's holding cell.
+ let (route_bc, payment_hash_bc, _payment_preimage_bc, payment_secret_bc) =
+ get_route_and_payment_hash!(nodes[1], nodes[2], DUST_HTLC_VALUE_MSAT);
+ let onion_bc = RecipientOnionFields::secret_only(payment_secret_bc);
+ let id = PaymentId(payment_hash_bc.0);
+ nodes[1].node.send_payment_with_route(route_bc, payment_hash_bc, onion_bc, id).unwrap();
+ check_added_monitors(&nodes[1], 1);
+ let send_bc = SendEvent::from_node(&nodes[1]);
+
+ let bs_chans = nodes[1].node.list_channels();
+ let bc_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap();
+ assert_eq!(
+ bc_chan.next_outbound_htlc_minimum_msat,
+ config.channel_handshake_config.our_htlc_minimum_msat
+ );
+
+ // Forward an additional HTLC from A through B to C. This will go in B's holding cell for node
+ // C as it is waiting on a response to the above messages.
+ let payment_params_ac = PaymentParameters::from_node_id(node_c_id, TEST_FINAL_CLTV)
+ .with_bolt11_features(nodes[2].node.bolt11_invoice_features())
+ .unwrap();
+ let (route_ac, payment_hash_cell, _, payment_secret_ac) =
+ get_route_and_payment_hash!(nodes[0], nodes[2], payment_params_ac, DUST_HTLC_VALUE_MSAT);
+ let onion_ac = RecipientOnionFields::secret_only(payment_secret_ac);
+ let id = PaymentId(payment_hash_cell.0);
+ nodes[0].node.send_payment_with_route(route_ac, payment_hash_cell, onion_ac, id).unwrap();
+ check_added_monitors(&nodes[0], 1);
+
+ let send_ab = SendEvent::from_node(&nodes[0]);
+ nodes[1].node.handle_update_add_htlc(node_a_id, &send_ab.msgs[0]);
+ do_commitment_signed_dance(&nodes[1], &nodes[0], &send_ab.commitment_msg, false, true);
+
+ // At this point when we process pending forwards the HTLC will go into the holding cell and no
+ // further messages will be generated. Node B will also be at its maximum dust exposure and
+ // will refuse to send any dust HTLCs (when it includes the holding cell HTLC).
+ expect_and_process_pending_htlcs(&nodes[1], false);
+ assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+ let bs_chans = nodes[1].node.list_channels();
+ let bc_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap();
+ assert!(bc_chan.next_outbound_htlc_minimum_msat > DUST_HTLC_VALUE_MSAT);
+
+ // Send an additional HTLC from C to B. This will make B unable to forward the HTLC already in
+ // its holding cell as it would be over-exposed to dust.
+ let (route_cb, payment_hash_cb, payment_preimage_cb, payment_secret_cb) =
+ get_route_and_payment_hash!(nodes[2], nodes[1], DUST_HTLC_VALUE_MSAT);
+ let onion_cb = RecipientOnionFields::secret_only(payment_secret_cb);
+ let id = PaymentId(payment_hash_cb.0);
+ nodes[2].node.send_payment_with_route(route_cb, payment_hash_cb, onion_cb, id).unwrap();
+ check_added_monitors(&nodes[2], 1);
+
+ // Now deliver all the messages and make sure that the HTLC is failed-back.
+ let send_event_cb = SendEvent::from_node(&nodes[2]);
+ nodes[1].node.handle_update_add_htlc(node_c_id, &send_event_cb.msgs[0]);
+ nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &send_event_cb.commitment_msg);
+ check_added_monitors(&nodes[1], 1);
+
+ nodes[2].node.handle_update_add_htlc(node_b_id, &send_bc.msgs[0]);
+ nodes[2].node.handle_commitment_signed_batch_test(node_b_id, &send_bc.commitment_msg);
+ check_added_monitors(&nodes[2], 1);
+
+ let cs_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, node_b_id);
+ nodes[1].node.handle_revoke_and_ack(node_c_id, &cs_raa);
+ check_added_monitors(&nodes[1], 1);
+ let (bs_raa, bs_cs) = get_revoke_commit_msgs(&nodes[1], &node_c_id);
+
+ // When we delivered the RAA above, we attempted (and failed) to add the HTLC to the channel,
+ // causing it to be ready to fail-back, which we do here:
+ let next_hop =
+ HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: bc_chan_id };
+ expect_htlc_forwarding_fails(&nodes[1], &[next_hop]);
+ check_added_monitors(&nodes[1], 1);
+ fail_payment_along_path(&[&nodes[0], &nodes[1]]);
+ let conditions = PaymentFailedConditions::new();
+ expect_payment_failed_conditions(&nodes[0], payment_hash_cell, false, conditions);
+
+ nodes[2].node.handle_revoke_and_ack(node_b_id, &bs_raa);
+ check_added_monitors(&nodes[2], 1);
+ let cs_cs = get_htlc_update_msgs(&nodes[2], &node_b_id);
+
+ nodes[2].node.handle_commitment_signed_batch_test(node_b_id, &bs_cs);
+ check_added_monitors(&nodes[2], 1);
+ let cs_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, node_b_id);
+
+ nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &cs_cs.commitment_signed);
+ check_added_monitors(&nodes[1], 1);
+ let bs_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, node_c_id);
+
+ nodes[1].node.handle_revoke_and_ack(node_c_id, &cs_raa);
+ check_added_monitors(&nodes[1], 1);
+ expect_and_process_pending_htlcs(&nodes[1], false);
+ expect_payment_claimable!(nodes[1], payment_hash_cb, payment_secret_cb, DUST_HTLC_VALUE_MSAT);
+
+ nodes[2].node.handle_revoke_and_ack(node_b_id, &bs_raa);
+ check_added_monitors(&nodes[2], 1);
+
+ // Now that everything has settled, make sure the channels still work with a simple claim.
+ claim_payment(&nodes[2], &[&nodes[1]], payment_preimage_cb);
+}
Why this scored 22/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.