diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index acdf3cb3ca2..3dd0cd5a45d 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -829,6 +829,7 @@ pub fn do_test(data: &[u8], out: Out) { } }, events::Event::PaymentSent { .. } => {}, + events::Event::PaymentPathSuccessful { .. } => {}, events::Event::PaymentPathFailed { .. } => {}, events::Event::PaymentForwarded { .. } if $node == 1 => {}, events::Event::PendingHTLCsForwardable { .. } => { diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index dd717ca2f69..cb39b64a448 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -551,6 +551,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) { nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); check_added_monitors!(nodes[0], 1); + expect_payment_path_successful!(nodes[0]); expect_pending_htlcs_forwardable!(nodes[1]); @@ -1090,12 +1091,12 @@ fn test_monitor_update_fail_reestablish() { let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known()); - let (our_payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000); + let (payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000); 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)); + assert!(nodes[2].node.claim_funds(payment_preimage)); 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()); @@ -1159,13 +1160,7 @@ fn test_monitor_update_fail_reestablish() { assert_eq!(updates.update_fulfill_htlcs.len(), 1); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false); - - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { payment_preimage, .. } => assert_eq!(payment_preimage, our_payment_preimage), - _ => panic!("Unexpected event"), - } + expect_payment_sent!(nodes[0], payment_preimage); } #[test] @@ -1300,7 +1295,7 @@ fn claim_while_disconnected_monitor_update_fail() { let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2; // Forward a payment for B to claim - let (payment_preimage_1, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000); + let (payment_preimage_1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000); 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); @@ -1395,16 +1390,7 @@ fn claim_while_disconnected_monitor_update_fail() { nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa); check_added_monitors!(nodes[0], 1); - - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => { - assert_eq!(*payment_preimage, payment_preimage_1); - assert_eq!(*payment_hash, payment_hash_1); - }, - _ => panic!("Unexpected event"), - } + expect_payment_sent!(nodes[0], payment_preimage_1); claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2); } @@ -1766,7 +1752,7 @@ fn monitor_update_claim_fail_no_response() { let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2; // Forward a payment for B to claim - let (payment_preimage_1, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000); + let (payment_preimage_1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000); // Now start forwarding a second payment, skipping the last RAA so B is in AwaitingRAA let (route, payment_hash_2, payment_preimage_2, payment_secret_2) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000); @@ -1802,16 +1788,7 @@ fn monitor_update_claim_fail_no_response() { let bs_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.update_fulfill_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, false); - - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => { - assert_eq!(*payment_preimage, payment_preimage_1); - assert_eq!(*payment_hash, payment_hash_1); - }, - _ => panic!("Unexpected event"), - } + expect_payment_sent!(nodes[0], payment_preimage_1); claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2); } @@ -2323,7 +2300,7 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { assert!(updates.update_fee.is_none()); assert_eq!(updates.update_fulfill_htlcs.len(), 1); nodes[1].node.handle_update_fulfill_htlc(&nodes[0].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]); - expect_payment_sent!(nodes[1], payment_preimage_0); + expect_payment_sent_without_paths!(nodes[1], payment_preimage_0); assert_eq!(updates.update_add_htlcs.len(), 1); nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]); updates.commitment_signed @@ -2342,7 +2319,18 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { commitment_signed_dance!(nodes[1], nodes[0], (), false, true, false); - expect_pending_htlcs_forwardable!(nodes[1]); + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); + match events[0] { + Event::PendingHTLCsForwardable { .. } => { }, + _ => panic!("Unexpected event"), + }; + match events[1] { + Event::PaymentPathSuccessful { .. } => { }, + _ => panic!("Unexpected event"), + }; + + nodes[1].node.process_pending_htlc_forwards(); expect_payment_received!(nodes[1], payment_hash_2, payment_secret_2, 100000); claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1); @@ -2427,9 +2415,10 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id())); assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]); - expect_payment_sent!(nodes[0], payment_preimage); + expect_payment_sent_without_paths!(nodes[0], payment_preimage); if htlc_status == HTLCStatusAtDupClaim::Cleared { commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false); + expect_payment_path_successful!(nodes[0]); } } else { assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); @@ -2453,10 +2442,11 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id())); assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]); - expect_payment_sent!(nodes[0], payment_preimage); + expect_payment_sent_without_paths!(nodes[0], payment_preimage); } if htlc_status != HTLCStatusAtDupClaim::Cleared { commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false); + expect_payment_path_successful!(nodes[0]); } } @@ -2620,7 +2610,7 @@ fn double_temp_error() { assert_eq!(node_id, nodes[0].node.get_our_node_id()); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_1); check_added_monitors!(nodes[0], 0); - expect_payment_sent!(nodes[0], payment_preimage_1); + expect_payment_sent_without_paths!(nodes[0], payment_preimage_1); nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &commitment_signed_b1); check_added_monitors!(nodes[0], 1); nodes[0].node.process_pending_htlc_forwards(); @@ -2658,6 +2648,7 @@ fn double_temp_error() { }; nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &raa_b2); check_added_monitors!(nodes[0], 1); + expect_payment_path_successful!(nodes[0]); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_2); check_added_monitors!(nodes[0], 0); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1e60dcdccd4..9ad97150205 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -449,6 +449,7 @@ pub(crate) enum PendingOutboundPayment { /// and add a pending payment that was already fulfilled. Fulfilled { session_privs: HashSet<[u8; 32]>, + payment_hash: Option, }, } @@ -472,15 +473,24 @@ impl PendingOutboundPayment { } } + fn payment_hash(&self) -> Option { + match self { + PendingOutboundPayment::Legacy { .. } => None, + PendingOutboundPayment::Retryable { payment_hash, .. } => Some(*payment_hash), + PendingOutboundPayment::Fulfilled { payment_hash, .. } => *payment_hash, + } + } + fn mark_fulfilled(&mut self) { let mut session_privs = HashSet::new(); core::mem::swap(&mut session_privs, match self { PendingOutboundPayment::Legacy { session_privs } | PendingOutboundPayment::Retryable { session_privs, .. } | - PendingOutboundPayment::Fulfilled { session_privs } + PendingOutboundPayment::Fulfilled { session_privs, .. } => session_privs }); - *self = PendingOutboundPayment::Fulfilled { session_privs }; + let payment_hash = self.payment_hash(); + *self = PendingOutboundPayment::Fulfilled { session_privs, payment_hash }; } /// panics if path is None and !self.is_fulfilled @@ -488,7 +498,7 @@ impl PendingOutboundPayment { let remove_res = match self { PendingOutboundPayment::Legacy { session_privs } | PendingOutboundPayment::Retryable { session_privs, .. } | - PendingOutboundPayment::Fulfilled { session_privs } => { + PendingOutboundPayment::Fulfilled { session_privs, .. } => { session_privs.remove(session_priv) } }; @@ -529,7 +539,7 @@ impl PendingOutboundPayment { match self { PendingOutboundPayment::Legacy { session_privs } | PendingOutboundPayment::Retryable { session_privs, .. } | - PendingOutboundPayment::Fulfilled { session_privs } => { + PendingOutboundPayment::Fulfilled { session_privs, .. } => { session_privs.len() } } @@ -3491,14 +3501,23 @@ impl ChannelMana } fn finalize_claims(&self, mut sources: Vec) { + let mut pending_events = self.pending_events.lock().unwrap(); for source in sources.drain(..) { - if let HTLCSource::OutboundRoute { session_priv, payment_id, .. } = source { + if let HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } = source { 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) { assert!(payment.get().is_fulfilled()); - payment.get_mut().remove(&session_priv_bytes, None); + if payment.get_mut().remove(&session_priv_bytes, None) { + pending_events.push( + events::Event::PaymentPathSuccessful { + payment_id, + payment_hash: payment.get().payment_hash(), + path, + } + ); + } if payment.get().remaining_parts() == 0 { payment.remove(); } @@ -3515,9 +3534,21 @@ impl ChannelMana 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) { - let found_payment = !payment.get().is_fulfilled(); - let fee_paid_msat = payment.get().get_pending_fee_msat(); - payment.get_mut().mark_fulfilled(); + let mut pending_events = self.pending_events.lock().unwrap(); + if !payment.get().is_fulfilled() { + let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); + let fee_paid_msat = payment.get().get_pending_fee_msat(); + pending_events.push( + events::Event::PaymentSent { + payment_id: Some(payment_id), + payment_preimage, + payment_hash, + fee_paid_msat, + } + ); + payment.get_mut().mark_fulfilled(); + } + if from_onchain { // We currently immediately remove HTLCs which were fulfilled on-chain. // This could potentially lead to removing a pending payment too early, @@ -3525,22 +3556,21 @@ impl ChannelMana // restart. // TODO: We should have a second monitor event that informs us of payments // irrevocably fulfilled. - payment.get_mut().remove(&session_priv_bytes, Some(&path)); + if payment.get_mut().remove(&session_priv_bytes, Some(&path)) { + let payment_hash = Some(PaymentHash(Sha256::hash(&payment_preimage.0).into_inner())); + pending_events.push( + events::Event::PaymentPathSuccessful { + payment_id, + payment_hash, + path, + } + ); + } + if payment.get().remaining_parts() == 0 { payment.remove(); } } - if found_payment { - let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); - self.pending_events.lock().unwrap().push( - events::Event::PaymentSent { - payment_id: Some(payment_id), - payment_preimage, - payment_hash: payment_hash, - fee_paid_msat, - } - ); - } } else { log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0)); } @@ -4635,6 +4665,11 @@ impl ChannelMana pub fn has_pending_payments(&self) -> bool { !self.pending_outbound_payments.lock().unwrap().is_empty() } + + #[cfg(test)] + pub fn clear_pending_payments(&self) { + self.pending_outbound_payments.lock().unwrap().clear() + } } impl MessageSendEventsProvider for ChannelManager @@ -5559,6 +5594,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, }, (1, Fulfilled) => { (0, session_privs, required), + (1, payment_hash, option), }, (2, Retryable) => { (0, session_privs, required), @@ -6325,9 +6361,10 @@ mod tests { nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa); check_added_monitors!(nodes[0], 1); - // Note that successful MPP payments will generate 1 event upon the first path's success. No - // further events will be generated for subsequence path successes. + // Note that successful MPP payments will generate a single PaymentSent event upon the first + // path's success and a PaymentPathSuccessful event for each path's success. let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 3); match events[0] { Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash, .. } => { assert_eq!(Some(payment_id), *id); @@ -6336,6 +6373,22 @@ mod tests { }, _ => panic!("Unexpected event"), } + match events[1] { + Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path } => { + assert_eq!(payment_id, *actual_payment_id); + assert_eq!(our_payment_hash, *payment_hash.as_ref().unwrap()); + assert_eq!(route.paths[0], *path); + }, + _ => panic!("Unexpected event"), + } + match events[2] { + Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path } => { + assert_eq!(payment_id, *actual_payment_id); + assert_eq!(our_payment_hash, *payment_hash.as_ref().unwrap()); + assert_eq!(route.paths[0], *path); + }, + _ => panic!("Unexpected event"), + } } #[test] diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index e9e36e607ec..deff0f704c0 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1079,24 +1079,65 @@ macro_rules! expect_payment_received { } } +#[cfg(test)] +macro_rules! expect_payment_sent_without_paths { + ($node: expr, $expected_payment_preimage: expr) => { + expect_payment_sent!($node, $expected_payment_preimage, None::, false); + }; + ($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr) => { + expect_payment_sent!($node, $expected_payment_preimage, $expected_fee_msat_opt, false); + } +} + macro_rules! expect_payment_sent { ($node: expr, $expected_payment_preimage: expr) => { - expect_payment_sent!($node, $expected_payment_preimage, None::); + expect_payment_sent!($node, $expected_payment_preimage, None::, true); }; ($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr) => { + expect_payment_sent!($node, $expected_payment_preimage, $expected_fee_msat_opt, true); + }; + ($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr, $expect_paths: expr) => { let events = $node.node.get_and_clear_pending_events(); let expected_payment_hash = PaymentHash(Sha256::hash(&$expected_payment_preimage.0).into_inner()); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => { + if $expect_paths { + assert!(events.len() > 1); + } else { + assert_eq!(events.len(), 1); + } + let expected_payment_id = match events[0] { + Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => { assert_eq!($expected_payment_preimage, *payment_preimage); assert_eq!(expected_payment_hash, *payment_hash); assert!(fee_paid_msat.is_some()); if $expected_fee_msat_opt.is_some() { assert_eq!(*fee_paid_msat, $expected_fee_msat_opt); } + payment_id.unwrap() }, _ => panic!("Unexpected event"), + }; + if $expect_paths { + for i in 1..events.len() { + match events[i] { + Event::PaymentPathSuccessful { payment_id, payment_hash, .. } => { + assert_eq!(payment_id, expected_payment_id); + assert_eq!(payment_hash, Some(expected_payment_hash)); + }, + _ => panic!("Unexpected event"), + } + } + } + } +} + +#[cfg(test)] +macro_rules! expect_payment_path_successful { + ($node: expr) => { + let events = $node.node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::PaymentPathSuccessful { .. } => {}, + _ => panic!("Unexpected event"), } } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 8e9893870da..370e94e636a 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1904,7 +1904,7 @@ fn channel_reserve_in_flight_removes() { nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_removes.commitment_signed); check_added_monitors!(nodes[0], 1); let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id()); - expect_payment_sent!(nodes[0], payment_preimage_1); + expect_payment_sent_without_paths!(nodes[0], payment_preimage_1); nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_1.msgs[0]); nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &send_1.commitment_msg); @@ -1933,7 +1933,7 @@ fn channel_reserve_in_flight_removes() { nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_cs.commitment_signed); check_added_monitors!(nodes[0], 1); let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id()); - expect_payment_sent!(nodes[0], payment_preimage_2); + expect_payment_sent_without_paths!(nodes[0], payment_preimage_2); nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa); check_added_monitors!(nodes[1], 1); @@ -1946,6 +1946,7 @@ fn channel_reserve_in_flight_removes() { // resolve the second HTLC from A's point of view. nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa); check_added_monitors!(nodes[0], 1); + expect_payment_path_successful!(nodes[0]); let as_cs = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); // Now that B doesn't have the second RAA anymore, but A still does, send a payment from B back @@ -1975,6 +1976,7 @@ fn channel_reserve_in_flight_removes() { nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa); check_added_monitors!(nodes[0], 1); + expect_payment_path_successful!(nodes[0]); let as_cs = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_cs.commitment_signed); @@ -2654,7 +2656,7 @@ fn test_htlc_on_chain_success() { check_closed_broadcast!(nodes[0], true); check_added_monitors!(nodes[0], 1); let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 3); + assert_eq!(events.len(), 5); let mut first_claimed = false; for event in events { match event { @@ -2667,6 +2669,7 @@ fn test_htlc_on_chain_success() { assert_eq!(payment_hash, payment_hash_2); } }, + Event::PaymentPathSuccessful { .. } => {}, Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {}, _ => panic!("Unexpected event"), } @@ -3301,13 +3304,13 @@ fn test_dup_events_on_peer_disconnect() { check_added_monitors!(nodes[1], 1); let claim_msgs = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &claim_msgs.update_fulfill_htlcs[0]); - expect_payment_sent!(nodes[0], payment_preimage); + expect_payment_sent_without_paths!(nodes[0], payment_preimage); 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); reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (0, 0), (false, false)); - assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + expect_payment_path_successful!(nodes[0]); } #[test] @@ -3347,7 +3350,7 @@ fn test_simple_peer_disconnect() { reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (1, 0), (1, 0), (false, false)); { let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 2); + assert_eq!(events.len(), 3); match events[0] { Event::PaymentSent { payment_preimage, payment_hash, .. } => { assert_eq!(payment_preimage, payment_preimage_3); @@ -3362,6 +3365,10 @@ fn test_simple_peer_disconnect() { }, _ => panic!("Unexpected event"), } + match events[2] { + Event::PaymentPathSuccessful { .. } => {}, + _ => panic!("Unexpected event"), + } } claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), payment_preimage_4); @@ -3551,15 +3558,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken if messages_delivered < 2 { reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (0, 0), (false, false)); if messages_delivered < 1 { - let events_4 = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events_4.len(), 1); - match events_4[0] { - Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => { - assert_eq!(payment_preimage_1, *payment_preimage); - assert_eq!(payment_hash_1, *payment_hash); - }, - _ => panic!("Unexpected event"), - } + expect_payment_sent!(nodes[0], payment_preimage_1); } else { assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); } @@ -3577,10 +3576,18 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false)); } + if messages_delivered == 1 || messages_delivered == 2 { + expect_payment_path_successful!(nodes[0]); + } + 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); reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false)); + if messages_delivered > 2 { + expect_payment_path_successful!(nodes[0]); + } + // Channel should still work fine... let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000); let payment_preimage_2 = send_along_route(&nodes[0], route, &[&nodes[1]], 1000000).0; @@ -3892,6 +3899,7 @@ fn test_drop_messages_peer_disconnect_dual_htlc() { assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); check_added_monitors!(nodes[0], 1); + expect_payment_path_successful!(nodes[0]); claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2); } @@ -5559,24 +5567,16 @@ fn do_htlc_claim_local_commitment_only(use_dust: bool) { let nodes = create_network(2, &node_cfgs, &node_chanmgrs); let chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); - let (our_payment_preimage, our_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1]], if use_dust { 50000 } else { 3000000 }); + let (payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1]], if use_dust { 50000 } else { 3000000 }); // Claim the payment, but don't deliver A's commitment_signed, resulting in the HTLC only being // present in B's local commitment transaction, but none of A's commitment transactions. - assert!(nodes[1].node.claim_funds(our_payment_preimage)); + assert!(nodes[1].node.claim_funds(payment_preimage)); check_added_monitors!(nodes[1], 1); let bs_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.update_fulfill_htlcs[0]); - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { payment_preimage, payment_hash, .. } => { - assert_eq!(payment_preimage, our_payment_preimage); - assert_eq!(payment_hash, our_payment_hash); - }, - _ => panic!("Unexpected event"), - } + expect_payment_sent_without_paths!(nodes[0], payment_preimage); nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_updates.commitment_signed); check_added_monitors!(nodes[0], 1); @@ -5998,15 +5998,7 @@ fn test_free_and_fail_holding_cell_htlcs() { let update_msgs = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_msgs.update_fulfill_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], update_msgs.commitment_signed, false, true); - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => { - assert_eq!(*payment_preimage, payment_preimage_1); - assert_eq!(*payment_hash, payment_hash_1); - } - _ => panic!("Unexpected event"), - } + expect_payment_sent!(nodes[0], payment_preimage_1); } // Test that if we fail to forward an HTLC that is being freed from the holding cell that the diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 0e4733ea526..cfde643bcb9 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -249,7 +249,7 @@ fn do_test_claim_value_force_close(prev_commitment_tx: bool) { if prev_commitment_tx { // To build a previous commitment transaction, deliver one round of commitment messages. nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &b_htlc_msgs.update_fulfill_htlcs[0]); - expect_payment_sent!(nodes[0], payment_preimage); + expect_payment_sent_without_paths!(nodes[0], payment_preimage); nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &b_htlc_msgs.commitment_signed); check_added_monitors!(nodes[0], 1); let (as_raa, as_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id()); @@ -400,7 +400,9 @@ fn do_test_claim_value_force_close(prev_commitment_tx: bool) { // After broadcasting the HTLC claim transaction, node A will still consider the HTLC // possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it. mine_transaction(&nodes[0], &b_broadcast_txn[0]); - if !prev_commitment_tx { + if prev_commitment_tx { + expect_payment_path_successful!(nodes[0]); + } else { expect_payment_sent!(nodes[0], payment_preimage); } assert_eq!(sorted_vec(vec![Balance::MaybeClaimableHTLCAwaitingTimeout { diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index f3681946d19..e80cf98d7b9 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -477,6 +477,11 @@ fn test_onion_failure() { bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward; run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, &payment_secret, |_| {}, ||{}, true, Some(UPDATE|11), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id)); + // Clear pending payments so that the following positive test has the correct payment hash. + for node in nodes.iter() { + node.node.clear_pending_payments(); + } + // Test a positive test-case with one extra msat, meeting the minimum. bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward + 1; let preimage = send_along_route(&nodes[0], bogus_route, &[&nodes[1], &nodes[2]], amt_to_forward+1).0; diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index f685c375f38..7c9ff3199d1 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -664,7 +664,7 @@ fn test_fulfill_restart_failure() { check_added_monitors!(nodes[1], 1); let htlc_fulfill_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &htlc_fulfill_updates.update_fulfill_htlcs[0]); - expect_payment_sent!(nodes[0], payment_preimage); + expect_payment_sent_without_paths!(nodes[0], payment_preimage); // Now reload nodes[1]... persister = test_utils::TestPersister::new(); @@ -707,4 +707,5 @@ fn test_fulfill_restart_failure() { commitment_signed_dance!(nodes[0], nodes[1], htlc_fail_updates.commitment_signed, false); // nodes[0] shouldn't generate any events here, while it just got a payment failure completion // it had already considered the payment fulfilled, and now they just got free money. + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); } diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index fcec4703e44..db76a2c760c 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -83,7 +83,7 @@ fn updates_shutdown_wait() { let logger = test_utils::TestLogger::new(); let scorer = test_utils::TestScorer::with_fixed_penalty(0); - let (our_payment_preimage, our_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100000); + let (payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100000); nodes[0].node.close_channel(&chan_1.2).unwrap(); let node_0_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id()); @@ -103,7 +103,7 @@ fn updates_shutdown_wait() { unwrap_send_err!(nodes[0].node.send_payment(&route_1, payment_hash, &Some(payment_secret)), true, APIError::ChannelUnavailable {..}, {}); unwrap_send_err!(nodes[1].node.send_payment(&route_2, payment_hash, &Some(payment_secret)), true, APIError::ChannelUnavailable {..}, {}); - assert!(nodes[2].node.claim_funds(our_payment_preimage)); + assert!(nodes[2].node.claim_funds(payment_preimage)); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); assert!(updates.update_add_htlcs.is_empty()); @@ -124,16 +124,7 @@ fn updates_shutdown_wait() { assert_eq!(updates_2.update_fulfill_htlcs.len(), 1); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates_2.update_fulfill_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], updates_2.commitment_signed, false, true); - - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => { - assert_eq!(our_payment_preimage, *payment_preimage); - assert_eq!(our_payment_hash, *payment_hash); - }, - _ => panic!("Unexpected event"), - } + expect_payment_sent!(nodes[0], payment_preimage); let node_0_closing_signed = get_event_msg!(nodes[0], MessageSendEvent::SendClosingSigned, nodes[1].node.get_our_node_id()); nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &node_0_closing_signed); @@ -241,7 +232,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) { let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known()); - let (our_payment_preimage, our_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100000); + let (payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100000); nodes[1].node.close_channel(&chan_1.2).unwrap(); let node_1_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id()); @@ -281,7 +272,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) { assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); - assert!(nodes[2].node.claim_funds(our_payment_preimage)); + assert!(nodes[2].node.claim_funds(payment_preimage)); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); assert!(updates.update_add_htlcs.is_empty()); @@ -302,16 +293,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) { assert_eq!(updates_2.update_fulfill_htlcs.len(), 1); nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates_2.update_fulfill_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], updates_2.commitment_signed, false, true); - - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => { - assert_eq!(our_payment_preimage, *payment_preimage); - assert_eq!(our_payment_hash, *payment_hash); - }, - _ => panic!("Unexpected event"), - } + expect_payment_sent!(nodes[0], payment_preimage); let node_0_closing_signed = get_event_msg!(nodes[0], MessageSendEvent::SendClosingSigned, nodes[1].node.get_our_node_id()); if recv_count > 0 { diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 6bbd6b9946c..b42556841ea 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -216,7 +216,7 @@ pub enum Event { /// Note that this serves as a payment receipt, if you wish to have such a thing, you must /// store it somehow! payment_preimage: PaymentPreimage, - /// The hash which was given to [`ChannelManager::send_payment`]. + /// The hash that was given to [`ChannelManager::send_payment`]. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment payment_hash: PaymentHash, @@ -240,7 +240,9 @@ pub enum Event { /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment payment_id: Option, - /// The hash which was given to ChannelManager::send_payment. + /// The hash that was given to [`ChannelManager::send_payment`]. + /// + /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment payment_hash: PaymentHash, /// Indicates the payment was rejected for some reason by the recipient. This implies that /// the payment has failed, not just the route in question. If this is not set, you may @@ -338,7 +340,27 @@ pub enum Event { channel_id: [u8; 32], /// The full transaction received from the user transaction: Transaction - } + }, + /// Indicates that a path for an outbound payment was successful. + /// + /// Always generated after [`Event::PaymentSent`] and thus useful for scoring channels. See + /// [`Event::PaymentSent`] for obtaining the payment preimage. + PaymentPathSuccessful { + /// The id returned by [`ChannelManager::send_payment`] and used with + /// [`ChannelManager::retry_payment`]. + /// + /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment + /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + payment_id: PaymentId, + /// The hash that was given to [`ChannelManager::send_payment`]. + /// + /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment + payment_hash: Option, + /// The payment path that was successful. + /// + /// May contain a closed channel if the HTLC sent along the path was fulfilled on chain. + path: Vec, + }, } impl Writeable for Event { @@ -438,6 +460,14 @@ impl Writeable for Event { (2, transaction, required) }) }, + &Event::PaymentPathSuccessful { ref payment_id, ref payment_hash, ref path } => { + 13u8.write(writer)?; + write_tlv_fields!(writer, { + (0, payment_id, required), + (2, payment_hash, option), + (4, path, vec_type) + }) + }, // Note that, going forward, all new events must only write data inside of // `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write // data via `write_tlv_fields`. @@ -600,6 +630,24 @@ impl MaybeReadable for Event { }; f() }, + 13u8 => { + let f = || { + let mut payment_id = PaymentId([0; 32]); + let mut payment_hash = None; + let mut path: Option> = Some(vec![]); + read_tlv_fields!(reader, { + (0, payment_id, required), + (2, payment_hash, option), + (4, path, vec_type), + }); + Ok(Some(Event::PaymentPathSuccessful { + payment_id, + payment_hash, + path: path.unwrap(), + })) + }; + f() + }, // Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue. // Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt // reads.