-
Notifications
You must be signed in to change notification settings - Fork 404
Add test coverage for failure of inconsistent MPP parts #1451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test coverage for failure of inconsistent MPP parts #1451
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1451 +/- ##
==========================================
+ Coverage 90.88% 90.92% +0.03%
==========================================
Files 75 75
Lines 41474 41663 +189
Branches 41474 41663 +189
==========================================
+ Hits 37695 37881 +186
- Misses 3779 3782 +3
Continue to review full report at Codecov.
|
a7e5050
to
958fa92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I need only to check in deep the last commit (958fa92)
|
||
let failure_events = nodes[0].node.get_and_clear_pending_events(); | ||
assert_eq!(failure_events.len(), 2); | ||
if let Event::PaymentPathFailed { .. } = failure_events[0] {} else { panic!(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it appears to me here that with the second fail we're actually not panicking. Is the boolean a misnomer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, its a bit confusing its really "test that the second fail doesnt panic". I force-pushed a rename to test_for_second_fail_panic
to make it clearer.
958fa92
to
acdd2dc
Compare
When we receive multiple HTLCs which claim to be a part of the same MPP but which are inconsistent for some reason, we should fail the inconsistent HTLCs but keep the first HTLCs up until the first inconsistency. This works, but it turns out there was no test coverage, so we add some here.
acdd2dc
to
7a8344a
Compare
Rebased after merge of #1435 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still lgtm
When we receive multiple HTLCs which claim to be a part of the same
MPP but which are inconsistent for some reason, we should fail the
inconsistent HTLCs but keep the first HTLCs up until the first
inconsistency.
This works, but it turns out there was no test coverage, so we add
some here.
Discovered when CI didn't fail even though I noted #1447 (comment)
Based on #1435 because it makes testing MPP routes a ton easier.