Explicitly reject routes that double-back

[shaavan]

Resolves#2215

• If a path within a route passes through the same channelID twice, that shows the path is looped and will be rejected by nodes.
• Add a check to explicitly reject such payments before trying to send them.

[tnull]

Thanks for taking a stab at this!

[shaavan]

Updated from pr2741.01 -> pr2741.02 (diff)
Addressed @tnull suggestion

• Modified the code to use nested for loops instead of a HashSet. Since the number of routeHops will always be very small (<20 for a path), this approach optimized the space complexity over time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (870a0f1) to head (a06d158).
⚠️ Report is 4939 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2741 +/- ##
==========================================
- Coverage 88.55% 88.52% -0.04% 
==========================================
Files 113 113 Lines 89330 89351 +21 Branches 89330 89351 +21 ==========================================
- Hits 79110 79097 -13 - Misses 7849 7877 +28 - Partials 2371 2377 +6 

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Pretty much looks good to me. Could we add some test coverage for it though?

You could add a tiny test case checking that we receive the correct APIError if we try to use a looping route. The setup should be similar to, e.g., fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() in functional_tests.rs.

[shaavan]

Updated from pr2741.02 -> pr2741.03 (diff)
Addressed @tnull comments

• Added test coverage for the added code.
• Also modified the unwrap_send_error!() macro to handle PaymentPathFailure case.

Thanks, @tnull, for the suggestion and for providing a reference to take inspiration for writing up the test.

[tnull]

LGTM

[shaavan]

Resolves#2215

• If a path within a route passes through the same channelID twice, that shows the path is looped and will be rejected by nodes.
• Add a check to explicitly reject such payments before trying to send them.

[tnull]

Thanks for taking a stab at this!

[shaavan]

Updated from pr2741.01 -> pr2741.02 (diff)
Addressed @tnull suggestion

• Modified the code to use nested for loops instead of a HashSet. Since the number of routeHops will always be very small (<20 for a path), this approach optimized the space complexity over time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (870a0f1) to head (a06d158).
⚠️ Report is 4939 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2741 +/- ##
==========================================
- Coverage 88.55% 88.52% -0.04% 
==========================================
Files 113 113 Lines 89330 89351 +21 Branches 89330 89351 +21 ==========================================
- Hits 79110 79097 -13 - Misses 7849 7877 +28 - Partials 2371 2377 +6 

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Pretty much looks good to me. Could we add some test coverage for it though?

You could add a tiny test case checking that we receive the correct APIError if we try to use a looping route. The setup should be similar to, e.g., fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() in functional_tests.rs.

[shaavan]

Updated from pr2741.02 -> pr2741.03 (diff)
Addressed @tnull comments

• Added test coverage for the added code.
• Also modified the unwrap_send_error!() macro to handle PaymentPathFailure case.

Thanks, @tnull, for the suggestion and for providing a reference to take inspiration for writing up the test.

[tnull]

LGTM

[shaavan]

Resolves#2215

• If a path within a route passes through the same channelID twice, that shows the path is looped and will be rejected by nodes.
• Add a check to explicitly reject such payments before trying to send them.

[tnull]

Thanks for taking a stab at this!

[shaavan]

Updated from pr2741.01 -> pr2741.02 (diff)
Addressed @tnull suggestion

• Modified the code to use nested for loops instead of a HashSet. Since the number of routeHops will always be very small (<20 for a path), this approach optimized the space complexity over time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (870a0f1) to head (a06d158).
⚠️ Report is 4939 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2741 +/- ##
==========================================
- Coverage 88.55% 88.52% -0.04% 
==========================================
Files 113 113 Lines 89330 89351 +21 Branches 89330 89351 +21 ==========================================
- Hits 79110 79097 -13 - Misses 7849 7877 +28 - Partials 2371 2377 +6 

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Pretty much looks good to me. Could we add some test coverage for it though?

You could add a tiny test case checking that we receive the correct APIError if we try to use a looping route. The setup should be similar to, e.g., fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() in functional_tests.rs.

[shaavan]

Updated from pr2741.02 -> pr2741.03 (diff)
Addressed @tnull comments

• Added test coverage for the added code.
• Also modified the unwrap_send_error!() macro to handle PaymentPathFailure case.

Thanks, @tnull, for the suggestion and for providing a reference to take inspiration for writing up the test.

[tnull]

LGTM

[shaavan]

Resolves#2215

• If a path within a route passes through the same channelID twice, that shows the path is looped and will be rejected by nodes.
• Add a check to explicitly reject such payments before trying to send them.

[tnull]

Thanks for taking a stab at this!

[shaavan]

Updated from pr2741.01 -> pr2741.02 (diff)
Addressed @tnull suggestion

• Modified the code to use nested for loops instead of a HashSet. Since the number of routeHops will always be very small (<20 for a path), this approach optimized the space complexity over time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (870a0f1) to head (a06d158).
⚠️ Report is 4939 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2741 +/- ##
==========================================
- Coverage 88.55% 88.52% -0.04% 
==========================================
Files 113 113 Lines 89330 89351 +21 Branches 89330 89351 +21 ==========================================
- Hits 79110 79097 -13 - Misses 7849 7877 +28 - Partials 2371 2377 +6 

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Pretty much looks good to me. Could we add some test coverage for it though?

