Impl Base AMP in the receive pipeline and expose payment_secret

Sadly a huge diff here, but almost all of it is changing the method
signatures for sending/receiving/failing HTLCs and the
PaymentReceived event, which all now need to expose an
Option<[u8; 32]> for the payment_secret.

It doesn't yet properly fail back pending HTLCs when the full AMP
payment is never received (which should result in accidental
channel force-closures). Further, as sending AMP payments is not
yet supported, the only test here is a simple single-path payment
with a payment_secret in it.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -422,7 +422,7 @@ pub fn do_test(data: &[u8]) {
 						fee_msat: 5000000,
 						cltv_expiry_delta: 200,
 					}],
-				}, PaymentHash(payment_hash.into_inner())) {
+				}, PaymentHash(payment_hash.into_inner()), None) {
 					// Probably ran out of funds
 					test_return!();
 				}
@@ -446,7 +446,7 @@ pub fn do_test(data: &[u8]) {
 						fee_msat: 5000000,
 						cltv_expiry_delta: 200,
 					}],
-				}, PaymentHash(payment_hash.into_inner())) {
+				}, PaymentHash(payment_hash.into_inner()), None) {
 					// Probably ran out of funds
 					test_return!();
 				}
@@ -607,9 +607,9 @@ pub fn do_test(data: &[u8]) {
 						events::Event::PaymentReceived { payment_hash, .. } => {
 							if claim_set.insert(payment_hash.0) {
 								if $fail {
-									assert!(nodes[$node].fail_htlc_backwards(&payment_hash));
+									assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &None));
 								} else {
-									assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), 5_000_000));
+									assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &None, 5_000_000));
 								}
 							}
 						},

fuzz/src/full_stack.rs

@@ -343,7 +343,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 	}, our_network_key, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 15, 0], Arc::clone(&logger)));
 
 	let mut should_forward = false;
-	let mut payments_received: Vec<(PaymentHash, u64)> = Vec::new();
+	let mut payments_received: Vec<(PaymentHash, Option<[u8; 32]>, u64)> = Vec::new();
 	let mut payments_sent = 0;
 	let mut pending_funding_generation: Vec<([u8; 32], u64, Script)> = Vec::new();
 	let mut pending_funding_signatures = HashMap::new();
@@ -401,7 +401,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 				sha.input(&payment_hash.0[..]);
 				payment_hash.0 = Sha256::from_engine(sha).into_inner();
 				payments_sent += 1;
