Skip to content

Commit

Permalink
Use MPPFragmentFailed event if other MPP fragments are pending upon o…
Browse files Browse the repository at this point in the history 
…ne path's failure
  • Loading branch information
@valentinewallace
valentinewallace committed Sep 2, 2021
1 parent 696b7f9 commit cbd2317
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 56 deletions.
120 changes: 75 additions & 45 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
/// The session_priv bytes of outbound payments which are pending resolution.
/// The authoritative state of these HTLCs resides either within Channels or ChannelMonitors
/// (if the channel has been force-closed), however we track them here to prevent duplicative
/// PaymentSent/PaymentFailed events. Specifically, in the case of a duplicative
/// PaymentSent/MPPFragmentFailed/PaymentFailed events. Specifically, in the case of a duplicative
/// update_fulfill_htlc message after a reconnect, we may "claim" a payment twice.
/// Additionally, because ChannelMonitors are often not re-serialized after connecting block(s)
/// which may generate a claim event, we may receive similar duplicate claim/fail MonitorEvents
Expand Down Expand Up @@ -2837,24 +2837,34 @@ 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, mpp_id, .. } => {
HTLCSource::OutboundRoute { session_priv, mpp_id, path, .. } => {
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 Some(sessions) = outbounds.get_mut(&mpp_id) {
if sessions.remove(&session_priv_bytes) {
self.pending_events.lock().unwrap().push(
events::Event::PaymentFailed {
payment_hash,
rejected_by_dest: false,
#[cfg(test)]
error_code: None,
#[cfg(test)]
error_data: None,
match sessions.len() {
0 => {
self.pending_events.lock().unwrap().push(
events::Event::PaymentFailed {
payment_hash,
rejected_by_dest: false,
#[cfg(test)]
error_code: None,
#[cfg(test)]
error_data: None,
}
);
outbounds.remove(&mpp_id);
},
_ => {
self.pending_events.lock().unwrap().push(
events::Event::MPPFragmentFailed {
payment_hash,
payment_path: path.clone()
}
);
}
);
if sessions.len() == 0 {
outbounds.remove(&mpp_id);
}
continue
}
Expand Down Expand Up @@ -2885,12 +2895,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
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 partial_failure = true;
if let Some(sessions) = outbounds.get_mut(&mpp_id) {
if !sessions.remove(&session_priv_bytes) {
log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
return;
}
if sessions.len() == 0 {
partial_failure = false;
outbounds.remove(&mpp_id);
}
} else {
Expand All @@ -2915,40 +2927,58 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
);
}
self.pending_events.lock().unwrap().push(
events::Event::PaymentFailed {
payment_hash: payment_hash.clone(),
rejected_by_dest: !payment_retryable,
#[cfg(test)]
error_code: onion_error_code,
#[cfg(test)]
error_data: onion_error_data
}
);
if partial_failure {
self.pending_events.lock().unwrap().push(
events::Event::MPPFragmentFailed {
payment_hash: payment_hash.clone(),
payment_path: path.clone()
}
);
} else {
self.pending_events.lock().unwrap().push(
events::Event::PaymentFailed {
payment_hash: payment_hash.clone(),
rejected_by_dest: !payment_retryable,
#[cfg(test)]
error_code: onion_error_code,
#[cfg(test)]
error_data: onion_error_data
}
);
}
},
&HTLCFailReason::Reason {
#[cfg(test)]
ref failure_code,
#[cfg(test)]
ref data,
.. } => {
// we get a fail_malformed_htlc from the first hop
// TODO: We'd like to generate a PaymentFailureNetworkUpdate for temporary
// failures here, but that would be insufficient as get_route
// generally ignores its view of our own channels as we provide them via
// ChannelDetails.
// TODO: For non-temporary failures, we really should be closing the
// channel here as we apparently can't relay through them anyway.
self.pending_events.lock().unwrap().push(
events::Event::PaymentFailed {
payment_hash: payment_hash.clone(),
rejected_by_dest: path.len() == 1,
#[cfg(test)]
error_code: Some(*failure_code),
#[cfg(test)]
error_data: Some(data.clone()),
}
);
#[cfg(test)]
ref failure_code,
#[cfg(test)]
ref data,
.. } => {
if partial_failure {
self.pending_events.lock().unwrap().push(
events::Event::MPPFragmentFailed {
payment_hash: payment_hash.clone(),
payment_path: path.clone()
}
);
} else {
// we get a fail_malformed_htlc from the first hop
// TODO: We'd like to generate a PaymentFailureNetworkUpdate for temporary
// failures here, but that would be insufficient as get_route
// generally ignores its view of our own channels as we provide them via
// ChannelDetails.
// TODO: For non-temporary failures, we really should be closing the
// channel here as we apparently can't relay through them anyway.
self.pending_events.lock().unwrap().push(
events::Event::PaymentFailed {
payment_hash: payment_hash.clone(),
rejected_by_dest: path.len() == 1,
#[cfg(test)]
error_code: Some(*failure_code),
#[cfg(test)]
error_data: Some(data.clone()),
}
);
}
}
}
},
Expand Down
33 changes: 23 additions & 10 deletions lightning/src/ln/functional_test_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,8 @@ pub fn send_payment<'a, 'b, 'c>(origin: &Node<'a, 'b, 'c>, expected_route: &[&No
claim_payment(&origin, expected_route, our_payment_preimage);
}

pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) {
pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) {
let mut expected_paths: Vec<_> = expected_paths_slice.iter().collect();
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());
}
Expand Down Expand Up @@ -1314,8 +1315,10 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
for ev in events.iter() {
per_path_msgs.push(msgs_from_ev!(ev));
}
per_path_msgs.sort_unstable_by(|(_, node_id_a), (_, node_id_b)| node_id_a.cmp(node_id_b));
expected_paths.sort_unstable_by(|path_a, path_b| path_a[path_a.len() - 2].node.get_our_node_id().cmp(&path_b[path_b.len() - 2].node.get_our_node_id()));

