diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 7226fd2980d..a6a0068eb6b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2097,9 +2097,11 @@ impl Channel { match err { None => { if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() && self.holding_cell_update_fee.is_none() { - // This should never actually happen and indicates we got some Errs back - // from update_fulfill_htlc/update_fail_htlc, but we handle it anyway in - // case there is some strange way to hit duplicate HTLC removes. + // Reaching this clause is an edge case: a user must (a) be at the edge + // of their channel send limits and (b) have a race occur where e.g. + // the remote adds an HTLC while another HTLC is in the holding cell + // or e.g. a fee update is finalized while an HTLC is in the holding + // cell. return Ok(None); } let update_fee = if let Some(feerate) = self.holding_cell_update_fee { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index f09963eb070..04a07c646ef 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5803,6 +5803,60 @@ fn bolt2_open_channel_sending_node_checks_part2() { assert!(PublicKey::from_slice(&node0_to_1_send_open_channel.delayed_payment_basepoint.serialize()).is_ok()); } +#[test] +fn test_holding_cell_htlc_with_pending_fee_update() { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::supported(), InitFeatures::supported()); + + // First nodes[0] generates an update_fee, setting the channel's + // pending_update_fee. + nodes[0].node.update_fee(chan.2, get_feerate!(nodes[0], chan.2) + 20).unwrap(); + check_added_monitors!(nodes[0], 1); + + let events_0 = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events_0.len(), 1); + let (update_msg, commitment_signed) = match events_0[0] { // (1) + MessageSendEvent::UpdateHTLCs { updates: msgs::CommitmentUpdate { ref update_fee, ref commitment_signed, .. }, .. } => { + (update_fee.as_ref(), commitment_signed) + }, + _ => panic!("Unexpected event"), + }; + + nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_msg.unwrap()); + + let mut chan_stat = get_channel_value_stat!(nodes[0], chan.2); + let their_channel_reserve = chan_stat.channel_reserve_msat; + let fee_spike_reserve = 2 * chan_stat.commit_tx_fee_per_htlc_msat; + + let max_can_send = 5000000 - their_channel_reserve - chan_stat.commit_tx_fee_base_msat - chan_stat.commit_tx_fee_per_htlc_msat - fee_spike_reserve; + let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], max_can_send, TEST_FINAL_CLTV).unwrap(); + let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]); + + // Send a payment which passes reserve checks but gets stuck in the holding cell. + nodes[0].node.send_payment(&route, our_payment_hash, &None).unwrap(); + chan_stat = get_channel_value_stat!(nodes[0], chan.2); + assert_eq!(chan_stat.holding_cell_outbound_amount_msat, max_can_send); + + // Flush the pending fee update. + nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), commitment_signed); + let (as_revoke_and_ack, _) = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + check_added_monitors!(nodes[1], 1); + nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_revoke_and_ack); + check_added_monitors!(nodes[0], 1); + + // Upon receipt of the RAA, there will be an attempt to resend the holding cell + // HTLC, but now that the fee has been raised the payment will now fail, causing + // it to be put back in the holding cell. + chan_stat = get_channel_value_stat!(nodes[0], chan.2); + assert_eq!(chan_stat.holding_cell_outbound_amount_msat, max_can_send); + nodes[0].logger.assert_log("lightning::ln::channel".to_string(), "Freeing holding cell with 1 HTLC updates".to_string(), 1); + let failure_log = format!("Failed to send HTLC with payment_hash {} due to Cannot send value that would put us over their reserve value", log_bytes!(our_payment_hash.0)); + nodes[0].logger.assert_log("lightning::ln::channel".to_string(), failure_log.to_string(), 1); +} + // BOLT 2 Requirements for the Sender when constructing and sending an update_add_htlc message. // BOLT 2 Requirement: MUST NOT offer amount_msat it cannot pay for in the remote commitment transaction at the current feerate_per_kw (see "Updating Fees") while maintaining its channel reserve. //TODO: I don't believe this is explicitly enforced when sending an HTLC but as the Fee aspect of the BOLT specs is in flux leaving this as a TODO.