Merge pull request #2043 from valentinewallace/2023-02-initial-send-path-fails

`PaymentPathFailed`: add initial send error details

Author: TheBlueMatt

lightning-background-processor/src/lib.rs

@@ -33,7 +33,7 @@ use lightning::routing::gossip::{NetworkGraph, P2PGossipSync};
 use lightning::routing::utxo::UtxoLookup;
 use lightning::routing::router::Router;
 use lightning::routing::scoring::{Score, WriteableScore};
-use lightning::util::events::{Event, EventHandler, EventsProvider};
+use lightning::util::events::{Event, EventHandler, EventsProvider, PathFailure};
 use lightning::util::logger::Logger;
 use lightning::util::persist::Persister;
 use lightning_rapid_gossip_sync::RapidGossipSync;
@@ -214,10 +214,10 @@ where
 fn handle_network_graph_update<L: Deref>(
 	network_graph: &NetworkGraph<L>, event: &Event
 ) where L::Target: Logger {
-	if let Event::PaymentPathFailed { ref network_update, .. } = event {
-		if let Some(network_update) = network_update {
-			network_graph.handle_network_update(&network_update);
-		}
+	if let Event::PaymentPathFailed {
+		failure: PathFailure::OnPath { network_update: Some(ref upd) }, .. } = event
+	{
+		network_graph.handle_network_update(upd);
 	}
 }
 
@@ -672,7 +672,7 @@ mod tests {
 	use lightning::routing::router::{DefaultRouter, RouteHop};
 	use lightning::routing::scoring::{ChannelUsage, Score};
 	use lightning::util::config::UserConfig;
-	use lightning::util::events::{Event, MessageSendEventsProvider, MessageSendEvent};
+	use lightning::util::events::{Event, PathFailure, MessageSendEventsProvider, MessageSendEvent};
 	use lightning::util::ser::Writeable;
 	use lightning::util::test_utils;
 	use lightning::util::persist::KVStorePersister;
@@ -1364,8 +1364,7 @@ mod tests {
 			payment_id: None,
 			payment_hash: PaymentHash([42; 32]),
 			payment_failed_permanently: false,
-			network_update: None,
-			all_paths_failed: true,
+			failure: PathFailure::OnPath { network_update: None },
 			path: path.clone(),
 			short_channel_id: Some(scored_scid),
 			retry: None,
@@ -1385,8 +1384,7 @@ mod tests {
 			payment_id: None,
 			payment_hash: PaymentHash([42; 32]),
 			payment_failed_permanently: true,
-			network_update: None,
-			all_paths_failed: true,
+			failure: PathFailure::OnPath { network_update: None },
 			path: path.clone(),
 			short_channel_id: None,
 			retry: None,

lightning/src/chain/channelmonitor.rs

@@ -49,7 +49,7 @@ use crate::chain::onchaintx::OnchainTxHandler;
 use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput};
 use crate::chain::Filter;
 use crate::util::logger::Logger;
-use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, U48, OptionDeserWrapper};
+use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, UpgradableRequired, Writer, Writeable, U48};
 use crate::util::byte_utils;
 use crate::util::events::Event;
 #[cfg(anchors)]
@@ -314,8 +314,8 @@ impl Readable for CounterpartyCommitmentParameters {
 				}
 			}
 
-			let mut counterparty_delayed_payment_base_key = OptionDeserWrapper(None);
-			let mut counterparty_htlc_base_key = OptionDeserWrapper(None);
+			let mut counterparty_delayed_payment_base_key = RequiredWrapper(None);
+			let mut counterparty_htlc_base_key = RequiredWrapper(None);
 			let mut on_counterparty_tx_csv: u16 = 0;
 			read_tlv_fields!(r, {
 				(0, counterparty_delayed_payment_base_key, required),
@@ -454,19 +454,15 @@ impl MaybeReadable for OnchainEventEntry {
 		let mut transaction = None;
 		let mut block_hash = None;
 		let mut height = 0;
-		let mut event = None;
+		let mut event = UpgradableRequired(None);
 		read_tlv_fields!(reader, {
 			(0, txid, required),
 			(1, transaction, option),
 			(2, height, required),
 			(3, block_hash, option),
-			(4, event, ignorable),
+			(4, event, upgradable_required),
 		});
-		if let Some(ev) = event {
-			Ok(Some(Self { txid, transaction, height, block_hash, event: ev }))
-		} else {
-			Ok(None)
-		}
+		Ok(Some(Self { txid, transaction, height, block_hash, event: _init_tlv_based_struct_field!(event, upgradable_required) }))
 	}
 }
 

lightning/src/chain/onchaintx.rs

@@ -36,7 +36,7 @@ use crate::chain::keysinterface::WriteableEcdsaChannelSigner;
 use crate::chain::package::PackageSolvingData;
 use crate::chain::package::PackageTemplate;
 use crate::util::logger::Logger;
-use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, VecWriter};
+use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, UpgradableRequired, Writer, Writeable, VecWriter};
 
 use crate::io;
 use crate::prelude::*;
