Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have get_route and Route take RouteParameters #2555

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Sep 6, 2023

These are the first few commits of #2417 split out to it's own PR in order to avoid having too many rebase conflicts.

@tnull tnull force-pushed the 2023-08-have-get-route-take-params branch 3 times, most recently from e0c032e to b092157 Compare September 6, 2023 09:30
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 96.16% and project coverage change: +0.55% 🎉

Comparison is base (073f078) 90.59% compared to head (b092157) 91.15%.
Report is 10 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2555      +/-   ##
==========================================
+ Coverage   90.59%   91.15%   +0.55%     
==========================================
  Files         110      110              
  Lines       57509    61782    +4273     
  Branches    57509    61782    +4273     
==========================================
+ Hits        52101    56315    +4214     
- Misses       5408     5467      +59     
Files Changed Coverage Δ
lightning/src/ln/onion_route_tests.rs 98.23% <ø> (-0.12%) ⬇️
lightning/src/ln/onion_utils.rs 91.78% <ø> (ø)
lightning/src/ln/functional_test_utils.rs 92.75% <87.50%> (+3.76%) ⬆️
lightning/src/routing/router.rs 94.33% <95.21%> (+0.12%) ⬆️
lightning-invoice/src/utils.rs 97.85% <100.00%> (+0.18%) ⬆️
lightning/src/ln/channelmanager.rs 89.70% <100.00%> (+2.50%) ⬆️
lightning/src/ln/functional_tests.rs 98.30% <100.00%> (+0.12%) ⬆️
lightning/src/ln/outbound_payment.rs 89.55% <100.00%> (-0.19%) ⬇️
lightning/src/ln/payment_tests.rs 97.45% <100.00%> (-0.06%) ⬇️
lightning/src/ln/shutdown_tests.rs 98.53% <100.00%> (+<0.01%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt
TheBlueMatt previously approved these changes Sep 6, 2023
lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved
lightning/src/routing/router.rs Show resolved Hide resolved
lightning/src/routing/router.rs Show resolved Hide resolved
lightning/src/routing/router.rs Outdated Show resolved Hide resolved
lightning/src/routing/router.rs Outdated Show resolved Hide resolved
lightning/src/routing/router.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking, thanks for undertaking this annoying sidequest 😛

jkczyz
jkczyz previously approved these changes Sep 6, 2023
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also nothing worth blocking.

lightning/src/routing/router.rs Show resolved Hide resolved
lightning/src/routing/router.rs Outdated Show resolved Hide resolved
lightning/src/routing/router.rs Outdated Show resolved Hide resolved
lightning/src/routing/router.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2023-08-have-get-route-take-params branch from b092157 to 543674f Compare September 6, 2023 17:35
@tnull
Copy link
Contributor Author

tnull commented Sep 6, 2023

Addressed feedback and force-pushed with the following changes:

> git diff-tree -U2 b0921575 543674f1
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 16e05db6..a43cfae9 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -1856,6 +1856,6 @@ pub fn get_route(send_node: &Node, route_params: &RouteParameters) -> Result<Rou
 macro_rules! get_route {
        ($send_node: expr, $payment_params: expr, $recv_value: expr) => {{
-                       let route_params = $crate::routing::router::RouteParameters::from_payment_params_and_value($payment_params, $recv_value);
-                       $crate::ln::functional_test_utils::get_route(&$send_node, &route_params)
+               let route_params = $crate::routing::router::RouteParameters::from_payment_params_and_value($payment_params, $recv_value);
+               $crate::ln::functional_test_utils::get_route(&$send_node, &route_params)
        }}
 }
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 63b2238d..9c5fe8e1 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -338,7 +338,9 @@ pub struct Route {
        /// the same.
        pub paths: Vec<Path>,
-       /// The `route_params` parameter passed [`find_route`].
+       /// The `route_params` parameter passed to [`find_route`].
        ///
        /// This is used by `ChannelManager` to track information which may be required for retries.
+       ///
+       /// Will be `None` for objects serialized with LDK versions prior to 0.0.117.
        pub route_params: Option<RouteParameters>,
 }
@@ -349,5 +351,5 @@ impl Route {
        /// For objects serialized with LDK 0.0.117 and after, this includes any extra payment made to
        /// the recipient, which can happen in excess of the amount passed to [`find_route`] via
-       /// [`RouteParameters::final_value_msat`], if we had to reach the [`htlc_minimum_msat`] limit.
+       /// [`RouteParameters::final_value_msat`], if we had to reach the [`htlc_minimum_msat`] limits.
        ///
        /// [`htlc_minimum_msat`]: https://github.com/lightning/bolts/blob/master/07-routing-gossip.md#the-channel_update-message
@@ -361,5 +363,5 @@ impl Route {
        ///
        /// Might be more than requested as part of the given [`RouteParameters::final_value_msat`] if
-       /// we had to reach the [`htlc_minimum_msat`] limit.
+       /// we had to reach the [`htlc_minimum_msat`] limits.
        ///
        /// [`htlc_minimum_msat`]: https://github.com/lightning/bolts/blob/master/07-routing-gossip.md#the-channel_update-message
@@ -393,7 +395,7 @@ impl Writeable for Route {
                }
                write_tlv_fields!(writer, {
-                       // For compatibility with LDK versions prior to 0.0.117, we the individual
+                       // For compatibility with LDK versions prior to 0.0.117, we take the individual
                        // RouteParameters' fields and reconstruct them on read.
-                       (1, self.route_params.as_ref().map(|p| p.payment_params.clone()), option),
+                       (1, self.route_params.as_ref().map(|p| &p.payment_params), option),
                        (2, blinded_tails, optional_vec),
                        (3, self.route_params.as_ref().map(|p| p.final_value_msat), option),
@@ -434,5 +436,5 @@ impl Readable for Route {
                }

-               // If we previously wrote, the corresponding fields, reconstruct RouteParameters.
+               // If we previously wrote the corresponding fields, reconstruct RouteParameters.
                let route_params = match (payment_params, final_value_msat) {
                        (Some(payment_params), Some(final_value_msat)) => {
diff --git a/pending_changelog/routes_route_params.txt b/pending_changelog/routes_route_params.txt
new file mode 100644
index 00000000..e88a1c78
--- /dev/null
+++ b/pending_changelog/routes_route_params.txt
@@ -0,0 +1,3 @@
+# Backwards Compatibility
+
+- `Route` objects written with LDK versions prior to 0.0.117 won't be retryable after being deserialized with LDK 0.0.117 or above.

@TheBlueMatt TheBlueMatt merged commit 072a6ff into lightningdevkit:main Sep 6, 2023
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants