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

`PaymentPathFailed`: add initial send error details

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 Event::PaymentPathFailed {
-		if let Some(network_update) = network_update {
+		failure: PathFailure::OnPath { network_update: Some(ref upd) }, .. } = event
-			network_graph.handle_network_update(&network_update);
+	{
-		}
+		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,
+			failure: PathFailure::OnPath { network_update: None },
-			all_paths_failed: true,
 			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,
+			failure: PathFailure::OnPath { network_update: None },
-			all_paths_failed: true,
 			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_delayed_payment_base_key = RequiredWrapper(None);
-			let mut counterparty_htlc_base_key = OptionDeserWrapper(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: _init_tlv_based_struct_field!(event, upgradable_required) }))
-			Ok(Some(Self { txid, transaction, height, block_hash, event: ev }))
-		} else {
-			Ok(None)
-		}
 	}
 }
 

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: _init_tlv_based_struct_field!(event, upgradable_required) }))
-			Ok(Some(Self { txid, height, block_hash, event: ev }))
-		} else {
-			Ok(None)
-		}
 	}
 }
 

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 {
+				if let PathFailure::OnPath { network_update: Some(upd) } = failure {
-					Some(NetworkUpdate::ChannelUpdateMessage { ref msg }) if !chan_closed => {
+					match upd {
-						if let Some(scid) = conditions.expected_blamed_scid {
+						NetworkUpdate::ChannelUpdateMessage { ref msg } if !chan_closed => {
-							assert_eq!(msg.contents.short_channel_id, scid);
+							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);
+							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 {
+						NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } if chan_closed => {
-							assert_eq!(*short_channel_id, scid);
+							if let Some(scid) = conditions.expected_blamed_scid {
-						}
+								assert_eq!(*short_channel_id, scid);
-						assert!(is_permanent);
+							}
-					},
+							assert!(is_permanent);
-					Some(_) => panic!("Unexpected update type"),
+						},
-					None => panic!("Expected update"),
+						_ => 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:
+		Event::PaymentPathFailed { error_code, failure: PathFailure::OnPath { network_update: Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) }, ..
-			Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }), error_code, ..
 		} => {
 			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;
 	}

lightning/src/ln/outbound_payment.rs

@@ -17,7 +17,6 @@ use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
 use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId};
 use crate::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA as LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA;
-use crate::ln::msgs::DecodeError;
 use crate::ln::onion_utils::HTLCFailReason;
 use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
 use crate::util::errors::APIError;
