Skip to content

Commit 51228df

Browse files
valentinewallacevalentinewallace
committed
Change PaymentPathFailed's optional network update to a Failure enum
This let us capture the errors when we fail without committing to an HTLC vs failing via update_fail.
1 parent 3ad4626 commit 51228df

File tree

7 files changed

+92
-63
lines changed

7 files changed

+92
-63
lines changed

‎lightning-background-processor/src/lib.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ use lightning::ln::peer_handler::{CustomMessageHandler, PeerManager, SocketDescr
3232
use lightning::routing::gossip::{NetworkGraph, P2PGossipSync};
3333
use lightning::routing::router::Router;
3434
use lightning::routing::scoring::{Score, WriteableScore};
35-
use lightning::util::events::{Event, EventHandler, EventsProvider};
35+
use lightning::util::events::{Event, EventHandler, EventsProvider, Failure};
3636
use lightning::util::logger::Logger;
3737
use lightning::util::persist::Persister;
3838
use lightning_rapid_gossip_sync::RapidGossipSync;
@@ -214,10 +214,10 @@ where
214214
fn handle_network_graph_update<L: Deref>(
215215
network_graph: &NetworkGraph<L>, event: &Event
216216
) where L::Target: Logger {
217-
if let Event::PaymentPathFailed { ref network_update, .. } = event {
218-
if let Some(network_update) = network_update {
219-
network_graph.handle_network_update(&network_update);
220-
}
217+
if let Event::PaymentPathFailed {
218+
failure: Failure::OnPath { network_update: Some(ref upd) }, .. } = event
219+
{
220+
network_graph.handle_network_update(upd);
221221
}
222222
}
223223

@@ -672,7 +672,7 @@ mod tests {
672672
use lightning::routing::router::{DefaultRouter, RouteHop};
673673
use lightning::routing::scoring::{ChannelUsage, Score};
674674
use lightning::util::config::UserConfig;
675-
use lightning::util::events::{Event, MessageSendEventsProvider, MessageSendEvent};
675+
use lightning::util::events::{Event, Failure, MessageSendEventsProvider, MessageSendEvent};
676676
use lightning::util::ser::Writeable;
677677
use lightning::util::test_utils;
678678
use lightning::util::persist::KVStorePersister;
@@ -1364,7 +1364,7 @@ mod tests {
13641364
payment_id: None,
13651365
payment_hash: PaymentHash([42; 32]),
13661366
payment_failed_permanently: false,
1367-
network_update: None,
1367+
failure: Failure::OnPath { network_update: None },
13681368
path: path.clone(),
13691369
short_channel_id: Some(scored_scid),
13701370
retry: None,
@@ -1384,7 +1384,7 @@ mod tests {
13841384
payment_id: None,
13851385
payment_hash: PaymentHash([42; 32]),
13861386
payment_failed_permanently: true,
1387-
network_update: None,
1387+
failure: Failure::OnPath { network_update: None },
13881388
path: path.clone(),
13891389
short_channel_id: None,
13901390
retry: None,

‎lightning/src/ln/functional_test_utils.rs

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ use crate::util::enforcing_trait_impls::EnforcingSigner;
2424
use crate::util::scid_utils;
2525
use crate::util::test_utils;
2626
use crate::util::test_utils::{panicking, TestChainMonitor};
27-
use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};
27+
use crate::util::events::{Event, Failure, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};
2828
use crate::util::errors::APIError;
2929
use crate::util::config::UserConfig;
3030
use crate::util::ser::{ReadableArgs, Writeable};
@@ -1800,7 +1800,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
18001800
expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e>
18011801
) {
18021802
let expected_payment_id = match &payment_failed_events[0] {
1803-
Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, network_update, short_channel_id,
1803+
Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, failure, short_channel_id,
18041804
#[cfg(test)]
18051805
error_code,
18061806
#[cfg(test)]
@@ -1825,23 +1825,24 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
18251825
}
18261826

18271827
if let Some(chan_closed) = conditions.expected_blamed_chan_closed {
1828-
match network_update {
1829-
Some(NetworkUpdate::ChannelUpdateMessage { ref msg }) if !chan_closed => {
1830-
if let Some(scid) = conditions.expected_blamed_scid {
1831-
assert_eq!(msg.contents.short_channel_id, scid);
1832-
}
1833-
const CHAN_DISABLED_FLAG: u8 = 2;
1834-
assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
1835-
},
1836-
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) if chan_closed => {
1837-
if let Some(scid) = conditions.expected_blamed_scid {
1838-
assert_eq!(*short_channel_id, scid);
1839-
}
1840-
assert!(is_permanent);
1841-
},
1842-
Some(_) => panic!("Unexpected update type"),
1843-
None => panic!("Expected update"),
1844-
}
1828+
if let Failure::OnPath { network_update: Some(upd) } = failure {
1829+
match upd {
1830+
NetworkUpdate::ChannelUpdateMessage { ref msg } if !chan_closed => {
1831+
if let Some(scid) = conditions.expected_blamed_scid {
1832+
assert_eq!(msg.contents.short_channel_id, scid);
1833+
}
1834+
const CHAN_DISABLED_FLAG: u8 = 2;
1835+
assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
1836+
},
1837+
NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } if chan_closed => {
1838+
if let Some(scid) = conditions.expected_blamed_scid {
1839+
assert_eq!(*short_channel_id, scid);
1840+
}
1841+
assert!(is_permanent);
1842+
},
1843+
_ => panic!("Unexpected update type"),
1844+
}
1845+
} else { panic!("Expected network update"); }
18451846
}
18461847

18471848
payment_id.unwrap()

‎lightning/src/ln/functional_tests.rs

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ use crate::ln::msgs;
3131
use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
3232
use crate::util::enforcing_trait_impls::EnforcingSigner;
3333
use crate::util::test_utils;
34-
use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination};
34+
use crate::util::events::{Event, Failure, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination};
3535
use crate::util::errors::APIError;
3636
use crate::util::ser::{Writeable, ReadableArgs};
3737
use crate::util::config::UserConfig;
@@ -3235,12 +3235,12 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
32353235
let events = nodes[0].node.get_and_clear_pending_events();
32363236
assert_eq!(events.len(), 6);
32373237
match events[0] {
3238-
Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
3238+
Event::PaymentPathFailed { ref payment_hash, ref failure, .. } => {
32393239
assert!(failed_htlcs.insert(payment_hash.0));
32403240
// If we delivered B's RAA we got an unknown preimage error, not something
32413241
// that we should update our routing table for.
32423242
if !deliver_bs_raa {
3243-
assert!(network_update.is_some());
3243+
if let Failure::OnPath { network_update: Some(_) } = failure { } else { panic!("Unexpected path failure") }
32443244
}
32453245
},
32463246
_ => panic!("Unexpected event"),
@@ -3252,9 +3252,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
32523252
_ => panic!("Unexpected event"),
32533253
}
32543254
match events[2] {
3255-
Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
3255+
Event::PaymentPathFailed { ref payment_hash, failure: Failure::OnPath { network_update: Some(_) }, .. } => {
32563256
assert!(failed_htlcs.insert(payment_hash.0));
3257-
assert!(network_update.is_some());
32583257
},
32593258
_ => panic!("Unexpected event"),
32603259
}
@@ -3265,9 +3264,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
32653264
_ => panic!("Unexpected event"),
32663265
}
32673266
match events[4] {
3268-
Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
3267+
Event::PaymentPathFailed { ref payment_hash, failure: Failure::OnPath { network_update: Some(_) }, .. } => {
32693268
assert!(failed_htlcs.insert(payment_hash.0));
3270-
assert!(network_update.is_some());
32713269
},
32723270
_ => panic!("Unexpected event"),
32733271
}
@@ -5147,14 +5145,14 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
51475145
let mut as_failds = HashSet::new();
51485146
let mut as_updates = 0;
51495147
for event in as_events.iter() {
5150-
if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event {
5148+
if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref failure, .. } = event {
51515149
assert!(as_failds.insert(*payment_hash));
51525150
if *payment_hash != payment_hash_2 {
51535151
assert_eq!(*payment_failed_permanently, deliver_last_raa);
51545152
} else {
51555153
assert!(!payment_failed_permanently);
51565154
}
5157-
if network_update.is_some() {
5155+
if let Failure::OnPath { network_update: Some(_) } = failure {
51585156
as_updates += 1;
51595157
}
51605158
} else if let &Event::PaymentFailed { .. } = event {
@@ -5173,14 +5171,14 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
51735171
let mut bs_failds = HashSet::new();
51745172
let mut bs_updates = 0;
51755173
for event in bs_events.iter() {
5176-
if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event {
5174+
if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref failure, .. } = event {
51775175
assert!(bs_failds.insert(*payment_hash));
51785176
if *payment_hash != payment_hash_1 && *payment_hash != payment_hash_5 {
51795177
assert_eq!(*payment_failed_permanently, deliver_last_raa);
51805178
} else {
51815179
assert!(!payment_failed_permanently);
51825180
}
5183-
if network_update.is_some() {
5181+
if let Failure::OnPath { network_update: Some(_) } = failure {
51845182
bs_updates += 1;
51855183
}
51865184
} else if let &Event::PaymentFailed { .. } = event {
@@ -5693,11 +5691,10 @@ fn test_fail_holding_cell_htlc_upon_free() {
56935691
let events = nodes[0].node.get_and_clear_pending_events();
56945692
assert_eq!(events.len(), 2);
56955693
match &events[0] {
5696-
&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref short_channel_id, .. } => {
5694+
&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, failure: Failure::OnPath { network_update: None }, ref short_channel_id, .. } => {
56975695
assert_eq!(PaymentId(our_payment_hash.0), *payment_id.as_ref().unwrap());
56985696
assert_eq!(our_payment_hash.clone(), *payment_hash);
56995697
assert_eq!(*payment_failed_permanently, false);
5700-
assert_eq!(*network_update, None);
57015698
assert_eq!(*short_channel_id, Some(route.paths[0][0].short_channel_id));
57025699
},
57035700
_ => panic!("Unexpected event"),
@@ -5783,11 +5780,10 @@ fn test_free_and_fail_holding_cell_htlcs() {
57835780
let events = nodes[0].node.get_and_clear_pending_events();
57845781
assert_eq!(events.len(), 2);
57855782
match &events[0] {
5786-
&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref short_channel_id, .. } => {
5783+
&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, failure: Failure::OnPath { network_update: None }, ref short_channel_id, .. } => {
57875784
assert_eq!(payment_id_2, *payment_id.as_ref().unwrap());
57885785
assert_eq!(payment_hash_2.clone(), *payment_hash);
57895786
assert_eq!(*payment_failed_permanently, false);
5790-
assert_eq!(*network_update, None);
57915787
assert_eq!(*short_channel_id, Some(route_2.paths[0][0].short_channel_id));
57925788
},
57935789
_ => panic!("Unexpected event"),
@@ -6685,8 +6681,7 @@ fn test_channel_failed_after_message_with_badonion_node_perm_bits_set() {
66856681
// Expect a PaymentPathFailed event with a ChannelFailure network update for the channel between
66866682
// the node originating the error to its next hop.
66876683
match events_5[0] {
6688-
Event::PaymentPathFailed { network_update:
6689-
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }), error_code, ..
6684+
Event::PaymentPathFailed { error_code, failure: Failure::OnPath { network_update: Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) }, ..
66906685
} => {
66916686
assert_eq!(short_channel_id, chan_2.0.contents.short_channel_id);
66926687
assert!(is_permanent);

‎lightning/src/ln/onion_route_tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use crate::ln::features::{InitFeatures, InvoiceFeatures};
2323
use crate::ln::msgs;
2424
use crate::ln::msgs::{ChannelMessageHandler, ChannelUpdate};
2525
use crate::ln::wire::Encode;
26-
use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
26+
use crate::util::events::{Event, Failure, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
2727
use crate::util::ser::{Writeable, Writer};
2828
use crate::util::test_utils;
2929
use crate::util::config::{UserConfig, ChannelConfig};
@@ -167,7 +167,7 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
167167

168168
let events = nodes[0].node.get_and_clear_pending_events();
169169
assert_eq!(events.len(), 2);
170-
if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref network_update, ref short_channel_id, ref error_code, .. } = &events[0] {
170+
if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref short_channel_id, ref error_code, failure: Failure::OnPath { ref network_update }, .. } = &events[0] {
171171
assert_eq!(*payment_failed_permanently, !expected_retryable);
172172
assert_eq!(*error_code, expected_error_code);
173173
if expected_channel_update.is_some() {

‎lightning/src/ln/outbound_payment.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1171,7 +1171,7 @@ impl OutboundPayments {
11711171
payment_id: Some(*payment_id),
11721172
payment_hash: payment_hash.clone(),
11731173
payment_failed_permanently: !payment_retryable,
1174-
network_update,
1174+
failure: events::Failure::OnPath { network_update },
11751175
path: path.clone(),
11761176
short_channel_id,
11771177
retry,
@@ -1222,13 +1222,13 @@ fn push_payment_path_failed_evs<I: Iterator<Item = Result<(), APIError>>>(
12221222
) {
12231223
let mut events = pending_events.lock().unwrap();
12241224
for (path, path_res) in paths.into_iter().zip(path_results) {
1225-
if path_res.is_err() {
1225+
if let Err(e) = path_res {
12261226
let scid = path[0].short_channel_id;
12271227
events.push(events::Event::PaymentPathFailed {
12281228
payment_id: Some(payment_id),
12291229
payment_hash,
12301230
payment_failed_permanently: false,
1231-
network_update: None,
1231+
failure: events::Failure::InitialSend(format!("{:?}", e)),
12321232
path,
12331233
short_channel_id: Some(scid),
12341234
retry: None,

‎lightning/src/ln/payment_tests.rs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ use crate::ln::msgs::ChannelMessageHandler;
2222
use crate::ln::outbound_payment::Retry;
2323
use crate::routing::gossip::RoutingFees;
2424
use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RouteParameters};
25-
use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
25+
use crate::util::events::{ClosureReason, Event, Failure, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
2626
use crate::util::test_utils;
2727
use crate::util::errors::APIError;
2828
use crate::util::ser::Writeable;
@@ -2055,9 +2055,11 @@ fn retry_multi_path_single_failed_payment() {
20552055
assert_eq!(events.len(), 1);
20562056
match events[0] {
20572057
Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
2058-
network_update: None, short_channel_id: Some(expected_scid), .. } => {
2058+
failure: Failure::InitialSend(ref err_msg), short_channel_id: Some(expected_scid), .. } =>
2059+
{
20592060
assert_eq!(payment_hash, ev_payment_hash);
20602061
assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
2062+
assert!(err_msg.contains("max HTLC"));
20612063
},
20622064
_ => panic!("Unexpected event"),
20632065
}
@@ -2126,9 +2128,11 @@ fn immediate_retry_on_failure() {
21262128
assert_eq!(events.len(), 1);
21272129
match events[0] {
21282130
Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
2129-
network_update: None, short_channel_id: Some(expected_scid), .. } => {
2131+
failure: Failure::InitialSend(ref err_msg), short_channel_id: Some(expected_scid), .. } =>
2132+
{
21302133
assert_eq!(payment_hash, ev_payment_hash);
21312134
assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
2135+
assert!(err_msg.contains("max HTLC"));
21322136
},
21332137
_ => panic!("Unexpected event"),
21342138
}

0 commit comments

Comments
 (0)