Skip to content

Store Payee info with HTLCs #1138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions fuzz/src/chanmon_consistency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ fn send_payment(source: &ChanMan, dest: &ChanMan, dest_chan_id: u64, amt: u64, p
fee_msat: amt,
cltv_expiry_delta: 200,
}]],
payee: None,
}, payment_hash, &Some(payment_secret)) {
check_payment_err(err);
false
Expand All @@ -330,6 +331,7 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des
fee_msat: amt,
cltv_expiry_delta: 200,
}]],
payee: None,
}, payment_hash, &Some(payment_secret)) {
check_payment_err(err);
false
Expand Down
1 change: 1 addition & 0 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5768,6 +5768,7 @@ mod tests {
first_hop_htlc_msat: 548,
payment_id: PaymentId([42; 32]),
payment_secret: None,
payee: None,
}
});

Expand Down
54 changes: 39 additions & 15 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use chain::transaction::{OutPoint, TransactionData};
use ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use ln::channel::{Channel, ChannelError, ChannelUpdateStatus, UpdateFulfillCommitFetch};
use ln::features::{InitFeatures, NodeFeatures};
use routing::router::{Route, RouteHop};
use routing::router::{Payee, PaymentPathRetry, Route, RouteHop};
use ln::msgs;
use ln::msgs::NetAddress;
use ln::onion_utils;
Expand Down Expand Up @@ -201,6 +201,7 @@ pub(crate) enum HTLCSource {
first_hop_htlc_msat: u64,
payment_id: PaymentId,
payment_secret: Option<PaymentSecret>,
payee: Option<Payee>,
},
}
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
Expand All @@ -211,13 +212,14 @@ impl core::hash::Hash for HTLCSource {
0u8.hash(hasher);
prev_hop_data.hash(hasher);
},
HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, first_hop_htlc_msat } => {
HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, first_hop_htlc_msat, payee } => {
1u8.hash(hasher);
path.hash(hasher);
session_priv[..].hash(hasher);
payment_id.hash(hasher);
payment_secret.hash(hasher);
first_hop_htlc_msat.hash(hasher);
payee.hash(hasher);
},
}
}
Expand All @@ -231,6 +233,7 @@ impl HTLCSource {
first_hop_htlc_msat: 0,
payment_id: PaymentId([2; 32]),
payment_secret: None,
payee: None,
}
}
}
Expand Down Expand Up @@ -2021,7 +2024,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}

// Only public for testing, this should otherwise never be called direcly
pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payee: &Option<Payee>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
let prng_seed = self.keys_manager.get_secure_random_bytes();
let session_priv_bytes = self.keys_manager.get_secure_random_bytes();
Expand Down Expand Up @@ -2071,6 +2074,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
first_hop_htlc_msat: htlc_msat,
payment_id,
payment_secret: payment_secret.clone(),
payee: payee.clone(),
}, onion_packet, &self.logger),
channel_state, chan);