-				match channelmanager.send_payment(route, payment_hash) {
+				match channelmanager.send_payment(route, payment_hash, None) {
 					Ok(_) => {},
 					Err(_) => return,
 				}
@@ -428,23 +428,23 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 				}
 			},
 			8 => {
-				for (payment, amt) in payments_received.drain(..) {
+				for (payment, payment_secret, amt) in payments_received.drain(..) {
 					// SHA256 is defined as XOR of all input bytes placed in the first byte, and 0s
 					// for the remaining bytes. Thus, if not all remaining bytes are 0s we cannot
 					// fulfill this HTLC, but if they are, we can just take the first byte and
 					// place that anywhere in our preimage.
 					if &payment.0[1..] != &[0; 31] {
-						channelmanager.fail_htlc_backwards(&payment);
+						channelmanager.fail_htlc_backwards(&payment, &payment_secret);
 					} else {
 						let mut payment_preimage = PaymentPreimage([0; 32]);
 						payment_preimage.0[0] = payment.0[0];
-						channelmanager.claim_funds(payment_preimage, amt);
+						channelmanager.claim_funds(payment_preimage, &payment_secret, amt);
 					}
 				}
 			},
 			9 => {
-				for (payment, _) in payments_received.drain(..) {
-					channelmanager.fail_htlc_backwards(&payment);
+				for (payment, payment_secret, _) in payments_received.drain(..) {
+					channelmanager.fail_htlc_backwards(&payment, &payment_secret);
 				}
 			},
 			10 => {
@@ -524,9 +524,9 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 				Event::FundingBroadcastSafe { funding_txo, .. } => {
 					pending_funding_relay.push(pending_funding_signatures.remove(&funding_txo).unwrap());
 				},
-				Event::PaymentReceived { payment_hash, amt } => {
+				Event::PaymentReceived { payment_hash, payment_secret, amt } => {
 					//TODO: enhance by fetching random amounts from fuzz input?
-					payments_received.push((payment_hash, amt));
+					payments_received.push((payment_hash, payment_secret, amt));
 				},
 				Event::PaymentSent {..} => {},
 				Event::PaymentFailed {..} => {},

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -284,11 +284,14 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
 	/// guarantees are made about the existence of a channel with the short id here, nor the short
 	/// ids in the PendingHTLCInfo!
 	pub(super) forward_htlcs: HashMap<u64, Vec<HTLCForwardInfo>>,
-	/// Tracks things that were to us and can be failed/claimed by the user
+	/// (payment_hash, payment_secret) -> Vec<HTLCs> for tracking things that
+	/// were to us and can be failed/claimed by the user
 	/// Note that while this is held in the same mutex as the channels themselves, no consistency
 	/// guarantees are made about the channels given here actually existing anymore by the time you
 	/// go to read them!
-	claimable_htlcs: HashMap<PaymentHash, Vec<ClaimableHTLC>>,
+	/// TODO: We need to time out HTLCs sitting here which are waiting on other AMP HTLCs to
+	/// arrive.
+	claimable_htlcs: HashMap<(PaymentHash, Option<[u8; 32]>), Vec<ClaimableHTLC>>,
 	/// Messages to send to peers - pushed to in the same lock that they are generated in (except
 	/// for broadcast messages, where ordering isn't as strict).
 	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
@@ -1180,7 +1183,14 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 	/// In case of APIError::MonitorUpdateFailed, the commitment update has been irrevocably
 	/// committed on our end and we're just waiting for a monitor update to send it. Do NOT retry
 	/// the payment via a different route unless you intend to pay twice!
-	pub fn send_payment(&self, route: Route, payment_hash: PaymentHash) -> Result<(), APIError> {
+	///
+	/// payment_secret is unrelated to payment_hash (or PaymentPreimage) and exists to authenticate
+	/// the sender to the recipient and prevent payment-probing (deanonymization) attacks. For
+	/// newer nodes, it will be provided to you in the invoice. If you do not have one, the Route
+	/// must not contain multiple paths as otherwise the multipath data cannot be sent.
+	/// If a payment_secret *is* provided, we assume that the invoice had the basic_mpp feature bit
+	/// set (either as required or as available).
+	pub fn send_payment(&self, route: Route, payment_hash: PaymentHash, payment_secret: Option<&[u8; 32]>) -> Result<(), APIError> {
 		if route.hops.len() < 1 || route.hops.len() > 20 {
 			return Err(APIError::RouteError{err: "Route didn't go anywhere/had bogus size"});
 		}
@@ -1197,7 +1207,7 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 
 		let onion_keys = secp_call!(onion_utils::construct_onion_keys(&self.secp_ctx, &route, &session_priv),
 				APIError::RouteError{err: "Pubkey along hop was maliciously selected"});
-		let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height)?;
+		let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route, payment_secret, cur_height)?;
 		if onion_utils::route_size_insane(&onion_payloads) {
 			return Err(APIError::RouteError{err: "Route size too large considering onion data"});
 		}
@@ -1413,7 +1423,9 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 											htlc_id: prev_htlc_id,
 											incoming_packet_shared_secret: forward_info.incoming_shared_secret,
 										});
-										failed_forwards.push((htlc_source, forward_info.payment_hash, 0x4000 | 10, None));
+										failed_forwards.push((htlc_source, forward_info.payment_hash,
+											HTLCFailReason::Reason { failure_code: 0x1000 | 7, data: Vec::new() }
+										));
 									},
 									HTLCForwardInfo::FailHTLC { .. } => {
 										// Channel went away before we could fail it. This implies
@@ -1449,7 +1461,9 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 												panic!("Stated return value requirements in send_htlc() were not met");
 											}
 											let chan_update = self.get_channel_update(chan.get()).unwrap();
-											failed_forwards.push((htlc_source, payment_hash, 0x1000 | 7, Some(chan_update)));
+											failed_forwards.push((htlc_source, payment_hash,
+												HTLCFailReason::Reason { failure_code: 0x1000 | 7, data: chan_update.encode_with_len() }
+											));
 											continue;
 										},
 										Ok(update_add) => {
@@ -1558,15 +1572,48 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 									htlc_id: prev_htlc_id,
 									incoming_packet_shared_secret: incoming_shared_secret,
 								};