@@ -783,19 +782,18 @@ impl OutboundPayments {
 		debug_assert_eq!(paths.len(), path_results.len());
 		for (path, path_res) in paths.into_iter().zip(path_results) {
 			if let Err(e) = path_res {
-				let failed_scid = if let APIError::InvalidRoute { .. } = e {
+				if let APIError::MonitorUpdateInProgress = e { continue }
-					None
+				let mut failed_scid = None;
-				} else {
+				if let APIError::ChannelUnavailable { .. } = e {
 					let scid = path[0].short_channel_id;
+					failed_scid = Some(scid);
 					route_params.payment_params.previously_failed_channels.push(scid);
-					Some(scid)
+				}
-				};
 				events.push(events::Event::PaymentPathFailed {
 					payment_id: Some(payment_id),
 					payment_hash,
 					payment_failed_permanently: false,
-					network_update: None,
+					failure: events::PathFailure::InitialSend { err: e },
-					all_paths_failed: false,
 					path,
 					short_channel_id: failed_scid,
 					retry: None,
@@ -897,22 +895,22 @@ impl OutboundPayments {
 		   u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
 	{
 		if route.paths.len() < 1 {
-			return Err(PaymentSendFailure::ParameterError(APIError::InvalidRoute{err: "There must be at least one path to send over"}));
+			return Err(PaymentSendFailure::ParameterError(APIError::InvalidRoute{err: "There must be at least one path to send over".to_owned()}));
 		}
 		if payment_secret.is_none() && route.paths.len() > 1 {
-			return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError{err: "Payment secret is required for multi-path payments".to_string()}));
+			return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError{err: "Payment secret is required for multi-path payments".to_owned()}));
 		}
 		let mut total_value = 0;
 		let our_node_id = node_signer.get_node_id(Recipient::Node).unwrap(); // TODO no unwrap
 		let mut path_errs = Vec::with_capacity(route.paths.len());
 		'path_check: for path in route.paths.iter() {
 			if path.len() < 1 || path.len() > 20 {
-				path_errs.push(Err(APIError::InvalidRoute{err: "Path didn't go anywhere/had bogus size"}));
+				path_errs.push(Err(APIError::InvalidRoute{err: "Path didn't go anywhere/had bogus size".to_owned()}));
 				continue 'path_check;
 			}
 			for (idx, hop) in path.iter().enumerate() {
 				if idx != path.len() - 1 && hop.pubkey == our_node_id {
-					path_errs.push(Err(APIError::InvalidRoute{err: "Path went through us but wasn't a simple rebalance loop to us"}));
+					path_errs.push(Err(APIError::InvalidRoute{err: "Path went through us but wasn't a simple rebalance loop to us".to_owned()}));
 					continue 'path_check;
 				}
 			}
@@ -1158,7 +1156,6 @@ impl OutboundPayments {
 			awaiting_retry
 		});
 
-		let mut all_paths_failed = false;
 		let mut full_failure_ev = None;
 		let mut pending_retry_ev = false;
 		let mut retry = None;
@@ -1207,7 +1204,6 @@ impl OutboundPayments {
 				is_retryable_now = false;
 			}
 			if payment.get().remaining_parts() == 0 {
-				all_paths_failed = true;
 				if payment.get().abandoned() {
 					if !payment_is_probe {
 						full_failure_ev = Some(events::Event::PaymentFailed {
@@ -1259,8 +1255,7 @@ impl OutboundPayments {
 					payment_id: Some(*payment_id),
 					payment_hash: payment_hash.clone(),
 					payment_failed_permanently: !payment_retryable,
-					network_update,
+					failure: events::PathFailure::OnPath { network_update },
-					all_paths_failed,
 					path: path.clone(),
 					short_channel_id,
 					retry,
@@ -1357,12 +1352,14 @@ mod tests {
 
 	use crate::ln::PaymentHash;
 	use crate::ln::channelmanager::PaymentId;
+	use crate::ln::features::{ChannelFeatures, NodeFeatures};
 	use crate::ln::msgs::{ErrorAction, LightningError};
 	use crate::ln::outbound_payment::{OutboundPayments, Retry, RetryableSendFailure};
 	use crate::routing::gossip::NetworkGraph;
-	use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteParameters};
+	use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters};
 	use crate::sync::{Arc, Mutex};
-	use crate::util::events::Event;
+	use crate::util::errors::APIError;
+	use crate::util::events::{Event, PathFailure};
 	use crate::util::test_utils;
 
 	#[test]
@@ -1457,4 +1454,92 @@ mod tests {
 			} else { panic!("Unexpected error"); }
 		}
 	}
+
+	#[test]
+	fn initial_send_payment_path_failed_evs() {
+		let outbound_payments = OutboundPayments::new();
+		let logger = test_utils::TestLogger::new();
+		let genesis_hash = genesis_block(Network::Testnet).header.block_hash();
+		let network_graph = Arc::new(NetworkGraph::new(genesis_hash, &logger));
+		let scorer = Mutex::new(test_utils::TestScorer::new());
+		let router = test_utils::TestRouter::new(network_graph, &scorer);
+		let secp_ctx = Secp256k1::new();
+		let keys_manager = test_utils::TestKeysInterface::new(&[0; 32], Network::Testnet);
+
+		let sender_pk = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+		let receiver_pk = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[43; 32]).unwrap());
+		let payment_params = PaymentParameters::from_node_id(sender_pk, 0);
+		let route_params = RouteParameters {
+			payment_params: payment_params.clone(),
+			final_value_msat: 0,
+			final_cltv_expiry_delta: 0,
+		};
+		let failed_scid = 42;
+		let route = Route {
+			paths: vec![vec![RouteHop {
+				pubkey: receiver_pk,
+				node_features: NodeFeatures::empty(),
+				short_channel_id: failed_scid,
+				channel_features: ChannelFeatures::empty(),
+				fee_msat: 0,
+				cltv_expiry_delta: 0,
+			}]],
+			payment_params: Some(payment_params),
+		};
+		router.expect_find_route(route_params.clone(), Ok(route.clone()));
+		let mut route_params_w_failed_scid = route_params.clone();
+		route_params_w_failed_scid.payment_params.previously_failed_channels.push(failed_scid);
+		router.expect_find_route(route_params_w_failed_scid, Ok(route.clone()));
+		router.expect_find_route(route_params.clone(), Ok(route.clone()));
+		router.expect_find_route(route_params.clone(), Ok(route.clone()));
+
+		// Ensure that a ChannelUnavailable error will result in blaming an scid in the
+		// PaymentPathFailed event.
+		let pending_events = Mutex::new(Vec::new());
+		outbound_payments.send_payment(
+			PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), route_params.clone(),
+			&&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
+			&pending_events,
+			|_, _, _, _, _, _, _, _, _| Err(APIError::ChannelUnavailable { err: "test".to_owned() }))
+			.unwrap();
+		let mut events = pending_events.lock().unwrap();
+		assert_eq!(events.len(), 2);
+		if let Event::PaymentPathFailed {
+			short_channel_id,
+			failure: PathFailure::InitialSend { err: APIError::ChannelUnavailable { .. }}, .. } = events[0]
+		{
+			assert_eq!(short_channel_id, Some(failed_scid));
+		} else { panic!("Unexpected event"); }
+		if let Event::PaymentFailed { .. } = events[1] { } else { panic!("Unexpected event"); }
+		events.clear();
+		core::mem::drop(events);
+
+		// Ensure that a MonitorUpdateInProgress "error" will not result in a PaymentPathFailed event.
+		outbound_payments.send_payment(
+			PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), route_params.clone(),
+			&&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
+			&pending_events, |_, _, _, _, _, _, _, _, _| Err(APIError::MonitorUpdateInProgress))
+			.unwrap();
+		{
+			let events = pending_events.lock().unwrap();
+			assert_eq!(events.len(), 0);
+		}
+
+		// Ensure that any other error will result in a PaymentPathFailed event but no blamed scid.
+		outbound_payments.send_payment(
+			PaymentHash([0; 32]), &None, PaymentId([1; 32]), Retry::Attempts(0), route_params.clone(),
+			&&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
+			&pending_events,
+			|_, _, _, _, _, _, _, _, _| Err(APIError::APIMisuseError { err: "test".to_owned() }))
+			.unwrap();
+		let events = pending_events.lock().unwrap();
+		assert_eq!(events.len(), 2);
+		if let Event::PaymentPathFailed {
+			short_channel_id,
+			failure: PathFailure::InitialSend { err: APIError::APIMisuseError { .. }}, .. } = events[0]
+		{
+			assert_eq!(short_channel_id, None);
+		} else { panic!("Unexpected event"); }
+		if let Event::PaymentFailed { .. } = events[1] { } else { panic!("Unexpected event"); }
+	}
 }