@@ -106,18 +106,14 @@ impl MaybeReadable for OnchainEventEntry {
 		let mut txid = Txid::all_zeros();
 		let mut height = 0;
 		let mut block_hash = None;
-		let mut event = None;
+		let mut event = UpgradableRequired(None);
 		read_tlv_fields!(reader, {
 			(0, txid, required),
 			(1, block_hash, option),
 			(2, height, required),
-			(4, event, ignorable),
+			(4, event, upgradable_required),
 		});
-		if let Some(ev) = event {
-			Ok(Some(Self { txid, height, block_hash, event: ev }))
-		} else {
-			Ok(None)
-		}
+		Ok(Some(Self { txid, height, block_hash, event: _init_tlv_based_struct_field!(event, upgradable_required) }))
 	}
 }
 

lightning/src/ln/channelmanager.rs

@@ -468,7 +468,7 @@ pub(crate) enum MonitorUpdateCompletionAction {
 
 impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction,
 	(0, PaymentClaimed) => { (0, payment_hash, required) },
-	(2, EmitEvent) => { (0, event, ignorable) },
+	(2, EmitEvent) => { (0, event, upgradable_required) },
 );
 
 /// State we hold per-peer.
@@ -2420,10 +2420,10 @@ where
 		let session_priv = SecretKey::from_slice(&session_priv_bytes[..]).expect("RNG is busted");
 
 		let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
-			.map_err(|_| APIError::InvalidRoute{err: "Pubkey along hop was maliciously selected"})?;
+			.map_err(|_| APIError::InvalidRoute{err: "Pubkey along hop was maliciously selected".to_owned()})?;
 		let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(path, total_value, payment_secret, cur_height, keysend_preimage)?;
 		if onion_utils::route_size_insane(&onion_payloads) {
-			return Err(APIError::InvalidRoute{err: "Route size too large considering onion data"});
+			return Err(APIError::InvalidRoute{err: "Route size too large considering onion data".to_owned()});
 		}
 		let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash);
 
