Fix holding cell freeing in case we fail to add some HTLC

Previously, if we went to free the holding cell HTLC updates, and
adding one failed as we hit our outbound HTLC limit (or in-flight
value limit), we would not send a commitment_signed, leaving us in
an invalid state. We first fix that bug, and then refuse to add
things to our holding cell once we reach our limits considering the
holding cell, as we shouldn't have multiple commitment dance rounds
worth of HTLCs in the holding cell anyway.

Author: TheBlueMatt

src/ln/channel.rs

@@ -337,7 +337,7 @@ pub(super) struct Channel {
 	logger: Arc<Logger>,
 }
 
-const OUR_MAX_HTLCS: u16 = 50; //TODO
+pub const OUR_MAX_HTLCS: u16 = 50; //TODO
 /// Confirmation count threshold at which we close a channel. Ideally we'd keep the channel around
 /// on ice until the funding transaction gets more confirmations, but the LN protocol doesn't
 /// really allow for this, so instead we're stuck closing it out at that point.
@@ -1534,14 +1534,23 @@ impl Channel {
 		(self.pending_inbound_htlcs.len() as u32, htlc_inbound_value_msat)
 	}
 
-	/// Returns (outbound_htlc_count, htlc_outbound_value_msat)
+	/// Returns (outbound_htlc_count, htlc_outbound_value_msat) *including* pending adds in our
+	/// holding cell.
 	fn get_outbound_pending_htlc_stats(&self) -> (u32, u64) {
 		let mut htlc_outbound_value_msat = 0;
 		for ref htlc in self.pending_outbound_htlcs.iter() {
 			htlc_outbound_value_msat += htlc.amount_msat;
 		}
 
-		(self.pending_outbound_htlcs.len() as u32, htlc_outbound_value_msat)
+		let mut htlc_outbound_count = self.pending_outbound_htlcs.len();
+		for update in self.holding_cell_htlc_updates.iter() {
+			if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update {
+				htlc_outbound_count += 1;
+				htlc_outbound_value_msat += amount_msat;
+			}
+		}
+
+		(htlc_outbound_count as u32, htlc_outbound_value_msat)
 	}
 
 	pub fn update_add_htlc(&mut self, msg: &msgs::UpdateAddHTLC, pending_forward_state: PendingHTLCStatus) -> Result<(), ChannelError> {
@@ -1858,6 +1867,11 @@ impl Channel {
 					}
 					if err.is_some() {
 						self.holding_cell_htlc_updates.push(htlc_update);
+						if let Some(ChannelError::Ignore(_)) = err {
+							// If we failed to add the HTLC, but got an Ignore error, we should
+							// still send the new commitment_signed, so reset the err to None.
+							err = None;
+						}
 					}
 				}
 			}
@@ -3152,19 +3166,9 @@ impl Channel {
 			return Err(ChannelError::Ignore("Cannot send value that would put us over our max HTLC value in flight"));
 		}
 
-		let mut holding_cell_outbound_amount_msat = 0;
-		for holding_htlc in self.holding_cell_htlc_updates.iter() {
-			match holding_htlc {
-				&HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } => {
-					holding_cell_outbound_amount_msat += *amount_msat;
-				}
-				_ => {}
-			}
-		}
-
 		// Check self.their_channel_reserve_satoshis (the amount we must keep as
 		// reserve for them to have something to claim if we misbehave)