lightning/src/ln/payment_tests.rs

@@ -24,7 +24,7 @@ use crate::ln::outbound_payment::Retry;
 use crate::routing::gossip::{EffectiveCapacity, RoutingFees};
 use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RouteParameters};
 use crate::routing::scoring::ChannelUsage;
-use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
+use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure};
 use crate::util::test_utils;
 use crate::util::errors::APIError;
 use crate::util::ser::Writeable;
@@ -2139,9 +2139,12 @@ fn retry_multi_path_single_failed_payment() {
 	assert_eq!(events.len(), 1);
 	match events[0] {
 		Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
-			network_update: None, all_paths_failed: false, short_channel_id: Some(expected_scid), .. } => {
+			failure: PathFailure::InitialSend { err: APIError::ChannelUnavailable { err: ref err_msg }},
+			short_channel_id: Some(expected_scid), .. } =>
+		{
 			assert_eq!(payment_hash, ev_payment_hash);
 			assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
+			assert!(err_msg.contains("max HTLC"));
 		},
 		_ => panic!("Unexpected event"),
 	}
@@ -2212,9 +2215,12 @@ fn immediate_retry_on_failure() {
 	assert_eq!(events.len(), 1);
 	match events[0] {
 		Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
-		network_update: None, all_paths_failed: false, short_channel_id: Some(expected_scid), .. } => {
+			failure: PathFailure::InitialSend { err: APIError::ChannelUnavailable { err: ref err_msg }},
+			short_channel_id: Some(expected_scid), .. } =>
+		{
 			assert_eq!(payment_hash, ev_payment_hash);
 			assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
+			assert!(err_msg.contains("max HTLC"));
 		},
 		_ => panic!("Unexpected event"),
 	}
