#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! 🎉