#2585 Preflight Test Coverage

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉

[alexanderwiederin]

Closes#2585.

• Refactors message handling for successful probe tests
• Adds test coverage for preflight probes (skipping private channel hops and not skipping any paths)

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (a38bdbe) 90.44%.
Report is 117 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2641 +/- ##
==========================================
+ Coverage 89.04% 90.44% +1.40% 
==========================================
Files 112 112 Lines 87229 99397 +12168 Branches 87229 99397 +12168 ==========================================
+ Hits 77674 89902 +12228 + Misses 7319 7276 -43 + Partials 2236 2219 -17 

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	98.41% <100.00%> (+0.21%)	⬆️
lightning/src/ln/reload_tests.rs	96.99% <100.00%> (+0.74%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.05% <96.29%> (+2.47%)	⬆️

... and 43 files with indirect coverage changes

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

[tnull]

Thanks for having a go at this! Already looks pretty good. I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

[alexanderwiederin]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.

2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.

3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

[tnull]

@tnull, appreciate your insights!

I however wonder if we somehow can keep the logic for 'send payment/probe along the forward path' and the 'return path' separate?

Agree that it would be nicer to complete all forward passes before passing failures back along the paths. The difficulty in this scenario, however, is that the commitment signed dance fails on the last hop of the second path. On that hop, we assert (do_commitment_signed_dance) that the receiving node does not have any pending message events. The issue is that the receiving node has already received the first probe (a failed payment), resulting in a pending update_fail_htlcs event.

From what I understand we have the following options:

1. Fail back (only) the last hop before continuing: Introduce a mechanism to fail back only the last hop before invoking do_pass_along_path for the next probe. After sending all probes and completing the first "return hop" for each path, we can finalise the roundtrips for all paths.
2. Adapt commitment_signed_dance for probes: Modify the commitment_signed_dance functions to accommodate pending message events for receiving nodes specifically in the case of probes.
3. Continue with staggered approach: Persist with the staggered approach, completing the round trip for a probe before initiating the next.

In my view, 1. introduces unnecessary complexity without a proportional benefit. Option 2, while potentially addressing our immediate concern, raises the risk of loosening the tests in other cases. Consequently, I lean towards 3. as it seems to strike a better balance.

Eager to hear your thoughts - let me know if there is another option I did not think of!

Alright, I think this makes sense to me!

[tnull]

Looks pretty much good from my side, CI failures should be unrelated, I think.

[TheBlueMatt]

Thanks! One nit.

[valentinewallace]

Looks good, thanks! 🎉