-		if self.value_to_self_msat < self.their_channel_reserve_satoshis * 1000 + amount_msat + holding_cell_outbound_amount_msat + htlc_outbound_value_msat {
+		if self.value_to_self_msat < self.their_channel_reserve_satoshis * 1000 + amount_msat + htlc_outbound_value_msat {
 			return Err(ChannelError::Ignore("Cannot send value that would put us over our reserve value"));
 		}
 

src/ln/functional_tests.rs

@@ -1823,6 +1823,132 @@ fn fake_network_test() {
 	close_channel(&nodes[1], &nodes[3], &chan_5.2, chan_5.3, false);
 }
 
+#[test]
+fn holding_cell_htlc_counting() {
+	// Tests that HTLCs in the holding cell count towards the pending HTLC limits on outbound HTLCs
+	// to ensure we don't end up with HTLCs sitting around in our holding cell for several
+	// commitment dance rounds.
+	let mut nodes = create_network(3);
+	create_announced_chan_between_nodes(&nodes, 0, 1);
+	let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2);
+
+	let mut payments = Vec::new();
+	for _ in 0..::ln::channel::OUR_MAX_HTLCS {
+		let route = nodes[1].router.get_route(&nodes[2].node.get_our_node_id(), None, &Vec::new(), 100000, TEST_FINAL_CLTV).unwrap();
+		let (payment_preimage, payment_hash) = get_payment_preimage_hash!(nodes[0]);
+		nodes[1].node.send_payment(route, payment_hash).unwrap();
+		payments.push((payment_preimage, payment_hash));
+	}
+	check_added_monitors!(nodes[1], 1);
+
+	let mut events = nodes[1].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	let initial_payment_event = SendEvent::from_event(events.pop().unwrap());
+	assert_eq!(initial_payment_event.node_id, nodes[2].node.get_our_node_id());
+
+	// There is now one HTLC in an outbound commitment transaction and (OUR_MAX_HTLCS - 1) HTLCs in
+	// the holding cell waiting on B's RAA to send. At this point we should not be able to add
+	// another HTLC.
+	let route = nodes[1].router.get_route(&nodes[2].node.get_our_node_id(), None, &Vec::new(), 100000, TEST_FINAL_CLTV).unwrap();
+	let (payment_preimage_1, payment_hash_1) = get_payment_preimage_hash!(nodes[0]);
+	if let APIError::ChannelUnavailable { err } = nodes[1].node.send_payment(route, payment_hash_1).unwrap_err() {
+		assert_eq!(err, "Cannot push more than their max accepted HTLCs");
+	} else { panic!("Unexpected event"); }
+
+	// This should also be true if we try to forward a payment.
+	let route = nodes[0].router.get_route(&nodes[2].node.get_our_node_id(), None, &Vec::new(), 100000, TEST_FINAL_CLTV).unwrap();
+	let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+	nodes[0].node.send_payment(route, payment_hash_2).unwrap();
+	check_added_monitors!(nodes[0], 1);
+
+	let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	let payment_event = SendEvent::from_event(events.pop().unwrap());
+	assert_eq!(payment_event.node_id, nodes[1].node.get_our_node_id());
+
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]).unwrap();
+	commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
+	// We have to forward pending HTLCs twice - once tries to forward the payment forward (and
+	// fails), the second will process the resulting failure.
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	check_added_monitors!(nodes[1], 1);
+
+	let bs_fail_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_updates.update_fail_htlcs[0]).unwrap();
+	commitment_signed_dance!(nodes[0], nodes[1], bs_fail_updates.commitment_signed, false, true);
+
+	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg }} => {
+			assert_eq!(msg.contents.short_channel_id, chan_2.0.contents.short_channel_id);
+		},
+		_ => panic!("Unexpected event"),
+	}
+
+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		Event::PaymentFailed { payment_hash, rejected_by_dest, .. } => {
+			assert_eq!(payment_hash, payment_hash_2);
+			assert!(!rejected_by_dest);
+		},
+		_ => panic!("Unexpected event"),
+	}
+
+	// Now foward all the pending HTLCs and claim them back
+	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &initial_payment_event.msgs[0]).unwrap();
+	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &initial_payment_event.commitment_msg).unwrap();
+	check_added_monitors!(nodes[2], 1);
+
+	let (bs_revoke_and_ack, bs_commitment_signed) = get_revoke_commit_msgs!(nodes[2], nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack).unwrap();
+	check_added_monitors!(nodes[1], 1);
+	let as_updates = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
+
+	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_commitment_signed).unwrap();
+	check_added_monitors!(nodes[1], 1);
+	let as_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());
+
+	for ref update in as_updates.update_add_htlcs.iter() {
+		nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), update).unwrap();
+	}
+	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &as_updates.commitment_signed).unwrap();
+	check_added_monitors!(nodes[2], 1);
+	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa).unwrap();
+	check_added_monitors!(nodes[2], 1);
+	let (bs_revoke_and_ack, bs_commitment_signed) = get_revoke_commit_msgs!(nodes[2], nodes[1].node.get_our_node_id());
+
+	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack).unwrap();
+	check_added_monitors!(nodes[1], 1);
+	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &bs_commitment_signed).unwrap();
+	check_added_monitors!(nodes[1], 1);
+	let as_final_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[2].node.get_our_node_id());
+
+	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_final_raa).unwrap();
+	check_added_monitors!(nodes[2], 1);
+
+	expect_pending_htlcs_forwardable!(nodes[2]);
+
+	let events = nodes[2].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), payments.len());
+	for (ref event, &(_, ref hash)) in events.iter().zip(payments.iter()) {
+		match event {
+			&Event::PaymentReceived { ref payment_hash, .. } => {
+				assert_eq!(*payment_hash, *hash);
+			},
+			_ => panic!("Unexpected event"),
+		};
+	}
+
+	for (preimage, _) in payments.drain(..) {
+		claim_payment(&nodes[1], &[&nodes[2]], preimage);
+	}
+
+	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
+}
+
 #[test]
 fn duplicate_htlc_test() {
 	// Test that we accept duplicate payment_hash HTLCs across the network and that