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

 // 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

 			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.

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 TheBlueMatt and wpaulino 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 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

Allow configurable min_final_cltv_expiry_delta #1878

Allow configurable min_final_cltv_expiry_delta #1878

Uh oh!

Conversation

dunxen commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Dec 13, 2022

Uh oh!

dunxen commented Dec 13, 2022

Uh oh!

dunxen commented Dec 13, 2022

Uh oh!

shamardy commented Dec 13, 2022

Uh oh!

dunxen commented Dec 13, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dunxen commented Dec 16, 2022

Uh oh!

codecov-commenter commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Dec 18, 2022

Uh oh!

dunxen commented Dec 18, 2022

Uh oh!

dunxen commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Dec 19, 2022

Uh oh!

dunxen commented Dec 19, 2022

Uh oh!

TheBlueMatt commented Dec 19, 2022

Uh oh!

TheBlueMatt commented Dec 21, 2022

Uh oh!

dunxen commented Jan 4, 2023

Uh oh!

dunxen commented Jan 17, 2023

Uh oh!

Uh oh!

wpaulino Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

dunxen Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Jan 20, 2023

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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