Handle self payments #2573
Handle self payments #2573
Conversation
|
I'd really like for this to be transparent from the The other thing we need here is that |
lightning/src/ln/channelmanager.rs
Outdated
|
|
||
| // Put payment in the outbound payment set as fulfilled | ||
| let mut pending_outbounds_lock = self.pending_outbound_payments.pending_outbound_payments.lock().unwrap(); | ||
| match pending_outbounds_lock.entry(payment_id) { |
There was a problem hiding this comment.
we should add a entry with pending status before, to avoid race condition and sending duplicate PaymentSent msgs.
lightning-invoice/src/payment.rs
Outdated
| @@ -159,8 +161,12 @@ fn pay_invoice_using_amount<P: Deref>( | |||
| payment_params, | |||
| final_value_msat: amount_msats, | |||
| }; | |||
|
|
|||
| payer.send_payment(payment_hash, recipient_onion, payment_id, route_params, retry_strategy) | |||
| if payee_pubkey == payer.get_payer_pub_key() { | |||
There was a problem hiding this comment.
we might also need to consider phantom nodes, whenver we do this, acc. to matt's suggestion and handling it in channel_manager
vladimirfomene
force-pushed
the
allow-self-payment
branch
6 times, most recently
from
3d9456c
to
14d8d4a
Compare
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
14d8d4a
to
8a06076
Compare
|
Please, this is ready for another round of reviews. |
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
8a06076
to
faafd82
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2573 +/- ##
==========================================
+ Coverage 89.15% 89.93% +0.77%
==========================================
Files 117 117
Lines 94934 99484 +4550
Branches 94934 99484 +4550
==========================================
+ Hits 84641 89468 +4827
+ Misses 7811 7570 -241
+ Partials 2482 2446 -36 ☔ View full report in Codecov by Sentry. |
lightning/src/ln/channelmanager.rs
Outdated
| @@ -3405,8 +3405,17 @@ where | |||
| pub fn send_payment(&self, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result<(), RetryableSendFailure> { | |||
| let best_block_height = self.best_block.read().unwrap().height(); | |||
| let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | |||
| let preimage = match route_params.payment_params.payee { | |||
There was a problem hiding this comment.
What about sending a spontaneous payment to ourselves?
|
@TheBlueMatt sorry I have been busy into other repos. Will look at your feedback this week. |
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
faafd82
to
dcf972c
Compare
|
Hi @TheBlueMatt ! I decided to try a new approach which I think might be an improvement on the previous approach. Let me know what you think. I also had a question. Don't we want to allow self payment in the case of a rebalance for example. Will this implementation not break our rebalancing functionality if we have one? |
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
dcf972c
to
b7bb1e6
Compare
Looks much cleaner, thanks! However, I realized I gave you bad advice at the outset here, we don't really need/want to have a special-case
Yes, we should allow users to pay with a route if they want. We currently have no problem handling this (AFAIK, at least it works with the manual path sending flow), so we'll want to maintain this. If a user returns a route from the router which goes back to them, we should use that, we should only shortcut the self-payment if we get an error, I think. |
vladimirfomene
force-pushed
the
allow-self-payment
branch
2 times, most recently
from
b3c1779
to
eb04f3e
Compare
|
@TheBlueMatt, thanks for the direction. I have updated as recommended. |
vladimirfomene
force-pushed
the
allow-self-payment
branch
2 times, most recently
from
92bc296
to
30f44af
Compare
lightning/src/ln/outbound_payment.rs
Outdated
| pending_events_lock.push_back((Event::PaymentClaimable { receiver_node_id: Some(payer), payment_hash, | ||
| onion_fields: Some(recipient_onion), amount_msat: route_params.final_value_msat, counterparty_skimmed_fee_msat: 0, | ||
| purpose: payment_purpose, via_channel_id: None, via_user_channel_id: None, claim_deadline: None }, None)); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
I think we should be generating PaymentClaimed instead of PaymentClaimable. The latter may cause the user to call claim_funds, which isn't harmful but is unnecessary IIUC.
There was a problem hiding this comment.
Good point, will make necessary changes.
There was a problem hiding this comment.
In general principle of idempotency, self-payments should ideally go through same set of events as a normal payment would.
I am not sure but what if user has some extra logic while handling PaymentClaimable.
In any case, if we choose to go this route to directly generate PaymentClaimed then we should update the docs on top of it to include this scenario.
There was a problem hiding this comment.
@G8XSU, you are right. I think adding documentation here will definitely help users.
There was a problem hiding this comment.
In general principle of idempotency, self-payments should ideally go through same set of events as a normal payment would. I am not sure but what if user has some extra logic while handling
PaymentClaimable.In any case, if we choose to go this route to directly generate
PaymentClaimedthen we should update the docs on top of it to include this scenario.
I was wondering about this too, like should we fully replicate the steps of a normal payment? That would mean we should generate PaymentClaimable, then have logic in ChannelManager::claim_funds and ::fail_htlc to handle this case and generate the final Payment{Claimed,Failed} events. Not sure if it's overkill though.
lightning/src/ln/outbound_payment.rs
Outdated
| if error.err == "Cannot generate a route to ourselves" { | ||
| let payment_secret = match recipient_onion.payment_secret { | ||
| Some(secret) => secret, | ||
| None => PaymentSecret([0; 32]) |
There was a problem hiding this comment.
Not sure this should ever be set to a dummy value. @TheBlueMatt Do we want to inbound_payment::verify the payment secret here? I'd think that's how we're mitigating the self-payment attack described on the ML.
There was a problem hiding this comment.
Yea, we should fail to send if we're "receiving" without a payment secret and should in fact verify it like any other incoming payment.
There was a problem hiding this comment.
@TheBlueMatt w/r/t treating this like any other payment, do we want to give users the opportunity to fail this payment? cc #2573 (comment)
There was a problem hiding this comment.
Makes sense to me, yea. Service providers should definitely have that option.
There was a problem hiding this comment.
@vladimirfomene I think senders still don't have the opportunity to fail these payments, it looks like they're always claimed immediately?
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
30f44af
to
ec170bd
Compare
lightning/src/ln/outbound_payment.rs
Outdated
| ) { | ||
| Ok(res) => Some(res), | ||
| Err(error) => { | ||
| if error.err == "Cannot generate a route to ourselves" { |
There was a problem hiding this comment.
Because the router is a public interface we shouldn't rely on the error message itself (a user could implement the router to use any error messages they want). Instead, we should just match on error + the target is our own node.
lightning/src/ln/outbound_payment.rs
Outdated
| if error.err == "Cannot generate a route to ourselves" { | ||
| let payment_secret = match recipient_onion.payment_secret { | ||
| Some(secret) => secret, | ||
| None => PaymentSecret([0; 32]) |
There was a problem hiding this comment.
Yea, we should fail to send if we're "receiving" without a payment secret and should in fact verify it like any other incoming payment.
lightning/src/ln/outbound_payment.rs
Outdated
| Some(secret) => secret, | ||
| None => PaymentSecret([0; 32]) | ||
| }; | ||
| let payment_preimage = PaymentPreimage([0; 32]); |
There was a problem hiding this comment.
We should use inbound_payment::get_payment_preimage for this, not set it to a dummy value
|
@valentinewallace @TheBlueMatt find in bd3f20c the new approach. Let me know what you think. If this is good by you, I will improve the test. |
vladimirfomene
force-pushed
the
allow-self-payment
branch
2 times, most recently
from
d000805
to
3eb1da6
Compare
There was a problem hiding this comment.
Grrrrr, so thinking about this more I do feel like we need to go through the PaymentClaimable path before actually generating PaymentSent/PaymentClaimed. That's gonna be a good bit more work, but some users will reject payments based on their own criteria (eg Cash App rejects inbound HTLCs if the user is over a KYC/AML receive limit) and we'd be effectively overriding that if we jump to PaymentClaimed as we do here. If we go through the PaymentClaimable path we'd also get support for user-provided payment preimages, which would be nice.
Sadly, to do that we'll need a chunk more work - we'll need to check for self-payments in claim_funds. We may need a new pending inbound payment variant for that :/
lightning/src/ln/channelmanager.rs
Outdated
| }; | ||
| if node_id == self.get_our_node_id() || is_phantom_payee { | ||
| // check if this is a self payment. If so, we verify that we are paying the right amount. | ||
| if let Ok(payment_preimage) = self.get_payment_preimage(payment_hash, recipient_onion.payment_secret.unwrap()) { |
There was a problem hiding this comment.
Hmmm, so this is a bit annoying. We support receiving payments both to an LDK-derived payment preimage as well as to a user-provided one. So I think we need to call inbound_payment::verify first, fail if it fails, and if it succeeds then move forward whether we have the preimage or not.
lightning/src/ln/channelmanager.rs
Outdated
| // check if this is a self payment. If so, we verify that we are paying the right amount. | ||
| if let Ok(payment_preimage) = self.get_payment_preimage(payment_hash, recipient_onion.payment_secret.unwrap()) { | ||
| preimage = Some(payment_preimage); | ||
| let payment_data = msgs::FinalOnionHopData{ payment_secret: recipient_onion.payment_secret.unwrap(), total_msat: route_params.final_value_msat}; |
There was a problem hiding this comment.
nit: you can drop the unwrap here (and above) if you swap the top if for an if let Some(..)
|
@TheBlueMatt thanks for the feedback. I really appreciate. Allow me to enhance based on your recommendation. |
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
3eb1da6
to
1dfd1f1
Compare
WalkthroughThe recent update introduces the capability to manage self payments within the Lightning network. It encompasses the addition of structures to track and handle claimable self payments, alongside modifications in the payment process to support this feature. The changes span across several files, enhancing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/ln/outbound_payment.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (1 hunks)
Files not reviewed due to errors (1)
- lightning/src/ln/payment_tests.rs (Error: unable to parse review)
Additional comments: 5
lightning/src/ln/outbound_payment.rs (4)
- 419-420: The addition of the
RecipientRejectedvariant to theRetryableSendFailureenum is appropriate for representing a new failure state where the recipient explicitly rejects a payment. This change aligns with the PR's objective to handle self-payments and related error states more gracefully.- 679-685: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [682-696]
The modification of the
send_paymentfunction to include an optionalpayment_preimageparameter is correctly implemented. This change enables the function to support self-payments by directly providing the payment preimage, which is essential for the feature introduced in this PR.
- 904-959: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [887-915]
The modification of the
send_payment_internalfunction to accept an optionalpayment_preimageparameter and the addition of logic to handle self-payments directly within the function are correctly implemented. The changes are consistent with the PR's objectives and are well-integrated into the existing payment flow.
- 915-950: The logic added to
send_payment_internalfor handling self-payments, including the generation of a dummy route and the immediate marking of the payment as fulfilled, is correctly implemented. This approach effectively bypasses the normal routing process for self-payments, aligning with the PR's goals to streamline the handling of such payments.lightning/src/ln/channelmanager.rs (1)
- 552-552: The addition of
amount_msatto theClaimablePaymentstruct is a significant change. Ensure that all instances whereClaimablePaymentis instantiated have been updated to include this new field. This change impacts the data structure's compatibility and serialization logic, so it's crucial to verify that these aspects have been adequately addressed.
lightning/src/ln/channelmanager.rs
Outdated
| let mut preimage: Option<PaymentPreimage> = None; | ||
| let mut payment_secret = PaymentSecret([0; 32]); | ||
| let mut is_self_pay = false; | ||
| if let Some(secret) = recipient_onion.payment_secret { | ||
| payment_secret = secret; | ||
| if let Payee::Clear{node_id, .. } = route_params.payment_params.payee { | ||
| let is_phantom_payee = match self.node_signer.get_node_id(Recipient::PhantomNode) { | ||
| Ok(phantom_node_id) => node_id == phantom_node_id, | ||
| Err(_) => false, | ||
| }; | ||
| if node_id == self.get_our_node_id() || is_phantom_payee { | ||
| let payment_data = msgs::FinalOnionHopData{ payment_secret, total_msat: route_params.final_value_msat}; | ||
| preimage = inbound_payment::verify(payment_hash, &payment_data, self.highest_seen_timestamp.load(Ordering::Acquire) as u64, &self.inbound_payment_key, &self.logger).map_err(|_| RetryableSendFailure::RecipientRejected)?.0; | ||
| is_self_pay = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| self.pending_outbound_payments | ||
| .send_payment(payment_hash, recipient_onion, payment_id, retry_strategy, route_params, | ||
| .send_payment(payment_hash, recipient_onion.clone(), payment_id, retry_strategy, route_params.clone(), preimage, | ||
| &self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(), | ||
| &self.entropy_source, &self.node_signer, best_block_height, &self.logger, | ||
| &self.pending_events, |args| self.send_payment_along_path(args)) | ||
| &self.pending_events, |args| self.send_payment_along_path(args))?; | ||
|
|
||
| if is_self_pay { | ||
| let mut claimable_payments = self.claimable_payments.lock().unwrap(); | ||
| let purpose = events::PaymentPurpose::InvoicePayment { payment_preimage: preimage, payment_secret }; | ||
| claimable_payments.claimable_payments.insert(payment_hash, ClaimablePayment{ purpose: purpose.clone(), onion_fields: Some(recipient_onion.clone()), htlcs: vec![], amount_msat: Some(route_params.final_value_msat)}); | ||
| let mut pending_events = self.pending_events.lock().unwrap(); | ||
| pending_events.push_back((events::Event::PaymentClaimable { receiver_node_id: Some(self.get_our_node_id()), payment_hash, onion_fields: Some(recipient_onion), amount_msat: route_params.final_value_msat, counterparty_skimmed_fee_msat: 0, purpose, via_channel_id: None, via_user_channel_id: None, claim_deadline: None }, None)); | ||
| } |
There was a problem hiding this comment.
The implementation of self-payment handling within send_payment introduces several new variables and logic branches. This complexifies the function significantly. Consider refactoring to improve readability and maintainability. Specifically, extracting the self-payment detection and handling logic into a separate function could make the code cleaner and easier to follow. Additionally, ensure that the error handling for inbound_payment::verify is robust and that all possible failure modes are considered.
lightning/src/ln/channelmanager.rs
Outdated
|
|
||
|
|
||
| let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | ||
|
|
||
| let mut sources = { | ||
| let mut claimable_payments = self.claimable_payments.lock().unwrap(); | ||
| if let Some(payment) = claimable_payments.claimable_payments.remove(&payment_hash) { | ||
| let mut receiver_node_id = self.our_network_pubkey; | ||
| if let events::PaymentPurpose::InvoicePayment { payment_secret, .. } = payment.purpose { | ||
| if let Ok(_) = self.get_payment_preimage(payment_hash, payment_secret) { | ||
| let mut pending_events_lock = self.pending_events.lock().unwrap(); | ||
| pending_events_lock.push_back((Event::PaymentClaimed { receiver_node_id: Some(receiver_node_id), payment_hash, | ||
| amount_msat: 0, purpose: payment.purpose, htlcs: vec![], sender_intended_total_msat: None }, None)); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
In claim_payment_internal, the handling of PaymentClaimed events does not update the amount_msat field, which remains hardcoded to 0. This oversight could lead to inaccurate event data, especially in scenarios where the actual claimed amount is crucial for auditing or user notifications. Update the event generation logic to include the correct amount_msat value from the claimable payment.
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
1dfd1f1
to
c1a2bf4
Compare
|
Hi @TheBlueMatt ! Will love to get your feedback on the new code. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/ln/outbound_payment.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/payment_tests.rs
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
c1a2bf4
to
1fa7497
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/ln/outbound_payment.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/payment_tests.rs
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
1fa7497
to
015c2ba
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/ln/outbound_payment.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/payment_tests.rs
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
015c2ba
to
dcb3512
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/ln/outbound_payment.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/payment_tests.rs
| if node_id == self.get_our_node_id() || is_phantom_payee { | ||
| let payment_data = msgs::FinalOnionHopData{ payment_secret, total_msat: route_params.final_value_msat }; | ||
| preimage = inbound_payment::verify(payment_hash, &payment_data, self.highest_seen_timestamp.load(Ordering::Acquire) as u64, &self.inbound_payment_key, &self.logger).map_err(|_| RetryableSendFailure::RecipientRejected)?.0; | ||
| is_self_pay = true; |
There was a problem hiding this comment.
We haven't fully decided if this is a self payment or not at this point - we call send_payment_internal which may call find_route_with_id and get a rebalance route rather than an actual send-to-self. Thus, we need to check with the outbound payments logic to see if its really a self-pay first.
There was a problem hiding this comment.
Our current code in find_route_with_id will throw an error if you attempt to find a route to yourself, so I don't think you will get a rebalance route.
There was a problem hiding this comment.
Our router, yes, but we cannot rely on that as users can provide their own custom router.
|
Thanks @TheBlueMatt for the review. I will amend asap. |
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
dcb3512
to
5e92105
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/channelmanager.rs (13 hunks)
- lightning/src/ln/outbound_payment.rs (4 hunks)
- lightning/src/ln/payment_tests.rs (1 hunks)
Additional comments: 19
lightning/src/ln/outbound_payment.rs (2)
- 22-22: The import of
Payeefromcrate::routing::routeris added.This change is straightforward and aligns with the modifications in the file to support handling self-payments more effectively.
- 424-425: A new variant
RecipientRejectedis added to theRetryableSendFailureenum.This addition is necessary to represent a new failure mode where the intended recipient of a payment rejects it. This is particularly relevant for the handling of self-payments and enhances the robustness of error handling.
lightning/src/ln/payment_tests.rs (7)
- 1553-1554: The addition of the
test_self_paymentfunction is a crucial step in ensuring the new self-payment functionality works as expected. It's good to see comprehensive testing around this feature.- 1555-1558: The setup for the test network and nodes is done correctly using the existing helper functions. This ensures that the test environment is isolated and consistent.
- 1559-1565: Creating an inbound payment and setting up the payment and route parameters are done correctly. However, it's important to ensure that the values used (e.g.,
final_value_msat: 100000) are representative of typical use cases to make the test as realistic as possible.- 1566-1567: The call to
send_paymentwithRetry::Attempts(0)is an interesting choice. It's worth verifying that this retry behavior is intentional and aligns with the expected behavior for self-payments, especially in testing scenarios.Verification successful
The use of
Retry::Attempts(0)is found consistently across multiple instances in thepayment_tests.rsfile, indicating that this retry behavior is an intentional choice for testing scenarios, particularly for self-payments. This consistency supports the original comment's inquiry about verifying the intentional use of such retry behavior.* 1568-1571: The assertion that only one event is generated and it's of type `PaymentClaimable` is a good check. It verifies that the initial part of the self-payment process (creating and sending the payment) triggers the expected event. * 1572-1574: Checking the pending payments and ensuring the payment is in the pending state before claiming it is a good practice. It verifies the state transition from pending to claimed is handled correctly. * 1575-1581: The process of claiming funds and verifying the events `PaymentSent` and `PaymentClaimed` are generated correctly is crucial. This ensures that the self-payment lifecycle is completed as expected. However, it's important to also verify that the payment state transitions to `Fulfilled` correctly, which is done in line 1581.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if Retry::Attempts(0) is used consistently across tests for self-payments rg "Retry::Attempts\(0\)" lightning/src/ln/payment_tests.rsLength of output: 372
lightning/src/ln/channelmanager.rs (10)
- 677-677: The addition of the
amount_msatfield to theClaimablePaymentstruct is a positive change, enhancing the tracking of payment amounts for claimable payments. Ensure that the handling of this field is consistent across all instances whereClaimablePaymentis created or updated.- 698-708: The introduction of
ClaimableSelfPaymentsandClaimableSelfPaymentstructs is well-aligned with the PR's objectives to manage self claimable payments effectively. Consider adding documentation or comments to explain the design choices, especially regarding the handling ofamount_msatinClaimableSelfPaymentcompared toClaimablePayment.- 1259-1263: The addition of
claimable_self_paymentsto theChannelManagerstruct, protected by aMutex, is a thoughtful integration for managing self claimable payments. Ensure that the lock order requirements are carefully reviewed to prevent potential deadlocks or concurrency issues.- 2450-2450: The initialization of
claimable_self_paymentswith an emptyHashMapin theChannelManagerconstructor is correct and consistent with the initialization of similar fields. Consider adding a comment to explain the purpose of this initialization for clarity.- 3525-3555: The modifications to the
send_paymentfunction to handle self payments and generatePaymentClaimableevents are well-aligned with the PR's objectives. Ensure that the self payment detection logic is thoroughly tested, and consider adding specific tests for the generation ofPaymentClaimableevents to verify correctness and robustness.- 5435-5448: The logic to fulfill and claim self payments in the
handle_pending_eventsfunction is well-implemented, effectively managing the removal of entries fromclaimable_self_paymentsand generating the appropriate events. Ensure that error handling is in place for cases where entries might be missing inclaimable_self_paymentsand confirm that the logic is thread-safe.- 9992-9992: The locking of
claimable_self_paymentsandpending_outbound_paymentsis necessary for thread safety. Review the lock order across the codebase to ensure consistency and prevent potential deadlocks.- 10008-10015: The serialization logic for
claimable_self_paymentsis correctly implemented, ensuring that all details are persisted. Review the corresponding deserialization logic to ensure symmetry and prevent data loss or corruption.- 10523-10537: The deserialization logic for
claimable_self_paymentsis correctly implemented, matching the serialization logic to ensure data integrity. Consider adding robust error handling for cases of data corruption or unexpected values to maintain data integrity.- 11168-11168: The initialization of
claimable_self_paymentsduring deserialization is correctly implemented, ensuring that self claimable payments are correctly restored. Review the entire deserialization process for robustness and error tolerance to maintain data integrity.
| @@ -170,7 +170,7 @@ impl PendingOutboundPayment { | |||
| } | |||
| } | |||
|
|
|||
| fn mark_fulfilled(&mut self) { | |||
| pub fn mark_fulfilled(&mut self) { | |||
There was a problem hiding this comment.
The visibility of the mark_fulfilled method is changed to public.
Changing the visibility of methods can have broader implications on how they are used throughout the codebase. It's essential to ensure that this method's public visibility does not allow for misuse or unintended side effects, especially since it marks payments as fulfilled which is a critical operation.
- fn mark_fulfilled(&mut self) {
+ pub fn mark_fulfilled(&mut self) {Consider adding documentation to clarify the intended use cases and any precautions that should be taken when calling this method.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub fn mark_fulfilled(&mut self) { | |
| pub fn mark_fulfilled(&mut self) { |
| @@ -4547,7 +4593,7 @@ where | |||
| .or_insert_with(|| { | |||
| committed_to_claimable = true; | |||
| ClaimablePayment { | |||
| purpose: $purpose.clone(), htlcs: Vec::new(), onion_fields: None, | |||
| purpose: $purpose.clone(), htlcs: Vec::new(), onion_fields: None, amount_msat: None, | |||
There was a problem hiding this comment.
When updating or inserting a ClaimablePayment instance, the amount_msat field is not updated. This omission could lead to inconsistencies in tracking and managing claimable payments. Ensure that amount_msat is correctly handled in all scenarios where ClaimablePayment is updated or inserted.
| purpose, htlcs, onion_fields: onion, amount_msat: None, | ||
| }); | ||
| if existing_payment.is_some() { return Err(DecodeError::InvalidValue); } | ||
| } | ||
| } else { | ||
| for (purpose, (payment_hash, htlcs)) in purposes.into_iter().zip(claimable_htlcs_list.into_iter()) { | ||
| let existing_payment = claimable_payments.insert(payment_hash, ClaimablePayment { | ||
| purpose, htlcs, onion_fields: None, | ||
| purpose, htlcs, onion_fields: None, amount_msat: None, |
There was a problem hiding this comment.
The handling of ClaimablePayment insertion during decoding does not account for the amount_msat field, potentially leading to incomplete data. Ensure that amount_msat is correctly handled during the decoding process to maintain data integrity.
| @@ -10906,7 +10998,7 @@ where | |||
| events::PaymentPurpose::SpontaneousPayment(*payment_preimage), | |||
| }; | |||
| claimable_payments.insert(payment_hash, ClaimablePayment { | |||
| purpose, htlcs, onion_fields: None, | |||
| purpose, htlcs, onion_fields: None, amount_msat: None, | |||
There was a problem hiding this comment.
Similar to previous comments, the insertion of ClaimablePayment for spontaneous payments does not include the amount_msat field, potentially affecting data integrity. Ensure that amount_msat is correctly handled for spontaneous payments to maintain consistency and integrity.
vladimirfomene
force-pushed
the
allow-self-payment
branch
3 times, most recently
from
906c1b9
to
3ca0328
Compare
This PR solves issue lightningdevkit#2462. If we asked to pay an invoice that we generated ourselves. We generate PaymentSent and PaymentClaimable event and mark the payment as fulfilled in our set of outbound payments. This PR is important because we realized users can easily screw up self payments when they implement it themselves. See here: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-June/003983.html
vladimirfomene
force-pushed
the
allow-self-payment
branch
from
3ca0328
to
2880c55
Compare
|
Hi @TheBlueMatt, please have a look at the updated implementation when you have some bandwidth. |
| @@ -697,6 +698,17 @@ struct ClaimablePayments { | |||
| pending_claiming_payments: HashMap<PaymentHash, ClaimingPayment>, | |||
| } | |||
|
|
|||
| /// Information about self claimable payments. | |||
| struct ClaimableSelfPayments { | |||
There was a problem hiding this comment.
Why does this need the struct indirection?
| let mut sources = { | ||
| let mut claimable_payments = self.claimable_payments.lock().unwrap(); | ||
| if let Some(payment) = claimable_payments.claimable_payments.remove(&payment_hash) { | ||
| let mut receiver_node_id = self.our_network_pubkey; | ||
| if payment.htlcs.is_empty() { |
There was a problem hiding this comment.
Not sure I understand the reason for this now that we have a separate self-payment map.
| @@ -10126,6 +10195,14 @@ where | |||
| htlc_onion_fields.push(&payment.onion_fields); | |||
| } | |||
|
|
|||
| (claimable_self_payments.claimable_payments.len() as u64).write(writer)?; | |||
There was a problem hiding this comment.
New fields have to be written with TLVs, as written here it would mean upgraded LDK can't read previous versions of LDK's data (and vice versa).
| @@ -677,6 +677,7 @@ struct ClaimablePayment { | |||
| purpose: events::PaymentPurpose, | |||
| onion_fields: Option<RecipientOnionFields>, | |||
| htlcs: Vec<ClaimableHTLC>, | |||
| amount_msat: Option<u64>, | |||
There was a problem hiding this comment.
What's the purpose of the new field here?
|
Hey @vladimirfomene any desire to keep working on this? |
This PR solves issue #2462. If we asked to
pay an invoice that we generated ourselves. We
generate PaymentSent and PaymentClaimable event
and mark the payment as fulfilled in our set
of outbound payments.
This PR is important because we realized users
can easily screw up self payments in a custodial service when
they implement it themselves. Read more here: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-June/003983.html