Fix blinded recipient fail on Channel error

If a blinded HTLC errors when added to a Channel, such as if the recipient has
already sent a shutdown message, they should malformed-fail backwards with
error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLT 4.

Author: valentinewallace

lightning/src/ln/blinded_payment_tests.rs

@@ -10,7 +10,7 @@
 use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
 use crate::blinded_path::BlindedPath;
 use crate::blinded_path::payment::{ForwardNode, ForwardTlvs, PaymentConstraints, PaymentRelay, ReceiveTlvs};
-use crate::events::{HTLCDestination, MessageSendEventsProvider};
+use crate::events::{HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
 use crate::ln::PaymentSecret;
 use crate::ln::channelmanager;
 use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
@@ -459,6 +459,7 @@ fn two_hop_blinded_path_success() {
 	claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);
 }
 
+#[derive(PartialEq)]
 enum ReceiveCheckFail {
 	// The recipient fails the payment upon `PaymentClaimable`.
 	RecipientFail,
@@ -467,13 +468,17 @@ enum ReceiveCheckFail {
 	// The incoming HTLC did not satisfy our requirements; in this case it underpaid us according to
 	// the expected receive amount in the onion.
 	ReceiveRequirements,
+	// The incoming HTLC errors when added to the Channel, in this case due to the HTLC being
+	// delivered out-of-order with a shutdown message.
+	ChannelCheck,
 }
 
 #[test]
 fn multi_hop_receiver_fail() {
 	do_multi_hop_receiver_fail(ReceiveCheckFail::RecipientFail);
 	do_multi_hop_receiver_fail(ReceiveCheckFail::OnionDecodeFail);
 	do_multi_hop_receiver_fail(ReceiveCheckFail::ReceiveRequirements);
+	do_multi_hop_receiver_fail(ReceiveCheckFail::ChannelCheck);
 }
 
 fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
@@ -486,7 +491,12 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 	let session_priv = [3; 32];
 	*nodes[0].keys_manager.override_random_bytes.lock().unwrap() = Some(session_priv);
 	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
-	let chan_upd_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).0.contents;
+	let (chan_upd_1_2, chan_id_1_2) = {
+		let (chan_upd, _, channel_id, ..) = create_announced_chan_between_nodes_with_value(
+			&nodes, 1, 2, 1_000_000, 0
+		);
+		(chan_upd.contents, channel_id)
+	};
 
 	let amt_msat = 5000;
 	let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), None);
@@ -566,6 +576,19 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 			check_added_monitors!(nodes[2], 0);
 			do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
 		},
+		ReceiveCheckFail::ChannelCheck => {
+			nodes[2].node.close_channel(&chan_id_1_2, &nodes[1].node.get_our_node_id()).unwrap();
+			let node_2_shutdown = get_event_msg!(nodes[2], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());
+			nodes[1].node.handle_shutdown(&nodes[2].node.get_our_node_id(), &node_2_shutdown);
+			let node_1_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[2].node.get_our_node_id());
+
+			nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
+			nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &payment_event_1_2.commitment_msg);
+			check_added_monitors!(nodes[2], 1);
+
+			nodes[2].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_1_shutdown);
+			commitment_signed_dance!(nodes[2], nodes[1], (), false, true, false, false);
+		}
 	}
 
 	let updates_2_1 = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
@@ -576,7 +599,20 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 	nodes[1].node.handle_update_fail_malformed_htlc(&nodes[2].node.get_our_node_id(), update_malformed);
 	do_commitment_signed_dance(&nodes[1], &nodes[2], &updates_2_1.commitment_signed, true, false);
 
-	let updates_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+	let updates_1_0 = if check == ReceiveCheckFail::ChannelCheck {
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 2);
+		events.into_iter().find_map(|ev| {
+			match ev {
+				MessageSendEvent:: UpdateHTLCs { node_id, updates } => {
+					assert_eq!(node_id, nodes[0].node.get_our_node_id());
+					return Some(updates)
+				},
+				MessageSendEvent::SendClosingSigned { .. } => None,
+				_ => panic!()
+			}
+		}).unwrap()
+	} else { get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()) };
 	assert_eq!(updates_1_0.update_fail_htlcs.len(), 1);
 	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates_1_0.update_fail_htlcs[0]);
 	do_commitment_signed_dance(&nodes[0], &nodes[1], &updates_1_0.commitment_signed, false, false);

lightning/src/ln/channelmanager.rs

@@ -6603,6 +6603,16 @@ where
 						Err(e) => PendingHTLCStatus::Fail(e)
 					};
 					let create_pending_htlc_status = |chan: &Channel<SP>, pending_forward_info: PendingHTLCStatus, error_code: u16| {
+						if msg.blinding_point.is_some() {
+							return PendingHTLCStatus::Fail(HTLCFailureMsg::Malformed(
+									msgs::UpdateFailMalformedHTLC {
+										channel_id: msg.channel_id,
+										htlc_id: msg.htlc_id,
+										sha256_of_onion: [0; 32],
+										failure_code: INVALID_ONION_BLINDING,
+									}
+							))
+						}
 						// If the update_add is completely bogus, the call will Err and we will close,
 						// but if we've sent a shutdown and they haven't acknowledged it yet, we just
 						// want to reject the new HTLC and fail it backwards instead of forwarding.