Support skimming fees when forwarding to phantom nodes

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -2529,16 +2529,24 @@ where
 					},
 				};
 
+				let is_phantom_forward = fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes,
+					short_channel_id, &self.genesis_hash);
 				PendingHTLCStatus::Forward(PendingHTLCInfo {
 					routing: PendingHTLCRouting::Forward {
 						onion_packet: outgoing_packet,
 						short_channel_id,
-						skimmed_fee_msat: None,
+						// If we don't check for phantom, then the node forwarding to us could force us to break
+						// backwards compat here by setting the skimmed_fee TLV in their update_add.
+						skimmed_fee_msat: if is_phantom_forward { msg.skimmed_fee_msat } else { None },
 					},
 					payment_hash: msg.payment_hash.clone(),
 					incoming_shared_secret: shared_secret,
 					incoming_amt_msat: Some(msg.amount_msat),
-					outgoing_amt_msat: next_hop_data.amt_to_forward,
+					outgoing_amt_msat:
+						// If an underpaying HTLC was sent to a phantom node, then the update_add's amount will
+						// be less than the sender-intended forward amount and we should use that.
+						if is_phantom_forward { cmp::min(next_hop_data.amt_to_forward, msg.amount_msat) }
+						else { next_hop_data.amt_to_forward },
 					outgoing_cltv_value: next_hop_data.outgoing_cltv_value,
 				})
 			}
@@ -3460,8 +3468,9 @@ where
 							}
 						}
 					}
