Drop now-useless PaymentSecret parameters when claiming/failing-back

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -687,12 +687,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 				let had_events = !events.is_empty();
 				for event in events.drain(..) {
 					match event {
-						events::Event::PaymentReceived { payment_hash, payment_secret, amt, user_payment_id: _ } => {
+						events::Event::PaymentReceived { payment_hash, payment_secret: _, amt, user_payment_id: _ } => {
 							if claim_set.insert(payment_hash.0) {
 								if $fail {
-									assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
+									assert!(nodes[$node].fail_htlc_backwards(&payment_hash));
 								} else {
-									assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
+									assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), amt));
 								}
 							}
 						},

fuzz/src/full_stack.rs

@@ -371,7 +371,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, Option<PaymentSecret>, u64)> = Vec::new();
+	let mut payments_received: Vec<(PaymentHash, 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();
@@ -476,23 +476,23 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 				}
 			},
 			8 => {
-				for (payment, payment_secret, amt) in payments_received.drain(..) {
+				for (payment, 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, &payment_secret);
+						channelmanager.fail_htlc_backwards(&payment);
 					} else {
 						let mut payment_preimage = PaymentPreimage([0; 32]);
 						payment_preimage.0[0] = payment.0[0];
-						channelmanager.claim_funds(payment_preimage, &payment_secret, amt);
+						channelmanager.claim_funds(payment_preimage, amt);
 					}
 				}
 			},
 			9 => {
-				for (payment, payment_secret, _) in payments_received.drain(..) {
-					channelmanager.fail_htlc_backwards(&payment, &payment_secret);
+				for (payment, _) in payments_received.drain(..) {
+					channelmanager.fail_htlc_backwards(&payment);
 				}
 			},
 			10 => {
@@ -571,9 +571,9 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 				Event::FundingGenerationReady { temporary_channel_id, channel_value_satoshis, output_script, .. } => {
 					pending_funding_generation.push((temporary_channel_id, channel_value_satoshis, output_script));
 				},
-				Event::PaymentReceived { payment_hash, payment_secret, amt, user_payment_id: _ } => {
+				Event::PaymentReceived { payment_hash, payment_secret: _, amt, user_payment_id: _ } => {
 					//TODO: enhance by fetching random amounts from fuzz input?
-					payments_received.push((payment_hash, payment_secret, amt));
+					payments_received.push((payment_hash, amt));
 				},
 				Event::PaymentSent {..} => {},
 				Event::PaymentFailed {..} => {},

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -121,7 +121,7 @@ fn test_monitor_and_persister_update_fail() {
 	persister.set_update_ret(Err(ChannelMonitorUpdateErr::TemporaryFailure));
 
 	// Try to update ChannelMonitor
-	assert!(nodes[1].node.claim_funds(preimage, &None, 9_000_000));
+	assert!(nodes[1].node.claim_funds(preimage, 9_000_000));
 	check_added_monitors!(nodes[1], 1);
 	let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	assert_eq!(updates.update_fulfill_htlcs.len(), 1);
@@ -305,7 +305,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 
 	// Claim the previous payment, which will result in a update_fulfill_htlc/CS from nodes[1]
 	// but nodes[0] won't respond since it is frozen.
-	assert!(nodes[1].node.claim_funds(payment_preimage_1, &None, 1_000_000));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 	let events_2 = nodes[1].node.get_and_clear_pending_msg_events();
 	assert_eq!(events_2.len(), 1);
@@ -846,7 +846,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 	let (_, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
 
 	// Fail the payment backwards, failing the monitor update on nodes[1]'s receipt of the RAA
-	assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1, &None));
+	assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1));
 	expect_pending_htlcs_forwardable!(nodes[2]);
 	check_added_monitors!(nodes[2], 1);
 
@@ -1108,7 +1108,7 @@ fn test_monitor_update_fail_reestablish() {
 	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
 	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 
-	assert!(nodes[2].node.claim_funds(our_payment_preimage, &None, 1_000_000));
+	assert!(nodes[2].node.claim_funds(our_payment_preimage, 1_000_000));
 	check_added_monitors!(nodes[2], 1);
 	let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
 	assert!(updates.update_add_htlcs.is_empty());
@@ -1315,7 +1315,7 @@ fn claim_while_disconnected_monitor_update_fail() {
 	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
 
-	assert!(nodes[1].node.claim_funds(payment_preimage_1, &None, 1_000_000));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 
 	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
@@ -1611,7 +1611,7 @@ fn test_monitor_update_fail_claim() {
 	let (payment_preimage_1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
 
 	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
-	assert!(nodes[1].node.claim_funds(payment_preimage_1, &None, 1_000_000));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 
 	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
@@ -1690,7 +1690,7 @@ fn test_monitor_update_on_pending_forwards() {
 	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000, 5_000_000);
 
 	let (_, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
-	assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1, &None));
+	assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1));
 	expect_pending_htlcs_forwardable!(nodes[2]);
 	check_added_monitors!(nodes[2], 1);
 
@@ -1777,7 +1777,7 @@ fn monitor_update_claim_fail_no_response() {
 	let as_raa = commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false, true, false, true);
 
 	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
-	assert!(nodes[1].node.claim_funds(payment_preimage_1, &None, 1_000_000));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 	let events = nodes[1].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 0);

lightning/src/ln/channelmanager.rs

@@ -2096,7 +2096,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// 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, _payment_secret: &Option<PaymentSecret>) -> bool {
+	pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash) -> bool {
 		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
 		let mut channel_state = Some(self.channel_state.lock().unwrap());
@@ -2265,18 +2265,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// generating message events for the net layer to claim the payment, if possible. Thus, you
 	/// should probably kick the net layer to go send messages if this returns true!
 	///
-	/// You must specify the expected amounts for this HTLC, and we will only claim HTLCs
-	/// available within a few percent of the expected amount. This is critical for several
-	/// reasons : a) it avoids providing senders with `proof-of-payment` (in the form of the
-	/// payment_preimage without having provided the full value and b) it avoids certain
-	/// privacy-breaking recipient-probing attacks which may reveal payment activity to
-	/// motivated attackers.
-	///
-	/// Note that the privacy concerns in (b) are not relevant in payments with a payment_secret
-	/// set. Thus, for such payments we will claim any payments which do not under-pay.
+	/// Note that if you did not set an `amount_msat` when calling [`get_payment_secret`] or
+	/// [`get_payment_secret_preimage`] you must check that the amount in the `PaymentReceived`
+	/// event matches your expectation. If you fail to do so and call this method, you may provide
+	/// the sender "proof-of-payment" when they did not fulfill the full expected payment.
 	///
 	/// May panic if called except in response to a PaymentReceived event.
-	pub fn claim_funds(&self, payment_preimage: PaymentPreimage, _payment_secret: &Option<PaymentSecret>, expected_amount: u64) -> bool {
+	///
+	/// [`get_payment_secret`]: Self::get_payment_secret
+	/// [`get_payment_secret_preimage`]: Self::get_payment_secret_preimage
+	pub fn claim_funds(&self, payment_preimage: PaymentPreimage, expected_amount: u64) -> bool {
 		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
 		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
@@ -4817,7 +4815,7 @@ pub mod bench {
 
 				expect_pending_htlcs_forwardable!(NodeHolder { node: &$node_b });
 				expect_payment_received!(NodeHolder { node: &$node_b }, payment_hash, payment_secret, 10_000);
-				assert!($node_b.claim_funds(payment_preimage, &Some(payment_secret), 10_000));
+				assert!($node_b.claim_funds(payment_preimage, 10_000));
 
 				match $node_b.get_and_clear_pending_msg_events().pop().unwrap() {
 					MessageSendEvent::UpdateHTLCs { node_id, updates } => {

lightning/src/ln/functional_test_utils.rs

@@ -1052,7 +1052,7 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
 	for path in expected_paths.iter() {
 		assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id());
 	}
-	assert!(expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage, &None, expected_amount));
+	assert!(expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage, expected_amount));
 	check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());
 
 	macro_rules! msgs_from_ev {
@@ -1176,7 +1176,7 @@ pub fn send_payment<'a, 'b, 'c>(origin: &Node<'a, 'b, 'c>, expected_route: &[&No
 }
 
 pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], skip_last: bool, our_payment_hash: PaymentHash)  {
-	assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash, &None));
+	assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash));
 	expect_pending_htlcs_forwardable!(expected_route.last().unwrap());
 	check_added_monitors!(expected_route.last().unwrap(), 1);