From f361aa62a42551be0de722ba4c886c1ebdcb13cd Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sun, 19 Feb 2023 18:32:06 -0500 Subject: [PATCH 01/10] Add missing import path in ser macro --- lightning/src/ln/outbound_payment.rs | 1 - lightning/src/util/events.rs | 1 - lightning/src/util/ser_macros.rs | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 1d8d46a916a..a851c8eda12 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/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; diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 4d68d01f6ed..98239fd487a 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -21,7 +21,6 @@ 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}; diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 0aefaf38306..d509995b04f 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -949,7 +949,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), } } } From 2037a241f4aa5ff976bfd556bc61c12b73890b3e Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 11 Feb 2023 19:38:48 -0500 Subject: [PATCH 02/10] Remove all_paths_failed from PaymentPathFailed This field was previous useful in manual retries for users to know when all paths of a payment have failed and it is safe to retry. Now that we support automatic retries in ChannelManager and no longer support manual retries, the field is no longer useful. For backwards compat, we now always write false for this field. If we didn't do this, previous versions would default this field's value to true, which can be problematic because some clients have relied on the field to indicate when a full payment retry is safe. --- lightning-background-processor/src/lib.rs | 2 -- lightning/src/ln/functional_test_utils.rs | 3 +-- lightning/src/ln/functional_tests.rs | 6 ++---- lightning/src/ln/onion_route_tests.rs | 3 +-- lightning/src/ln/outbound_payment.rs | 4 ---- lightning/src/ln/payment_tests.rs | 7 ++++--- lightning/src/util/events.rs | 14 +++----------- pending_changelog/2043.txt | 11 +++++++++++ 8 files changed, 22 insertions(+), 28 deletions(-) create mode 100644 pending_changelog/2043.txt diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index 97d0462eb50..3b692ac07e2 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -1366,7 +1366,6 @@ mod tests { payment_hash: PaymentHash([42; 32]), payment_failed_permanently: false, network_update: None, - all_paths_failed: true, path: path.clone(), short_channel_id: Some(scored_scid), retry: None, @@ -1387,7 +1386,6 @@ mod tests { payment_hash: PaymentHash([42; 32]), payment_failed_permanently: true, network_update: None, - all_paths_failed: true, path: path.clone(), short_channel_id: None, retry: None, diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index fd867bc6ab7..2786d754abb 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2240,10 +2240,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); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index d6d2972d073..32c3b51688a 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5695,11 +5695,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, ref network_update, 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)); }, @@ -5786,11 +5785,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, ref network_update, 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)); }, diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index da5aadb8306..bc6dd1168bf 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -167,9 +167,8 @@ fn run_onion_failure_test_with_fail_intercept(_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 network_update, ref short_channel_id, ref error_code, .. } = &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 { diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index a851c8eda12..16037c7f656 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -794,7 +794,6 @@ impl OutboundPayments { payment_hash, payment_failed_permanently: false, network_update: None, - all_paths_failed: false, path, short_channel_id: failed_scid, retry: None, @@ -1157,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; @@ -1206,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,7 +1256,6 @@ impl OutboundPayments { payment_hash: payment_hash.clone(), payment_failed_permanently: !payment_retryable, network_update, - all_paths_failed, path: path.clone(), short_channel_id, retry, diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 86648e5fb72..1d30bceff96 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2139,7 +2139,7 @@ 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), .. } => { + network_update: None, short_channel_id: Some(expected_scid), .. } => { assert_eq!(payment_hash, ev_payment_hash); assert_eq!(expected_scid, route.paths[1][0].short_channel_id); }, @@ -2212,7 +2212,7 @@ 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), .. } => { + network_update: None, short_channel_id: Some(expected_scid), .. } => { assert_eq!(payment_hash, ev_payment_hash); assert_eq!(expected_scid, route.paths[1][0].short_channel_id); }, @@ -2226,7 +2226,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 diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 98239fd487a..80ecc228429 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -630,13 +630,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`]. @@ -660,10 +659,6 @@ pub enum Event { /// /// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph network_update: Option, - /// 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, /// The channel responsible for the failed payment path. @@ -966,7 +961,7 @@ impl Writeable for Event { }, &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, - 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)] @@ -981,7 +976,7 @@ impl Writeable for Event { (0, payment_hash, required), (1, network_update, option), (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), @@ -1198,7 +1193,6 @@ 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> = Some(vec![]); let mut short_channel_id = None; let mut retry = None; @@ -1207,7 +1201,6 @@ impl MaybeReadable for Event { (0, payment_hash, required), (1, network_update, ignorable), (2, payment_failed_permanently, required), - (3, all_paths_failed, option), (5, path, vec_type), (7, short_channel_id, option), (9, retry, option), @@ -1218,7 +1211,6 @@ impl MaybeReadable for Event { payment_hash, payment_failed_permanently, network_update, - all_paths_failed: all_paths_failed.unwrap(), path: path.unwrap(), short_channel_id, retry, diff --git a/pending_changelog/2043.txt b/pending_changelog/2043.txt new file mode 100644 index 00000000000..2fc7c750ce7 --- /dev/null +++ b/pending_changelog/2043.txt @@ -0,0 +1,11 @@ +## API Updates +- `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::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: + ), + and then migrate to 0.0.114. From 52551a9fc847972acbe83500b3a4886af3215b7b Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 13 Feb 2023 16:49:22 -0500 Subject: [PATCH 03/10] Support deserializing an Option-al MaybeReadable Prior to this change, our impl_writeable_tlv_based macros only supported deserializing a MaybeReadable if it's non-Optional. --- lightning/src/util/ser_macros.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index d509995b04f..c7dcc870178 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -42,6 +42,9 @@ macro_rules! _encode_tlv { ($stream: expr, $type: expr, $field: expr, ignorable) => { $crate::_encode_tlv!($stream, $type, $field, required); }; + ($stream: expr, $type: expr, $field: expr, ignorable_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); }; @@ -161,6 +164,9 @@ macro_rules! _get_varint_length_prefixed_tlv_length { ($len: expr, $type: expr, $field: expr, ignorable) => { $crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, required); }; + ($len: expr, $type: expr, $field: expr, ignorable_option) => { + $crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, option); + }; } /// See the documentation of [`write_tlv_fields`]. @@ -213,6 +219,9 @@ macro_rules! _check_decoded_tlv_order { ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, ignorable) => {{ // no-op }}; + ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, ignorable_option) => {{ + // no-op + }}; ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{ // no-op }}; @@ -252,6 +261,9 @@ macro_rules! _check_missing_tlv { ($last_seen_type: expr, $type: expr, $field: ident, ignorable) => {{ // no-op }}; + ($last_seen_type: expr, $type: expr, $field: ident, ignorable_option) => {{ + // no-op + }}; ($last_seen_type: expr, $type: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{ // no-op }}; @@ -283,6 +295,9 @@ macro_rules! _decode_tlv { ($reader: expr, $field: ident, ignorable) => {{ $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; }}; + ($reader: expr, $field: ident, ignorable_option) => {{ + $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; + }}; ($reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{ $field = Some($trait::read(&mut $reader $(, $read_arg)*)?); }}; @@ -623,6 +638,9 @@ macro_rules! _init_tlv_based_struct_field { ($field: ident, ignorable) => { if $field.is_none() { return Ok(None); } else { $field.unwrap() } }; + ($field: ident, ignorable_option) => { + $field + }; ($field: ident, required) => { $field.0.unwrap() }; @@ -655,6 +673,9 @@ macro_rules! _init_tlv_field_var { ($field: ident, ignorable) => { let mut $field = None; }; + ($field: ident, ignorable_option) => { + let mut $field = None; + }; } /// Equivalent to running [`_init_tlv_field_var`] then [`read_tlv_fields`]. From 0e88538b783a753a93f17419f5fabbe645fe1fba Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 20 Feb 2023 12:42:45 -0500 Subject: [PATCH 04/10] Disambiguate ignorable and ignorable_option Would rather not rename ignorable to ignorable_required, so rename both of them to upgradable_* --- lightning/src/chain/channelmonitor.rs | 2 +- lightning/src/chain/onchaintx.rs | 2 +- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/routing/gossip.rs | 6 +++--- lightning/src/util/events.rs | 8 ++++---- lightning/src/util/ser_macros.rs | 28 +++++++++++++-------------- 6 files changed, 24 insertions(+), 24 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 509ebed6fba..ea24f8cf61d 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -460,7 +460,7 @@ impl MaybeReadable for OnchainEventEntry { (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 })) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 039fb5ff13a..23140068a34 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -111,7 +111,7 @@ impl MaybeReadable for OnchainEventEntry { (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 })) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 577e0984448..f659d63182b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/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. diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index cf51b6ab528..957be2951fc 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -883,9 +883,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_required), (6, node_two, required), - (8, two_to_one_wrap, ignorable), + (8, two_to_one_wrap, upgradable_required), (10, capacity_sats, required), (12, announcement_message, required), }); @@ -1161,7 +1161,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_required), (4, channels, vec_type), }); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 80ecc228429..70bf07f958e 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -1199,7 +1199,7 @@ impl MaybeReadable for Event { let mut payment_id = None; read_tlv_fields!(reader, { (0, payment_hash, required), - (1, network_update, ignorable), + (1, network_update, upgradable_required), (2, payment_failed_permanently, required), (5, path, vec_type), (7, short_channel_id, option), @@ -1282,7 +1282,7 @@ impl MaybeReadable for Event { 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); } @@ -1355,7 +1355,7 @@ impl MaybeReadable for Event { 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); } @@ -1413,7 +1413,7 @@ impl MaybeReadable for Event { let mut failed_next_destination_opt = 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 { diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index c7dcc870178..3f4b7153a7f 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -39,10 +39,10 @@ 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, ignorable_option) => { + ($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))) => { @@ -161,10 +161,10 @@ 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, ignorable_option) => { + ($len: expr, $type: expr, $field: expr, upgradable_option) => { $crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, option); }; } @@ -216,10 +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) => {{ // no-op }}; - ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, ignorable_option) => {{ + ($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)?)) => {{ @@ -258,10 +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) => {{ // no-op }}; - ($last_seen_type: expr, $type: expr, $field: ident, ignorable_option) => {{ + ($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)?)) => {{ @@ -292,10 +292,10 @@ macro_rules! _decode_tlv { ($reader: expr, $field: ident, option) => {{ $field = Some($crate::util::ser::Readable::read(&mut $reader)?); }}; - ($reader: expr, $field: ident, ignorable) => {{ + ($reader: expr, $field: ident, upgradable_required) => {{ $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; }}; - ($reader: expr, $field: ident, ignorable_option) => {{ + ($reader: expr, $field: ident, upgradable_option) => {{ $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; }}; ($reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{ @@ -635,10 +635,10 @@ 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: ident, ignorable_option) => { + ($field: ident, upgradable_option) => { $field }; ($field: ident, required) => { @@ -670,10 +670,10 @@ macro_rules! _init_tlv_field_var { ($field: ident, option) => { let mut $field = None; }; - ($field: ident, ignorable) => { + ($field: ident, upgradable_required) => { let mut $field = None; }; - ($field: ident, ignorable_option) => { + ($field: ident, upgradable_option) => { let mut $field = None; }; } From 70c7161dbead8e4b840e81caacd84e024f9eab21 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 24 Feb 2023 19:50:24 -0500 Subject: [PATCH 05/10] Rename OptionDeserWrapper->RequiredWrapper Makes more sense and sets us up for the UpgradableRequired version of it --- lightning/src/chain/channelmonitor.rs | 6 +++--- lightning/src/ln/channelmanager.rs | 4 ++-- lightning/src/util/events.rs | 6 +++--- lightning/src/util/ser.rs | 10 +++++----- lightning/src/util/ser_macros.rs | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index ea24f8cf61d..44e1d8c509e 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/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, 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), diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f659d63182b..99cf4046fd5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6693,7 +6693,7 @@ impl Writeable for ClaimableHTLC { impl Readable for ClaimableHTLC { fn read(reader: &mut R) -> Result { - 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 = 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 = crate::util::ser::OptionDeserWrapper(None); + let mut session_priv: crate::util::ser::RequiredWrapper = crate::util::ser::RequiredWrapper(None); let mut first_hop_htlc_msat: u64 = 0; let mut path = Some(Vec::new()); let mut payment_id = None; diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 70bf07f958e..6e8dd506c75 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -23,7 +23,7 @@ use crate::ln::features::ChannelTypeFeatures; use crate::ln::msgs; 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::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, WithoutLength}; use crate::routing::router::{RouteHop, RouteParameters}; use bitcoin::{PackedLockTime, Transaction}; @@ -1434,8 +1434,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 channel_type = OptionDeserWrapper(None); + let mut counterparty_node_id = RequiredWrapper(None); + let mut channel_type = RequiredWrapper(None); read_tlv_fields!(reader, { (0, channel_id, required), (2, user_channel_id, required), diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 847005e324e..36b64b63900 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -289,18 +289,18 @@ impl MaybeReadable for T { } /// Wrapper to read a required (non-optional) TLV record. -pub struct OptionDeserWrapper(pub Option); -impl Readable for OptionDeserWrapper { +pub struct RequiredWrapper(pub Option); +impl Readable for RequiredWrapper { #[inline] fn read(reader: &mut R) -> Result { Ok(Self(Some(Readable::read(reader)?))) } } /// When handling `default_values`, we want to map the default-value T directly -/// to a `OptionDeserWrapper` in a way that works for `field: T = t;` as +/// to a `RequiredWrapper` in a way that works for `field: T = t;` as /// well. Thus, we assume `Into for T` does nothing and use that. -impl From for OptionDeserWrapper { - fn from(t: T) -> OptionDeserWrapper { OptionDeserWrapper(Some(t)) } +impl From for RequiredWrapper { + fn from(t: T) -> RequiredWrapper { RequiredWrapper(Some(t)) } } pub(crate) struct U48(pub u64); diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 3f4b7153a7f..2aa3846ad99 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -656,13 +656,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()); From 5d0ee867ea6c9670ebfe6004614e9ebaa7502c64 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sun, 19 Feb 2023 16:52:22 -0500 Subject: [PATCH 06/10] Fix upgradable_required fields to actually be required in lower level macros When using lower level macros such as read_tlv_stream, upgradable_required fields have been treated as regular options. This is incorrect, they should either be upgradable_options or treated as required fields. --- lightning/src/chain/channelmonitor.rs | 10 ++--- lightning/src/chain/onchaintx.rs | 10 ++--- lightning/src/routing/gossip.rs | 6 +-- lightning/src/util/events.rs | 31 ++++++--------- lightning/src/util/ser.rs | 12 ++++++ lightning/src/util/ser_macros.rs | 54 ++++++++++++++++++++++++--- 6 files changed, 81 insertions(+), 42 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 44e1d8c509e..045ed10b138 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/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, RequiredWrapper, MaybeReadable, Writer, Writeable, U48}; +use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, UpgradableRequired, Writer, Writeable, U48}; use crate::util::byte_utils; use crate::util::events::Event; #[cfg(anchors)] @@ -454,7 +454,7 @@ 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), @@ -462,11 +462,7 @@ impl MaybeReadable for OnchainEventEntry { (3, block_hash, option), (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) })) } } diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 23140068a34..d3ca02ca7a5 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/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, 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) })) } } diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 957be2951fc..167f79fb269 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -883,9 +883,9 @@ impl Readable for ChannelInfo { (0, features, required), (1, announcement_received_time, (default_value, 0)), (2, node_one, required), - (4, one_to_two_wrap, upgradable_required), + (4, one_to_two_wrap, upgradable_option), (6, node_two, required), - (8, two_to_one_wrap, upgradable_required), + (8, two_to_one_wrap, upgradable_option), (10, capacity_sats, required), (12, announcement_message, required), }); @@ -1161,7 +1161,7 @@ impl Readable for NodeInfo { read_tlv_fields!(reader, { (0, _lowest_inbound_channel_fees, option), - (2, announcement_info_wrap, upgradable_required), + (2, announcement_info_wrap, upgradable_option), (4, channels, vec_type), }); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 6e8dd506c75..e083036bdac 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -23,7 +23,7 @@ use crate::ln::features::ChannelTypeFeatures; use crate::ln::msgs; use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::routing::gossip::NetworkUpdate; -use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, WithoutLength}; +use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength}; use crate::routing::router::{RouteHop, RouteParameters}; use bitcoin::{PackedLockTime, Transaction}; @@ -1199,7 +1199,7 @@ impl MaybeReadable for Event { let mut payment_id = None; read_tlv_fields!(reader, { (0, payment_hash, required), - (1, network_update, upgradable_required), + (1, network_update, upgradable_option), (2, payment_failed_permanently, required), (5, path, vec_type), (7, short_channel_id, option), @@ -1276,7 +1276,7 @@ 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 = None; let mut user_channel_id_high_opt: Option = None; read_tlv_fields!(reader, { @@ -1285,7 +1285,6 @@ impl MaybeReadable for Event { (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 @@ -1293,7 +1292,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() }, @@ -1349,7 +1348,7 @@ 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, { @@ -1358,11 +1357,10 @@ impl MaybeReadable for Event { (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, })) }; @@ -1410,22 +1408,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, upgradable_required), }); - if let Some(failed_next_destination) = failed_next_destination_opt { - Ok(Some(Event::HTLCHandlingFailed { - prev_channel_id, - 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) - } + Ok(Some(Event::HTLCHandlingFailed { + prev_channel_id, + failed_next_destination: _init_tlv_based_struct_field!(failed_next_destination_opt, upgradable_required), + })) }; f() }, diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 36b64b63900..5adf5758131 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -303,6 +303,18 @@ impl From for RequiredWrapper { fn from(t: T) -> RequiredWrapper { RequiredWrapper(Some(t)) } } +/// Wrapper to read a required (non-optional) TLV record that may have been upgraded without +/// backwards compat. +pub struct UpgradableRequired(pub Option); +impl MaybeReadable for UpgradableRequired { + #[inline] + fn read(reader: &mut R) -> Result, 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); impl Writeable for U48 { #[inline] diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 2aa3846ad99..1f617de40b3 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -217,7 +217,7 @@ macro_rules! _check_decoded_tlv_order { // no-op }}; ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_required) => {{ - // no-op + _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 @@ -259,7 +259,7 @@ macro_rules! _check_missing_tlv { // no-op }}; ($last_seen_type: expr, $type: expr, $field: ident, upgradable_required) => {{ - // no-op + _check_missing_tlv!($last_seen_type, $type, $field, required) }}; ($last_seen_type: expr, $type: expr, $field: ident, upgradable_option) => {{ // no-op @@ -293,7 +293,10 @@ macro_rules! _decode_tlv { $field = Some($crate::util::ser::Readable::read(&mut $reader)?); }}; ($reader: expr, $field: ident, upgradable_required) => {{ - $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; + $field = match $crate::util::ser::MaybeReadable::read(&mut $reader)? { + Some(res) => res, + _ => return Ok(None) + }; }}; ($reader: expr, $field: ident, upgradable_option) => {{ $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; @@ -636,7 +639,7 @@ macro_rules! _init_tlv_based_struct_field { $field }; ($field: ident, upgradable_required) => { - if $field.is_none() { return Ok(None); } else { $field.unwrap() } + $field.0.unwrap() }; ($field: ident, upgradable_option) => { $field @@ -671,7 +674,7 @@ macro_rules! _init_tlv_field_var { let mut $field = None; }; ($field: ident, upgradable_required) => { - let mut $field = None; + let mut $field = $crate::util::ser::UpgradableRequired(None); }; ($field: ident, upgradable_option) => { let mut $field = None; @@ -1050,6 +1053,47 @@ mod tests { (0xdeadbeef1badbeef, 0x1bad1dea, Some(0x01020304))); } + #[derive(Debug, PartialEq)] + struct TestUpgradable { + a: u32, + b: u32, + c: Option, + } + + fn upgradable_tlv_reader(s: &[u8]) -> Result, DecodeError> { + let mut s = Cursor::new(s); + let mut a = 0; + let mut b = 0; + let mut c: Option = 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>, Option, Option<(PublicKey, u64, u64)>, Option), DecodeError> { let mut s = Cursor::new(s); From 34f8c396303f794fe5a3f7b3569a53fd70447777 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 25 Feb 2023 16:03:43 -0500 Subject: [PATCH 07/10] ser_macros: Document behavior of upgradable_* variants --- lightning/src/util/ser_macros.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 1f617de40b3..54085686540 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -292,12 +292,19 @@ macro_rules! _decode_tlv { ($reader: expr, $field: ident, option) => {{ $field = Some($crate::util::ser::Readable::read(&mut $reader)?); }}; + // `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)?; }}; From 8d686d83cb0feeff4b8b0a53613f0e592738fbd8 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sun, 19 Feb 2023 18:32:29 -0500 Subject: [PATCH 08/10] Implement writeable for APIError --- lightning/src/ln/channelmanager.rs | 4 ++-- lightning/src/ln/onion_utils.rs | 4 ++-- lightning/src/ln/outbound_payment.rs | 8 ++++---- lightning/src/util/errors.rs | 14 +++++++++++++- 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 99cf4046fd5..1fd70ee6fed 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -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); diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 1abaf5920a5..2916829f259 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -182,11 +182,11 @@ pub(super) fn build_onion_payloads(path: &Vec, 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; } diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 16037c7f656..473e050de60 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -895,22 +895,22 @@ impl OutboundPayments { u32, PaymentId, &Option, [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; } } diff --git a/lightning/src/util/errors.rs b/lightning/src/util/errors.rs index b7d562e54dd..aa740044676 100644 --- a/lightning/src/util/errors.rs +++ b/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 { From 1dcb3ecb6c6f145bd22a64ce149e939c75aeae3d Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 13 Feb 2023 17:55:42 -0500 Subject: [PATCH 09/10] Change PaymentPathFailed's optional network update to a Failure enum This let us capture the errors when we fail without committing to an HTLC vs failing via update_fail. --- lightning-background-processor/src/lib.rs | 16 +++---- lightning/src/ln/functional_test_utils.rs | 39 ++++++++-------- lightning/src/ln/functional_tests.rs | 29 +++++------- lightning/src/ln/onion_route_tests.rs | 4 +- lightning/src/ln/outbound_payment.rs | 4 +- lightning/src/ln/payment_tests.rs | 12 +++-- lightning/src/util/events.rs | 55 ++++++++++++++++++----- pending_changelog/2043.txt | 5 +++ 8 files changed, 103 insertions(+), 61 deletions(-) diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index 3b692ac07e2..51d8a7f866f 100644 --- a/lightning-background-processor/src/lib.rs +++ b/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; @@ -215,10 +215,10 @@ where fn handle_network_graph_update( network_graph: &NetworkGraph, 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); } } @@ -673,7 +673,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; @@ -1365,7 +1365,7 @@ mod tests { payment_id: None, payment_hash: PaymentHash([42; 32]), payment_failed_permanently: false, - network_update: None, + failure: PathFailure::OnPath { network_update: None }, path: path.clone(), short_channel_id: Some(scored_scid), retry: None, @@ -1385,7 +1385,7 @@ mod tests { payment_id: None, payment_hash: PaymentHash([42; 32]), payment_failed_permanently: true, - network_update: None, + failure: PathFailure::OnPath { network_update: None }, path: path.clone(), short_channel_id: None, retry: None, diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 2786d754abb..d1203297dec 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/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() diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 32c3b51688a..03eef437262 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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,11 +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 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!(*network_update, None); assert_eq!(*short_channel_id, Some(route.paths[0][0].short_channel_id)); }, _ => panic!("Unexpected event"), @@ -5785,11 +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 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!(*network_update, None); assert_eq!(*short_channel_id, Some(route_2.paths[0][0].short_channel_id)); }, _ => panic!("Unexpected event"), @@ -6687,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); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index bc6dd1168bf..6ea8ab4ce1b 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/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,7 +167,7 @@ fn run_onion_failure_test_with_fail_intercept(_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 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!(*error_code, expected_error_code); if expected_channel_update.is_some() { diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 473e050de60..e6454b0ae51 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -793,7 +793,7 @@ impl OutboundPayments { payment_id: Some(payment_id), payment_hash, payment_failed_permanently: false, - network_update: None, + failure: events::PathFailure::InitialSend { err: e }, path, short_channel_id: failed_scid, retry: None, @@ -1255,7 +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 }, path: path.clone(), short_channel_id, retry, diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 1d30bceff96..4aa14dfa831 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/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, 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, 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"), } diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index e083036bdac..5bc8a5084f2 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -23,6 +23,7 @@ use crate::ln::features::ChannelTypeFeatures; use crate::ln::msgs; use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::routing::gossip::NetworkUpdate; +use crate::util::errors::APIError; use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength}; use crate::routing::router::{RouteHop, RouteParameters}; @@ -81,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, + }, +} + +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 { @@ -588,7 +622,7 @@ pub enum Event { fee_paid_msat: Option, }, /// 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 @@ -651,14 +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 - /// payment route. - /// - /// Should be applied to the [`NetworkGraph`] so that routing decisions can take into - /// account the update. + /// Extra error details based on the failure type. May contain an update that needs to be + /// applied to the [`NetworkGraph`]. /// /// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph - network_update: Option, + failure: PathFailure, /// The payment path that failed. path: Vec, /// The channel responsible for the failed payment path. @@ -960,7 +991,7 @@ 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 path, ref short_channel_id, ref retry, #[cfg(test)] ref error_code, @@ -974,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::, option), // network_update in LDK versions prior to 0.0.114 (2, payment_failed_permanently, 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: _ } => { @@ -1197,6 +1229,7 @@ impl MaybeReadable for Event { 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, upgradable_option), @@ -1205,12 +1238,14 @@ impl MaybeReadable for Event { (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, path: path.unwrap(), short_channel_id, retry, diff --git a/pending_changelog/2043.txt b/pending_changelog/2043.txt index 2fc7c750ce7..bcd460ed885 100644 --- a/pending_changelog/2043.txt +++ b/pending_changelog/2043.txt @@ -1,8 +1,13 @@ ## 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 From f2f90d5fb0c7a6f04b43e9475aa89bc2d58ee319 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 23 Feb 2023 22:46:45 -0500 Subject: [PATCH 10/10] Fix PaymentPathFailed generation and scid on initial send --- lightning/src/ln/outbound_payment.rs | 104 +++++++++++++++++++++++++-- 1 file changed, 97 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index e6454b0ae51..b83eb41748f 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -782,13 +782,13 @@ 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 { - None - } else { + if let APIError::MonitorUpdateInProgress = e { continue } + let mut failed_scid = None; + 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, @@ -1353,12 +1353,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] @@ -1455,4 +1457,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"); } + } }