`PaymentPathFailed`: add initial send error details #2043

valentinewallace · 2023-02-21T18:16:01Z

As of #2014, we generate PaymentPathFailed events for retryable payment paths that fail without committing to an HTLC. Here we adapt said events to include the APIError for the failed path.

Partially addresses #1932
~~Based on #1320~~

codecov-commenter · 2023-02-21T20:23:09Z

Codecov Report

Base: 87.33% // Head: 87.56% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (f2f90d5) compared to base (46fd703).
Patch coverage: 75.17% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2043      +/-   ##
==========================================
+ Coverage   87.33%   87.56%   +0.22%     
==========================================
  Files         101      101              
  Lines       44364    45779    +1415     
  Branches    44364    45779    +1415     
==========================================
+ Hits        38747    40086    +1339     
- Misses       5617     5693      +76

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`86.54% <0.00%> (+<0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`86.70% <ø> (ø)`
lightning/src/routing/gossip.rs	`84.04% <ø> (ø)`
lightning/src/util/errors.rs	`32.35% <0.00%> (+0.21%)`	⬆️
lightning/src/util/events.rs	`25.50% <28.57%> (-0.43%)`	⬇️
lightning/src/util/ser.rs	`84.27% <33.33%> (-0.46%)`	⬇️
lightning-background-processor/src/lib.rs	`86.25% <60.00%> (+4.57%)`	⬆️
lightning/src/util/ser_macros.rs	`93.02% <82.75%> (+0.32%)`	⬆️
lightning/src/ln/outbound_payment.rs	`80.49% <90.90%> (+1.35%)`	⬆️
lightning/src/chain/channelmonitor.rs	`89.63% <100.00%> (+0.04%)`	⬆️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2023-02-23T21:58:20Z

Needs rebase, also probably worth addressing #2014 (comment) here

valentinewallace · 2023-02-23T22:14:38Z

Rebased, will address #2014 (comment) shortly

lightning/src/util/events.rs

lightning/src/util/ser_macros.rs

jkczyz · 2023-02-24T14:31:12Z

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


If we're going to add serialization, should we just make this an enum? Seems wasteful to serialize static strings.

Eh, seems a bit unrelated to the PR, there'd be about 7 variants and they should all basically never happen..

Ah, right. I guess the others are all strings anyhow, so might as well be consistent.

lightning/src/util/events.rs

jkczyz · 2023-02-24T14:41:33Z

lightning/src/util/events.rs

@@ -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::<NetworkUpdate>, option), // network_update in LDK versions prior to 0.0.114


Should we write the NetworkUpdate if we have a Failure with one?

We could, I don't have a strong opinion. It's not horrible to break forwards compat here, old versions will just have None as their network_update. The downside is possibly double-(de)serializing a ChannelUpdate + its signature and might make the ser code a bit uglier

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.

Prior to this change, our impl_writeable_tlv_based macros only supported deserializing a MaybeReadable if it's non-Optional.

Would rather not rename ignorable to ignorable_required, so rename both of them to upgradable_*

lightning/src/util/ser_macros.rs

jkczyz · 2023-02-24T23:39:24Z

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


Ah, right. I guess the others are all strings anyhow, so might as well be consistent.

jkczyz · 2023-02-25T00:02:08Z

lightning/src/util/events.rs

-					}
+					Ok(Some(Event::HTLCHandlingFailed {
+						prev_channel_id,
+						failed_next_destination: _init_tlv_based_struct_field!(failed_next_destination_opt, upgradable_required),


Shouldn't the early return in _init_tlv_based_struct_field already be hit in _decode_tlv via read_tlv_fields above?

Removed the early return from _init_tlv_based_struct_field

Why not unwrap instead of needing to use _init_tlv_based_struct_field? Seems strange that this is a requirement for using upgradable_required.

It's not a requirement, we could unwrap. One reason to use it is that In the past, not using _init_tlv_based_struct_field hid a bug in our ser macros where ignorable was treated differently when used in lower-level macros (eg read_tlv_stream) vs higher-level macros (eg impl_writeable_*) (see #2014 (comment)). So this ensures they're consistent and also means you don't need to check an unwrap for safety.

Hmm... not sure using it really ensures they are consistent. Fixing the bug did that, no? As far as unwrap safety, isn't that an invariant of using upgradable_required?

Well, using _init_tlv_based_struct_field in lower level macros would've caught the inconsistency, so it's nice to know it works now. And yeah there's always an unwrap but this way you can just check that both ser macros specify upgradable_required instead of checking the unwrap itself, but, not a huge deal there.

Makes more sense and sets us up for the UpgradableRequired version of it

jkczyz · 2023-02-25T03:40:09Z

lightning/src/util/events.rs

-					}
+					Ok(Some(Event::HTLCHandlingFailed {
+						prev_channel_id,
+						failed_next_destination: _init_tlv_based_struct_field!(failed_next_destination_opt, upgradable_required),


Why not unwrap instead of needing to use _init_tlv_based_struct_field? Seems strange that this is a requirement for using upgradable_required.

jkczyz · 2023-02-25T03:40:41Z

lightning/src/util/events.rs

@@ -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);


Can UpgradableRequired be hidden inside the macros like we do for WithoutLength?

Could do, but then it wouldn't have a purpose anymore. The purpose of RequiredWrapper + UpgradableRequired is that we can treat fields as non-Options in _decode_tlv, i.e. do $field = Readable::read(..)? instead of $field = Some(Readable::read(..)?). We'd have to do the latter if we hid it.

Why is it desirable for the caller to need additional boilerplate when using upgradable_required? Seems avoiding that would be preferable to using Option in _decode_tlv?

Note also that the variables here are still suffixed with _opt and we are already exposing the fact that UpgradableRequired is wrapping an Option. So needing to explicitly use the wrapper and _init_tlv_based_struct_field later is adding more cognitive load, IMHO.

We just made it uniform with the required type in this PR, so we could change both of them, I don't mind, but I think it should be done in follow-up.

TheBlueMatt · 2023-02-25T19:18:58Z

LGTM, feel free to squash.

TheBlueMatt · 2023-02-25T19:20:16Z

Oh, can you include some docs...somewhere about what upgradable_option and upgradable_requied mean now?

… 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.

This let us capture the errors when we fail without committing to an HTLC vs failing via update_fail.

naumenkogs · 2023-02-27T12:28:51Z

pending_changelog/2043.txt

+## 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


nit: should should :)

0.0.114 - Mar 3, 2023 - "Faster Async BOLT12 Retries" API Updates =========== * `InvoicePayer` has been removed and its features moved directly into `ChannelManager`. As such it now requires a simplified `Router` and supports `send_payment_with_retry` (and friends). `ChannelManager::retry_payment` was removed in favor of the automated retries. Invoice payment utilities in `lightning-invoice` now call the new code (lightningdevkit#1812, lightningdevkit#1916, lightningdevkit#1929, lightningdevkit#2007, etc). * `Sign`/`BaseSign` has been renamed `ChannelSigner`, with `EcdsaChannelSigner` split out in anticipation of future schnorr/taproot support (lightningdevkit#1967). * The catch-all `KeysInterface` was split into `EntropySource`, `NodeSigner`, and `SignerProvider`. `KeysManager` implements all three (lightningdevkit#1910, lightningdevkit#1930). * `KeysInterface::get_node_secret` is now `KeysManager::get_node_secret_key` and is no longer required for external signers (lightningdevkit#1951, lightningdevkit#2070). * A `lightning-transaction-sync` crate has been added which implements keeping LDK in sync with the chain via an esplora server (lightningdevkit#1870). Note that it can only be used on nodes that *never* ran a previous version of LDK. * `Score` is updated in `BackgroundProcessor` instead of via `Router` (lightningdevkit#1996). * `ChainAccess::get_utxo` (now `UtxoAccess`) can now be resolved async (lightningdevkit#1980). * BOLT12 `Offer`, `InvoiceRequest`, `Invoice` and `Refund` structs as well as associated builders have been added. Such invoices cannot yet be paid due to missing support for blinded path payments (lightningdevkit#1927, lightningdevkit#1908, lightningdevkit#1926). * A `lightning-custom-message` crate has been added to make combining multiple custom messages into one enum/handler easier (lightningdevkit#1832). * `Event::PaymentPathFailure` is now generated for failure to send an HTLC over the first hop on our local channel (lightningdevkit#2014, lightningdevkit#2043). * `lightning-net-tokio` no longer requires an `Arc` on `PeerManager` (lightningdevkit#1968). * `ChannelManager::list_recent_payments` was added (lightningdevkit#1873). * `lightning-background-processor` `std` is now optional in async mode (lightningdevkit#1962). * `create_phantom_invoice` can now be used in `no-std` (lightningdevkit#1985). * The required final CLTV delta on inbound payments is now configurable (lightningdevkit#1878) * bitcoind RPC error code and message are now surfaced in `block-sync` (lightningdevkit#2057). * Get `historical_estimated_channel_liquidity_probabilities` was added (lightningdevkit#1961). * `ChannelManager::fail_htlc_backwards_with_reason` was added (lightningdevkit#1948). * Macros which implement serialization using TLVs or straight writing of struct fields are now public (lightningdevkit#1823, lightningdevkit#1976, lightningdevkit#1977). Backwards Compatibility ======================= * Any inbound payments with a custom final CLTV delta will be rejected by LDK if you downgrade prior to receipt (lightningdevkit#1878). * `Event::PaymentPathFailed::network_update` will always be `None` if an 0.0.114-generated event is read by a prior version of LDK (lightningdevkit#2043). * `Event::PaymentPathFailed::all_paths_removed` will always be false if an 0.0.114-generated event is read by a prior version of LDK. Users who rely on it to determine payment retries should migrate to `Event::PaymentFailed`, in a separate release prior to upgrading to LDK 0.0.114 if downgrading is supported (lightningdevkit#2043). Performance Improvements ======================== * Channel data is now stored per-peer and channel updates across multiple peers can be operated on simultaneously (lightningdevkit#1507). * Routefinding is roughly 1.5x faster (lightningdevkit#1799). * Deserializing a `NetworkGraph` is roughly 6x faster (lightningdevkit#2016). * Memory usage for a `NetworkGraph` has been reduced substantially (lightningdevkit#2040). * `KeysInterface::get_secure_random_bytes` is roughly 200x faster (lightningdevkit#1974). Bug Fixes ========= * Fixed a bug where a delay in processing a `PaymentSent` event longer than the time taken to persist a `ChannelMonitor` update, when occurring immediately prior to a crash, may result in the `PaymentSent` event being lost (lightningdevkit#2048). * Fixed spurious rejections of rapid gossip sync data when the graph has been updated by other means between gossip syncs (lightningdevkit#2046). * Fixed a panic in `KeysManager` when the high bit of `starting_time_nanos` is set (lightningdevkit#1935). * Resolved an issue where the `ChannelManager::get_persistable_update_future` future would fail to wake until a second notification occurs (lightningdevkit#2064). * Resolved a memory leak when using `ChannelManager::send_probe` (lightningdevkit#2037). * Fixed a deadlock on some platforms at least when using async `ChannelMonitor` updating (lightningdevkit#2006). * Removed debug-only assertions which were reachable in threaded code (lightningdevkit#1964). * In some cases when payment sending fails on our local channel retries no longer take the same path and thus never succeed (lightningdevkit#2014). * Retries for spontaneous payments have been fixed (lightningdevkit#2002). * Return an `Err` if `lightning-persister` fails to read the directory listing rather than panicing (lightningdevkit#1943). * `peer_disconnected` will now never be called without `peer_connected` (lightningdevkit#2035) Security ======== 0.0.114 fixes several denial-of-service vulnerabilities which are reachable from untrusted input from channel counterparties or in deployments accepting inbound connections or channels. It also fixes a denial-of-service vulnerability in rare cases in the route finding logic. * The number of pending un-funded channels as well as peers without funded channels is now limited to avoid denial of service (lightningdevkit#1988). * A second `channel_ready` message received immediately after the first could lead to a spurious panic (lightningdevkit#2071). This issue was introduced with 0conf support in LDK 0.0.107. * A division-by-zero issue was fixed in the `ProbabilisticScorer` if the amount being sent (including previous-hop fees) is equal to a channel's capacity while walking the graph (lightningdevkit#2072). The division-by-zero was introduced with historical data tracking in LDK 0.0.112. In total, this release features 130 files changed, 21457 insertions, 10113 deletions in 343 commits from 18 authors, in alphabetical order: * Alec Chen * Allan Douglas R. de Oliveira * Andrei * Arik Sosman * Daniel Granhão * Duncan Dean * Elias Rohrer * Jeffrey Czyz * John Cantrell * Kurtsley * Matt Corallo * Max Fang * Omer Yacine * Valentine Wallace * Viktor Tigerström * Wilmer Paulino * benthecarman * jurvis

valentinewallace added the blocked on dependent pr label Feb 21, 2023

valentinewallace added this to the 0.0.114 milestone Feb 21, 2023

valentinewallace mentioned this pull request Feb 21, 2023

Rework auto-retry send errors #2014

Merged

valentinewallace force-pushed the 2023-02-initial-send-path-fails branch from aad8757 to 55c286e Compare February 21, 2023 20:03

douglaz reviewed Feb 22, 2023

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

Add missing import path in ser macro

f361aa6

valentinewallace force-pushed the 2023-02-initial-send-path-fails branch from 55c286e to 10bb76e Compare February 23, 2023 22:09

valentinewallace removed the blocked on dependent pr label Feb 23, 2023

TheBlueMatt reviewed Feb 23, 2023

View reviewed changes

lightning/src/util/events.rs Outdated Show resolved Hide resolved

lightning/src/util/ser_macros.rs Outdated Show resolved Hide resolved

lightning/src/util/ser_macros.rs Outdated Show resolved Hide resolved

jkczyz reviewed Feb 24, 2023

View reviewed changes

valentinewallace added 3 commits February 24, 2023 14:21

Support deserializing an Option-al MaybeReadable

52551a9

Prior to this change, our impl_writeable_tlv_based macros only supported deserializing a MaybeReadable if it's non-Optional.

Disambiguate ignorable and ignorable_option

0e88538

Would rather not rename ignorable to ignorable_required, so rename both of them to upgradable_*

valentinewallace force-pushed the 2023-02-initial-send-path-fails branch 2 times, most recently from e0aa282 to 0353336 Compare February 24, 2023 19:41

valentinewallace requested review from TheBlueMatt and jkczyz February 24, 2023 20:24

TheBlueMatt reviewed Feb 24, 2023

View reviewed changes

lightning/src/util/ser_macros.rs Outdated Show resolved Hide resolved

jkczyz reviewed Feb 25, 2023

View reviewed changes

Rename OptionDeserWrapper->RequiredWrapper

70c7161

Makes more sense and sets us up for the UpgradableRequired version of it

valentinewallace force-pushed the 2023-02-initial-send-path-fails branch from 0353336 to 0b20a6c Compare February 25, 2023 01:14

jkczyz reviewed Feb 25, 2023

View reviewed changes

valentinewallace added 3 commits February 25, 2023 16:13

ser_macros: Document behavior of upgradable_* variants

34f8c39

Implement writeable for APIError

8d686d8

valentinewallace added 2 commits February 25, 2023 16:13

Change PaymentPathFailed's optional network update to a Failure enum

1dcb3ec

This let us capture the errors when we fail without committing to an HTLC vs failing via update_fail.

Fix PaymentPathFailed generation and scid on initial send

f2f90d5

valentinewallace force-pushed the 2023-02-initial-send-path-fails branch from 0b20a6c to f2f90d5 Compare February 25, 2023 21:19

naumenkogs reviewed Feb 27, 2023

View reviewed changes

naumenkogs approved these changes Feb 27, 2023

View reviewed changes

TheBlueMatt approved these changes Feb 27, 2023

View reviewed changes

jkczyz approved these changes Feb 27, 2023

View reviewed changes

TheBlueMatt merged commit a0c65c3 into lightningdevkit:main Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`PaymentPathFailed`: add initial send error details #2043

`PaymentPathFailed`: add initial send error details #2043

valentinewallace commented Feb 21, 2023 •

edited

Loading

codecov-commenter commented Feb 21, 2023 •

edited

Loading

TheBlueMatt commented Feb 23, 2023

valentinewallace commented Feb 23, 2023

jkczyz Feb 24, 2023

valentinewallace Feb 24, 2023

jkczyz Feb 24, 2023

jkczyz Feb 24, 2023

valentinewallace Feb 24, 2023 •

edited

Loading

jkczyz Feb 24, 2023

jkczyz Feb 25, 2023

valentinewallace Feb 25, 2023

jkczyz Feb 25, 2023

valentinewallace Feb 25, 2023

jkczyz Feb 27, 2023

valentinewallace Feb 27, 2023

jkczyz Feb 25, 2023

jkczyz Feb 25, 2023

valentinewallace Feb 25, 2023

jkczyz Feb 27, 2023

valentinewallace Feb 27, 2023

TheBlueMatt commented Feb 25, 2023

TheBlueMatt commented Feb 25, 2023

naumenkogs Feb 27, 2023

PaymentPathFailed: add initial send error details #2043

PaymentPathFailed: add initial send error details #2043

Conversation

valentinewallace commented Feb 21, 2023 • edited Loading

codecov-commenter commented Feb 21, 2023 • edited Loading

Codecov Report

TheBlueMatt commented Feb 23, 2023

valentinewallace commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 25, 2023

TheBlueMatt commented Feb 25, 2023

Choose a reason for hiding this comment

`PaymentPathFailed`: add initial send error details #2043

`PaymentPathFailed`: add initial send error details #2043

valentinewallace commented Feb 21, 2023 •

edited

Loading

codecov-commenter commented Feb 21, 2023 •

edited

Loading

valentinewallace Feb 24, 2023 •

edited

Loading