Allow configurable `min_final_cltv_expiry_delta` #1878

dunxen · 2022-11-28T15:42:54Z

Adds two new payment Methods for identifying payments with custom
min_final_cltv_expiry_delta as payments with LDK or user payment
hashes.

The min_final_cltv_expiry_delta value is packed into the metadata
bytes of the payment secret, taking up 12 bits.

Fixes #1850

lightning/src/ln/inbound_payment.rs

TheBlueMatt · 2022-12-13T03:34:52Z

Ugh, needs rebase, sorry about that.

dunxen · 2022-12-13T06:55:48Z

Ugh, needs rebase, sorry about that.

it's the way of the git lol

dunxen · 2022-12-13T10:37:44Z

I went the "add param onto existing methods" route because I feel it's compatible with all utility methods. Didn't feel right at the time to create more variants of each method which also seems a bit much. I know optional params are usually not ideal. Open to ideas.

shamardy · 2022-12-13T11:03:25Z

I know optional params are usually not ideal. Open to ideas.

You can have min_final_cltv_expiry as optional in the functions that the user should use only and use unwrap_or(DEFAULT_MIN_FINAL_CLTV_EXPIRY) to provide non optional values to other functions.

In my opinion, having this parameter as optional in lightning-invoice crate functions (create_phantom_invoice, create_phantom_invoice_with_description_hash, create_invoice_from_channelmanager, create_invoice_from_channelmanager_with_description_hash) is enough. Other functions are more advanced, and if the user chooses to use them, he/she should provide a value for min_final_cltv_expiry.

dunxen · 2022-12-13T11:30:13Z

I know optional params are usually not ideal. Open to ideas.

You can have min_final_cltv_expiry as optional in the functions that the user should use only and use unwrap_or(DEFAULT_MIN_FINAL_CLTV_EXPIRY) to provide non optional values to other functions.

Yeah that too, but wanted to avoid validating the value in many places and just in the lowest level internal functions, so that's why some internal functions just pass through the Option.

TheBlueMatt

Thanks! This is pretty close I think, just some notes about missing test coverage and docs.

lightning-invoice/src/lib.rs

lightning-invoice/src/utils.rs

lightning/src/ln/inbound_payment.rs

TheBlueMatt

Oh, two more comments.

lightning/src/ln/inbound_payment.rs

dunxen · 2022-12-16T11:20:53Z

Still working on that test coverage, but rebased an pushed some fixes so long.

codecov-commenter · 2022-12-16T11:21:32Z

Codecov Report

Base: 90.80% // Head: 90.69% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (0eb4550) compared to base (e0a0add).
Patch coverage: 95.45% of modified lines in pull request are covered.

❗ Current head 0eb4550 differs from pull request most recent head 8cd50d8. Consider uploading reports for the commit 8cd50d8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1878      +/-   ##
==========================================
- Coverage   90.80%   90.69%   -0.12%     
==========================================
  Files          98       97       -1     
  Lines       51507    50871     -636     
  Branches    51507    50871     -636     
