-
Notifications
You must be signed in to change notification settings - Fork 418
feat: Implement self payment #3934
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
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
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 it would be preferable if we'd avoid adding a special API for self-payments at this point. Can't we just detect that it's a self payment in send_payment
and act accordingly?
lightning/src/ln/channelmanager.rs
Outdated
/// | ||
/// # Returns | ||
/// Returns the payment hash on success, or an APIError on failure. | ||
pub fn send_self_payment( |
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.
Hmm, not sure we want to add a special API just for this 1-hop circular route. Can we reuse the existing APIs, i.e. detect that we're the recipient and act accordingly? Then, for #3791, we'll need to add an API that allows longer circular routes.
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 originally thought of using send_payment_with_route
since we know the route we want the payment to take . I should be able to incorporate it in that .
lightning/src/util/scid_utils.rs
Outdated
// Self payment scid is 0 to avoid confusion with real channels | ||
/// A special short channel ID (0) used to indicate a self-payment. | ||
/// When routing a payment to ourselves, the first hop will have this SCID. | ||
pub const SCID_SELF_PAYMENT: u64 = 0; |
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.
Hmm, when handling HTLCs we're already leaning on the short_channel_id == 0
special case for receiving. So I guess that's preexisting. Still, I'm not the biggest fan of such magic numbers. Do you think there would be a way to avoid this?
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.
A way I thought of is to change Route
or RouteParameters
to include a is_self_payment
flag which can be used to detect if a payment is a self-payment or we could use if path.hops.first().unwrap().pubkey == our_node_pubkey
but I don't feel good about the idea of having to pass our_node_pubkey
deep in the api calls to send_payment_along_path
. either way we would want to have a magic number for dummy scid (not specific for self-payments but general ) and settle on it ?
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.
or we could use
if path.hops.first().unwrap().pubkey == our_node_pubkey
but I don't feel good about the idea of having to passour_node_pubkey
deep in the api calls tosend_payment_along_path
Hmm, but in ChannelManager
(hence also in send_payment_along_path
) we should have our pubkey readily available at self.our_network_pubkey
, no?
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
955c11c
to
7d78e4e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3934 +/- ##
==========================================
+ Coverage 88.84% 90.80% +1.96%
==========================================
Files 166 167 +1
Lines 119487 143472 +23985
Branches 119487 143472 +23985
==========================================
+ Hits 106157 130285 +24128
+ Misses 11006 10722 -284
- Partials 2324 2465 +141
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Please undraft/request review when you think this is ready for another look or have any more particular questions!
fixes #2462
revives #2573
This PR implements the option for self payment for ldk nodes .
PR is right now marked as draft as this is a very rough implementation and I am looking for suggestions / improvements , some of them
1 . should an option to make BlindedPath for self-routes be made ?
2 . should MPP be allowed ? (Personally I think this is a bad idea , we should use BlindedPath instead if we want privacy otherwise normal way should suffice)
3. Should all self-payments be spontaneous ? an observer might be able to figure out that that we are paying to ourselves if there are a lot of spontaneous payments !
4. My bad code ?