-								channel_state.claimable_htlcs.entry(payment_hash).or_insert(Vec::new()).push(ClaimableHTLC {
+
+								let mut total_value = 0;
+								let htlcs = channel_state.claimable_htlcs.entry((payment_hash, if let &Some(ref data) = &payment_data {
+									Some(data.payment_secret.clone()) } else { None }))
+									.or_insert(Vec::new());
+								htlcs.push(ClaimableHTLC {
 									src: prev_hop_data,
 									value: amt_to_forward,
-									payment_data,
-								});
-								new_events.push(events::Event::PaymentReceived {
-									payment_hash: payment_hash,
-									amt: amt_to_forward,
+									payment_data: payment_data.clone(),
 								});
+								if let &Some(ref data) = &payment_data {
+									for htlc in htlcs.iter() {
+										total_value += htlc.value;
+										if htlc.payment_data.as_ref().unwrap().total_msat != data.total_msat {
+											total_value = msgs::MAX_VALUE_MSAT;
+										}
+										if total_value >= msgs::MAX_VALUE_MSAT { break; }
+									}
+									if total_value >= msgs::MAX_VALUE_MSAT {
+										for htlc in htlcs.iter() {
+											failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
+													short_channel_id: htlc.src.short_channel_id,
+													htlc_id: htlc.src.htlc_id,
+													incoming_packet_shared_secret: htlc.src.incoming_packet_shared_secret,
+												}), payment_hash,
+												HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(htlc.value).to_vec() }
+											));
+										}
+									} else if total_value >= data.total_msat {
+										new_events.push(events::Event::PaymentReceived {
+											payment_hash: payment_hash,
+											payment_secret: Some(data.payment_secret),
+											amt: total_value,
+										});
+									}
+								} else {
+									new_events.push(events::Event::PaymentReceived {
+										payment_hash: payment_hash,
+										payment_secret: None,
+										amt: amt_to_forward,
+									});
+								}
 							},
 							HTLCForwardInfo::AddHTLC { .. } => {
 								panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
@@ -1580,11 +1627,8 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 			}
 		}
 