==========================================
- Hits        46770    46135     -635     
+ Misses       4737     4736       -1

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`91.21% <ø> (ø)`
lightning-invoice/src/de.rs	`82.38% <83.33%> (ø)`
lightning-invoice/src/lib.rs	`87.37% <83.33%> (-0.11%)`	⬇️
lightning/src/ln/inbound_payment.rs	`89.75% <88.88%> (-3.74%)`	⬇️
lightning-invoice/src/utils.rs	`97.65% <97.29%> (-0.12%)`	⬇️
lightning-invoice/src/payment.rs	`89.63% <100.00%> (ø)`
lightning-invoice/src/ser.rs	`92.19% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`87.24% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.93% <100.00%> (-0.18%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.73% <100.00%> (ø)`
... and 17 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.

TheBlueMatt · 2022-12-18T16:38:50Z

Let me know when you get the test coverage in place and I'll take another look. In the mean time, we should get another reviewer on this.

dunxen · 2022-12-18T17:57:38Z

Let me know when you get the test coverage in place and I'll take another look.

Should have it up in the morning tomorrow.

dunxen · 2022-12-19T13:15:23Z

Oops. This needs to be changed and tested, right?:

rust-lightning/lightning/src/ln/channelmanager.rs

Line 2075 in 975984f

    
           if (hop_data.outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 {

Also, I'm not sure where we ever enforced the MIN_FINAL_CLTV_EXPIRY(_DELTA) on current main branch... I just see that HTLC_FAIL_BACK_BUFFER above.

TheBlueMatt · 2022-12-19T16:51:48Z

Oops. This needs to be changed and tested, right?:

Yes, indeed, we need to compare against the inbound_payment decoded version and reject the payment.

Also, I'm not sure where we ever enforced the MIN_FINAL_CLTV_EXPIRY(_DELTA) on current main branch... I just see that HTLC_FAIL_BACK_BUFFER above.

We dont - we enforce HTLC_FAIL_BACK_BUFFER on purpose - MIN_FINAL_CLTV_EXPIRY_DELTA is defined as HTLC_FAIL_BACK_BUFFER + 3 so we just enforce the former to enforce the latter but with a bit of a buffer in case a block or two were found while the HTLC was being forwarded to us.

dunxen · 2022-12-19T20:15:40Z

Yes, indeed, we need to compare against the inbound_payment decoded version and reject the payment.

Cool. Happy if I squash and rebase after fixing up to get it to a cleaner state for reviewers?

There's also no rush on this one since I guess 114 is coming in Q1 so I can jump on more review/dual funding while I wait.

TheBlueMatt · 2022-12-19T20:58:11Z

Yes, feel free to squash. There's no specific rush, no, but of course always better to have fewer open PRs :)

TheBlueMatt · 2022-12-21T21:54:49Z

Looks like this needs rebase.

dunxen · 2023-01-04T16:46:19Z

Back on this now. Rebased locally, but busy debugging a failing test.

dunxen · 2023-01-17T09:59:39Z

The rebase conflicts were a little intense so had to squash some of the fixups otherwise things would get a little wild here.

lightning-invoice/src/utils.rs

wpaulino · 2023-01-17T19:35:16Z

lightning/src/ln/channelmanager.rs

@@ -882,7 +882,7 @@ pub(super) const CLTV_FAR_FAR_AWAY: u32 = 14 * 24 * 6;
 // Note that we fail if exactly HTLC_FAIL_BACK_BUFFER + 1 was used, so we need to add one for
 // any payments to succeed. Further, we don't want payments to fail if a block was found while
 // a payment was being routed, so we add an extra block to be safe.
-pub const MIN_FINAL_CLTV_EXPIRY_DELTA: u32 = HTLC_FAIL_BACK_BUFFER + 3;
+pub const MIN_FINAL_CLTV_EXPIRY_DELTA: u16 = HTLC_FAIL_BACK_BUFFER as u16 + 3;


Is it too much hassle to make HTLC_FAIL_BACK_BUFFER a u16?

Was considering it and reckoned it might blow this PR up, but it would be more sanitary. Will wait for some more feedback on it. 👍

lightning-invoice/src/utils.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/inbound_payment.rs

lightning-invoice/src/utils.rs

lightning/src/ln/inbound_payment.rs

wpaulino · 2023-01-20T19:26:31Z

Feel free to squash.

TheBlueMatt

This looks great. Some style nits but otherwise happy here.

lightning/src/ln/inbound_payment.rs

This matches the spec and helps avoid any confusion around naming. We're also then consistent with `cltv_expiry` in an HTLC being the actual block height value for the CLTV and not a delta.

All utility functions for invoice construction will now also accept an Option<>al `min_final_cltv_expiry_delta` which is useful for things like swaps etc. The `min_final_cltv_expiry_delta` will default back to `MIN_FINAL_CLTV_EXPIRY_DELTA` if `None` is provided.

TheBlueMatt · 2023-01-24T17:39:06Z

Feel free to squash

lightning/src/ln/inbound_payment.rs

Adds two new payment `Method`s for identifying payments with custom `min_final_cltv_expiry_delta` as payments with LDK or user payment hashes. The `min_final_cltv_expiry_delta` value is packed into the first 2 bytes of the expiry timestamp in the payment secret metadata.

lightning-invoice/src/utils.rs

wpaulino · 2023-01-24T20:41:08Z

lightning/src/ln/inbound_payment.rs

-pub(super) fn verify<L: Deref>(payment_hash: PaymentHash, payment_data: &msgs::FinalOnionHopData, highest_seen_timestamp: u64, keys: &ExpandedKey, logger: &L) -> Result<Option<PaymentPreimage>, ()>
+pub(super) fn verify<L: Deref>(payment_hash: PaymentHash, payment_data: &msgs::FinalOnionHopData,
+	highest_seen_timestamp: u64, keys: &ExpandedKey, logger: &L) -> Result<
+	(Option<PaymentPreimage>, Option<u16>), ()>


Nit: this wrapping is not consistent

TheBlueMatt

Don't bother changing it unless you need to address other feedback.

TheBlueMatt · 2023-01-24T21:44:50Z

lightning-invoice/src/utils.rs

@@ -362,13 +392,18 @@ fn _create_invoice_from_channelmanager_and_duration_since_epoch<M: Deref, T: Der
 			R::Target: Router,
 			L::Target: Logger,
 {
+	if min_final_cltv_expiry_delta.is_some() && min_final_cltv_expiry_delta.unwrap().saturating_add(3) < MIN_FINAL_CLTV_EXPIRY_DELTA {


nit: this check is redundant

I don't think so, the others are part of a different flow.

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

dunxen commented Nov 28, 2022

View reviewed changes

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

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch 3 times, most recently from a297bba to 354fab2 Compare November 29, 2022 20:07

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch 2 times, most recently from 64b8e36 to 91db879 Compare December 12, 2022 15:39

dunxen changed the title ~~DRAFT: Allow configurable min_final_cltv_expiry_delta~~ Allow configurable min_final_cltv_expiry_delta Dec 12, 2022

dunxen marked this pull request as ready for review December 12, 2022 15:41

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch 2 times, most recently from 01197fb to 147be43 Compare December 13, 2022 10:19

TheBlueMatt added the Seeking Code Review label Dec 13, 2022

TheBlueMatt reviewed Dec 13, 2022

View reviewed changes

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

lightning/src/ln/inbound_payment.rs Show resolved Hide resolved

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch from 147be43 to 652c45d Compare December 16, 2022 11:20

TheBlueMatt added this to the 0.0.114 milestone Dec 21, 2022

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch 2 times, most recently from d10f48c to 30772df Compare January 17, 2023 13:20

wpaulino reviewed Jan 17, 2023

View reviewed changes

TheBlueMatt removed the Seeking Code Review label Jan 17, 2023

TheBlueMatt assigned TheBlueMatt and wpaulino Jan 17, 2023

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch from 30772df to 0eb4550 Compare January 19, 2023 09:22

wpaulino reviewed Jan 19, 2023

View reviewed changes

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch from 0eb4550 to f180062 Compare January 20, 2023 06:38

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch from f180062 to c2fe4d0 Compare January 21, 2023 12:45

wpaulino requested review from wpaulino and TheBlueMatt January 23, 2023 23:43

TheBlueMatt reviewed Jan 24, 2023

View reviewed changes

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

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

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

dunxen added 2 commits January 24, 2023 13:07

Add _delta suffix to min_final_cltv_expiry

fb10fc9

This matches the spec and helps avoid any confusion around naming. We're also then consistent with `cltv_expiry` in an HTLC being the actual block height value for the CLTV and not a delta.

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch 3 times, most recently from 3a74c8d to 8cd50d8 Compare January 24, 2023 14:24

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch from 8cd50d8 to 6fc348d Compare January 24, 2023 18:53

dunxen commented Jan 24, 2023

View reviewed changes

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

dunxen force-pushed the 2022-11-config-minfinalcltvexpiry branch from 6fc348d to 5b53670 Compare January 24, 2023 19:02

wpaulino approved these changes Jan 24, 2023

View reviewed changes

TheBlueMatt approved these changes Jan 24, 2023

View reviewed changes

TheBlueMatt merged commit 8bb6614 into lightningdevkit:main Jan 24, 2023

dunxen deleted the 2022-11-config-minfinalcltvexpiry branch February 2, 2023 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configurable `min_final_cltv_expiry_delta` #1878

Allow configurable `min_final_cltv_expiry_delta` #1878

dunxen commented Nov 28, 2022 •

edited

Loading

TheBlueMatt commented Dec 13, 2022

dunxen commented Dec 13, 2022

dunxen commented Dec 13, 2022

shamardy commented Dec 13, 2022

dunxen commented Dec 13, 2022

TheBlueMatt left a comment

TheBlueMatt left a comment

dunxen commented Dec 16, 2022

codecov-commenter commented Dec 16, 2022 •

edited

Loading

TheBlueMatt commented Dec 18, 2022

dunxen commented Dec 18, 2022

dunxen commented Dec 19, 2022 •

edited

Loading

TheBlueMatt commented Dec 19, 2022

dunxen commented Dec 19, 2022

TheBlueMatt commented Dec 19, 2022

TheBlueMatt commented Dec 21, 2022

dunxen commented Jan 4, 2023

dunxen commented Jan 17, 2023

wpaulino Jan 17, 2023

dunxen Jan 17, 2023

wpaulino commented Jan 20, 2023

TheBlueMatt left a comment

TheBlueMatt commented Jan 24, 2023

wpaulino Jan 24, 2023

TheBlueMatt left a comment

TheBlueMatt Jan 24, 2023

wpaulino Jan 24, 2023

Allow configurable min_final_cltv_expiry_delta #1878

Allow configurable min_final_cltv_expiry_delta #1878

Conversation

dunxen commented Nov 28, 2022 • edited Loading

TheBlueMatt commented Dec 13, 2022

dunxen commented Dec 13, 2022

dunxen commented Dec 13, 2022

shamardy commented Dec 13, 2022

dunxen commented Dec 13, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

dunxen commented Dec 16, 2022

codecov-commenter commented Dec 16, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Dec 18, 2022

dunxen commented Dec 18, 2022

dunxen commented Dec 19, 2022 • edited Loading

TheBlueMatt commented Dec 19, 2022

dunxen commented Dec 19, 2022

TheBlueMatt commented Dec 19, 2022

TheBlueMatt commented Dec 21, 2022

dunxen commented Jan 4, 2023

dunxen commented Jan 17, 2023

wpaulino Jan 17, 2023

Choose a reason for hiding this comment

dunxen Jan 17, 2023

Choose a reason for hiding this comment

wpaulino commented Jan 20, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 24, 2023

wpaulino Jan 24, 2023

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Jan 24, 2023

Choose a reason for hiding this comment

wpaulino Jan 24, 2023

Choose a reason for hiding this comment

Allow configurable `min_final_cltv_expiry_delta` #1878

Allow configurable `min_final_cltv_expiry_delta` #1878

dunxen commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Dec 16, 2022 •

edited

Loading

dunxen commented Dec 19, 2022 •

edited

Loading