-
Notifications
You must be signed in to change notification settings - Fork 345
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
Fix keysend not being sent in OutboundPayment::send_payment_internal
#2475
Fix keysend not being sent in OutboundPayment::send_payment_internal
#2475
Conversation
Fixes a bug where we wouldn't use the provided keysend preimage when piping through OutboundPayment::pay_route_internal. Also simplifies and refactors existing keysend tests to make sure this gets hit.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2475 +/- ##
==========================================
- Coverage 90.34% 90.34% -0.01%
==========================================
Files 106 106
Lines 55784 55771 -13
Branches 55784 55771 -13
==========================================
- Hits 50398 50386 -12
+ Misses 5386 5385 -1
☔ View full report in Codecov by Sentry. |
nodes[0].logger, &scorer, &(), &random_seed_bytes | ||
).unwrap(); | ||
|
||
let test_preimage = PaymentPreimage([42; 32]); |
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.
The real issue is that the test just reused the preimage instead of extracting it from the event. Can you put the preimage and send in a {}, that way the claiming logic further down has no access to it at all.
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.
I think that's okay since pass_along_path
checks that the preimage from Event::PaymentClaimable
matches the expected one passed in?
I had mainly noticed that a problem was that we never tested a full payment using spontaneous_with_retry
which uses OutboundPayment::send_payment_internal
whereas the method without retries doesn't
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.
Not really a big deal, I wrote up more of what I was suggesting as #2481, which I'm fine with not merging if anyone disagrees, but I like the explicit scope to demonstrate information lifetime.
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, new test fails when I revert the change. Not the first time we've unknowingly broken keysend :( We have existing coverage of keysend retrying when initiated with send_spontaneous_payment_with_retry
, but were missing first-try-success coverage.
I don't quite get @TheBlueMatt's comment but it sounds like it can be addressed in follow-up so I'll merge
Following up on #2465. I kept thinking that it doesn't make sense that we'd reject a keysend payment based on our feature bit not being set. After some digging, I found the issue was that we weren't sending the preimage.