Avoid retrying over previously failed blinded paths #2818
Conversation
valentinewallace
force-pushed
the
2024-01-blinded-path-retries
branch
from
228edfa
to
9575735
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2818 +/- ##
==========================================
+ Coverage 88.42% 89.36% +0.93%
==========================================
Files 114 114
Lines 91946 97543 +5597
Branches 91946 97543 +5597
==========================================
+ Hits 81304 87168 +5864
+ Misses 8129 7971 -158
+ Partials 2513 2404 -109 ☔ View full report in Codecov by Sentry. |
valentinewallace
force-pushed
the
2024-01-blinded-path-retries
branch
from
9575735
to
5092908
Compare
valentinewallace
force-pushed
the
2024-01-blinded-path-retries
branch
from
5092908
to
1a7f79d
Compare
WalkthroughThe updates focus on improving the handling of blinded payment paths within the Lightning Network's codebase. New functionalities include tracking payment failure reasons, refining the process of establishing blinded payment paths, and enhancing route parameter construction. Additionally, there's an emphasis on retry logic for failed payment paths and better error handling within the onion routing mechanism, specifically noting when failures occur within blinded paths. Changes
Poem
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 (4)
- lightning/src/ln/blinded_payment_tests.rs (4 hunks)
- lightning/src/ln/onion_utils.rs (9 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/routing/router.rs (9 hunks)
Additional comments: 28
lightning/src/ln/blinded_payment_tests.rs (4)
13-13: The addition of
PaymentFailureReasonto theeventsmodule is a good practice as it allows for more descriptive error handling in the payment process.59-78: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [30-66]
The refactoring of the
blinded_payment_pathfunction to return a tuple ofBlindedPayInfoandBlindedPathis a significant change. Ensure that all usages of this function have been updated to handle the new return type.
- 68-75: The
get_blinded_route_parametersfunction has been modified to use the refactoredblinded_payment_pathfunction. This change should be verified to ensure that theRouteParametersare constructed correctly with the new return type ofblinded_payment_path.Verification successful
The
get_blinded_route_parametersfunction correctly uses the tuple returned byblinded_payment_pathto constructRouteParameters. Theblinded_payment_pathfunction returns a tuple(BlindedPayInfo, BlindedPath), which is then passed intoPaymentParameters::blindedwithin avec![]to create theRouteParameters.* 712-815: The new test `blinded_path_retries` is added to verify the retry behavior of blinded payment paths. It's important to ensure that this test covers all relevant scenarios, including the failure and retry mechanisms. The test seems comprehensive, but it's crucial to check that it fails when expected and that the retry logic is sound.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `get_blinded_route_parameters` correctly constructs `RouteParameters` using the new `blinded_payment_path` return type. ast-grep --lang rust --pattern $'get_blinded_route_parameters($_, $_, $_, $_, $_)'Length of output: 2489
Script:
#!/bin/bash # Check the implementation of `get_blinded_route_parameters` and the return type of `blinded_payment_path` rg "fn get_blinded_route_parameters" lightning/src/ln/blinded_payment_tests.rs rg "fn blinded_payment_path" lightning/src/ln/blinded_payment_tests.rsLength of output: 215
Script:
#!/bin/bash # Retrieve the full implementation of `get_blinded_route_parameters` and `blinded_payment_path` to verify the return type and usage. rg -A 10 "fn get_blinded_route_parameters" lightning/src/ln/blinded_payment_tests.rs rg -A 10 "fn blinded_payment_path" lightning/src/ln/blinded_payment_tests.rsLength of output: 1061
Verification successful
The
blinded_path_retriestest function has been successfully located and its contents displayed. The test sets up a network topology, creates channels, and simulates a payment process with retries over blinded paths. It includes assertions to check the correct behavior when a payment fails and when all blinded paths have been exhausted.The test appears to cover:
- The setup of a network with a specific topology for testing blinded paths.
- The creation of a payment with blinded route hints.
- The simulation of a payment failure through a blinded path.
- The handling of a failed payment and the processing of the retry logic.
- The assertion that the payment fails permanently when all blinded paths have been tried and no more retries are available.
Based on the provided output, the test seems to be comprehensive in terms of testing the retry behavior of blinded payment paths, including the failure and retry mechanisms. It checks for both temporary failure (allowing for a retry) and permanent failure (when no more retries are possible).
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new test `blinded_path_retries` is comprehensive and covers all relevant scenarios. ast-grep --lang rust --pattern $'fn blinded_path_retries()'Length of output: 60
Script:
#!/bin/bash # Adjusting the pattern to match the function definition correctly. ast-grep --lang rust --pattern $'fn blinded_path_retries($_) { $$$ }'Length of output: 70
Script:
#!/bin/bash # Using rg to search for the function definition within the file. rg "fn blinded_path_retries"Length of output: 100
Script:
#!/bin/bash # Display the contents of the `blinded_path_retries` function to examine its comprehensiveness. cat lightning/src/ln/blinded_payment_tests.rs | sed -n '/fn blinded_path_retries()/,/^}/p'Length of output: 4727
lightning/src/ln/onion_utils.rs (10)
432-432: The addition of the
failed_within_blinded_pathfield to theDecodedOnionFailurestruct is consistent with the PR's objective to track failures within blinded paths.467-467: The
failed_within_blinded_pathfield is correctly initialized within theFailureLearningsstruct, which is used to capture information about the failure for further processing.493-494: The
failed_within_blinded_pathfield is correctly set totruewhen an error is detected from within a blinded route, which aligns with the PR's goal.526-527: Again, the
failed_within_blinded_pathfield is set totruewhen the failing hop is within a multi-hop blinded path, which is consistent with the intended logic.557-558: The
failed_within_blinded_pathfield is set tofalsewhen the error is not related to a blinded path, which is correct behavior.714-715: The
failed_within_blinded_pathfield is correctly assigned to theDecodedOnionFailurestruct based on theFailureLearningsstruct, ensuring that the information about the failure is preserved.729-729: The
failed_within_blinded_pathfield is correctly included in the construction of theDecodedOnionFailurestruct, which is the final step in processing the onion failure.740-740: The
failed_within_blinded_pathfield is set tofalseby default when no specific failure learnings are available, which is a safe default.888-888: The
failed_within_blinded_pathfield is correctly set tofalsewhen decoding a failure from the first hop, as this is not a blinded path scenario.723-732: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [432-888]
The changes made to the
DecodedOnionFailurestruct and theprocess_onion_failurefunction are consistent with the PR's objectives to enhance the reliability of blinded routes by tracking and avoiding failed paths. Thefailed_within_blinded_pathfield has been correctly added and utilized throughout the relevant functions to ensure that once a blinded path fails, it is not retried in subsequent payment attempts.lightning/src/ln/outbound_payment.rs (6)
22-22: The import of
BlindedTailon line 22 suggests that there are changes related to blinded paths, which aligns with the PR's objectives.132-136: The method
insert_previously_failed_blinded_pathon lines 132-136 is a new addition that allows marking a blinded path as failed. This is a key part of the PR's functionality to avoid retrying failed blinded paths.1612-1617: The code on lines 1612-1617 is under a
cfg(test)flag, which means it's only compiled for testing purposes. It's important to ensure that the test code is consistent with the production code logic. TheDecodedOnionFailurestruct is being used here, which is relevant to the handling of onion routing failures.1656-1661: Lines 1656-1661 handle the case where a failure occurs within a blinded path. This is a new check introduced in this PR, and it's crucial for the new functionality to track failures in blinded paths correctly.
1612-1617: The use of
cfg(test)on lines 1612-1617 indicates that the following code is only for testing purposes. It's important to ensure that the test code is consistent with the production code logic. TheDecodedOnionFailurestruct is being used here, which is relevant to the handling of onion routing failures.1656-1661: Lines 1656-1661 handle the case where a failure occurs within a blinded path. This is a new check introduced in this PR, and it's crucial for the new functionality to track failures in blinded paths correctly.
lightning/src/routing/router.rs (8)
715-718: The addition of
previously_failed_blinded_path_idxstoPaymentParametersis consistent with the PR's objective to track failed blinded paths. Ensure that the typeu64is appropriate for indexing and that there are no potential overflows or underflows when casting indices to this type.740-740: In the
Writeableimplementation forPaymentParameters, the new fieldpreviously_failed_blinded_path_idxsis serialized with a tag of11. Verify that this tag does not conflict with any existing serialization tags and that it follows the serialization format's guidelines.Verification successful
The executed script was intended to search for the use of tag
11in the serialization code withinrouter.rs. The absence of output from thergcommand suggests that there are no other instances of tag11being used in theWriteableimplementation forPaymentParametersor elsewhere in therouter.rsfile. This indicates that the tag11does not conflict with any existing serialization tags within that file.Based on the information gathered, it can be concluded that the use of tag
11for the fieldpreviously_failed_blinded_path_idxsin theWriteableimplementation forPaymentParametersdoes not conflict with any other serialization tags in therouter.rsfile.* 759-759: In the `ReadableArgs` implementation, `previously_failed_blinded_path_idxs` is marked as optional. This is a good approach for backward compatibility. However, ensure that the rest of the code correctly handles the case when this vector is not present in the data.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that tag 11 is not used by other fields in PaymentParameters serialization rg "write_tlv!.*\(11," src/routing/router.rsLength of output: 108
Script:
#!/bin/bash # First, find the correct path for `router.rs` fd router.rs # If the file exists, then search for the use of tag 11 in the serialization within `router.rs` # This command will be executed only if `router.rs` is found fd router.rs --exec rg "write_tlv!.*\(11,"Length of output: 110
Script:
#!/bin/bash # Directly search for the use of tag 11 in the serialization within `router.rs` rg "write_tlv!.*\(11," lightning/src/routing/router.rsLength of output: 56
783-783: The default value for
previously_failed_blinded_path_idxsis an empty vector, which is appropriate. Confirm that there are no scenarios where a non-empty default would be necessary.802-802: The initialization of
previously_failed_blinded_path_idxswith an empty vector in thenewandwith_max_channel_saturation_power_of_halfmethods is consistent and ensures that the field is properly initialized in all constructors.Also applies to: 841-841
918-929: The
insert_previously_failed_blinded_pathmethod adds an index topreviously_failed_blinded_path_idxs. Ensure that there is logic in place to prevent duplicate indices from being added, as this could lead to inefficiencies or unexpected behavior.1386-1394: The
blinded_hint_idxmethod provides a way to retrieve the index of a blinded hint. This is a utility method that will likely be used in multiple places. Confirm that the method is used consistently and that the returned index is always valid.2146-2154: The logic to check if a payment failed on a specific channel or blinded path is correct. It uses the new fields to determine if a channel or blinded path index should be avoided. Ensure that the
unwrapcall on line 2150 is safe and will not cause a panic in any scenario.Verification successful
The review comment has been verified. The
unwrap()call on line 2150 inrouter.rsis preceded by a check that ensures it is safe to use, as it is only called ifblinded_hint_idx().is_some()returnstrue. This means theunwrap()will not cause a panic in this scenario.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for all usages of blinded_hint_idx to ensure it's always called when Some rg "blinded_hint_idx\(\)" src/routing/router.rsLength of output: 111
Script:
#!/bin/bash # First, find the correct path for the router.rs file fd 'router.rs' # If the file is found, then search for the usages of blinded_hint_idx() # The output of fd will be used to ensure the correct file path is usedLength of output: 65
Script:
#!/bin/bash # Now that we have the correct path, search for the usages of blinded_hint_idx() in router.rs rg "blinded_hint_idx\(\)" lightning/src/routing/router.rsLength of output: 175
| if failed_blinded_tail.hops == path.blinded_hops && | ||
| failed_blinded_tail.blinding_point == path.blinding_point |
There was a problem hiding this comment.
In practice, would simply checking the blinding_point be sufficient? Or at very least only checking the blinded_node_id of each hop instead of all its data?
There was a problem hiding this comment.
In practice, that would most likely be fine. In practice there also shouldn't be too much data to compare, though.
Will be used to ensure correctness when we store previously failed blinded paths to avoid retrying over them.
Will be used to ensure correctness when we store previously failed blinded paths to avoid retrying over them.
Useful so we don't retry over these paths.
Useful so we don't retry over these paths.
valentinewallace
force-pushed
the
2024-01-blinded-path-retries
branch
from
1a7f79d
to
5c87f40
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/ln/blinded_payment_tests.rs (4 hunks)
- lightning/src/ln/onion_utils.rs (9 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/routing/router.rs (9 hunks)
Files skipped from review as they are similar to previous changes (4)
- lightning/src/ln/blinded_payment_tests.rs
- lightning/src/ln/onion_utils.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/routing/router.rs
This is important for production usage of blinded routes, for payment reliability.
Partially addresses #1970.