You could add a tiny test case checking that we receive the correct APIError if we try to use a looping route. The setup should be similar to, e.g., fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() in functional_tests.rs.

[shaavan]

Updated from pr2741.02 -> pr2741.03 (diff)
Addressed @tnull comments

• Added test coverage for the added code.
• Also modified the unwrap_send_error!() macro to handle PaymentPathFailure case.

Thanks, @tnull, for the suggestion and for providing a reference to take inspiration for writing up the test.

[tnull]

LGTM

[shaavan]

Resolves#2215

• If a path within a route passes through the same channelID twice, that shows the path is looped and will be rejected by nodes.
• Add a check to explicitly reject such payments before trying to send them.

[tnull]

Thanks for taking a stab at this!

[shaavan]

Updated from pr2741.01 -> pr2741.02 (diff)
Addressed @tnull suggestion

• Modified the code to use nested for loops instead of a HashSet. Since the number of routeHops will always be very small (<20 for a path), this approach optimized the space complexity over time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (870a0f1) to head (a06d158).
⚠️ Report is 4939 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2741 +/- ##
==========================================
- Coverage 88.55% 88.52% -0.04% 
==========================================
Files 113 113 Lines 89330 89351 +21 Branches 89330 89351 +21 ==========================================
- Hits 79110 79097 -13 - Misses 7849 7877 +28 - Partials 2371 2377 +6 

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Pretty much looks good to me. Could we add some test coverage for it though?

You could add a tiny test case checking that we receive the correct APIError if we try to use a looping route. The setup should be similar to, e.g., fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() in functional_tests.rs.

[shaavan]

Updated from pr2741.02 -> pr2741.03 (diff)
Addressed @tnull comments

• Added test coverage for the added code.
• Also modified the unwrap_send_error!() macro to handle PaymentPathFailure case.

Thanks, @tnull, for the suggestion and for providing a reference to take inspiration for writing up the test.

[tnull]

LGTM

[shaavan]

Resolves#2215

• If a path within a route passes through the same channelID twice, that shows the path is looped and will be rejected by nodes.
• Add a check to explicitly reject such payments before trying to send them.

[tnull]

Thanks for taking a stab at this!

[shaavan]

Updated from pr2741.01 -> pr2741.02 (diff)
Addressed @tnull suggestion

• Modified the code to use nested for loops instead of a HashSet. Since the number of routeHops will always be very small (<20 for a path), this approach optimized the space complexity over time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (870a0f1) to head (a06d158).
⚠️ Report is 4939 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2741 +/- ##
==========================================
- Coverage 88.55% 88.52% -0.04% 
==========================================
Files 113 113 Lines 89330 89351 +21 Branches 89330 89351 +21 ==========================================
- Hits 79110 79097 -13 - Misses 7849 7877 +28 - Partials 2371 2377 +6 

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Pretty much looks good to me. Could we add some test coverage for it though?

You could add a tiny test case checking that we receive the correct APIError if we try to use a looping route. The setup should be similar to, e.g., fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() in functional_tests.rs.

[shaavan]

Updated from pr2741.02 -> pr2741.03 (diff)
Addressed @tnull comments

• Added test coverage for the added code.
• Also modified the unwrap_send_error!() macro to handle PaymentPathFailure case.

Thanks, @tnull, for the suggestion and for providing a reference to take inspiration for writing up the test.

[tnull]

LGTM

[shaavan]

Resolves#2215

• If a path within a route passes through the same channelID twice, that shows the path is looped and will be rejected by nodes.
• Add a check to explicitly reject such payments before trying to send them.

[tnull]

Thanks for taking a stab at this!

[shaavan]

Updated from pr2741.01 -> pr2741.02 (diff)
Addressed @tnull suggestion

• Modified the code to use nested for loops instead of a HashSet. Since the number of routeHops will always be very small (<20 for a path), this approach optimized the space complexity over time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (870a0f1) to head (a06d158).
⚠️ Report is 4939 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2741 +/- ##
==========================================
- Coverage 88.55% 88.52% -0.04% 
==========================================
Files 113 113 Lines 89330 89351 +21 Branches 89330 89351 +21 ==========================================
- Hits 79110 79097 -13 - Misses 7849 7877 +28 - Partials 2371 2377 +6 

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Pretty much looks good to me. Could we add some test coverage for it though?

You could add a tiny test case checking that we receive the correct APIError if we try to use a looping route. The setup should be similar to, e.g., fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() in functional_tests.rs.

[shaavan]

Updated from pr2741.02 -> pr2741.03 (diff)
Addressed @tnull comments

• Added test coverage for the added code.
• Also modified the unwrap_send_error!() macro to handle PaymentPathFailure case.

Thanks, @tnull, for the suggestion and for providing a reference to take inspiration for writing up the test.

[tnull]

LGTM