Remove the final_cltv_expiry_delta in RouteParameters entirely #2015

TheBlueMatt · 2023-02-06T22:26:25Z

fbc0847 purported to "move" the
final_cltv_expiry_delta field to PaymentParamters from
RouteParameters. However, for naive backwards-compatibility
reasons it left the existing on in place and only added a new,
redundant field in PaymentParameters.

It turns out there's really no reason for this - if we take a more
critical eye towards backwards compatibility we can figure out the
correct value in every PaymentParameters while deserializing.

We do this here - making PaymentParameters a ReadableArgs
taking a "default" cltv_expiry_delta when it goes to read. This
allows existing RouteParameters objects to pass the read
final_cltv_expiry_delta field in to be used if the new field
wasn't present.

Sadly, if we want to do this, we have to do it in the same release as the above commit, so it has to land for 114 :(

valentinewallace · 2023-02-14T19:27:46Z

Needs rebase

TheBlueMatt · 2023-02-16T01:05:09Z

Rebased.

dunxen · 2023-02-20T08:03:55Z

Looks like fuzz stuff needs updating

diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 9b3b76c2..3d9be984 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -513,7 +513,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
                                let params = RouteParameters {
                                        payment_params,
                                        final_value_msat,
-                                       final_cltv_expiry_delta: 42,
                                };
                                let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes();
                                let route = match find_route(&our_id, &params, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) {
@@ -537,7 +536,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
                                let params = RouteParameters {
                                        payment_params,
                                        final_value_msat,
-                                       final_cltv_expiry_delta: 42,
                                };
                                let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes();
                                let mut route = match find_route(&our_id, &params, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) {
diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs
index a7c50de4..66d68040 100644
--- a/fuzz/src/router.rs
+++ b/fuzz/src/router.rs
@@ -302,7 +302,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                                                payment_params: PaymentParameters::from_node_id(*target, final_cltv_expiry_delta)
                                                        .with_route_hints(last_hops.clone()),
                                                final_value_msat,
-                                               final_cltv_expiry_delta,
                                        };
                                        let _ = find_route(&our_pubkey, &route_params, &net_graph,
                                                first_hops.map(|c| c.iter().collect::<Vec<_>>()).as_ref().map(|a| a.as_slice()),

TheBlueMatt · 2023-02-21T17:09:07Z

Oops, thanks. Also rebased.

valentinewallace · 2023-02-21T16:53:59Z

lightning/src/routing/router.rs

+			(9, final_cltv_expiry_delta, (default_value, default_final_cltv_expiry_delta)),
+		});
+		Ok(Self {
+			payee_pubkey: payee_pubkey.0.unwrap(),


Wouldn't it provide better compile-time checks to use _init_tlv_based_struct_field ?

Yea, I guess, I mean it currently does this but I agree if we ever change it we don't want this to blow up.

lightning/src/routing/router.rs

valentinewallace · 2023-02-21T17:16:44Z

lightning/src/routing/router.rs

+	pub final_cltv_expiry_delta: u32,
+}
+
+impl Writeable for PaymentParameters {


Why not keep using the macro but default to 0 (or MIN_CLTV_EXPIRY_DELTA), so the field can be set by higher-level reads? Do we expect users to be serializing this struct?

If a user has a RouteParameters serialized in 113, eg because of a payment failure, and they retry it on restart in 114, we want to make sure the final_cltv_expiry_delta parameter moves over to the right place. Because its a given parameter I dont think we want to just assume its some specific value.

Hmm, if we kept PayParams using the ser macro with a default value, RouteParams deser would always ensure it sets the correct value, though. I see why we need to break out of the macro for RouteParams, but just not sure I understand why we need to break out for PaymentParams.

If a user has a RouteParameters serialized in 113, eg because of a payment failure, and they retry it on restart in 114

+ retries on restart aren't supported in 114 (do you mean using send_payment?).

The value read from the old field in RouteParameters needs to make it into the PaymentParameters somehow on read, I'm not sure how we do that without breaking up the macro as well?

retries on restart aren't supported in 114 (do you mean using send_payment?).

Right, I guess I mean if you're using the PaymentPathFailed data to retry? I guess it borderline doesn't matter but it does feel weird to synthesize the wrong value in the serialized event.

We wouldn't be synthesizing the wrong value, though, IIUC. Shouldn't this code in RouteParams deser cover this case, if they do want to use PaymentPathFailed::retry?

let mut payment_params: PaymentParameters = payment_params.0.unwrap(); if payment_params.final_cltv_expiry_delta == 0 { payment_params.final_cltv_expiry_delta = final_cltv_expiry_delta.0.unwrap(); }

Another thought could be removing retry entirely, which we want to do at some point anyway, maybe even more ser complexity introduced there though.

Anyway, we may be talking past each other, and it doesn't matter that much though the broken-up macro is somewhat ugly, so I won't die on this hill :)

Ah, okay, I indeed didn't understand your suggestion here - yes, I think all the places we read a RouteParameters in LDK we are happy with the special-case of 0 for min_final_cltv_delta being treated as "no value" and overwriting it. However, that's...weird in a public API? Like, there's an implicit Option, there, basically, and we can't have an actual Option there because the router can't deal with it. So I'd kinda rather force the caller to pass the value in via the ReadableArgs implementation rather than have an implicit Option?

Yeah, just seems unlikely this struct would ever be serialized on its own, even if it's technically possible. Fine to leave as-is

valentinewallace

LGTM

lightning/src/routing/router.rs

codecov-commenter · 2023-02-22T18:14:16Z

Codecov Report

Base: 87.26% // Head: 87.42% // Increases project coverage by +0.15% 🎉

Coverage data is based on head (a4d85db) compared to base (16b3c72).
Patch coverage: 58.00% of modified lines in pull request are covered.

❗ Current head a4d85db differs from pull request most recent head f03b7cd. Consider uploading reports for the commit f03b7cd to get more accurate results

📣 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    #2015      +/-   ##
==========================================
+ Coverage   87.26%   87.42%   +0.15%     
==========================================
  Files         101      101              
  Lines       44414    47347    +2933     
  Branches    44414    47347    +2933     
==========================================
+ Hits        38759    41394    +2635     
- Misses       5655     5953     +298

Impacted Files	Coverage Δ
lightning-invoice/src/payment.rs	`75.78% <ø> (+0.03%)`	⬆️
lightning-invoice/src/utils.rs	`96.14% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.33% <ø> (+0.04%)`	⬆️
lightning/src/ln/outbound_payment.rs	`80.56% <ø> (+1.49%)`	⬆️
lightning/src/util/ser.rs	`82.92% <0.00%> (-1.80%)`	⬇️
lightning/src/util/ser_macros.rs	`92.69% <ø> (ø)`
lightning/src/routing/router.rs	`92.24% <56.25%> (-0.54%)`	⬇️
lightning/src/ln/channelmanager.rs	`89.10% <63.63%> (+2.56%)`	⬆️
lightning/src/ln/payment_tests.rs	`95.65% <100.00%> (+<0.01%)`	⬆️
lightning/src/sync/nostd_sync.rs	`37.50% <0.00%> (-5.00%)`	⬇️
... and 35 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.

dunxen · 2023-02-22T19:00:13Z

Looks good for squash

alecchendev

Don't have much to add, just learned a lot about macros while trying to understand what this PR was doing.

One comprehension check: Is the main reason that RouteParameters even needs to be serializable in the first place because it's included in Event::PaymentFailed which may be persisted (because a user would want to respond to the event after a crash)?

valentinewallace · 2023-02-22T22:48:02Z

Is the main reason that RouteParameters even needs to be serializable in the first place because it's included in Event::PaymentFailed which may be persisted (because a user would want to respond to the event after a crash)?

We're actually planning to get rid of PaymentPathFailed::retry, so seems it's mostly serializable for historical reasons or in case users want to store it for whatever reason. PaymentParameters definitely need to be serializable because it's serialized in ChannelManager

TheBlueMatt · 2023-02-22T23:07:51Z

Squashed fixups down.

valentinewallace · 2023-02-24T19:50:56Z

Needs rebase

TheBlueMatt · 2023-02-24T20:32:42Z

Rebased.

TheBlueMatt · 2023-02-27T18:35:20Z

Rebased.

valentinewallace · 2023-02-27T21:37:18Z

Some rebase errors need fixing sadly

TheBlueMatt · 2023-02-27T22:31:03Z

Sorry was waiting on #2055

When we read a `Route` (or a list of `RouteHop`s), we should never have zero paths or zero `RouteHop`s in a path. As such, its fine to simply reject these at deserialization-time. Technically this could lead to something which we can generate not round-trip'ing serialization, but that seems okay here.

This adds `required` support for trait-wrapped reading (e.g. for objects read via `ReadableArgs`) as well as support for the trait-wrapped reading syntax across the TLV struct/enum serialization macros.

fbc0847 purported to "move" the `final_cltv_expiry_delta` field to `PaymentParamters` from `RouteParameters`. However, for naive backwards-compatibility reasons it left the existing on in place and only added a new, redundant field in `PaymentParameters`. It turns out there's really no reason for this - if we take a more critical eye towards backwards compatibility we can figure out the correct value in every `PaymentParameters` while deserializing. We do this here - making `PaymentParameters` a `ReadableArgs` taking a "default" `cltv_expiry_delta` when it goes to read. This allows existing `RouteParameters` objects to pass the read `final_cltv_expiry_delta` field in to be used if the new field wasn't present.

dunxen · 2023-02-28T07:20:46Z

Phew. What a rebase rally! :P

TheBlueMatt · 2023-02-28T07:55:49Z

Heh, it wasn't that important compared to the things it conflicted with 🤷‍♂️

TheBlueMatt added this to the 0.0.114 milestone Feb 6, 2023

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch 2 times, most recently from f106a82 to 76a1530 Compare February 16, 2023 01:04

dunxen self-assigned this Feb 20, 2023

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch from 76a1530 to 1e1cb91 Compare February 21, 2023 17:09

valentinewallace reviewed Feb 21, 2023

View reviewed changes

valentinewallace reviewed Feb 22, 2023

View reviewed changes

lightning/src/routing/router.rs Show resolved Hide resolved

alecchendev reviewed Feb 22, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch from a4d85db to de3fab7 Compare February 22, 2023 23:07

valentinewallace previously approved these changes Feb 22, 2023

View reviewed changes

TheBlueMatt dismissed valentinewallace’s stale review via 102e088 February 24, 2023 20:32

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch from de3fab7 to 102e088 Compare February 24, 2023 20:32

valentinewallace previously approved these changes Feb 27, 2023

View reviewed changes

TheBlueMatt dismissed valentinewallace’s stale review via d2307db February 27, 2023 18:27

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch from 102e088 to d2307db Compare February 27, 2023 18:27

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch from d2307db to 28a94c1 Compare February 27, 2023 18:35

Support ReadableArgs types across in the TLV struct serialization

6aa5ebb

This adds `required` support for trait-wrapped reading (e.g. for objects read via `ReadableArgs`) as well as support for the trait-wrapped reading syntax across the TLV struct/enum serialization macros.

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch from 28a94c1 to 280f087 Compare February 27, 2023 22:31

TheBlueMatt force-pushed the 2023-02-no-dumb-redundant-fields branch from 280f087 to f03b7cd Compare February 27, 2023 22:33

valentinewallace approved these changes Feb 27, 2023

View reviewed changes

dunxen approved these changes Feb 28, 2023

View reviewed changes

dunxen removed their assignment Feb 28, 2023

naumenkogs approved these changes Feb 28, 2023

View reviewed changes

TheBlueMatt merged commit b8bea74 into lightningdevkit:main Feb 28, 2023

valentinewallace mentioned this pull request Mar 3, 2023

Cut 0.0.114 #2065

Merged

Remove the final_cltv_expiry_delta in RouteParameters entirely #2015

Remove the final_cltv_expiry_delta in RouteParameters entirely #2015

Uh oh!

Conversation

TheBlueMatt commented Feb 6, 2023

Uh oh!

valentinewallace commented Feb 14, 2023

Uh oh!

TheBlueMatt commented Feb 16, 2023

Uh oh!

dunxen commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Feb 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dunxen commented Feb 22, 2023

Uh oh!

alecchendev left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Feb 22, 2023

Uh oh!

TheBlueMatt commented Feb 22, 2023

Uh oh!

valentinewallace commented Feb 24, 2023

Uh oh!

TheBlueMatt commented Feb 24, 2023

Uh oh!

TheBlueMatt commented Feb 27, 2023

Uh oh!

valentinewallace commented Feb 27, 2023

Uh oh!

TheBlueMatt commented Feb 27, 2023

Uh oh!

dunxen commented Feb 28, 2023

Uh oh!

TheBlueMatt commented Feb 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dunxen commented Feb 20, 2023 •

edited

Loading

codecov-commenter commented Feb 22, 2023 •

edited

Loading