for (expected_route, (path_msgs, next_hop)) in expected_paths.iter().zip(per_path_msgs.drain(..)) {
for (i, (expected_route, (path_msgs, next_hop))) in expected_paths.iter().zip(per_path_msgs.drain(..)).enumerate() {
let mut next_msgs = Some(path_msgs);
let mut expected_next_node = next_hop;
let mut prev_node = expected_route.last().unwrap();
Expand All @@ -1324,8 +1327,9 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
assert_eq!(expected_next_node, node.node.get_our_node_id());
if next_msgs.is_some() {
node.node.handle_update_fail_htlc(&prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
commitment_signed_dance!(node, prev_node, next_msgs.as_ref().unwrap().1, !(skip_last && idx == expected_route.len() - 1));
if skip_last && idx == expected_route.len() - 1 {
let last_node = !(skip_last && idx == expected_route.len() - 1);
commitment_signed_dance!(node, prev_node, next_msgs.as_ref().unwrap().1, last_node);
if !last_node {
expect_pending_htlcs_forwardable!(node);
}
}
Expand Down Expand Up @@ -1362,12 +1366,21 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
commitment_signed_dance!(origin_node, prev_node, next_msgs.as_ref().unwrap().1, false);
let events = origin_node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentFailed { payment_hash, rejected_by_dest, .. } => {
assert_eq!(payment_hash, our_payment_hash);
assert!(rejected_by_dest);
},
_ => panic!("Unexpected event"),
if i != expected_paths.len() - 1 {
match events[0] {
Event::MPPFragmentFailed { payment_hash, .. } => {
assert_eq!(payment_hash, our_payment_hash);
},
_ => panic!("Unexpected event"),
}
} else {
match events[0] {
Event::PaymentFailed { payment_hash, rejected_by_dest, .. } => {
assert_eq!(payment_hash, our_payment_hash);
assert!(rejected_by_dest);
},
_ => panic!("Unexpected event"),
}
}
}
}
Expand Down
28 changes: 28 additions & 0 deletions lightning/src/ln/functional_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4165,6 +4165,34 @@ fn test_no_txn_manager_serialize_deserialize() {
send_payment(&nodes[0], &[&nodes[1]], 1000000);
}

#[test]
fn mpp_failure() {
let chanmon_cfgs = create_chanmon_cfgs(4);
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
let nodes = create_network(4, &node_cfgs, &node_chanmgrs);

let chan_1_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
let chan_3_id = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
let chan_4_id = create_announced_chan_between_nodes(&nodes, 2, 3, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
let logger = test_utils::TestLogger::new();

let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[3]);
let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
let mut route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &[], 100000, TEST_FINAL_CLTV, &logger).unwrap();
let path = route.paths[0].clone();
route.paths.push(path);
route.paths[0][0].pubkey = nodes[1].node.get_our_node_id();
route.paths[0][0].short_channel_id = chan_1_id;
route.paths[0][1].short_channel_id = chan_3_id;
route.paths[1][0].pubkey = nodes[2].node.get_our_node_id();
route.paths[1][0].short_channel_id = chan_2_id;
route.paths[1][1].short_channel_id = chan_4_id;
send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], 200_000, payment_hash, payment_secret);
fail_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_hash);
}

#[test]
fn test_dup_htlc_onchain_fails_on_reload() {
// When a Channel is closed, any outbound HTLCs which were relayed through it are simply
Expand Down
3 changes: 2 additions & 1 deletion lightning/src/util/events.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ pub enum Event {
payment_preimage: PaymentPreimage,
},
/// Indicates an outbound payment we made failed. Probably some intermediary node dropped
/// something. You may wish to retry with a different route.
/// something. You may wish to retry with a different route. In the case of MPP payments, this
/// event indicates that all payment paths failed.
PaymentFailed {
/// The hash which was given to ChannelManager::send_payment.
payment_hash: PaymentHash,
Expand Down

0 comments on commit cbd2317

Please sign in to comment.