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

@@ -1540,14 +1540,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> {
@@ -1834,6 +1843,14 @@ impl Channel {
 							match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone()) {
 								Ok(update_add_msg_option) => update_add_htlcs.push(update_add_msg_option.unwrap()),
 								Err(e) => {
+									match e {
+										ChannelError::Ignore(ref msg) => {
+											log_info!(self, "Failed to send HTLC with payment_hash {} due to {}", log_bytes!(payment_hash.0), msg);
+										},
+										_ => {
+											log_info!(self, "Failed to send HTLC with payment_hash {} resulting in a channel closure during holding_cell freeing", log_bytes!(payment_hash.0));
+										},
+									}
 									err = Some(e);
 								}
 							}
@@ -1863,6 +1880,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;
+						}
 					}
 				}
 			}
@@ -3154,30 +3176,19 @@ impl Channel {
 		//TODO: Spec is unclear if this is per-direction or in total (I assume per direction):
 		// Check their_max_htlc_value_in_flight_msat
 		if htlc_outbound_value_msat + amount_msat > self.their_max_htlc_value_in_flight_msat {
-			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;
-				}
-				_ => {}
-			}
+			return Err(ChannelError::Ignore("Cannot send value that would put us over the max HTLC value in flight"));
 		}
 
 		// 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 {
-			return Err(ChannelError::Ignore("Cannot send value that would put us over our reserve value"));
+		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 the reserve value"));
 		}
 
 		//TODO: Check cltv_expiry? Do this in channel manager?
 
 		// Now update local state:
 		if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
-			//TODO: Check the limits *including* other pending holding cell HTLCs!
 			self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
 				amount_msat: amount_msat,
 				payment_hash: payment_hash,

src/ln/functional_tests.rs

@@ -718,7 +718,7 @@ fn route_over_limit(origin_node: &Node, expected_route: &[&Node], recv_value: u6
 
 	let err = origin_node.node.send_payment(route, our_payment_hash).err().unwrap();
 	match err {
-		APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight"),
+		APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over the max HTLC value in flight"),
 		_ => panic!("Unknown error variants"),
 	};
 }
@@ -1824,6 +1824,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_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_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 and fail the HTLC backward.
+	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 forward 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 (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
@@ -2035,7 +2161,7 @@ fn do_channel_reserve_test(test_recv: bool) {
 		assert!(route.hops.iter().rev().skip(1).all(|h| h.fee_msat == feemsat));
 		let err = nodes[0].node.send_payment(route, our_payment_hash).err().unwrap();
 		match err {
-			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight"),
+			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over the max HTLC value in flight"),
 			_ => panic!("Unknown error variants"),
 		}
 	}
@@ -2071,7 +2197,7 @@ fn do_channel_reserve_test(test_recv: bool) {
 		let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value + 1);
 		let err = nodes[0].node.send_payment(route.clone(), our_payment_hash).err().unwrap();
 		match err {
-			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
+			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over the reserve value"),
 			_ => panic!("Unknown error variants"),
 		}
 	}
@@ -2096,7 +2222,7 @@ fn do_channel_reserve_test(test_recv: bool) {
 	{
 		let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_2 + 1);
 		match nodes[0].node.send_payment(route, our_payment_hash).err().unwrap() {
-			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
+			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over the reserve value"),
 			_ => panic!("Unknown error variants"),
 		}
 	}
@@ -2159,7 +2285,7 @@ fn do_channel_reserve_test(test_recv: bool) {
 	{
 		let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_22+1);
 		match nodes[0].node.send_payment(route, our_payment_hash).err().unwrap() {
-			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
+			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over the reserve value"),
 			_ => panic!("Unknown error variants"),
 		}
 	}
@@ -6704,7 +6830,7 @@ fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_value_in_flight() {
 	let err = nodes[0].node.send_payment(route, our_payment_hash);
 
 	if let Err(APIError::ChannelUnavailable{err}) = err {
-		assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight");
+		assert_eq!(err, "Cannot send value that would put us over the max HTLC value in flight");
 	} else {
 		assert!(false);
 	}