Test blinded forwarding #2823
Test blinded forwarding #2823
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2823 +/- ##
==========================================
+ Coverage 89.13% 89.29% +0.16%
==========================================
Files 117 117
Lines 94657 96565 +1908
Branches 94657 96565 +1908
==========================================
+ Hits 84369 86231 +1862
- Misses 7811 7868 +57
+ Partials 2477 2466 -11 ☔ View full report in Codecov by Sentry. |
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
2 times, most recently
from
0c61494
to
53de611
Compare
WalkthroughThe recent updates focus on refining argument passing and function calling within the codebase, particularly around payment forwarding and claiming logic. New structures like 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 (
|
|
Rebased. |
![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: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/ln/blinded_payment_tests.rs (14 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (3 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/blinded_payment_tests.rs: Error: Message exceeds token limit
Additional comments: 31
lightning/src/ln/reload_tests.rs (2)
- 825-830: The refactoring of
do_pass_along_pathto usePassAlongPathArgsstruct improves the readability and maintainability of the code by encapsulating the parameters into a single object. This change also allows for method chaining which can make the code more expressive.- 825-830: The changes in
do_test_partial_claim_before_restartreflect the refactoring to usePassAlongPathArgs. Ensure that the test still covers the intended scenarios and that the behavior ofdo_pass_along_pathremains consistent with the test's expectations.lightning/src/ln/blinded_payment_tests.rs (15)
- 46-73: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [30-70]
The function
get_blinded_route_parameterscorrectly sets upRouteParametersfor blinded route tests. It uses the provided node IDs and channel updates to construct a blinded path and then creates route parameters from it.
- 20-40: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [76-114]
The test
one_hop_blinded_pathis well-structured and covers both success and failure scenarios for a one-hop blinded path. It uses utility functions to create the network, channels, and route parameters, and then sends a payment through the blinded path.
- 169-230: The test
mpp_to_one_hop_blinded_pathcorrectly sets up a multi-part payment scenario and tests the forwarding and claiming of payments over a one-hop blinded path. It also checks for the correct number of message events, which is crucial for validating the multi-part payment logic.- 169-231: The test
mpp_to_three_hop_blinded_pathsis comprehensive and tests the multi-part payment over a more complex three-hop blinded path. It ensures that the payment is split correctly and that the claiming process works as expected.- 253-342: The test
forward_checks_failurethoroughly tests different failure modes related to forwarding checks. It manipulates the HTLCs to simulate various error conditions and ensures that the errors are handled correctly.- 365-371: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [368-423]
The test
failed_backwards_to_intro_nodecorrectly simulates a scenario where the payment fails at the final node and the error is propagated back to the introductory node. It checks that the error is handled correctly and that the payment is marked as failed.
- 461-540: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [429-531]
The test
forward_fail_in_process_pending_htlc_fwdsis well-structured and tests the error handling in theprocess_pending_htlc_forwardsfunction. It simulates different failure scenarios and ensures that the errors are propagated back correctly.
- 639-645: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [565-651]
The test
blinded_intercept_paymenteffectively tests the interception of a payment by an intermediate node. It covers both the successful forwarding and the failure to forward the intercepted payment, ensuring that the payment state is correctly updated in both cases.
- 642-642: The test
two_hop_blinded_path_successcorrectly demonstrates a successful payment over a two-hop blinded path. It uses utility functions to set up the network and channels, constructs the route parameters, and then sends and claims the payment.- 665-665: The test
three_hop_blinded_path_successcorrectly demonstrates a successful payment over a three-hop blinded path. It follows the same pattern as the previous tests, ensuring consistency in the testing approach.- 676-724: The test
three_hop_blinded_path_failcorrectly simulates a failure scenario over a three-hop blinded path. It ensures that the payment failure is propagated back to the sender and that the correct error message is returned.- 775-781: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [725-938]
The test
multi_hop_receiver_failis comprehensive and tests multiple failure scenarios at the receiver's end in a multi-hop blinded path. It manipulates the HTLCs and the route parameters to simulate different error conditions and checks that they are handled correctly.
- 940-1001: The test
min_htlccorrectly tests the minimum HTLC amount calculation for a blinded path. It also tests a scenario where the payment amount is just below the minimum, ensuring that the payment fails as expected.- 1004-1052: The test
high_prop_feesensures that the payment succeeds even with high proportional fees. It tests the fee calculation logic in the presence of high fees and ensures that the payment is claimed correctly.- 1056-1149: The test
prop_fees_rnguses random fee rates to test the proportional fee calculation. It ensures that the payment succeeds regardless of the fee rates and that the payment is claimed correctly. The use of randomness increases the robustness of the test.lightning/src/ln/shutdown_tests.rs (1)
- 437-437: The
do_htlc_fail_async_shutdownfunction has been modified to include additional parameters in theget_blinded_route_parameterscall whenblinded_recipientis true. Specifically, the function now passes two extra arguments,1and100000000, toget_blinded_route_parameters. This change is consistent with the PR's objective to test the handling of long blinded paths, which involves multiple hops and potentially complex fee calculations.lightning/src/ln/functional_test_utils.rs (9)
- 2195-2195: The function now returns
fee_earned_msat, which is a change from the previous version. This return value is used to assert the correctness of fee calculations in tests. Ensure that all tests calling this function handle theOption<u64>return type correctly.Verification successful
Based on the context provided by the script output, the
expect_payment_forwardedfunction is used within tests primarily for its side effects, such as assertions and event handling. The return value (Option<u64>) does not appear to be captured or used directly in the test cases, suggesting that the tests rely on the function's internal behavior rather than its return value. This is consistent with the typical usage of such functions in test suites, where the focus is on the correctness of the behavior rather than the return value.* 2420-2432: The introduction of `PassAlongPathArgs` struct is a good practice for encapsulating parameters, which improves code readability and maintainability. Ensure that all instances where `do_pass_along_path` is called have been updated to use the new `PassAlongPathArgs` struct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to `expect_payment_forwarded` handle the new return type. rg --type rust "expect_payment_forwarded"Length of output: 2994
Script:
#!/bin/bash # Inspect the usage of `expect_payment_forwarded` in the test files to see how the return value is handled. rg --type rust --vimgrep "expect_payment_forwarded" | xargs -I {} sh -c 'echo {}; rg --type rust --context 2 "{}"'Length of output: 1240
Script:
#!/bin/bash # Adjust the script to properly extract the file and line number information from the `rg --vimgrep` output # and then use that to display the context around each usage of `expect_payment_forwarded`. rg --type rust --vimgrep "expect_payment_forwarded" | while read -r line; do file=$(echo "$line" | cut -d: -f1) line_number=$(echo "$line" | cut -d: -f2) rg --type rust --context 2 -N "$line_number" "$file" doneLength of output: 11050
Script:
#!/bin/bash # Adjust the script to ensure that the context is properly retrieved for each usage of `expect_payment_forwarded`. rg --type rust --vimgrep "expect_payment_forwarded" | while read -r line; do file=$(echo "$line" | cut -d: -f1) line_number=$(echo "$line" | cut -d: -f2) # Display 5 lines of context around the match to ensure we see how the return value is handled rg --type rust --context 5 "$line_number" "$file" doneLength of output: 10981
Script:
#!/bin/bash # Attempt a different approach to retrieve the context for each usage of `expect_payment_forwarded`. # This time, we will use the `--context` flag directly with the `rg` command to show lines around the matches. rg --type rust "expect_payment_forwarded" --context 5Length of output: 22679
Verification successful
All calls to
do_pass_along_pathwithin the provided output use the newPassAlongPathArgsstruct, confirming that the instances where this function is called have been updated accordingly.* 2521-2522: The assertions for the payment amount ensure that the amount is within the expected range, considering the overpay limit. This is a good check to have in place to validate the correctness of payment forwarding with overpayment scenarios. * 2547-2555: The `pass_along_path` function has been refactored to use the new `PassAlongPathArgs` struct. This change should be cross-verified with all test cases to ensure they have been updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to `do_pass_along_path` use the new `PassAlongPathArgs` struct. rg --type rust "do_pass_along_path"Length of output: 753
Verification successful
The output from the
rgcommand has provided the definition of thepass_along_pathfunction within thefunctional_test_utils.rsfile. This function takes multiple parameters related to payment forwarding and constructs aPassAlongPathArgsstruct with them. It then callsdo_pass_along_pathwith this struct.This confirms that
pass_along_pathis indeed a wrapper function that has been updated to use thePassAlongPathArgsstruct, which is then passed to thedo_pass_along_pathfunction. Therefore, the calls topass_along_pathwith individual parameters are valid because this function is responsible for constructing thePassAlongPathArgsstruct internally.Based on this information, the review comment is addressed, and all calls to
pass_along_pathhave been correctly updated to construct and use thePassAlongPathArgsstruct before callingdo_pass_along_path.* 2567-2570: The `send_probe_along_route` function now uses the `PassAlongPathArgs` struct with the `is_probe` method, which is a clear and expressive way to indicate the purpose of the payment. Ensure that the `is_probe` method is used correctly in all relevant test cases.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to `pass_along_path` have been updated to use `PassAlongPathArgs`. rg --type rust "pass_along_path"Length of output: 7066
Script:
#!/bin/bash # Extract the definitions of `pass_along_path` and `do_pass_along_path` to understand their relationship and usage. ast-grep --lang rust --pattern $'fn pass_along_path($$$) { $$$ }' ast-grep --lang rust --pattern $'fn do_pass_along_path($$$) { $$$ }'Length of output: 1237
Script:
#!/bin/bash # Search for the definition of `do_pass_along_path` directly in the source code. rg --type rust --context 5 "fn do_pass_along_path"Length of output: 905
Script:
#!/bin/bash # Search for the definition of `pass_along_path` in the context of payment forwarding. rg --type rust --context 5 "fn pass_along_path"Length of output: 2052
Verification successful
The
is_probemethod is correctly used within thesend_probe_along_routefunction infunctional_test_utils.rs, and the calls tosend_probe_along_routeinpayment_tests.rsare consistent with its intended use. There is no direct interaction withis_probein the test cases, as it is encapsulated within thesend_probe_along_routefunction. Based on the provided context, theis_probemethod is used correctly in all relevant test cases.* 2604-2607: The `pass_claimed_payment_along_route` function now accepts a `ClaimAlongRouteArgs` struct, which is a good refactor for parameter encapsulation. Verify that all calls to this function have been updated to use the new struct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `is_probe` method is used correctly wherever `send_probe_along_route` is called. rg --type rust "send_probe_along_route"Length of output: 542
Script:
#!/bin/bash # Check the implementation of `send_probe_along_route` to see how it uses `is_probe`. rg --type rust -A 10 "fn send_probe_along_route" lightning/src/ln/functional_test_utils.rs # Verify the usage of `is_probe` in `payment_tests.rs` where `send_probe_along_route` is called. rg --type rust -C 5 "send_probe_along_route" lightning/src/ln/payment_tests.rsLength of output: 1956
Verification successful
The verification process has confirmed that all calls to the
pass_claimed_payment_along_routefunction across the codebase are using the newClaimAlongRouteArgsstruct. This includes calls withinfunctional_test_utils.rs,payment_tests.rs, andblinded_payment_tests.rs. Each instance provided in the output shows the use ofClaimAlongRouteArgseither directly or through a variable that is expected to be of this type.* 2675-2675: The variable `fwd_amt_msat` is set to the amount of the payment that was claimed. This is important for tracking the amount that is forwarded and ensuring that the fees are calculated correctly. * 2720-2724: The comments and the code here are clear and explain the purpose of the message event removal for MPP payments. This is a good example of clear code documentation. * 2748-2768: The calculation of fees and the handling of events are done within a macro, which could be prone to errors if not used carefully. Ensure that the macro is used correctly and that the fee calculations are accurate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to `pass_claimed_payment_along_route` use the new `ClaimAlongRouteArgs` struct. rg --type rust "pass_claimed_payment_along_route"Length of output: 848
Script:
#!/bin/bash # Check the arguments passed to `pass_claimed_payment_along_route` in each call to ensure they are instances of `ClaimAlongRouteArgs`. # Check in functional_test_utils.rs ast-grep --lang rust --pattern $'pass_claimed_payment_along_route($_)' lightning/src/ln/functional_test_utils.rs # Check in payment_tests.rs ast-grep --lang rust --pattern $'pass_claimed_payment_along_route($_)' lightning/src/ln/payment_tests.rs # Check in blinded_payment_tests.rs ast-grep --lang rust --pattern $'pass_claimed_payment_along_route($_)' lightning/src/ln/blinded_payment_tests.rsLength of output: 1274
Verification successful
The verification process has confirmed that the
expect_payment_forwardedmacro is used correctly in the context of the provided code snippet, and the fee calculation logic is accurate based on the standard practices in the Lightning Network. The macro encapsulates the assertion of a single event and the call to theexpect_payment_forwardedfunction with the calculated fee and other parameters, which is consistent with its intended use.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the macro for handling update fulfill HTLC and fee calculation is used correctly. rg --type rust "expect_payment_forwarded"Length of output: 2994
Script:
#!/bin/bash # Let's examine the macro definition for `expect_payment_forwarded` to understand its intended usage. rg --type rust -A 20 "macro_rules! expect_payment_forwarded" lightning/src/ln/functional_test_utils.rsLength of output: 1003
Script:
#!/bin/bash # Let's examine the usage of the `expect_payment_forwarded` macro in the code snippet provided in the review comment. # We will look for the macro usage within the file and the surrounding context to understand how it's being used. rg --type rust -C 10 "expect_payment_forwarded\(events.pop\(\).unwrap\(\)" lightning/src/ln/functional_test_utils.rsLength of output: 1081
Script:
#!/bin/bash # Let's examine the fee calculation logic in the code snippet provided in the review comment. # We will look for the fee calculation logic within the file and the surrounding context to understand how it's being used. rg --type rust -C 10 "let \(base_fee, prop_fee\) =" lightning/src/ln/functional_test_utils.rsLength of output: 1219
lightning/src/ln/payment_tests.rs (3)
- 278-283: The refactoring in these lines replaces a direct call to
claim_fundswith a more structured approach usingClaimAlongRouteArgsandpass_claimed_payment_along_route. Ensure that the newextra_feesvector andexpected_routeare correctly used in theargsconstruction and that theexpect_payment_sent!macro is called with the correct arguments.- 2158-2161: The changes here also involve the use of
ClaimAlongRouteArgsandpass_claimed_payment_along_route. Verify that theexpected_pathsarray is correctly populated and that thewith_expected_extra_feesmethod is called with the appropriate arguments. Additionally, check that theexpect_payment_sentfunction accounts for theskimmed_fee_msatcorrectly.- 3726-3726: The update in this line is consistent with the previous hunks, using the
ClaimAlongRouteArgsstruct andpass_claimed_payment_along_routefunction. Confirm that theexpect_payment_sent!macro is called with the correctexpected_total_fee_msatvalue.lightning/src/routing/router.rs (1)
- 969-969: The change in visibility of the
blinded_route_hintsmethod from private to crate-level is noted and appears to be intentional to allow access within the same crate. This should be fine as long as there are no unintended side effects of making this method more accessible within the crate.
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
from
53de611
to
6fe4cbc
Compare
|
Rebased. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/ln/blinded_payment_tests.rs (15 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (3 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/blinded_payment_tests.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (5)
- lightning/src/ln/chanmon_update_fail_tests.rs
- lightning/src/ln/payment_tests.rs
- lightning/src/ln/reload_tests.rs
- lightning/src/ln/shutdown_tests.rs
- lightning/src/routing/router.rs
Additional comments: 21
lightning/src/ln/blinded_payment_tests.rs (13)
- 23-23: The previous comment about the unused import
IDEMPOTENCY_TIMEOUT_TICKSis still valid.- 47-88: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [31-71]
The function
blinded_payment_pathconstructs aBlindedPathobject. It usestake()to consumeintro_node_min_htlc_optandintro_node_max_htlc_optonly once, which is correct to avoid reusing these values for subsequent nodes. The logic appears sound, and the use ofunwrap_or_elseprovides a fallback to the previous channel update's values if the options are already taken, which is a sensible default.
- 76-85: The function
get_blinded_route_parameterscorrectly uses theblinded_payment_pathfunction to constructRouteParameters. It encapsulates the parameters for a payment path, which is a good practice for maintainability.- 181-243: The test
mpp_to_three_hop_blinded_pathssets up a multi-hop payment scenario with blinded paths. The test logic is comprehensive, covering the creation of channels, sending payments, and claiming along the route. The test also includes failure scenarios, which is essential for robust testing. The use ofunwrapin tests is acceptable as it will panic if there is an error, indicating a test failure.- 254-361: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [244-262]
The
ForwardCheckFailenum and thedo_forward_checks_failurefunction are used to test various failure scenarios in forwarding payments. The tests are well-structured and cover different failure modes, which is crucial for ensuring the reliability of the forwarding logic.
- 577-577: The function
do_blinded_intercept_paymenttests the interception of payments on a blinded path. The test is well-structured and includes both success and failure scenarios. The use of configuration flags to control the behavior of the nodes is a good practice for testing different conditions.- 654-654: The test
two_hop_blinded_path_successis straightforward and tests the successful forwarding of a payment over a two-hop blinded path. The test setup and execution are clear and concise.- 677-677: The test
three_hop_blinded_path_successis similar to the previous test but with an additional hop. It is also clear and concise, successfully testing the forwarding of a payment over a three-hop blinded path.- 687-736: The test
three_hop_blinded_path_failchecks the failure scenario for a three-hop blinded path. It ensures that the payment fails back correctly when an intermediate node cannot forward the payment. The test is comprehensive and includes assertions to verify the expected behavior.- 737-737: The
ReceiveCheckFailenum and thedo_multi_hop_receiver_failfunction test various failure scenarios at the receiver's end of a multi-hop blinded path. The tests are thorough and cover a wide range of failure conditions, which is essential for ensuring the robustness of the receiving logic.- 977-981: The
blinded_path_retriestest checks the retry mechanism for blinded paths. It ensures that the payment is retried over a different path if the first attempt fails. The test is well-structured and covers the retry logic effectively.- 1057-1118: The
min_htlctest verifies that the minimum HTLC amount for a blinded path is calculated correctly and that payments succeed when this minimum is met. The test is well-structured and includes a scenario where the payment fails if the amount is below the minimum, which is a good practice for testing edge cases.- 1121-1169: The
high_prop_feestest ensures that proportional fees are calculated correctly for blinded paths. The test is comprehensive and includes a scenario with high proportional fees to catch potential rounding issues. The test is well-structured and covers an important aspect of fee calculation.lightning/src/ln/functional_test_utils.rs (8)
- 2201-2208: The update to the
expect_payment_forwardedmacro to include theallow_1_msat_fee_overpayparameter with a default value offalseis correct.- 2419-2478: The introduction of
PassAlongPathArgsstruct and its usage indo_pass_along_pathfunction is a good refactor for encapsulating parameters and improving code readability and maintainability.- 2521-2522: The assertions to ensure that the forwarded amount is within the expected range, considering the
overpay_limit, are correct.- 2547-2555: Refactoring the
pass_along_pathfunction to use thePassAlongPathArgsstruct for parameter encapsulation aligns with the changes indo_pass_along_pathand is a good improvement.- 2567-2570: The use of
PassAlongPathArgsinsend_probe_along_routewith method chaining foris_probeandclear_recipient_eventsis a clean and readable way to configure the arguments.- 2610-2654: The introduction of
ClaimAlongRouteArgsstruct and its usage inpass_claimed_payment_along_routefunction is a good refactor for encapsulating parameters and improving code readability and maintainability.- 2675-2675: Setting
fwd_amt_msattoamount_msatwithin the match arm is correct and aligns with the expected logic.- 2720-2724: The changes to message event handling for multi-path payments, ensuring the correct node is targeted, are correct.
| fn prop_fees_rng() { | ||
| do_prop_fees_rng(true); | ||
| do_prop_fees_rng(false); | ||
| } | ||
|
|
||
| #[cfg(feature = "std")] | ||
| fn do_prop_fees_rng(send_min: bool) { | ||
| use std::hash::{BuildHasher, Hasher}; | ||
| // Ensure the proportional fees are calculated correctly for `BlindedPayInfo`. | ||
| let chanmon_cfgs = create_chanmon_cfgs(5); | ||
| const PROP_LIMIT: u64 = 1_000_000; | ||
| let base_limit: u64 = if send_min { 1_000_000 } else { 15_000_000 }; | ||
| const MIN_HTLC_LIMIT: u64 = 15_000_000; | ||
| let node_cfgs = create_node_cfgs(5, &chanmon_cfgs); | ||
|
|
||
| let mut node_1_cfg = test_default_channel_config(); | ||
| node_1_cfg.channel_config.forwarding_fee_base_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % base_limit) as u32; | ||
| node_1_cfg.channel_config.forwarding_fee_proportional_millionths = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % PROP_LIMIT) as u32; | ||
| if send_min { | ||
| node_1_cfg.channel_handshake_config.our_htlc_minimum_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % MIN_HTLC_LIMIT) as u64; | ||
| } | ||
|
|
||
| let mut node_2_cfg = test_default_channel_config(); | ||
| node_2_cfg.channel_config.forwarding_fee_base_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % base_limit) as u32; | ||
| node_2_cfg.channel_config.forwarding_fee_proportional_millionths = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % PROP_LIMIT) as u32; | ||
| if send_min { | ||
| node_2_cfg.channel_handshake_config.our_htlc_minimum_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % MIN_HTLC_LIMIT) as u64; | ||
| } | ||
|
|
||
| let mut node_3_cfg = test_default_channel_config(); | ||
| node_3_cfg.channel_config.forwarding_fee_base_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % base_limit) as u32; | ||
| node_3_cfg.channel_config.forwarding_fee_proportional_millionths = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % PROP_LIMIT) as u32; | ||
| if send_min { | ||
| node_3_cfg.channel_handshake_config.our_htlc_minimum_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % MIN_HTLC_LIMIT) as u64; | ||
| } | ||
|
|
||
| let node_chanmgrs = create_node_chanmgrs(5, &node_cfgs, &[None, Some(node_1_cfg), Some(node_2_cfg), Some(node_3_cfg), None]); | ||
| let nodes = create_network(5, &node_cfgs, &node_chanmgrs); | ||
| create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0); | ||
| let chan_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0); | ||
| let chan_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0); | ||
| let chan_3_4 = create_announced_chan_between_nodes_with_value(&nodes, 3, 4, 1_000_000, 0); | ||
|
|
||
| let amt_msat = if send_min { | ||
| get_blinded_route_parameters( | ||
| 42, PaymentSecret([0; 32]), chan_1_2.1.contents.htlc_minimum_msat, | ||
| chan_1_2.1.contents.htlc_maximum_msat, | ||
| nodes.iter().skip(1).map(|node| node.node.get_our_node_id()).collect(), | ||
| &[&chan_1_2.0.contents, &chan_2_3.0.contents, &chan_3_4.0.contents], | ||
| &chanmon_cfgs[4].keys_manager | ||
| ) | ||
| .payment_params.payee.blinded_route_hints()[0].0.htlc_minimum_msat | ||
| } else { 100_000 }; | ||
| let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[4], Some(amt_msat), None); | ||
|
|
||
| let mut route_params = get_blinded_route_parameters(amt_msat, payment_secret, | ||
| chan_1_2.1.contents.htlc_minimum_msat, chan_1_2.1.contents.htlc_maximum_msat, | ||
| nodes.iter().skip(1).map(|node| node.node.get_our_node_id()).collect(), | ||
| &[&chan_1_2.0.contents, &chan_2_3.0.contents, &chan_3_4.0.contents], | ||
| &chanmon_cfgs[4].keys_manager); | ||
| route_params.max_total_routing_fee_msat = None; | ||
|
|
||
| nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params.clone(), Retry::Attempts(0)).unwrap(); | ||
| check_added_monitors(&nodes[0], 1); | ||
| let mut events = nodes[0].node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(events.len(), 1); | ||
| let event = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events); | ||
| let expected_path = &[&nodes[1], &nodes[2], &nodes[3], &nodes[4]]; | ||
|
|
||
| let args = PassAlongPathArgs::new(&nodes[0], expected_path, amt_msat, payment_hash, event) | ||
| .with_payment_secret(payment_secret) | ||
| .with_overpay_limit(if send_min { 40 } else { 3 }); | ||
| let claimable_ev = do_pass_along_path(args); | ||
| let claim_amt = if let Some(crate::events::Event::PaymentClaimable { amount_msat, .. }) = claimable_ev { | ||
| amount_msat | ||
| } else { panic!() }; | ||
| let overpayment_amt = claim_amt.checked_sub(amt_msat).unwrap(); | ||
|
|
||
| nodes[4].node.claim_funds(payment_preimage); | ||
| let expected_route = &[&expected_path[..]]; | ||
| let mut claim_args = ClaimAlongRouteArgs::new(&nodes[0], &expected_route[..], payment_preimage) | ||
| .allow_1_msat_fee_overpay(); | ||
| let expected_fee = pass_claimed_payment_along_route(claim_args); | ||
| expect_payment_sent(&nodes[0], payment_preimage, Some(Some(expected_fee + overpayment_amt)), true, true); | ||
| } |
There was a problem hiding this comment.
The prop_fees_rng test uses random values for fee configuration to ensure that the proportional fee calculation is robust against a wide range of inputs. The test is comprehensive and includes both minimum and non-minimum payment amounts to cover different fee scenarios. The use of randomness in tests can be good for catching edge cases, but it may also introduce non-determinism. Ensure that the test is still deterministic and reproducible.
Consider adding a fixed seed for the random number generator to ensure test determinism and reproducibility.
There was a problem hiding this comment.
While randomized tests are fine, we should always ensure we at least print the seed at the start of the test so that we can reproduce failures.
There was a problem hiding this comment.
#[cfg(feature = "std")]
fn do_prop_fees_rng(send_min: bool) {
use std::hash::{BuildHasher, Hasher};
// Ensure the proportional fees are calculated correctly for `BlindedPayInfo`.
let chanmon_cfgs = create_chanmon_cfgs(5);
const PROP_LIMIT: u64 = 1_000_000;
let base_limit: u64 = if send_min { 1_000_000 } else { 15_000_000 };
const MIN_HTLC_LIMIT: u64 = 15_000_000;
let node_cfgs = create_node_cfgs(5, &chanmon_cfgs);
// Create a new random state and print the seed
let random_state = std::collections::hash_map::RandomState::new();
let seed = random_state.build_hasher().finish();
println!("Random test seed: {}", seed);
// Use the random state with the fixed seed for the rest of the test
// ...
}@TheBlueMatt I've added a code snippet that prints the seed at the start of the do_prop_fees_rng test. You can incorporate this change into the test to ensure that the seed is logged, allowing for reproducible test runs.
There was a problem hiding this comment.
@TheBlueMatt Wouldn't this be better as a fuzz test?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Maybe? Depends on how rare we expect bugs to be - if we think there's some probability greater than, say, 10% that a given seed hits any bug, then its fine, if we think the probability is vanishingly small, then it should be a fuzz test. I think we're okay with a non-fuzz test here, but if you think bugs would be rarer than we can move it.
|
Plus #2823 (comment) |
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
from
6fe4cbc
to
0e8d116
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/ln/blinded_payment_tests.rs (15 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (3 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/blinded_payment_tests.rs: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- lightning/src/ln/chanmon_update_fail_tests.rs (Error: unable to parse review)
Additional comments: 4
lightning/src/ln/reload_tests.rs (1)
- 825-830: The refactoring of
do_pass_along_pathto usePassAlongPathArgswith method chaining improves readability and maintainability by encapsulating parameters into a single object. This change also aligns with best practices for reducing the number of function parameters, making the code more concise and easier to understand.lightning/src/ln/blinded_payment_tests.rs (1)
- 254-361: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [181-737]
The tests cover a wide range of scenarios for blinded payment paths, including success, failure, and retry mechanisms. Ensure that all edge cases and potential failure modes are thoroughly tested, especially for complex scenarios involving multiple hops and fee calculations.
Ensure comprehensive coverage and consider adding more tests if any scenarios are not fully covered.
lightning/src/ln/shutdown_tests.rs (1)
- 437-437: The call to
get_blinded_route_parametersincludes hardcoded values1and100000000. These values seem arbitrary and lack context within the testdo_htlc_fail_async_shutdown. It's important to ensure these values are intentionally chosen and correctly represent the test scenario being simulated.lightning/src/routing/router.rs (1)
- 992-992: The visibility of the
blinded_route_hintsmethod has been changed to crate-level. Ensure that this change does not impact its usage across the crate, especially in modules that previously relied on its public access.Verification successful
The change in visibility of the
blinded_route_hintsmethod to crate-level is correctly implemented, with its usage confined within the crate and its tests. This suggests the change does not adversely affect the crate's functionality or external access.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of `blinded_route_hints` across the crate to ensure no external access is broken. rg 'blinded_route_hints' --type rustLength of output: 2347
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
from
0e8d116
to
2794d54
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (8)
- lightning/src/ln/blinded_payment_tests.rs (15 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (11 hunks)
- lightning/src/ln/offers_tests.rs (1 hunks)
- lightning/src/ln/payment_tests.rs (3 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/blinded_payment_tests.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (5)
- lightning/src/ln/chanmon_update_fail_tests.rs
- lightning/src/ln/payment_tests.rs
- lightning/src/ln/reload_tests.rs
- lightning/src/ln/shutdown_tests.rs
- lightning/src/routing/router.rs
Additional comments: 1
lightning/src/ln/offers_tests.rs (1)
- 115-116: The introduction of
PassAlongPathArgsstruct to encapsulate arguments inroute_bolt12_paymentfunction is a good practice for code maintainability and readability. However, ensure that theclear_recipient_eventsmethod's usage aligns with the intended logic, especially considering its impact on event handling.
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
from
2794d54
to
ed0d138
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/ln/blinded_payment_tests.rs (15 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (15 hunks)
- lightning/src/ln/offers_tests.rs (1 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/blinded_payment_tests.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (6)
- lightning/src/ln/chanmon_update_fail_tests.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/offers_tests.rs
- lightning/src/ln/reload_tests.rs
- lightning/src/ln/shutdown_tests.rs
- lightning/src/routing/router.rs
Additional comments: 3
lightning/src/ln/blinded_payment_tests.rs (3)
- 47-88: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-71]
The function
blinded_payment_pathcorrectly constructs aBlindedPathfor a given set of nodes and channel updates. The logic for handlingintro_node_min_htlc_optandintro_node_max_htlc_optensures that the first node's min and max HTLC values are used only once and then defaults to the previous channel's values for subsequent nodes. This approach is logical and ensures that the path's HTLC constraints are correctly initialized and then propagated based on the channel updates.
- 76-85: The function
get_blinded_route_parameterscorrectly constructsRouteParametersfrom a set of blinded payment path parameters. It utilizes theblinded_payment_pathfunction to generate aBlindedPayInfoandBlindedPath, which are then used to create the route parameters. This encapsulation of route parameter generation is a good practice for maintainability and readability.- 181-243: The test
mpp_to_three_hop_blinded_pathsis well-structured and covers the scenario of multi-part payments over three-hop blinded paths. It correctly sets up the network topology, creates the blinded route parameters, and sends the payment. The use ofpass_along_pathandclaim_payment_along_routefunctions to simulate the forwarding and claiming of payments along the route is appropriate. The test ensures that the payment is successfully processed across the blinded paths.
|
Rebased and went ahead and squashed the minor fixups. |
|
Ugh, this needs rebase again. I promise this time we'll review + land it quick 😅 |
…_add Will be useful in the next commit for adapting this test for non-intro-node failures.
Will be useful in the next commit for adapting these tests for non-intro node failures.
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
from
ed0d138
to
d91024a
Compare
Oops, github ate that notification. Rebased! |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/ln/blinded_payment_tests.rs (15 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (15 hunks)
- lightning/src/ln/offers_tests.rs (1 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/blinded_payment_tests.rs: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- lightning/src/ln/chanmon_update_fail_tests.rs (Error: unable to parse review)
Additional comments: 13
lightning/src/ln/offers_tests.rs (1)
- 115-117: The introduction of
PassAlongPathArgsstruct for passing arguments todo_pass_along_pathis a positive change for code readability and maintainability. This encapsulation makes the function call cleaner and the code more understandable. However, ensure that thePassAlongPathArgsstruct is defined with appropriate fields and methods to support this usage, including thenewconstructor andclear_recipient_eventsmethod. Additionally, verify that all necessary fields are included in the struct to support the operations withindo_pass_along_path.lightning/src/ln/reload_tests.rs (1)
- 826-831: The refactoring to use
PassAlongPathArgswith method chaining forpayment_secretandclear_recipient_eventsin thedo_test_partial_claim_before_restartfunction enhances readability and maintainability. This approach allows for a more structured way of passing arguments to thedo_pass_along_pathfunction, making the code easier to understand and modify in the future. The use of method chaining for setting optional parameters is a good practice, as it provides a clear and concise way to configure the behavior of the function call.lightning/src/ln/blinded_payment_tests.rs (4)
- 23-23: The removal of
IDEMPOTENCY_TIMEOUT_TICKSfrom the import statement is consistent with the previous review comment indicating it was unused. This change improves code cleanliness by removing unnecessary imports.- 47-88: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-71]
The function
blinded_payment_pathconstructs aBlindedPathandBlindedPayInfofor a given set of parameters. It's well-structured and follows the logic for creating a blinded path correctly. However, it's important to ensure that theenumerate()usage and the.unwrap()calls are safe and won't lead to panics in expected scenarios. Given the controlled test environment and the nature of the inputs, these seem acceptable here.
- 76-85: The function
get_blinded_route_parametersis a utility function that simplifies the creation ofRouteParametersfor tests involving blinded paths. It correctly utilizes theblinded_payment_pathfunction and constructs the route parameters as expected. This function enhances modularity and reusability in the test suite.- 183-245: The test
mpp_to_three_hop_blinded_pathsis designed to simulate a multi-part payment (MPP) over three-hop blinded paths. It correctly sets up the network topology, creates the necessary channels, and initiates the payment process. The logic for simulating the payment forwarding and claiming along the route appears correct. This test enhances coverage for MPP scenarios in the context of blinded paths.lightning/src/ln/shutdown_tests.rs (1)
- 440-440: The addition of parameters
1and100000000in theget_blinded_route_parameterscall whenblinded_recipientis true seems to be testing the handling of blinded payments with specific route parameters. It's important to ensure that these values are chosen to adequately test the intended scenarios and that they align with realistic use cases or edge cases that need to be covered.
- Please verify that the chosen parameters (
1and100000000) forget_blinded_route_parametersare appropriate for the scenarios being tested. Consider whether these values effectively cover the intended edge cases or use cases related to blinded payments.lightning/src/ln/functional_test_utils.rs (5)
- 2484-2536: The
PassAlongPathArgsstruct and itsnewmethod are correctly defined, providing a clear and structured way to pass arguments for thedo_pass_along_pathfunction. The builder pattern methods likeclear_recipient_events,is_probe,expect_payment_claimable,with_payment_secret,with_payment_preimage, andwith_overpay_limitare well-implemented, allowing for fluent and readable configuration ofPassAlongPathArgsinstances. These methods correctly returnSelfto enable method chaining, enhancing usability.
- 2538-2543: The
do_pass_along_pathfunction is well-structured and uses thePassAlongPathArgsstruct to receive its parameters, which enhances code readability and maintainability. The function correctly deconstructs thePassAlongPathArgsstruct to access its fields, which is a good practice for handling multiple parameters.
- 2612-2620: The
pass_along_pathfunction correctly creates aPassAlongPathArgsinstance using thenewmethod and configures it using the builder pattern methods based on the provided arguments. This approach maintains consistency with thedo_pass_along_pathfunction's expectations and demonstrates effective use of the builder pattern for configuring complex parameter sets.
- 2632-2635: The
do_pass_along_pathfunction call withinsend_probe_along_routecorrectly configures thePassAlongPathArgsfor a probe scenario by settingis_probetotrueand optionally disabling recipient events clearing. This usage demonstrates flexibility in handling different scenarios (like probing) with the same underlying function by adjusting the arguments accordingly.
- 2679-2692: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2682-2718]
The addition of the
allow_1_msat_fee_overpayfield to thePassAlongPathArgsstruct and its corresponding builder methodallow_1_msat_fee_overpayis a thoughtful enhancement. It addresses the potential for minor discrepancies in fee calculations due to rounding errors, especially in scenarios involving blinded paths. This addition shows attention to detail and consideration for practical issues that may arise in payment routing.
lightning/src/routing/router.rs (1)
- 989-989: The change in visibility of the
blinded_route_hintsfunction topub(crate)is noted. It's crucial to ensure that this adjustment aligns with the principle of least privilege, exposing the function only as much as necessary within the crate. Please verify that this change is essential for the intended refactoring or feature enhancements and does not inadvertently expose sensitive functionality beyond its intended scope.
lightning/src/ln/offers_tests.rs
Outdated
| // invoice contains the payment_hash but it was encrypted inside an onion message. | ||
| let amount_msats = invoice.amount_msats(); | ||
| let payment_hash = invoice.payment_hash(); | ||
| do_pass_along_path( | ||
| node, path, amount_msats, payment_hash, None, ev, false, false, None, false | ||
| ); | ||
| let args = PassAlongPathArgs::new(node, path, amount_msats, payment_hash, ev) | ||
| .clear_recipient_events(false); | ||
| do_pass_along_path(args); | ||
| } | ||
|
|
||
| fn claim_bolt12_payment<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, path: &[&Node<'a, 'b, 'c>]) { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-117]
Overall, the tests in offers_tests.rs are comprehensive and cover a wide range of scenarios for the BOLT 12 Offers payment flow. It's important to ensure that each test case is well-documented with comments explaining the purpose of the test and the expected outcomes. This will enhance the maintainability of the test suite and make it easier for future contributors to understand the test cases and the specific aspects of the payment flow they are designed to verify. Additionally, consider adding more detailed assertions where possible to ensure that the tests not only pass but also validate the correctness of the payment flow in more depth.
| node_cfgs[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); | ||
| nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap(); | ||
| check_added_monitors(&nodes[0], 1); | ||
|
|
||
| let mut events = nodes[0].node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(events.len(), 1); | ||
| let ev = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events); | ||
| let mut payment_event = SendEvent::from_event(ev); | ||
| macro_rules! cause_error { | ||
| ($src_node_idx: expr, $target_node_idx: expr, $update_add: expr) => { | ||
| match check { | ||
| ForwardCheckFail::InboundOnionCheck => { | ||
| $update_add.cltv_expiry = 10; // causes outbound CLTV expiry to underflow | ||
| }, | ||
| ForwardCheckFail::ForwardPayloadEncodedAsReceive => { | ||
| let session_priv = SecretKey::from_slice(&[3; 32]).unwrap(); | ||
| let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); | ||
| let cur_height = nodes[0].best_block_info().1; | ||
| let (mut onion_payloads, ..) = onion_utils::build_onion_payloads( | ||
| &route.paths[0], amt_msat, RecipientOnionFields::spontaneous_empty(), cur_height, &None).unwrap(); | ||
| // Remove the receive payload so the blinded forward payload is encoded as a final payload | ||
| // (i.e. next_hop_hmac == [0; 32]) | ||
| onion_payloads.pop(); | ||
| if $target_node_idx + 1 < nodes.len() { | ||
| onion_payloads.pop(); | ||
| } | ||
| $update_add.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash).unwrap(); | ||
| }, | ||
| ForwardCheckFail::OutboundChannelCheck => { | ||
| // The intro node will see that the next-hop peer is disconnected and fail the HTLC backwards. | ||
| nodes[$src_node_idx].node.peer_disconnected(&nodes[$target_node_idx].node.get_our_node_id()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut update_add = &mut payment_event.msgs[0]; | ||
| match check { | ||
| ForwardCheckFail::InboundOnionCheck => { | ||
| update_add.cltv_expiry = 10; // causes outbound CLTV expiry to underflow | ||
| }, | ||
| ForwardCheckFail::ForwardPayloadEncodedAsReceive => { | ||
| let session_priv = SecretKey::from_slice(&[3; 32]).unwrap(); | ||
| let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); | ||
| let cur_height = nodes[0].best_block_info().1; | ||
| let (mut onion_payloads, ..) = onion_utils::build_onion_payloads( | ||
| &route.paths[0], amt_msat, RecipientOnionFields::spontaneous_empty(), cur_height, &None).unwrap(); | ||
| // Remove the receive payload so the blinded forward payload is encoded as a final payload | ||
| // (i.e. next_hop_hmac == [0; 32]) | ||
| onion_payloads.pop(); | ||
| update_add.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash).unwrap(); | ||
| }, | ||
| ForwardCheckFail::OutboundChannelCheck => { | ||
| // The intro node will see that the next-hop peer is disconnected and fail the HTLC backwards. | ||
| nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id()); | ||
| }, | ||
| let mut updates_0_1 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); | ||
| let update_add = &mut updates_0_1.update_add_htlcs[0]; | ||
|
|
||
| if intro_fails { | ||
| cause_error!(1, 2, update_add); | ||
| } | ||
| nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); | ||
|
|
||
| nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); | ||
| check_added_monitors!(nodes[1], 0); | ||
| do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event.commitment_msg, true, true); | ||
| do_commitment_signed_dance(&nodes[1], &nodes[0], &updates_0_1.commitment_signed, true, true); | ||
|
|
||
| if intro_fails { | ||
| let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); | ||
| nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]); | ||
| do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false); | ||
| expect_payment_failed_conditions(&nodes[0], payment_hash, false, | ||
| PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32])); | ||
| return | ||
| } | ||
|
|
||
| expect_pending_htlcs_forwardable!(nodes[1]); | ||
| check_added_monitors!(nodes[1], 1); | ||
|
|
||
| let mut updates_1_2 = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id()); | ||
| let mut update_add = &mut updates_1_2.update_add_htlcs[0]; | ||
|
|
||
| cause_error!(2, 3, update_add); | ||
|
|
||
| nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &update_add); | ||
| check_added_monitors!(nodes[2], 0); | ||
| do_commitment_signed_dance(&nodes[2], &nodes[1], &updates_1_2.commitment_signed, true, true); | ||
|
|
||
| let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); | ||
| let update_malformed = &mut updates.update_fail_malformed_htlcs[0]; | ||
| assert_eq!(update_malformed.failure_code, INVALID_ONION_BLINDING); | ||
| assert_eq!(update_malformed.sha256_of_onion, [0; 32]); | ||
|
|
||
| // Ensure the intro node will properly blind the error if its downstream node failed to do so. | ||
| update_malformed.sha256_of_onion = [1; 32]; | ||
| update_malformed.failure_code = INVALID_ONION_BLINDING ^ 1; | ||
| nodes[1].node.handle_update_fail_malformed_htlc(&nodes[2].node.get_our_node_id(), update_malformed); | ||
| do_commitment_signed_dance(&nodes[1], &nodes[2], &updates.commitment_signed, true, false); |
There was a problem hiding this comment.
The function do_forward_checks_failure and its associated test forward_checks_failure are designed to verify the correct handling of various forwarding check failures. The use of a macro to simulate different failure scenarios is a clever approach to reduce code duplication. However, the complexity of this function could be a maintenance concern in the future. Consider refactoring to improve readability if this becomes an issue.
Consider breaking down the do_forward_checks_failure function into smaller, more focused functions to improve readability and maintainability.
| } = args; | ||
| let claim_event = expected_paths[0].last().unwrap().node.get_and_clear_pending_events(); | ||
| assert_eq!(claim_event.len(), 1); | ||
| #[allow(unused)] |
There was a problem hiding this comment.
Then why are we adding it?
There was a problem hiding this comment.
It's only unused if mid_update_fulfill_dance isn't called, IIRC, which can happen for 2-hop paths.
| @@ -2539,7 +2592,15 @@ pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_p | |||
| } | |||
|
|
|||
| pub fn pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_claimable_expected: bool, expected_preimage: Option<PaymentPreimage>) -> Option<Event> { | |||
There was a problem hiding this comment.
Should this also take PassAlongPathArgs?
There was a problem hiding this comment.
Yeah probably, it has like 50 callsites though so I skipped it for now. Open to doing it in follow-up.
| expected_extra_fees_msat: Option<u64>, upstream_force_closed: bool, | ||
| downstream_force_closed: bool | ||
| ) { | ||
| downstream_force_closed: bool, allow_1_msat_fee_overpay: bool, |
There was a problem hiding this comment.
Should we make a struct for these args?
There was a problem hiding this comment.
We can but I opted to skip that refactor here because we also have the macro expect_payment_forwarded which is used in the majority of cases. So it seems ok to me to just always add rarely-used args to the function and keep the macro for the general case.
| fn prop_fees_rng() { | ||
| do_prop_fees_rng(true); | ||
| do_prop_fees_rng(false); | ||
| } | ||
|
|
||
| #[cfg(feature = "std")] | ||
| fn do_prop_fees_rng(send_min: bool) { | ||
| use std::hash::{BuildHasher, Hasher}; | ||
| // Ensure the proportional fees are calculated correctly for `BlindedPayInfo`. | ||
| let chanmon_cfgs = create_chanmon_cfgs(5); | ||
| const PROP_LIMIT: u64 = 1_000_000; | ||
| let base_limit: u64 = if send_min { 1_000_000 } else { 15_000_000 }; | ||
| const MIN_HTLC_LIMIT: u64 = 15_000_000; | ||
| let node_cfgs = create_node_cfgs(5, &chanmon_cfgs); | ||
|
|
||
| let mut node_1_cfg = test_default_channel_config(); | ||
| node_1_cfg.channel_config.forwarding_fee_base_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % base_limit) as u32; | ||
| node_1_cfg.channel_config.forwarding_fee_proportional_millionths = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % PROP_LIMIT) as u32; | ||
| if send_min { | ||
| node_1_cfg.channel_handshake_config.our_htlc_minimum_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % MIN_HTLC_LIMIT) as u64; | ||
| } | ||
|
|
||
| let mut node_2_cfg = test_default_channel_config(); | ||
| node_2_cfg.channel_config.forwarding_fee_base_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % base_limit) as u32; | ||
| node_2_cfg.channel_config.forwarding_fee_proportional_millionths = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % PROP_LIMIT) as u32; | ||
| if send_min { | ||
| node_2_cfg.channel_handshake_config.our_htlc_minimum_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % MIN_HTLC_LIMIT) as u64; | ||
| } | ||
|
|
||
| let mut node_3_cfg = test_default_channel_config(); | ||
| node_3_cfg.channel_config.forwarding_fee_base_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % base_limit) as u32; | ||
| node_3_cfg.channel_config.forwarding_fee_proportional_millionths = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % PROP_LIMIT) as u32; | ||
| if send_min { | ||
| node_3_cfg.channel_handshake_config.our_htlc_minimum_msat = | ||
| (std::collections::hash_map::RandomState::new().build_hasher().finish() % MIN_HTLC_LIMIT) as u64; | ||
| } | ||
|
|
||
| let node_chanmgrs = create_node_chanmgrs(5, &node_cfgs, &[None, Some(node_1_cfg), Some(node_2_cfg), Some(node_3_cfg), None]); | ||
| let nodes = create_network(5, &node_cfgs, &node_chanmgrs); | ||
| create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0); | ||
| let chan_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0); | ||
| let chan_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0); | ||
| let chan_3_4 = create_announced_chan_between_nodes_with_value(&nodes, 3, 4, 1_000_000, 0); | ||
|
|
||
| let amt_msat = if send_min { | ||
| get_blinded_route_parameters( | ||
| 42, PaymentSecret([0; 32]), chan_1_2.1.contents.htlc_minimum_msat, | ||
| chan_1_2.1.contents.htlc_maximum_msat, | ||
| nodes.iter().skip(1).map(|node| node.node.get_our_node_id()).collect(), | ||
| &[&chan_1_2.0.contents, &chan_2_3.0.contents, &chan_3_4.0.contents], | ||
| &chanmon_cfgs[4].keys_manager | ||
| ) | ||
| .payment_params.payee.blinded_route_hints()[0].0.htlc_minimum_msat | ||
| } else { 100_000 }; | ||
| let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[4], Some(amt_msat), None); | ||
|
|
||
| let mut route_params = get_blinded_route_parameters(amt_msat, payment_secret, | ||
| chan_1_2.1.contents.htlc_minimum_msat, chan_1_2.1.contents.htlc_maximum_msat, | ||
| nodes.iter().skip(1).map(|node| node.node.get_our_node_id()).collect(), | ||
| &[&chan_1_2.0.contents, &chan_2_3.0.contents, &chan_3_4.0.contents], | ||
| &chanmon_cfgs[4].keys_manager); | ||
| route_params.max_total_routing_fee_msat = None; | ||
|
|
||
| nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params.clone(), Retry::Attempts(0)).unwrap(); | ||
| check_added_monitors(&nodes[0], 1); | ||
| let mut events = nodes[0].node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(events.len(), 1); | ||
| let event = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events); | ||
| let expected_path = &[&nodes[1], &nodes[2], &nodes[3], &nodes[4]]; | ||
|
|
||
| let args = PassAlongPathArgs::new(&nodes[0], expected_path, amt_msat, payment_hash, event) | ||
| .with_payment_secret(payment_secret) | ||
| .with_overpay_limit(if send_min { 40 } else { 3 }); | ||
| let claimable_ev = do_pass_along_path(args); | ||
| let claim_amt = if let Some(crate::events::Event::PaymentClaimable { amount_msat, .. }) = claimable_ev { | ||
| amount_msat | ||
| } else { panic!() }; | ||
| let overpayment_amt = claim_amt.checked_sub(amt_msat).unwrap(); | ||
|
|
||
| nodes[4].node.claim_funds(payment_preimage); | ||
| let expected_route = &[&expected_path[..]]; | ||
| let mut claim_args = ClaimAlongRouteArgs::new(&nodes[0], &expected_route[..], payment_preimage) | ||
| .allow_1_msat_fee_overpay(); | ||
| let expected_fee = pass_claimed_payment_along_route(claim_args); | ||
| expect_payment_sent(&nodes[0], payment_preimage, Some(Some(expected_fee + overpayment_amt)), true, true); | ||
| } |
There was a problem hiding this comment.
@TheBlueMatt Wouldn't this be better as a fuzz test?
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
from
d91024a
to
33f16de
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/ln/blinded_payment_tests.rs (15 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (14 hunks)
- lightning/src/ln/offers_tests.rs (1 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 28
lightning/src/ln/offers_tests.rs (1)
- 115-117: The refactoring to use
PassAlongPathArgsfor passing arguments todo_pass_along_pathis a positive change, enhancing code readability and maintainability. This approach allows for cleaner argument passing and aligns with the PR's objectives to improve testing infrastructure. It's important to ensure that all fields inPassAlongPathArgsare properly validated and that the struct is used consistently across the codebase for similar patterns.lightning/src/ln/reload_tests.rs (3)
- 826-831: The refactoring of
do_pass_along_pathcalls to usePassAlongPathArgswith method chaining for settingpayment_secretandkeep_recipient_eventsis a good improvement for readability and maintainability. This change makes the function calls more concise and easier to understand, especially when dealing with multiple parameters.- 823-834: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [832-1032]
The addition of
do_forwarded_payment_no_manager_persistence,intercepted_payment_no_manager_persistence, andremoved_payment_no_manager_persistencetests significantly enhances the coverage of scenarios involving partial persistence of ChannelManager and ChannelMonitor states across node restarts. These tests effectively simulate and validate the behavior of the system in handling forwarded, intercepted, and removed payments when the persistence of state changes is not fully completed. This is crucial for ensuring the robustness and reliability of the payment forwarding and failure handling mechanisms in the face of unexpected restarts or partial state persistence.
- 823-834: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1033-1078]
The
test_reload_partial_funding_batchtest is a valuable addition that simulates a scenario where a node is restarted with only a subset of channels from a funding batch having their monitors persisted. This test effectively checks the system's behavior in handling partial persistence of channel states during the funding process, ensuring that the system can correctly handle such scenarios and maintain consistency. The test's logic and assertions appear to be well-designed to validate the expected outcomes accurately.lightning/src/ln/blinded_payment_tests.rs (14)
- 47-88: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-71]
The
blinded_payment_pathfunction introducesintro_node_min_htlcandintro_node_max_htlcparameters to adjust the HTLC minimum and maximum values for the intro node. This change allows for more flexible testing scenarios, especially in testing long blinded paths and associated fees. The logic for handling these parameters seems correct, as it usesOption::taketo ensure that the values are used only once, which aligns with the expected behavior where these constraints apply only to the first hop in the blinded path.
- 76-85: The
get_blinded_route_parametersfunction correctly encapsulates the creation ofRouteParametersfrom blinded payment path parameters. This function simplifies the process of generating route parameters for tests involving blinded paths, enhancing code readability and maintainability.- 183-245: The
mpp_to_three_hop_blinded_pathstest function effectively simulates a multi-part payment (MPP) over two 3-hop blinded paths. This test is crucial for ensuring that the system can handle complex routing scenarios involving blinded paths. The test setup, execution, and assertions are logically structured and cover the intended scenarios well.- 256-363: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [259-364]
The
forward_checks_failuretest function checks various failure scenarios during forwarding, such as inbound onion check failures, payload encoding issues, and outbound channel checks. The use of a macro to simulate different failure scenarios is a clever way to reduce code duplication and increase test coverage. However, ensure that the complexity introduced by the macro remains manageable and does not hinder the readability and maintainability of the test code.
- 379-385: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [367-431]
The
failed_backwards_to_intro_nodetest function ensures that the intro node correctly handles errors propagated backwards from downstream nodes. This test is important for verifying the robustness of error handling in the payment forwarding process. The test setup and execution are well-designed to simulate the scenario of interest.
- 475-554: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [438-548]
The
forward_fail_in_process_pending_htlc_fwdstest function checks the behavior when an HTLC fails in theprocess_pending_htlc_forwardsstage due to peer disconnection or channel closure. The test effectively covers these scenarios, ensuring that the system behaves as expected under such conditions. The use of a macro to simulate the failure conditions is efficient, though it's important to keep the macro logic straightforward to maintain test clarity.
- 576-582: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [553-584]
The
blinded_intercept_paymenttest function simulates the interception of a payment by an intermediate node configured to accept HTLC intercepts. This test is crucial for validating the functionality of HTLC interception along blinded paths. The test is well-structured, with clear setup, execution, and assertions that align with the intended test scenario.
- 655-656: The
two_hop_blinded_path_successtest function successfully tests a payment over a two-hop blinded path, ensuring that such payments are processed correctly. This test is essential for verifying the basic functionality of blinded path payments. The test setup and execution are straightforward and effectively demonstrate the intended functionality.- 684-685: The
three_hop_blinded_path_successtest function extends the testing of blinded path payments to a three-hop scenario. This test is important for ensuring that the system can handle more complex blinded path payments. The test is well-designed, with a clear setup and execution that accurately simulates the intended scenario.- 696-746: The
three_hop_blinded_path_failtest function simulates a failure scenario in a three-hop blinded path, where an intermediate node fails the payment. This test is crucial for verifying the system's ability to correctly handle failures in complex routing scenarios. The test setup, execution, and assertions are well-structured and effectively capture the intended failure scenario.- 1062-1185: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [747-1185]
The
multi_hop_receiver_failtest function covers various failure scenarios at the receiver end in a multi-hop blinded path context. This comprehensive test is essential for ensuring robust error handling at the receiver side. The test is well-organized, with different sub-tests for each failure scenario, ensuring thorough coverage of potential receiver-side failures.
- 1066-1185: The
blinded_path_retriestest function simulates retrying payments over different blinded paths when initial attempts fail. This test is crucial for validating the retry mechanism in the context of blinded paths. The test setup, execution, and assertions are well-designed to simulate the retry scenarios and validate the expected behavior.- 1067-1134: The
min_htlctest function verifies that payments succeed when the amount is equal to the calculated minimum HTLC value for a blinded path. This test is important for ensuring that the system correctly calculates and respects the minimum HTLC value along a path. The test setup, execution, and assertions are logically structured and effectively demonstrate the intended functionality.- 1137-1185: The
conditionally_round_fwd_amttest function addresses a specific rounding issue in fee calculations along a multi-hop path. This test is crucial for ensuring that intermediate nodes correctly handle rounding in their fee calculations to avoid undercharging. The test setup, execution, and assertions are well-designed to capture and validate the intended behavior.lightning/src/ln/shutdown_tests.rs (3)
- 437-443: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1065-1092]
The test
do_simple_legacy_shutdown_testcorrectly implements the legacy shutdown fee negotiation logic, including handling of high initiator fees. It properly initiates channel closure, exchanges shutdown and closing_signed messages, and asserts the cooperative closure of the channel.
- 437-443: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1094-1168]
The test
do_outbound_update_no_early_closing_signedcorrectly verifies that no early closing_signed message is sent when there are pending inbound HTLCs or fee updates during the shutdown process. It ensures that the channel closure process waits for the channel to be fully resolved before proceeding with closing_signed negotiation.
- 437-443: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1170-1205]
The test
batch_funding_failurecorrectly simulates a scenario where batch funding fails for one of the channels while succeeding for another. It ensures that the failure is handled appropriately without causing a deadlock, and that the successful channel proceeds as expected.lightning/src/ln/functional_test_utils.rs (7)
- 2482-2541: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2226-2530]
The
PassAlongPathArgsstruct and its associatednewand modifier methods provide a clear and flexible way to specify arguments for thedo_pass_along_pathfunction. This approach enhances readability and maintainability by grouping related parameters together and providing a fluent interface for configuration.
- 2532-2537: The
do_pass_along_pathfunction is well-structured, leveraging thePassAlongPathArgsstruct to neatly pass a set of related arguments. This design choice improves code readability and maintainability.- 2605-2615: The use of the
PassAlongPathArgsstruct in thepass_along_pathfunction demonstrates good consistency and reuse of the structured approach to passing multiple related arguments. This consistency is beneficial for maintainability and readability across related functions.- 2627-2630: The
do_pass_along_pathinvocation with theis_probeandkeep_recipient_eventsmodifiers showcases the flexibility and readability benefits of using thePassAlongPathArgsstruct and its fluent interface. This pattern effectively communicates the intent of the function call.- 2674-2687: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2677-2713]
The addition of the
allow_1_msat_fee_overpayfield to thePassAlongPathArgsstruct and its corresponding modifier methodallow_1_msat_fee_overpayaddresses a specific testing scenario related to fee overpayment due to rounding errors. This change demonstrates attention to detail and the ability to adapt test utilities to cover edge cases in the Lightning Network protocol.
- 2710-2713: The introduction of the
allow_1_msat_fee_overpaymodifier method for thePassAlongPathArgsstruct is a thoughtful addition that enhances the testing framework's flexibility in handling fee calculation nuances. This method allows for more precise control over test conditions related to fee payments.- 2532-2537: The
do_pass_along_pathfunction's implementation is comprehensive, covering various scenarios in the payment path traversal process. However, it's crucial to ensure that all possible error paths and edge cases are adequately handled, especially in testing utilities where robustness is key to uncovering potential issues in the implementation under test.
|
Feedback should be addressed. I removed the rng test for now since another failure came up that I'm still debugging. |
|
Needs squash. |
This tests an intermediate forwarding node being failed back to with malformed and then failing back themselves with malformed.
Lays groundwork to make pass_along_path easier to adapt without changing a million callsites.
We're currently not setting each node's min/max htlc correctly, see next commit.
…rams.
In testing, we use channel updates to construct blinded paths and the
{Forward,Receive}Tlvs encoded within. Given a blinded path from node A > B > C,
we currently use channel_update_A->B to construct the payment constraints for
A’s blinded payload.
This is incorrect for setting A's PaymentConstraints::htlc_minimum_msat,
because channel_update_A->B contains the minimum value that *B* will accept,
and we want the constraints to contain the min value that *A* will accept.
This never caused test failures before because min/max htlc values were always
identical in both channel directions.
Therefore, set A’s htlc min/max values to the min/max that A will accept.
See ClaimAlongRouteArgs::allow_1_msat_fee_overpay.
See docs on the new field.
See comment in the new test.
valentinewallace
force-pushed
the
2024-01-blinded-forwarding-tests
branch
from
33f16de
to
2027bf4
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/ln/blinded_payment_tests.rs (15 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (14 hunks)
- lightning/src/ln/offers_tests.rs (1 hunks)
- lightning/src/ln/reload_tests.rs (1 hunks)
- lightning/src/ln/shutdown_tests.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/blinded_payment_tests.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (6)
- lightning/src/ln/chanmon_update_fail_tests.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/offers_tests.rs
- lightning/src/ln/reload_tests.rs
- lightning/src/ln/shutdown_tests.rs
- lightning/src/routing/router.rs
Additional comments: 2
lightning/src/ln/blinded_payment_tests.rs (2)
- 183-245: The test
mpp_to_three_hop_blinded_pathsis well-structured and covers the scenario of multi-part payments (MPP) over 3-hop blinded paths effectively. It simulates a network topology and tests the payment forwarding and claiming process across multiple paths. This test is crucial for ensuring the robustness of MPP over blinded paths.Consider adding more inline comments explaining the setup and assertions for better readability and maintainability, especially for new contributors or when revisiting this test in the future.
- 1066-1134: The test
min_htlceffectively demonstrates the calculation and enforcement of the minimum HTLC amount along a blinded path. It's a critical test for ensuring that the payment forwarding logic respects the minimum HTLC amount constraints.To enhance the test's clarity and maintainability, consider adding more detailed comments explaining the rationale behind the calculations and the expected outcomes. This would help future maintainers understand the purpose and mechanics of the test more easily.
|
Squashed! |
Adds test coverage for forwarding blinded payments as not-the-intro-node, and for long blinded paths and their fees generally. Also includes some test util refactors that should save us some time going forward.
Based on #2812.