Test failing phantom payments

Also fix a bug where we were failing back phantom payments with the wrong scid, causing
them to never actually be failed backwards.

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -358,7 +358,7 @@ mod inbound_payment {
 // our payment, which we can use to decode errors or inform the user that the payment was sent.
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
-enum PendingHTLCRouting {
+pub(super) enum PendingHTLCRouting {
 	Forward {
 		onion_packet: msgs::OnionPacket,
 		short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
@@ -375,8 +375,8 @@ enum PendingHTLCRouting {
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
 pub(super) struct PendingHTLCInfo {
-	routing: PendingHTLCRouting,
-	incoming_shared_secret: [u8; 32],
+	pub(super) routing: PendingHTLCRouting,
+	pub(super) incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,
 	pub(super) amt_to_forward: u64,
 	pub(super) outgoing_cltv_value: u32,
@@ -3011,45 +3011,63 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 									HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
 										routing, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value },
 										prev_funding_outpoint } => {
+											macro_rules! get_failure_htlc_source {
+												() => {
+													HTLCSource::PreviousHopData(HTLCPreviousHopData {
+														short_channel_id: prev_short_channel_id,
+														outpoint: prev_funding_outpoint,
+														htlc_id: prev_htlc_id,
+														incoming_packet_shared_secret: incoming_shared_secret,
+													})
+												}
+											}
 											macro_rules! fail_forward {
 												($msg: expr, $err_code: expr, $err_data: expr) => {
 													{
 														log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
-														let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData {
-															short_channel_id: short_chan_id,
-															outpoint: prev_funding_outpoint,
-															htlc_id: prev_htlc_id,
-															incoming_packet_shared_secret: incoming_shared_secret,
-														});
-														failed_forwards.push((htlc_source, payment_hash,
+														failed_forwards.push((get_failure_htlc_source!(), payment_hash,
 																HTLCFailReason::Reason { failure_code: $err_code, data: $err_data }
 														));
 														continue;
 													}
 												}
 											}
+											macro_rules! fail_phantom_forward {
+												($msg: expr, $err_code: expr, $err_data: expr, $phantom_shared_secret: expr) => {
+													{
+														log_info!(self.logger, "Failed to accept/forward incoming phantom node HTLC: {}", $msg);
+														let packet = onion_utils::build_failure_packet(&$phantom_shared_secret, $err_code, &$err_data[..]).encode();
+														let error_data =  onion_utils::encrypt_failure_packet(&$phantom_shared_secret, &packet);
+														failed_forwards.push((get_failure_htlc_source!(), payment_hash,
+																HTLCFailReason::LightningError { err: error_data }
+														));
+														continue;
+													}
+												}
+											}
 											if let PendingHTLCRouting::Forward { onion_packet, .. } = routing {
 												let phantom_secret_res = self.keys_manager.get_node_secret(Recipient::PhantomNode);
 												if phantom_secret_res.is_ok() && fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, short_chan_id) {
-													let shared_secret = {
+													let phantom_shared_secret = {
 														let mut arr = [0; 32];
 														arr.copy_from_slice(&SharedSecret::new(&onion_packet.public_key.unwrap(), &phantom_secret_res.unwrap())[..]);
 														arr
 													};
-													let next_hop = match onion_utils::decode_next_hop(shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) {
+													let next_hop = match onion_utils::decode_next_hop(phantom_shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) {
 														Ok(res) => res,
 														Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => {
-															fail_forward!(err_msg, err_code, Vec::new());
+															let sha256_of_onion = Sha256::hash(&onion_packet.hop_data).into_inner();
+															fail_forward!(err_msg, err_code, sha256_of_onion.to_vec());
 														},
 														Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => {
-															fail_forward!(err_msg, err_code, Vec::new());
+															fail_phantom_forward!(err_msg, err_code, Vec::new(), phantom_shared_secret);
 														},
 													};
 													match next_hop {
 														onion_utils::Hop::Receive(hop_data) => {
-															match self.construct_recv_pending_htlc_info(hop_data, shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value) {
+															match self.construct_recv_pending_htlc_info(hop_data, phantom_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value) {
 																Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, vec![(info, prev_htlc_id)])),
-																Err(ReceiveError { err_code, err_data, msg }) => fail_forward!(msg, err_code, err_data)
+																Err(ReceiveError { err_code, err_data, msg }) => fail_phantom_forward!(msg, err_code, err_data, phantom_shared_secret)
 															}
 														},
 														_ => panic!(),

lightning/src/ln/msgs.rs

@@ -1289,39 +1289,58 @@ impl Readable for FinalOnionHopData {
 	}
 }
 
-impl Writeable for OnionHopData {
-	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		// Note that this should never be reachable if Rust-Lightning generated the message, as we
-		// check values are sane long before we get here, though its possible in the future
-		// user-generated messages may hit this.
-		if self.amt_to_forward > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); }
-		match self.format {
+macro_rules! write_onion_hop_data {
+	($self: expr, $w: expr) => {
+		match $self.format {
 			OnionHopDataFormat::Legacy { short_channel_id } => {
-				0u8.write(w)?;
-				short_channel_id.write(w)?;
-				self.amt_to_forward.write(w)?;
-				self.outgoing_cltv_value.write(w)?;
-				w.write_all(&[0;12])?;
+				0u8.write($w)?;
+				short_channel_id.write($w)?;
+				$self.amt_to_forward.write($w)?;
+				$self.outgoing_cltv_value.write($w)?;
+				$w.write_all(&[0;12])?;
 			},
 			OnionHopDataFormat::NonFinalNode { short_channel_id } => {
-				encode_varint_length_prefixed_tlv!(w, {
-					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward), required),
-					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value), required),
+				encode_varint_length_prefixed_tlv!($w, {
+					(2, HighZeroBytesDroppedVarInt($self.amt_to_forward), required),
+					(4, HighZeroBytesDroppedVarInt($self.outgoing_cltv_value), required),
 					(6, short_channel_id, required)
 				});
 			},
 			OnionHopDataFormat::FinalNode { ref payment_data, ref keysend_preimage } => {
-				if let Some(final_data) = payment_data {
-					if final_data.total_msat > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); }
-				}
-				encode_varint_length_prefixed_tlv!(w, {
-					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward), required),
-					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value), required),
+				encode_varint_length_prefixed_tlv!($w, {
+					(2, HighZeroBytesDroppedVarInt($self.amt_to_forward), required),
+					(4, HighZeroBytesDroppedVarInt($self.outgoing_cltv_value), required),
 					(8, payment_data, option),
 					(5482373484, keysend_preimage, option)
 				});
 			},
 		}
+	};
+}
+
+impl Writeable for OnionHopData {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		// Note that this should never be reachable if Rust-Lightning generated the message, as we
+		// check values are sane long before we get here, though its possible in the future
+		// user-generated messages may hit this.
+		if self.amt_to_forward > MAX_VALUE_MSAT {
+			panic!("We should never be sending infinite/overflow onion payments");
+		}
+
+		if let OnionHopDataFormat::FinalNode { payment_data: Some(data), .. } = &self.format {
+			if data.total_msat > MAX_VALUE_MSAT {
+				panic!("We should never be sending infinite/overflow onion payments");
+			}
+		}
+		write_onion_hop_data!(&self, w);
+		Ok(())
+	}
+}
+
+impl OnionHopData {
+	#[cfg(test)]
+	pub(crate) fn write_without_checks<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		write_onion_hop_data!(&self, w);
 		Ok(())
 	}
 }