-					let (counterparty_node_id, forward_chan_id) = match self.short_to_chan_info.read().unwrap().get(&short_chan_id) {
-						Some((cp_id, chan_id)) => (cp_id.clone(), chan_id.clone()),
+					let id_opt = self.short_to_chan_info.read().unwrap().get(&short_chan_id).cloned();
+					let (counterparty_node_id, forward_chan_id) = match id_opt {
+						Some((cp_id, chan_id)) => (cp_id, chan_id),
 						None => {
 							forwarding_channel_not_found!();
 							continue;

lightning/src/ln/payment_tests.rs

@@ -13,7 +13,7 @@
 
 use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
 use crate::chain::channelmonitor::{ANTI_REORG_DELAY, HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS};
-use crate::sign::EntropySource;
+use crate::sign::{EntropySource, NodeSigner, PhantomKeysManager, Recipient};
 use crate::chain::transaction::OutPoint;
 use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason};
 use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
@@ -1673,6 +1673,127 @@ fn do_accept_underpaying_htlcs_config(num_mpp_parts: usize) {
 		Some(Some(total_fee_msat - skimmed_fee_msat * num_mpp_parts as u64)), true);
 }
 
+#[test]
+fn underpay_to_phantom() {
+	// Weird case, but it's possible for a user to set up their route hints as:
+	// Liquidity provider -(intercept scid)-> user -(phantom scid)-> phantom
+	let mut chanmon_cfgs = create_chanmon_cfgs(3);
+	let seed = [42u8; 32];
+	let cross_node_seed = [43u8; 32];
+	chanmon_cfgs[2].keys_manager.backing = PhantomKeysManager::new(&seed, 43, 44, &cross_node_seed);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let mut intercept_forwards_config = test_default_channel_config();
+	intercept_forwards_config.accept_intercept_htlcs = true;
+	let mut underpay_config = test_default_channel_config();
+	underpay_config.channel_config.accept_underpaying_htlcs = true;
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(intercept_forwards_config), Some(underpay_config)]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 2_000_000, 0);
+	let channel_id = create_unannounced_chan_between_nodes_with_value(&nodes, 1, 2, 2_000_000, 0).0.channel_id;
+	let amt_msat = 900_000;
+	let skimmed_fee_msat = 20;
+	let route_hint = vec!(RouteHint(vec![RouteHintHop {
+		src_node_id: nodes[1].node.get_our_node_id(),
+		short_channel_id: nodes[1].node.get_intercept_scid(),
+		fees: RoutingFees {
+			base_msat: 1000,
+			proportional_millionths: 0,
+		},
+		cltv_expiry_delta: MIN_CLTV_EXPIRY_DELTA,
+		htlc_minimum_msat: None,
+		htlc_maximum_msat: None,
+	}, RouteHintHop {
+		src_node_id: nodes[2].node.get_our_node_id(),
+		short_channel_id: nodes[2].node.get_phantom_scid(),
+		fees: RoutingFees {
+			base_msat: 0,
+			proportional_millionths: 0,
+		},
+		cltv_expiry_delta: MIN_CLTV_EXPIRY_DELTA,
+		htlc_minimum_msat: None,
+		htlc_maximum_msat: None,
+	}]));
+	let phantom_node_id = chanmon_cfgs[2].keys_manager.backing.get_node_id(Recipient::PhantomNode).unwrap();
+	let payment_params = PaymentParameters::from_node_id(phantom_node_id, TEST_FINAL_CLTV)
+		.with_route_hints(route_hint).unwrap()
+		.with_bolt11_features(nodes[2].node.invoice_features()).unwrap();
+	let route_params = RouteParameters { payment_params, final_value_msat: amt_msat, };
+	let (payment_hash, payment_secret) = nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60, None).unwrap();
+	nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
+		PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap();
+	check_added_monitors!(nodes[0], 1);
+	let mut events: Vec<SendEvent> = nodes[0].node.get_and_clear_pending_msg_events().into_iter().map(|e| SendEvent::from_event(e)).collect();
+	assert_eq!(events.len(), 1);
+
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &events[0].msgs[0]);
+	do_commitment_signed_dance(&nodes[1], &nodes[0], &events[0].commitment_msg, false, true);
+
+	let events = nodes[1].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	let (intercept_id, expected_outbound_amt_msat) = match events[0] {
+		crate::events::Event::HTLCIntercepted {
+			intercept_id, expected_outbound_amount_msat, payment_hash: pmt_hash, ..
+		} => {
+			assert_eq!(pmt_hash, payment_hash);
+			assert_eq!(expected_outbound_amount_msat, amt_msat);
+			(intercept_id, expected_outbound_amount_msat)
+		},
+		_ => panic!()
+	};
+	nodes[1].node.forward_intercepted_htlc(intercept_id, &channel_id,
+		nodes[2].node.get_our_node_id(), expected_outbound_amt_msat - skimmed_fee_msat).unwrap();
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	let payment_event = {
+		{
+			let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
+			assert_eq!(added_monitors.len(), 1);
+			added_monitors.clear();
+		}
+		let mut events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		SendEvent::from_event(events.remove(0))
+	};
+	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
+	do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event.commitment_msg, false, true);
+	expect_pending_htlcs_forwardable!(nodes[2]);
+
+	// Claim the payment and check that the skimmed fee is as expected.
+	let payment_preimage = nodes[2].node.get_payment_preimage(payment_hash, payment_secret).unwrap();
+	let events = nodes[2].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 2);
+	match events[0] {
+		crate::events::Event::PendingHTLCsForwardable { .. } => {},
+		_ => { panic!() }
+	}
+	match events[1] {
+		crate::events::Event::PaymentClaimable {
+			ref payment_hash, ref purpose, amount_msat, counterparty_skimmed_fee_msat, receiver_node_id, ..
+		} => {
+			assert_eq!(payment_hash, payment_hash);
+			assert_eq!(skimmed_fee_msat, counterparty_skimmed_fee_msat);
+			assert_eq!(amt_msat - skimmed_fee_msat, amount_msat);
+			assert_eq!(phantom_node_id, receiver_node_id.unwrap());
+			match purpose {
+				crate::events::PaymentPurpose::InvoicePayment { payment_preimage: ev_payment_preimage,
+				payment_secret: ev_payment_secret, .. } =>
+				{
+					assert_eq!(payment_preimage, ev_payment_preimage.unwrap());
+					assert_eq!(payment_secret, *ev_payment_secret);
+				},
+				_ => panic!(),
+			}
+		},
+		_ => panic!("Unexpected event"),
+	}
+	let total_fee_msat = do_claim_payment_along_route_with_extra_penultimate_hop_fees(
+		&nodes[0], &[&[&nodes[1], &nodes[2]]], &vec![skimmed_fee_msat as u32; 1][..], false,
+		payment_preimage);
+	// The sender doesn't know that the penultimate hop took an extra fee.
+	expect_payment_sent(&nodes[0], payment_preimage,
+		Some(Some(total_fee_msat - skimmed_fee_msat)), true);
+}
+
 #[derive(PartialEq)]
 enum AutoRetry {
 	Success,