Fix Route serialization round-trip
#2897
Conversation
|
Warning Rate Limit Exceeded@TheBlueMatt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 40 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent modifications aim to refine the handling of routing fees within the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lightning/src/routing/router.rs (1 hunks)
- lightning/src/util/test_utils.rs (1 hunks)
Additional comments: 2
lightning/src/util/test_utils.rs (1)
- 154-165: The added code segment in
TestRouter'sfind_routemethod correctly implements the round-trip serialization test for routes. This change addresses the PR's objective to enhance testing for serialization issues. The serialization and deserialization process is tested by encoding the route and then decoding it, asserting equality to ensure the round-trip maintains integrity. This is a crucial addition for catching serialization issues early and preventing similar bugs in the future.However, there are a few points to consider:
- The use of
unwrapin tests is generally acceptable for prototyping or if you're certain the operation won't fail. However, in a library context, especially one as critical as a routing component in a payment network, it might be beneficial to handle errors gracefully even in tests to ensure test failures are clear and actionable.- The comment on line 161 has a typo: "hwere" should be corrected to "where".
- It's good practice to include a brief explanation or reference to the specific issue this test is designed to catch, ensuring future maintainers understand the context and importance of this test case.
Overall, the change is aligned with the PR's objectives and significantly improves the robustness of the serialization process for
Routeobjects.However, consider refining error handling in tests and correcting the typo in the comment for clarity and maintainability.
lightning/src/routing/router.rs (1)
- 413-413: The change from using
maptoand_thenfor accessingmax_total_routing_fee_msatinroute_paramsis a crucial fix for the serialization issue described. This adjustment ensures thatmax_total_routing_fee_msatis correctly serialized asOption<u64>, addressing the problem of unintended nesting. It's important to verify that this change aligns well with the overall serialization and deserialization processes ofRouteobjects to ensure no unintended side effects occur.
TheBlueMatt
force-pushed
the
2024-02-fix-route-ser
branch
2 times, most recently
from
c847773
to
eda574d
Compare
|
|
||
| self.router.find_route(payer, params, first_hops, inflight_htlcs) | ||
| if let Ok(route) = &route_res { |
There was a problem hiding this comment.
Wonder if this needs to happen for all our tests. Wouldn't it suffice (and be quicker) to add a single test case for this?
There was a problem hiding this comment.
Maybe, but this gets us a lot more coverage, and potentially gets us coverage of new fields in the future "for free", vs a freestanding test does not.
There was a problem hiding this comment.
Seems apart from the bug in max_total_routing_fee_msat serialization there is also another issue with serializing the BlindedTails: currently, we seem to write one tail per hop, but expect to read one tail per path (which makes sense). Not sure if there was a reason for the prior scheme, but writing once per path seems to fix it for me, see below (cc @valentinewallace).
There is also a minor test failure due to a BOLT12 test expecting to find an empty route, which we however refuse to read. Expecting to find the empty route might also be a mistake here? (cc @jkczyz)
This diff makes makes tests pass for me:
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index c260bac85..be029ae8d 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -2314,14 +2314,6 @@ mod tests {
);
assert!(outbound_payments.has_pending_payments());
- let route_params = RouteParameters::from_payment_params_and_value(
- PaymentParameters::from_bolt12_invoice(&invoice),
- invoice.amount_msats(),
- );
- router.expect_find_route(
- route_params.clone(), Ok(Route { paths: vec![], route_params: Some(route_params) })
- );
-
assert_eq!(
outbound_payments.send_payment_for_bolt12_invoice(
&invoice, payment_id, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager,
@@ -2332,7 +2324,7 @@ mod tests {
assert!(!outbound_payments.has_pending_payments());
let payment_hash = invoice.payment_hash();
- let reason = Some(PaymentFailureReason::UnexpectedError);
+ let reason = Some(PaymentFailureReason::RouteNotFound);
assert!(!pending_events.lock().unwrap().is_empty());
assert_eq!(
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index d8a5ba122..5b555b3b2 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -507,18 +507,10 @@ impl Writeable for Route {
let mut blinded_tails = Vec::new();
for path in self.paths.iter() {
(path.hops.len() as u8).write(writer)?;
- for (idx, hop) in path.hops.iter().enumerate() {
+ for hop in &path.hops {
hop.write(writer)?;
- if let Some(blinded_tail) = &path.blinded_tail {
- if blinded_tails.is_empty() {
- blinded_tails = Vec::with_capacity(path.hops.len());
- for _ in 0..idx {
- blinded_tails.push(None);
- }
- }
- blinded_tails.push(Some(blinded_tail));
- } else if !blinded_tails.is_empty() { blinded_tails.push(None); }
}
+ blinded_tails.push(&path.blinded_tail);
}
write_tlv_fields!(writer, {
// For compatibility with LDK versions prior to 0.0.117, we take the individual
|
One thought: since we wrote bogus data anyways and broke serialization, could we now make the best of it and have |
Sadly, I think the one user who does use this logic may want that :( |
|
`fails_paying_for_bolt12_invoice` tests that we fail to send a payment if the router returns `Ok` but includes a bogus route (one with 0-length paths). While this marginally increases our test coverage, in the next commit we'll be testing that all routes round-trip serialization, which fails here as bogus routes are not supported in deserialization. Because this isn't particularly critical test coverage, we simply opt to drop the test entirely here.
`Route`'s blinded_path serialization logic writes a blinded path `Option` per path hop, however on read we (correctly) only read one blinded path `Option` per path. This causes serialization of `Route`s with blinded paths to fail to round-trip. Here we fix this by writing blinded paths per path.
TheBlueMatt
force-pushed
the
2024-02-fix-route-ser
branch
from
eda574d
to
ed66196
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/outbound_payment.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
- lightning/src/util/test_utils.rs (2 hunks)
Files skipped from review due to trivial changes (1)
- lightning/src/ln/outbound_payment.rs
Files skipped from review as they are similar to previous changes (2)
- lightning/src/routing/router.rs
- lightning/src/util/test_utils.rs
|
Note that we can't swap to just |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2897 +/- ##
========================================
Coverage 89.11% 89.12%
========================================
Files 115 115
Lines 94232 94354 +122
Branches 94232 94354 +122
========================================
+ Hits 83978 84095 +117
- Misses 7781 7787 +6
+ Partials 2473 2472 -1 ☔ View full report in Codecov by Sentry. |
When the `max_total_routing_fee_msat` parameter was added to `RouteParameters`, the serialization used `map` to get the max fee, accidentally writing an `Option<Option<u64>>`, but then read it as an `Option<u64>`. Thus, any `Route`s with a `route_params` written will fail to be read back. Luckily, this is an incredibly rarely-used bit of code, so only one user managed to hit it.
This adds testing for the previous two commits by testing that all routes generated in testing are able to survive a serialization round-trip.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/util/test_utils.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- lightning/src/util/test_utils.rs
TheBlueMatt
force-pushed
the
2024-02-fix-route-ser
branch
from
38a01ed
to
24d02ff
Compare
|
Redid the commit to make testing a separate commit to aid backporting. |
When the
max_total_routing_fee_msatparameter was added toRouteParameters, the serialization usedmapto get the max fee, accidentally writing anOption<Option<u64>>, but then read it as anOption<u64>. Thus, anyRoutes with aroute_paramswritten will fail to be read back.Luckily, this is an incredibly rarely-used bit of code, so only one user managed to hit it.