Expand Down Expand Up @@ -2209,7 +2213,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
let cur_height = self.best_block.read().unwrap().height() + 1;
let mut results = Vec::new();
for path in route.paths.iter() {
results.push(self.send_payment_along_path(&path, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage));
results.push(self.send_payment_along_path(&path, &route.payee, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage));
}
let mut has_ok = false;
let mut has_err = false;
Expand Down Expand Up @@ -3098,14 +3102,22 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
self.fail_htlc_backwards_internal(channel_state,
htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data});
},
HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
HTLCSource::OutboundRoute { session_priv, payment_id, path, payee, .. } => {
let mut session_priv_bytes = [0; 32];
session_priv_bytes.copy_from_slice(&session_priv[..]);
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
if payment.get_mut().remove(&session_priv_bytes, Some(path.last().unwrap().fee_msat)) &&
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
if payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) &&
!payment.get().is_fulfilled()
{
let retry = if let Some(payee_data) = payee {
Some(PaymentPathRetry {
payee: payee_data,
final_value_msat: path_last_hop.fee_msat,
final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta,
})
} else { None };
self.pending_events.lock().unwrap().push(
events::Event::PaymentPathFailed {
payment_hash,
Expand All @@ -3114,7 +3126,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
all_paths_failed: payment.get().remaining_parts() == 0,
path: path.clone(),
short_channel_id: None,
retry: None,
retry,
#[cfg(test)]
error_code: None,
#[cfg(test)]
Expand Down Expand Up @@ -3146,13 +3158,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// from block_connected which may run during initialization prior to the chain_monitor
// being fully configured. See the docs for `ChannelManagerReadArgs` for more.
match source {
HTLCSource::OutboundRoute { ref path, session_priv, payment_id, .. } => {
HTLCSource::OutboundRoute { ref path, session_priv, payment_id, ref payee, .. } => {
let mut session_priv_bytes = [0; 32];
session_priv_bytes.copy_from_slice(&session_priv[..]);
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
let mut all_paths_failed = false;
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
if !payment.get_mut().remove(&session_priv_bytes, Some(path.last().unwrap().fee_msat)) {
if !payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) {
log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
return;
}
Expand All @@ -3167,8 +3180,15 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
return;
}
log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
mem::drop(channel_state_lock);
let retry = if let Some(payee_data) = payee {
Some(PaymentPathRetry {
payee: payee_data.clone(),
final_value_msat: path_last_hop.fee_msat,
final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta,
})
} else { None };
log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
match &onion_error {
&HTLCFailReason::LightningError { ref err } => {
#[cfg(test)]
Expand All @@ -3186,7 +3206,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
all_paths_failed,
path: path.clone(),
short_channel_id,
retry: None,
retry,
#[cfg(test)]
error_code: onion_error_code,
#[cfg(test)]
Expand Down Expand Up @@ -3215,7 +3235,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
all_paths_failed,
path: path.clone(),
short_channel_id: Some(path.first().unwrap().short_channel_id),
retry: None,
retry,
#[cfg(test)]
error_code: Some(*failure_code),
#[cfg(test)]
Expand Down Expand Up @@ -5378,12 +5398,14 @@ impl Readable for HTLCSource {
let mut path = Some(Vec::new());
let mut payment_id = None;
let mut payment_secret = None;
let mut payee = None;
read_tlv_fields!(reader, {
(0, session_priv, required),
(1, payment_id, option),
(2, first_hop_htlc_msat, required),
(3, payment_secret, option),
(4, path, vec_type),
(5, payee, option),
});
if payment_id.is_none() {
// For backwards compat, if there was no payment_id written, use the session_priv bytes
Expand All @@ -5396,6 +5418,7 @@ impl Readable for HTLCSource {
path: path.unwrap(),
payment_id: payment_id.unwrap(),
payment_secret,
payee,
})
}
1 => Ok(HTLCSource::PreviousHopData(Readable::read(reader)?)),
Expand All @@ -5407,7 +5430,7 @@ impl Readable for HTLCSource {
impl Writeable for HTLCSource {
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::io::Error> {
match self {
HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, payment_id, payment_secret } => {
HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, payment_id, payment_secret, payee } => {
0u8.write(writer)?;
let payment_id_opt = Some(payment_id);
write_tlv_fields!(writer, {
Expand All @@ -5416,6 +5439,7 @@ impl Writeable for HTLCSource {
(2, first_hop_htlc_msat, required),
(3, payment_secret, option),
(4, path, vec_type),
(5, payee, option),
});
}
HTLCSource::PreviousHopData(ref field) => {
Expand Down Expand Up @@ -6160,7 +6184,7 @@ mod tests {
// Use the utility function send_payment_along_path to send the payment with MPP data which
// indicates there are more HTLCs coming.
let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
check_added_monitors!(nodes[0], 1);
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
Expand Down Expand Up @@ -6190,7 +6214,7 @@ mod tests {
expect_payment_failed!(nodes[0], our_payment_hash, true);

// Send the second half of the original MPP payment.
nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
check_added_monitors!(nodes[0], 1);
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
Expand Down
10 changes: 8 additions & 2 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1114,9 +1114,12 @@ macro_rules! expect_payment_failed_with_update {
let events = $node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data, .. } => {
Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data, ref path, ref retry, .. } => {
assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash");
assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value");
assert!(retry.is_some(), "expected retry.is_some()");
assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
assert_eq!(retry.as_ref().unwrap().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
assert!(error_code.is_some(), "expected error_code.is_some() = true");
assert!(error_data.is_some(), "expected error_data.is_some() = true");
match network_update {
Expand All @@ -1143,9 +1146,12 @@ macro_rules! expect_payment_failed {
let events = $node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data, .. } => {
Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data, ref path, ref retry, .. } => {
assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash");
assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value");
assert!(retry.is_some(), "expected retry.is_some()");
assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
assert_eq!(retry.as_ref().unwrap().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
assert!(error_code.is_some(), "expected error_code.is_some() = true");
assert!(error_data.is_some(), "expected error_data.is_some() = true");
$(
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ fn fake_network_test() {
});
hops[1].fee_msat = chan_4.1.contents.fee_base_msat as u64 + chan_4.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000;
hops[0].fee_msat = chan_3.0.contents.fee_base_msat as u64 + chan_3.0.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000;
let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops] }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0;
let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops], payee: None }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0;

let mut hops = Vec::with_capacity(3);
hops.push(RouteHop {
Expand Down Expand Up @@ -948,7 +948,7 @@ fn fake_network_test() {
});
hops[1].fee_msat = chan_2.1.contents.fee_base_msat as u64 + chan_2.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000;
hops[0].fee_msat = chan_3.1.contents.fee_base_msat as u64 + chan_3.1.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000;
let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops] }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1;
let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops], payee: None }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1;

// Claim the rebalances...
fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2);
Expand Down Expand Up @@ -3912,7 +3912,7 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
// indicates there are more HTLCs coming.
let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
let payment_id = PaymentId([42; 32]);
nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, payment_id, &None).unwrap();
nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &our_payment_hash, &Some(payment_secret), 200000, cur_height, payment_id, &None).unwrap();
check_added_monitors!(nodes[0], 1);
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
Expand Down
1 change: 1 addition & 0 deletions lightning/src/ln/onion_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,7 @@ mod tests {
short_channel_id: 0, fee_msat: 0, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually
},
]],
payee: None,
};

let session_priv = SecretKey::from_slice(&hex::decode("4141414141414141414141414141414141414141414141414141414141414141").unwrap()[..]).unwrap();
Expand Down
30 changes: 23 additions & 7 deletions lightning/src/routing/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ pub struct Route {
/// given path is variable, keeping the length of any path to less than 20 should currently
/// ensure it is viable.
pub paths: Vec<Vec<RouteHop>>,
/// The `payee` parameter passed to [`get_route`].
/// This is used by `ChannelManager` to track information which may be required for retries,
/// provided back to you via [`Event::PaymentPathFailed`].
///
/// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed
pub payee: Option<Payee>,
}

impl Route {
Expand Down Expand Up @@ -106,7 +112,9 @@ impl Writeable for Route {
hop.write(writer)?;
}
}
write_tlv_fields!(writer, {});
write_tlv_fields!(writer, {
(1, self.payee, option),
});
Ok(())
}
}
Expand All @@ -124,8 +132,11 @@ impl Readable for Route {
}
paths.push(hops);
}
read_tlv_fields!(reader, {});
Ok(Route { paths })
let mut payee = None;
read_tlv_fields!(reader, {
(1, payee, option),
});
Ok(Route { paths, payee })
}
}

Expand Down Expand Up @@ -153,10 +164,10 @@ impl_writeable_tlv_based!(PaymentPathRetry, {
});

/// The recipient of a payment.
#[derive(Clone, Debug)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub struct Payee {
/// The node id of the payee.
pubkey: PublicKey,
pub pubkey: PublicKey,

/// Features supported by the payee.
///
Expand Down Expand Up @@ -1442,7 +1453,10 @@ where L::Target: Logger {
}
}

let route = Route { paths: selected_paths.into_iter().map(|path| path.into_iter().collect()).collect::<Result<Vec<_>, _>>()? };
let route = Route {
paths: selected_paths.into_iter().map(|path| path.into_iter().collect()).collect::<Result<Vec<_>, _>>()?,
payee: Some(payee.clone()),
};
log_info!(logger, "Got route to {}: {}", payee.pubkey, log_route!(route));
Ok(route)
}
Expand Down Expand Up @@ -4600,6 +4614,7 @@ mod tests {
short_channel_id: 0, fee_msat: 225, cltv_expiry_delta: 0
},
]],
payee: None,
};

assert_eq!(route.get_total_fees(), 250);
Expand Down Expand Up @@ -4632,6 +4647,7 @@ mod tests {
short_channel_id: 0, fee_msat: 150, cltv_expiry_delta: 0
},
]],
payee: None,
};

assert_eq!(route.get_total_fees(), 200);
Expand All @@ -4643,7 +4659,7 @@ mod tests {
// In an earlier version of `Route::get_total_fees` and `Route::get_total_amount`, they
// would both panic if the route was completely empty. We test to ensure they return 0
// here, even though its somewhat nonsensical as a route.
let route = Route { paths: Vec::new() };
let route = Route { paths: Vec::new(), payee: None };

assert_eq!(route.get_total_fees(), 0);
assert_eq!(route.get_total_amount(), 0);
Expand Down