@@ -2226,7 +2232,8 @@ fn immediate_retry_on_failure() {
 #[test]
 fn no_extra_retries_on_back_to_back_fail() {
 	// In a previous release, we had a race where we may exceed the payment retry count if we
-	// get two failures in a row with the second having `all_paths_failed` set.
+	// get two failures in a row with the second indicating that all paths had failed (this field,
+	// `all_paths_failed`, has since been removed).
 	// Generally, when we give up trying to retry a payment, we don't know for sure what the
 	// current state of the ChannelManager event queue is. Specifically, we cannot be sure that
 	// there are not multiple additional `PaymentPathFailed` or even `PaymentSent` events

lightning/src/routing/gossip.rs

@@ -886,9 +886,9 @@ impl Readable for ChannelInfo {
 			(0, features, required),
 			(1, announcement_received_time, (default_value, 0)),
 			(2, node_one, required),
-			(4, one_to_two_wrap, ignorable),
+			(4, one_to_two_wrap, upgradable_option),
 			(6, node_two, required),
-			(8, two_to_one_wrap, ignorable),
+			(8, two_to_one_wrap, upgradable_option),
 			(10, capacity_sats, required),
 			(12, announcement_message, required),
 		});
@@ -1164,7 +1164,7 @@ impl Readable for NodeInfo {
 
 		read_tlv_fields!(reader, {
 			(0, _lowest_inbound_channel_fees, option),
-			(2, announcement_info_wrap, ignorable),
+			(2, announcement_info_wrap, upgradable_option),
 			(4, channels, vec_type),
 		});
 

lightning/src/util/errors.rs

@@ -37,7 +37,7 @@ pub enum APIError {
 	/// too-many-hops, etc).
 	InvalidRoute {
 		/// A human-readable error message
-		err: &'static str
+		err: String
 	},
 	/// We were unable to complete the request as the Channel required to do so is unable to
 	/// complete the request (or was not found). This can take many forms, including disconnected
@@ -84,6 +84,18 @@ impl fmt::Debug for APIError {
 	}
 }
 
+impl_writeable_tlv_based_enum_upgradable!(APIError,
+	(0, APIMisuseError) => { (0, err, required), },
+	(2, FeeRateTooHigh) => {
+		(0, err, required),
+		(2, feerate, required),
+	},
+	(4, InvalidRoute) => { (0, err, required), },
+	(6, ChannelUnavailable) => { (0, err, required), },
+	(8, MonitorUpdateInProgress) => {},
+	(10, IncompatibleShutdownScript) => { (0, script, required), },
+);
+
 #[inline]
 pub(crate) fn get_onion_debug_field(error_code: u16) -> (&'static str, usize) {
 	match error_code & 0xff {

lightning/src/util/events.rs

@@ -21,10 +21,10 @@ use crate::ln::channelmanager::{InterceptId, PaymentId};
 use crate::ln::channel::FUNDING_CONF_DEADLINE_BLOCKS;
 use crate::ln::features::ChannelTypeFeatures;
 use crate::ln::msgs;
-use crate::ln::msgs::DecodeError;
 use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 use crate::routing::gossip::NetworkUpdate;
-use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, WithoutLength, OptionDeserWrapper};
+use crate::util::errors::APIError;
+use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength};
 use crate::routing::router::{RouteHop, RouteParameters};
 
 use bitcoin::{PackedLockTime, Transaction};
@@ -82,6 +82,39 @@ impl_writeable_tlv_based_enum!(PaymentPurpose,
 	(2, SpontaneousPayment)
 );
 
+/// When the payment path failure took place and extra details about it. [`PathFailure::OnPath`] may
+/// contain a [`NetworkUpdate`] that needs to be applied to the [`NetworkGraph`].
+///
+/// [`NetworkUpdate`]: crate::routing::gossip::NetworkUpdate
+/// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum PathFailure {
+	/// We failed to initially send the payment and no HTLC was committed to. Contains the relevant
+	/// error.
+	InitialSend {
+		/// The error surfaced from initial send.
+		err: APIError,
+	},
+	/// A hop on the path failed to forward our payment.
+	OnPath {
+		/// If present, this [`NetworkUpdate`] should be applied to the [`NetworkGraph`] so that routing
+		/// decisions can take into account the update.
+		///
+		/// [`NetworkUpdate`]: crate::routing::gossip::NetworkUpdate
+		/// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph
+		network_update: Option<NetworkUpdate>,
+	},
+}
+
+impl_writeable_tlv_based_enum_upgradable!(PathFailure,
+	(0, OnPath) => {
+		(0, network_update, upgradable_option),
+	},
+	(2, InitialSend) => {
+		(0, err, upgradable_required),
+	},
+);
+
 #[derive(Clone, Debug, PartialEq, Eq)]
 /// The reason the channel was closed. See individual variants more details.
 pub enum ClosureReason {
@@ -589,7 +622,7 @@ pub enum Event {
 		fee_paid_msat: Option<u64>,
 	},
 	/// Indicates an outbound payment failed. Individual [`Event::PaymentPathFailed`] events
-	/// provide failure information for each MPP part in the payment.
+	/// provide failure information for each path attempt in the payment, including retries.
 	///
 	/// This event is provided once there are no further pending HTLCs for the payment and the
 	/// payment is no longer retryable, due either to the [`Retry`] provided or
@@ -631,13 +664,12 @@ pub enum Event {
 	/// handle the HTLC.
 	///
 	/// Note that this does *not* indicate that all paths for an MPP payment have failed, see
-	/// [`Event::PaymentFailed`] and [`all_paths_failed`].
+	/// [`Event::PaymentFailed`].
 	///
 	/// See [`ChannelManager::abandon_payment`] for giving up on this payment before its retries have
 	/// been exhausted.
 	///
 	/// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment
-	/// [`all_paths_failed`]: Self::PaymentPathFailed::all_paths_failed
 	PaymentPathFailed {
 		/// The id returned by [`ChannelManager::send_payment`] and used with
 		/// [`ChannelManager::abandon_payment`].
@@ -653,18 +685,11 @@ pub enum Event {
 		/// the payment has failed, not just the route in question. If this is not set, the payment may
 		/// be retried via a different route.
 		payment_failed_permanently: bool,
-		/// Any failure information conveyed via the Onion return packet by a node along the failed
+		/// Extra error details based on the failure type. May contain an update that needs to be
-		/// payment route.
+		/// applied to the [`NetworkGraph`].
-		///
-		/// Should be applied to the [`NetworkGraph`] so that routing decisions can take into
-		/// account the update.
 		///
 		/// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph
-		network_update: Option<NetworkUpdate>,
+		failure: PathFailure,
-		/// For both single-path and multi-path payments, this is set if all paths of the payment have
-		/// failed. This will be set to false if (1) this is an MPP payment and (2) other parts of the
-		/// larger MPP payment were still in flight when this event was generated.
-		all_paths_failed: bool,
 		/// The payment path that failed.
 		path: Vec<RouteHop>,
 		/// The channel responsible for the failed payment path.
@@ -966,8 +991,8 @@ impl Writeable for Event {
 				});
 			},
 			&Event::PaymentPathFailed {
-				ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update,
+				ref payment_id, ref payment_hash, ref payment_failed_permanently, ref failure,
-				ref all_paths_failed, ref path, ref short_channel_id, ref retry,
+				ref path, ref short_channel_id, ref retry,
 				#[cfg(test)]
 				ref error_code,
 				#[cfg(test)]
@@ -980,13 +1005,14 @@ impl Writeable for Event {
 				error_data.write(writer)?;
 				write_tlv_fields!(writer, {
 					(0, payment_hash, required),
-					(1, network_update, option),
+					(1, None::<NetworkUpdate>, option), // network_update in LDK versions prior to 0.0.114
 					(2, payment_failed_permanently, required),
-					(3, all_paths_failed, required),
+					(3, false, required), // all_paths_failed in LDK versions prior to 0.0.114
 					(5, *path, vec_type),
 					(7, short_channel_id, option),
 					(9, retry, option),
 					(11, payment_id, option),
+					(13, failure, required),
 				});
 			},
 			&Event::PendingHTLCsForwardable { time_forwardable: _ } => {
@@ -1199,27 +1225,27 @@ impl MaybeReadable for Event {
 					let mut payment_hash = PaymentHash([0; 32]);
 					let mut payment_failed_permanently = false;
 					let mut network_update = None;
-					let mut all_paths_failed = Some(true);
 					let mut path: Option<Vec<RouteHop>> = Some(vec![]);
 					let mut short_channel_id = None;
 					let mut retry = None;
 					let mut payment_id = None;
+					let mut failure_opt = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
-						(1, network_update, ignorable),
+						(1, network_update, upgradable_option),
 						(2, payment_failed_permanently, required),
-						(3, all_paths_failed, option),
 						(5, path, vec_type),
 						(7, short_channel_id, option),
 						(9, retry, option),
 						(11, payment_id, option),
+						(13, failure_opt, upgradable_option),
 					});
+					let failure = failure_opt.unwrap_or_else(|| PathFailure::OnPath { network_update });
 					Ok(Some(Event::PaymentPathFailed {
 						payment_id,
 						payment_hash,
 						payment_failed_permanently,
-						network_update,
+						failure,
-						all_paths_failed: all_paths_failed.unwrap(),
 						path: path.unwrap(),
 						short_channel_id,
 						retry,
@@ -1285,16 +1311,15 @@ impl MaybeReadable for Event {
 			9u8 => {
 				let f = || {
 					let mut channel_id = [0; 32];
-					let mut reason = None;
+					let mut reason = UpgradableRequired(None);
 					let mut user_channel_id_low_opt: Option<u64> = None;
 					let mut user_channel_id_high_opt: Option<u64> = None;
 					read_tlv_fields!(reader, {
 						(0, channel_id, required),
 						(1, user_channel_id_low_opt, option),
-						(2, reason, ignorable),
+						(2, reason, upgradable_required),
 						(3, user_channel_id_high_opt, option),
 					});
-					if reason.is_none() { return Ok(None); }
 
 					// `user_channel_id` used to be a single u64 value. In order to remain
 					// backwards compatible with versions prior to 0.0.113, the u128 is serialized
@@ -1302,7 +1327,7 @@ impl MaybeReadable for Event {
 					let user_channel_id = (user_channel_id_low_opt.unwrap_or(0) as u128) +
 						((user_channel_id_high_opt.unwrap_or(0) as u128) << 64);
 
-					Ok(Some(Event::ChannelClosed { channel_id, user_channel_id, reason: reason.unwrap() }))
+					Ok(Some(Event::ChannelClosed { channel_id, user_channel_id, reason: _init_tlv_based_struct_field!(reason, upgradable_required) }))
 				};
 				f()
 			},
@@ -1358,20 +1383,19 @@ impl MaybeReadable for Event {
 			19u8 => {
 				let f = || {
 					let mut payment_hash = PaymentHash([0; 32]);
-					let mut purpose = None;
+					let mut purpose = UpgradableRequired(None);
 					let mut amount_msat = 0;
 					let mut receiver_node_id = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
 						(1, receiver_node_id, option),
-						(2, purpose, ignorable),
+						(2, purpose, upgradable_required),
 						(4, amount_msat, required),
 					});
-					if purpose.is_none() { return Ok(None); }
 					Ok(Some(Event::PaymentClaimed {
 						receiver_node_id,
 						payment_hash,
-						purpose: purpose.unwrap(),
+						purpose: _init_tlv_based_struct_field!(purpose, upgradable_required),
 						amount_msat,
 					}))
 				};
@@ -1419,22 +1443,15 @@ impl MaybeReadable for Event {
 			25u8 => {
 				let f = || {
 					let mut prev_channel_id = [0; 32];
-					let mut failed_next_destination_opt = None;
+					let mut failed_next_destination_opt = UpgradableRequired(None);
 					read_tlv_fields!(reader, {
 						(0, prev_channel_id, required),
-						(2, failed_next_destination_opt, ignorable),
+						(2, failed_next_destination_opt, upgradable_required),
 					});
-					if let Some(failed_next_destination) = failed_next_destination_opt {
+					Ok(Some(Event::HTLCHandlingFailed {
-						Ok(Some(Event::HTLCHandlingFailed {
+						prev_channel_id,
-							prev_channel_id,
+						failed_next_destination: _init_tlv_based_struct_field!(failed_next_destination_opt, upgradable_required),
-							failed_next_destination,
+					}))
-						}))
-					} else {
-						// If we fail to read a `failed_next_destination` assume it's because
-						// `MaybeReadable::read` returned `Ok(None)`, though it's also possible we
-						// were simply missing the field.
-						Ok(None)
-					}
 				};
 				f()
 			},
@@ -1443,8 +1460,8 @@ impl MaybeReadable for Event {
 				let f = || {
 					let mut channel_id = [0; 32];
 					let mut user_channel_id: u128 = 0;
-					let mut counterparty_node_id = OptionDeserWrapper(None);
+					let mut counterparty_node_id = RequiredWrapper(None);
-					let mut channel_type = OptionDeserWrapper(None);
+					let mut channel_type = RequiredWrapper(None);
 					read_tlv_fields!(reader, {
 						(0, channel_id, required),
 						(2, user_channel_id, required),

lightning/src/util/ser.rs

@@ -289,18 +289,30 @@ impl<T: Readable> MaybeReadable for T {
 }
 
 /// Wrapper to read a required (non-optional) TLV record.
-pub struct OptionDeserWrapper<T: Readable>(pub Option<T>);
+pub struct RequiredWrapper<T: Readable>(pub Option<T>);
-impl<T: Readable> Readable for OptionDeserWrapper<T> {
+impl<T: Readable> Readable for RequiredWrapper<T> {
 	#[inline]
 	fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
 		Ok(Self(Some(Readable::read(reader)?)))
 	}
 }
 /// When handling `default_values`, we want to map the default-value T directly
-/// to a `OptionDeserWrapper<T>` in a way that works for `field: T = t;` as
+/// to a `RequiredWrapper<T>` in a way that works for `field: T = t;` as
 /// well. Thus, we assume `Into<T> for T` does nothing and use that.
-impl<T: Readable> From<T> for OptionDeserWrapper<T> {
+impl<T: Readable> From<T> for RequiredWrapper<T> {
-	fn from(t: T) -> OptionDeserWrapper<T> { OptionDeserWrapper(Some(t)) }
+	fn from(t: T) -> RequiredWrapper<T> { RequiredWrapper(Some(t)) }
+}
+
+/// Wrapper to read a required (non-optional) TLV record that may have been upgraded without
+/// backwards compat.
+pub struct UpgradableRequired<T: MaybeReadable>(pub Option<T>);
+impl<T: MaybeReadable> MaybeReadable for UpgradableRequired<T> {
+	#[inline]
+	fn read<R: Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> {
+		let tlv = MaybeReadable::read(reader)?;
+		if let Some(tlv) = tlv { return Ok(Some(Self(Some(tlv)))) }
+		Ok(None)
+	}
 }
 
 pub(crate) struct U48(pub u64);

lightning/src/util/ser_macros.rs

@@ -39,9 +39,12 @@ macro_rules! _encode_tlv {
 			field.write($stream)?;
 		}
 	};
-	($stream: expr, $type: expr, $field: expr, ignorable) => {
+	($stream: expr, $type: expr, $field: expr, upgradable_required) => {
 		$crate::_encode_tlv!($stream, $type, $field, required);
 	};
+	($stream: expr, $type: expr, $field: expr, upgradable_option) => {
+		$crate::_encode_tlv!($stream, $type, $field, option);
+	};
 	($stream: expr, $type: expr, $field: expr, (option, encoding: ($fieldty: ty, $encoding: ident))) => {
 		$crate::_encode_tlv!($stream, $type, $field.map(|f| $encoding(f)), option);
 	};
@@ -158,9 +161,12 @@ macro_rules! _get_varint_length_prefixed_tlv_length {
 			$len.0 += field_len;
 		}
 	};
-	($len: expr, $type: expr, $field: expr, ignorable) => {
+	($len: expr, $type: expr, $field: expr, upgradable_required) => {
 		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, required);
 	};
+	($len: expr, $type: expr, $field: expr, upgradable_option) => {
+		$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, option);
+	};
 }
 
 /// See the documentation of [`write_tlv_fields`].
@@ -210,7 +216,10 @@ macro_rules! _check_decoded_tlv_order {
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, vec_type) => {{
 		// no-op
 	}};
-	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, ignorable) => {{
+	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_required) => {{
+		_check_decoded_tlv_order!($last_seen_type, $typ, $type, $field, required)
+	}};
+	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_option) => {{
 		// no-op
 	}};
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
@@ -249,7 +258,10 @@ macro_rules! _check_missing_tlv {
 	($last_seen_type: expr, $type: expr, $field: ident, option) => {{
 		// no-op
 	}};
-	($last_seen_type: expr, $type: expr, $field: ident, ignorable) => {{
+	($last_seen_type: expr, $type: expr, $field: ident, upgradable_required) => {{
+		_check_missing_tlv!($last_seen_type, $type, $field, required)
+	}};
+	($last_seen_type: expr, $type: expr, $field: ident, upgradable_option) => {{
 		// no-op
 	}};
 	($last_seen_type: expr, $type: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
@@ -280,7 +292,20 @@ macro_rules! _decode_tlv {
 	($reader: expr, $field: ident, option) => {{
 		$field = Some($crate::util::ser::Readable::read(&mut $reader)?);
 	}};
-	($reader: expr, $field: ident, ignorable) => {{
+	// `upgradable_required` indicates we're reading a required TLV that may have been upgraded
+	// without backwards compat. We'll error if the field is missing, and return `Ok(None)` if the
+	// field is present but we can no longer understand it.
+	// Note that this variant can only be used within a `MaybeReadable` read.
+	($reader: expr, $field: ident, upgradable_required) => {{
+		$field = match $crate::util::ser::MaybeReadable::read(&mut $reader)? {
+			Some(res) => res,
+			_ => return Ok(None)
+		};
+	}};
+	// `upgradable_option` indicates we're reading an Option-al TLV that may have been upgraded
+	// without backwards compat. $field will be None if the TLV is missing or if the field is present
+	// but we can no longer understand it.
+	($reader: expr, $field: ident, upgradable_option) => {{
 		$field = $crate::util::ser::MaybeReadable::read(&mut $reader)?;
 	}};
 	($reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
@@ -619,8 +644,11 @@ macro_rules! _init_tlv_based_struct_field {
 	($field: ident, option) => {
 		$field
 	};
-	($field: ident, ignorable) => {
+	($field: ident, upgradable_required) => {
-		if $field.is_none() { return Ok(None); } else { $field.unwrap() }
+		$field.0.unwrap()
+	};
+	($field: ident, upgradable_option) => {
+		$field
 	};
 	($field: ident, required) => {
 		$field.0.unwrap()
@@ -637,13 +665,13 @@ macro_rules! _init_tlv_based_struct_field {
 #[macro_export]
 macro_rules! _init_tlv_field_var {
 	($field: ident, (default_value, $default: expr)) => {
-		let mut $field = $crate::util::ser::OptionDeserWrapper(None);
+		let mut $field = $crate::util::ser::RequiredWrapper(None);
 	};
 	($field: ident, (static_value, $value: expr)) => {
 		let $field;
 	};
 	($field: ident, required) => {
-		let mut $field = $crate::util::ser::OptionDeserWrapper(None);
+		let mut $field = $crate::util::ser::RequiredWrapper(None);
 	};
 	($field: ident, vec_type) => {
 		let mut $field = Some(Vec::new());
@@ -651,7 +679,10 @@ macro_rules! _init_tlv_field_var {
 	($field: ident, option) => {
 		let mut $field = None;
 	};
-	($field: ident, ignorable) => {
+	($field: ident, upgradable_required) => {
+		let mut $field = $crate::util::ser::UpgradableRequired(None);
+	};
+	($field: ident, upgradable_option) => {
 		let mut $field = None;
 	};
 }
@@ -948,7 +979,7 @@ macro_rules! impl_writeable_tlv_based_enum_upgradable {
 						Ok(Some($st::$tuple_variant_name(Readable::read(reader)?)))
 					}),*)*
 					_ if id % 2 == 1 => Ok(None),
-					_ => Err(DecodeError::UnknownRequiredFeature),
+					_ => Err($crate::ln::msgs::DecodeError::UnknownRequiredFeature),
 				}
 			}
 		}
@@ -1028,6 +1059,47 @@ mod tests {
 			(0xdeadbeef1badbeef, 0x1bad1dea, Some(0x01020304)));
 	}
 
+	#[derive(Debug, PartialEq)]
+	struct TestUpgradable {
+		a: u32,
+		b: u32,
+		c: Option<u32>,
+	}
+
+	fn upgradable_tlv_reader(s: &[u8]) -> Result<Option<TestUpgradable>, DecodeError> {
+		let mut s = Cursor::new(s);
+		let mut a = 0;
+		let mut b = 0;
+		let mut c: Option<u32> = None;
+		decode_tlv_stream!(&mut s, {(2, a, upgradable_required), (3, b, upgradable_required), (4, c, upgradable_option)});
+		Ok(Some(TestUpgradable { a, b, c, }))
+	}
+
+	#[test]
+	fn upgradable_tlv_simple_good_cases() {
+		assert_eq!(upgradable_tlv_reader(&::hex::decode(
+			concat!("0204deadbeef", "03041bad1dea", "0404deadbeef")
+		).unwrap()[..]).unwrap(),
+		Some(TestUpgradable { a: 0xdeadbeef, b: 0x1bad1dea, c: Some(0xdeadbeef) }));
+
+		assert_eq!(upgradable_tlv_reader(&::hex::decode(
+			concat!("0204deadbeef", "03041bad1dea")
+		).unwrap()[..]).unwrap(),
+		Some(TestUpgradable { a: 0xdeadbeef, b: 0x1bad1dea, c: None}));
+	}
+
+	#[test]
+	fn missing_required_upgradable() {
+		if let Err(DecodeError::InvalidValue) = upgradable_tlv_reader(&::hex::decode(
+			concat!("0100", "0204deadbeef")
+			).unwrap()[..]) {
+		} else { panic!(); }
+		if let Err(DecodeError::InvalidValue) = upgradable_tlv_reader(&::hex::decode(
+			concat!("0100", "03041bad1dea")
+		).unwrap()[..]) {
+		} else { panic!(); }
+	}
+
 	// BOLT TLV test cases
 	fn tlv_reader_n1(s: &[u8]) -> Result<(Option<HighZeroBytesDroppedBigSize<u64>>, Option<u64>, Option<(PublicKey, u64, u64)>, Option<u16>), DecodeError> {
 		let mut s = Cursor::new(s);

pending_changelog/2043.txt

@@ -0,0 +1,16 @@
+## API Updates
+- `Event::PaymentPathFailed::network_update` has been replaced by a new `Failure` enum, which may
+	contain the `network_update` within it. See `Event::PaymentPathFailed::failure` and `Failure` docs
+	for more
+- `Event::PaymentPathFailed::all_paths_failed` has been removed, as we've dropped support for manual
+	payment retries.
+
+## Backwards Compatibility
+- If downgrading from 0.0.114 to a previous version, `Event::PaymentPathFailed::network_update` will
+	always be `None`.
+- If downgrading from 0.0.114 to a previous version, `Event::PaymentPathFailed::all_paths_failed`
+	will always be set to `false`. Users who wish to support downgrading and currently rely on the
+	field should should first migrate to always calling `ChannelManager::abandon_payment` and awaiting
+	`PaymentFailed` events before retrying (see the field docs for more info on this approach:
+	<https://docs.rs/lightning/0.0.113/lightning/util/events/enum.Event.html#variant.PaymentPathFailed.field.all_paths_failed>),
+	and then migrate to 0.0.114.