-		for (htlc_source, payment_hash, failure_code, update) in failed_forwards.drain(..) {
-			match update {
-				None => self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source, &payment_hash, HTLCFailReason::Reason { failure_code, data: Vec::new() }),
-				Some(chan_update) => self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source, &payment_hash, HTLCFailReason::Reason { failure_code, data: chan_update.encode_with_len() }),
-			};
+		for (htlc_source, payment_hash, failure_reason) in failed_forwards.drain(..) {
+			self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source, &payment_hash, failure_reason);
 		}
 
 		if handle_errors.len() > 0 {
@@ -1629,11 +1673,11 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 	/// along the path (including in our own channel on which we received it).
 	/// Returns false if no payment was found to fail backwards, true if the process of failing the
 	/// HTLC backwards has been started.
-	pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash) -> bool {
+	pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash, payment_secret: &Option<[u8; 32]>) -> bool {
 		let _ = self.total_consistency_lock.read().unwrap();
 
 		let mut channel_state = Some(self.channel_state.lock().unwrap());
-		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash);
+		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(*payment_hash, *payment_secret));
 		if let Some(mut sources) = removed_source {
 			for htlc in sources.drain(..) {
 				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
@@ -1755,13 +1799,13 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 	/// motivated attackers.
 	///
 	/// May panic if called except in response to a PaymentReceived event.
-	pub fn claim_funds(&self, payment_preimage: PaymentPreimage, expected_amount: u64) -> bool {
+	pub fn claim_funds(&self, payment_preimage: PaymentPreimage, payment_secret: &Option<[u8; 32]>, expected_amount: u64) -> bool {
 		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
 		let _ = self.total_consistency_lock.read().unwrap();
 
 		let mut channel_state = Some(self.channel_state.lock().unwrap());
-		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
+		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(payment_hash, *payment_secret));
 		if let Some(mut sources) = removed_source {
 			for htlc in sources.drain(..) {
 				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }

lightning/src/ln/channelmanager.rs

@@ -614,8 +614,9 @@ macro_rules! expect_payment_received {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {
-			Event::PaymentReceived { ref payment_hash, amt } => {
+			Event::PaymentReceived { ref payment_hash, ref payment_secret, amt } => {
 				assert_eq!($expected_payment_hash, *payment_hash);
+				assert_eq!(*payment_secret, None);
 				assert_eq!($expected_recv_value, amt);
 			},
 			_ => panic!("Unexpected event"),
@@ -636,9 +637,9 @@ macro_rules! expect_payment_sent {
 	}
 }
 
-pub fn send_along_route_with_hash<'a, 'b>(origin_node: &Node<'a, 'b>, route: Route, expected_route: &[&Node<'a, 'b>], recv_value: u64, our_payment_hash: PaymentHash) {
+pub fn send_along_route_with_secret<'a, 'b>(origin_node: &Node<'a, 'b>, route: Route, expected_route: &[&Node<'a, 'b>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<[u8; 32]>) {
 	let mut payment_event = {
-		origin_node.node.send_payment(route, our_payment_hash).unwrap();
+		origin_node.node.send_payment(route, our_payment_hash, our_payment_secret.as_ref()).unwrap();
 		check_added_monitors!(origin_node, 1);
 
 		let mut events = origin_node.node.get_and_clear_pending_msg_events();
@@ -660,8 +661,9 @@ pub fn send_along_route_with_hash<'a, 'b>(origin_node: &Node<'a, 'b>, route: Rou
 			let events_2 = node.node.get_and_clear_pending_events();
 			assert_eq!(events_2.len(), 1);
 			match events_2[0] {
-				Event::PaymentReceived { ref payment_hash, amt } => {
+				Event::PaymentReceived { ref payment_hash, ref payment_secret, amt } => {
 					assert_eq!(our_payment_hash, *payment_hash);
+					assert_eq!(our_payment_secret, *payment_secret);
 					assert_eq!(amt, recv_value);
 				},
 				_ => panic!("Unexpected event"),
@@ -678,14 +680,18 @@ pub fn send_along_route_with_hash<'a, 'b>(origin_node: &Node<'a, 'b>, route: Rou
 	}
 }
 
+pub fn send_along_route_with_hash<'a, 'b>(origin_node: &Node<'a, 'b>, route: Route, expected_route: &[&Node<'a, 'b>], recv_value: u64, our_payment_hash: PaymentHash) {
+	send_along_route_with_secret(origin_node, route, expected_route, recv_value, our_payment_hash, None);
+}
+
 pub fn send_along_route<'a, 'b>(origin_node: &Node<'a, 'b>, route: Route, expected_route: &[&Node<'a, 'b>], recv_value: u64) -> (PaymentPreimage, PaymentHash) {
 	let (our_payment_preimage, our_payment_hash) = get_payment_preimage_hash!(origin_node);
 	send_along_route_with_hash(origin_node, route, expected_route, recv_value, our_payment_hash);
 	(our_payment_preimage, our_payment_hash)
 }
 
-pub fn claim_payment_along_route<'a, 'b>(origin_node: &Node<'a, 'b>, expected_route: &[&Node<'a, 'b>], skip_last: bool, our_payment_preimage: PaymentPreimage, expected_amount: u64) {
-	assert!(expected_route.last().unwrap().node.claim_funds(our_payment_preimage, expected_amount));
+pub fn claim_payment_along_route_with_secret<'a, 'b>(origin_node: &Node<'a, 'b>, expected_route: &[&Node<'a, 'b>], skip_last: bool, our_payment_preimage: PaymentPreimage, our_payment_secret: Option<[u8; 32]>, expected_amount: u64) {
+	assert!(expected_route.last().unwrap().node.claim_funds(our_payment_preimage, &our_payment_secret, expected_amount));
 	check_added_monitors!(expected_route.last().unwrap(), 1);
 
 	let mut next_msgs: Option<(msgs::UpdateFulfillHTLC, msgs::CommitmentSigned)> = None;
@@ -762,6 +768,10 @@ pub fn claim_payment_along_route<'a, 'b>(origin_node: &Node<'a, 'b>, expected_ro
 	}
 }
 
+pub fn claim_payment_along_route<'a, 'b>(origin_node: &Node<'a, 'b>, expected_route: &[&Node<'a, 'b>], skip_last: bool, our_payment_preimage: PaymentPreimage, expected_amount: u64) {
+	claim_payment_along_route_with_secret(origin_node, expected_route, skip_last, our_payment_preimage, None, expected_amount);
+}
+
 pub fn claim_payment<'a, 'b>(origin_node: &Node<'a, 'b>, expected_route: &[&Node<'a, 'b>], our_payment_preimage: PaymentPreimage, expected_amount: u64) {
 	claim_payment_along_route(origin_node, expected_route, false, our_payment_preimage, expected_amount);
 }
@@ -787,7 +797,7 @@ pub fn route_over_limit<'a, 'b>(origin_node: &Node<'a, 'b>, expected_route: &[&N
 
 	let (_, our_payment_hash) = get_payment_preimage_hash!(origin_node);
 
-	let err = origin_node.node.send_payment(route, our_payment_hash).err().unwrap();
+	let err = origin_node.node.send_payment(route, our_payment_hash, None).err().unwrap();
 	match err {
 		APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over the max HTLC value in flight our peer will accept"),
 		_ => panic!("Unknown error variants"),
@@ -800,7 +810,7 @@ pub fn send_payment<'a, 'b>(origin: &Node<'a, 'b>, expected_route: &[&Node<'a, '
 }
 
 pub fn fail_payment_along_route<'a, 'b>(origin_node: &Node<'a, 'b>, expected_route: &[&Node<'a, 'b>], skip_last: bool, our_payment_hash: PaymentHash)  {
-	assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash));
+	assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash, &None));
 	expect_pending_htlcs_forwardable!(expected_route.last().unwrap());
 	check_added_monitors!(expected_route.last().unwrap(), 1);