@@ -6693,7 +6693,7 @@ impl Writeable for ClaimableHTLC {
 
 impl Readable for ClaimableHTLC {
 	fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
-		let mut prev_hop = crate::util::ser::OptionDeserWrapper(None);
+		let mut prev_hop = crate::util::ser::RequiredWrapper(None);
 		let mut value = 0;
 		let mut payment_data: Option<msgs::FinalOnionHopData> = None;
 		let mut cltv_expiry = 0;
@@ -6743,7 +6743,7 @@ impl Readable for HTLCSource {
 		let id: u8 = Readable::read(reader)?;
 		match id {
 			0 => {
-				let mut session_priv: crate::util::ser::OptionDeserWrapper<SecretKey> = crate::util::ser::OptionDeserWrapper(None);
+				let mut session_priv: crate::util::ser::RequiredWrapper<SecretKey> = crate::util::ser::RequiredWrapper(None);
 				let mut first_hop_htlc_msat: u64 = 0;
 				let mut path = Some(Vec::new());
 				let mut payment_id = None;

lightning/src/ln/functional_test_utils.rs

@@ -24,7 +24,7 @@ use crate::util::enforcing_trait_impls::EnforcingSigner;
 use crate::util::scid_utils;
 use crate::util::test_utils;
 use crate::util::test_utils::{panicking, TestChainMonitor};
-use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};
+use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose};
 use crate::util::errors::APIError;
 use crate::util::config::UserConfig;
 use crate::util::ser::{ReadableArgs, Writeable};
@@ -1818,7 +1818,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 ) {
 	if conditions.expected_mpp_parts_remain { assert_eq!(payment_failed_events.len(), 1); } else { assert_eq!(payment_failed_events.len(), 2); }
 	let expected_payment_id = match &payment_failed_events[0] {
-		Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, network_update, short_channel_id,
+		Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, failure, short_channel_id,
 			#[cfg(test)]
 			error_code,
 			#[cfg(test)]
@@ -1843,23 +1843,24 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 			}
 
 			if let Some(chan_closed) = conditions.expected_blamed_chan_closed {
-				match network_update {
-					Some(NetworkUpdate::ChannelUpdateMessage { ref msg }) if !chan_closed => {
-						if let Some(scid) = conditions.expected_blamed_scid {
-							assert_eq!(msg.contents.short_channel_id, scid);
-						}
-						const CHAN_DISABLED_FLAG: u8 = 2;
-						assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
-					},
-					Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) if chan_closed => {
-						if let Some(scid) = conditions.expected_blamed_scid {
-							assert_eq!(*short_channel_id, scid);
-						}
-						assert!(is_permanent);
-					},
-					Some(_) => panic!("Unexpected update type"),
-					None => panic!("Expected update"),
-				}
+				if let PathFailure::OnPath { network_update: Some(upd) } = failure {
+					match upd {
+						NetworkUpdate::ChannelUpdateMessage { ref msg } if !chan_closed => {
+							if let Some(scid) = conditions.expected_blamed_scid {
+								assert_eq!(msg.contents.short_channel_id, scid);
+							}
+							const CHAN_DISABLED_FLAG: u8 = 2;
+							assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
+						},
+						NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } if chan_closed => {
+							if let Some(scid) = conditions.expected_blamed_scid {
+								assert_eq!(*short_channel_id, scid);
+							}
+							assert!(is_permanent);
+						},
+						_ => panic!("Unexpected update type"),
+					}
+				} else { panic!("Expected network update"); }
 			}
 
 			payment_id.unwrap()
@@ -2240,10 +2241,9 @@ pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
 			if i == expected_paths.len() - 1 { assert_eq!(events.len(), 2); } else { assert_eq!(events.len(), 1); }
 
 			let expected_payment_id = match events[0] {
-				Event::PaymentPathFailed { payment_hash, payment_failed_permanently, all_paths_failed, ref path, ref payment_id, .. } => {
+				Event::PaymentPathFailed { payment_hash, payment_failed_permanently, ref path, ref payment_id, .. } => {
 					assert_eq!(payment_hash, our_payment_hash);
 					assert!(payment_failed_permanently);
-					assert_eq!(all_paths_failed, i == expected_paths.len() - 1);
 					for (idx, hop) in expected_route.iter().enumerate() {
 						assert_eq!(hop.node.get_our_node_id(), path[idx].pubkey);
 					}

lightning/src/ln/functional_tests.rs

@@ -31,7 +31,7 @@ use crate::ln::msgs;
 use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
 use crate::util::enforcing_trait_impls::EnforcingSigner;
 use crate::util::test_utils;
-use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination};
+use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination};
 use crate::util::errors::APIError;
 use crate::util::ser::{Writeable, ReadableArgs};
 use crate::util::config::UserConfig;
@@ -3235,12 +3235,12 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 			let events = nodes[0].node.get_and_clear_pending_events();
 			assert_eq!(events.len(), 6);
 			match events[0] {
-				Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
+				Event::PaymentPathFailed { ref payment_hash, ref failure, .. } => {
 					assert!(failed_htlcs.insert(payment_hash.0));
 					// If we delivered B's RAA we got an unknown preimage error, not something
 					// that we should update our routing table for.
 					if !deliver_bs_raa {
-						assert!(network_update.is_some());
+						if let PathFailure::OnPath { network_update: Some(_) } = failure { } else { panic!("Unexpected path failure") }
 					}
 				},
 				_ => panic!("Unexpected event"),
@@ -3252,9 +3252,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 				_ => panic!("Unexpected event"),
 			}
 			match events[2] {
-				Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
+				Event::PaymentPathFailed { ref payment_hash, failure: PathFailure::OnPath { network_update: Some(_) }, .. } => {
 					assert!(failed_htlcs.insert(payment_hash.0));
-					assert!(network_update.is_some());
 				},
 				_ => panic!("Unexpected event"),
 			}
@@ -3265,9 +3264,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 				_ => panic!("Unexpected event"),
 			}
 			match events[4] {
-				Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
+				Event::PaymentPathFailed { ref payment_hash, failure: PathFailure::OnPath { network_update: Some(_) }, .. } => {
 					assert!(failed_htlcs.insert(payment_hash.0));
-					assert!(network_update.is_some());
 				},
 				_ => panic!("Unexpected event"),
 			}
@@ -5148,14 +5146,14 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 	let mut as_failds = HashSet::new();
 	let mut as_updates = 0;
 	for event in as_events.iter() {
-		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event {
+		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref failure, .. } = event {
 			assert!(as_failds.insert(*payment_hash));
 			if *payment_hash != payment_hash_2 {
 				assert_eq!(*payment_failed_permanently, deliver_last_raa);
 			} else {
 				assert!(!payment_failed_permanently);
 			}
-			if network_update.is_some() {
+			if let PathFailure::OnPath { network_update: Some(_) } = failure {
 				as_updates += 1;
 			}
 		} else if let &Event::PaymentFailed { .. } = event {
@@ -5174,14 +5172,14 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 	let mut bs_failds = HashSet::new();
 	let mut bs_updates = 0;
 	for event in bs_events.iter() {
-		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event {
+		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref failure, .. } = event {
 			assert!(bs_failds.insert(*payment_hash));
 			if *payment_hash != payment_hash_1 && *payment_hash != payment_hash_5 {
 				assert_eq!(*payment_failed_permanently, deliver_last_raa);
 			} else {
 				assert!(!payment_failed_permanently);
 			}
-			if network_update.is_some() {
+			if let PathFailure::OnPath { network_update: Some(_) } = failure {
 				bs_updates += 1;
 			}
 		} else if let &Event::PaymentFailed { .. } = event {
@@ -5695,12 +5693,10 @@ fn test_fail_holding_cell_htlc_upon_free() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match &events[0] {
-		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, failure: PathFailure::OnPath { network_update: None }, ref short_channel_id, .. } => {
 			assert_eq!(PaymentId(our_payment_hash.0), *payment_id.as_ref().unwrap());
 			assert_eq!(our_payment_hash.clone(), *payment_hash);
 			assert_eq!(*payment_failed_permanently, false);
-			assert_eq!(*all_paths_failed, true);
-			assert_eq!(*network_update, None);
 			assert_eq!(*short_channel_id, Some(route.paths[0][0].short_channel_id));
 		},
 		_ => panic!("Unexpected event"),
@@ -5786,12 +5782,10 @@ fn test_free_and_fail_holding_cell_htlcs() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match &events[0] {
-		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, failure: PathFailure::OnPath { network_update: None }, ref short_channel_id, .. } => {
 			assert_eq!(payment_id_2, *payment_id.as_ref().unwrap());
 			assert_eq!(payment_hash_2.clone(), *payment_hash);
 			assert_eq!(*payment_failed_permanently, false);
-			assert_eq!(*all_paths_failed, true);
-			assert_eq!(*network_update, None);
 			assert_eq!(*short_channel_id, Some(route_2.paths[0][0].short_channel_id));
 		},
 		_ => panic!("Unexpected event"),
@@ -6689,8 +6683,7 @@ fn test_channel_failed_after_message_with_badonion_node_perm_bits_set() {
 	// Expect a PaymentPathFailed event with a ChannelFailure network update for the channel between
 	// the node originating the error to its next hop.
 	match events_5[0] {
-		Event::PaymentPathFailed { network_update:
-			Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }), error_code, ..
+		Event::PaymentPathFailed { error_code, failure: PathFailure::OnPath { network_update: Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) }, ..
 		} => {
 			assert_eq!(short_channel_id, chan_2.0.contents.short_channel_id);
 			assert!(is_permanent);

lightning/src/ln/onion_route_tests.rs

@@ -23,7 +23,7 @@ use crate::ln::features::{InitFeatures, InvoiceFeatures};
 use crate::ln::msgs;
 use crate::ln::msgs::{ChannelMessageHandler, ChannelUpdate};
 use crate::ln::wire::Encode;
-use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
+use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure};
 use crate::util::ser::{Writeable, Writer};
 use crate::util::test_utils;
 use crate::util::config::{UserConfig, ChannelConfig};
@@ -167,9 +167,8 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
-	if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, .. } = &events[0] {
+	if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref short_channel_id, ref error_code, failure: PathFailure::OnPath { ref network_update }, .. } = &events[0] {
 		assert_eq!(*payment_failed_permanently, !expected_retryable);
-		assert_eq!(*all_paths_failed, true);
 		assert_eq!(*error_code, expected_error_code);
 		if expected_channel_update.is_some() {
 			match network_update {

lightning/src/ln/onion_utils.rs

@@ -182,11 +182,11 @@ pub(super) fn build_onion_payloads(path: &Vec<RouteHop>, total_msat: u64, paymen
 		});
 		cur_value_msat += hop.fee_msat;
 		if cur_value_msat >= 21000000 * 100000000 * 1000 {
-			return Err(APIError::InvalidRoute{err: "Channel fees overflowed?"});
+			return Err(APIError::InvalidRoute{err: "Channel fees overflowed?".to_owned()});
 		}
 		cur_cltv += hop.cltv_expiry_delta as u32;
 		if cur_cltv >= 500000000 {
-			return Err(APIError::InvalidRoute{err: "Channel CLTV overflowed?"});
+			return Err(APIError::InvalidRoute{err: "Channel CLTV overflowed?".to_owned()});
 		}
 		last_short_channel_id = hop.short_channel_id;
 	}