refactor: improve hold TLC settlement logic #1022
Conversation
doitian
force-pushed
the
fix-over-paid-invoice
branch
4 times, most recently
from
08c3ea7 to
f9525ed
Compare
| @@ -0,0 +1,262 @@ | |||
| use crate::{ | |||
There was a problem hiding this comment.
tile file name is not following our current naming style, suggest something like settle_tlcs.
| let expiry_duration = | ||
| Duration::from_millis(hold_expire_at.saturating_sub(now_timestamp_as_millis_u64())); | ||
| if !expiry_duration.is_zero() { | ||
| if expiry_duration.is_zero() { |
There was a problem hiding this comment.
it's better to use hold_expire_at.is_zero(), then
let expiry_duration =
Duration::from_millis(hold_expire_at.saturating_sub(now_timestamp_as_millis_u64()));
self.network.send_after(expiry_duration, timeout_command);in latter code.
There was a problem hiding this comment.
I think even we add a new field for is_hold in PendingNotifySettleTlc seems more readable.
| } | ||
|
|
||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct TlcInfoWithChannelId { |
There was a problem hiding this comment.
We probably don’t need the TlcInfoWithChannelId structure. I’ve looked through the related code before, and from a logical perspective, only the shared_secret and channel_id are actually required to construct the error message.
There was a problem hiding this comment.
Do you mean using a struct like TlcErrorReplyContext?
There was a problem hiding this comment.
Do you mean using a struct like
TlcErrorReplyContext?
yes
doitian
force-pushed
the
fix-over-paid-invoice
branch
from
f9525ed to
2e9bd7a
Compare
- Add invoice status validation - Ensure `RemoveTlc` is called for invalid TLCs - Reject overpaid TLCs for MPP invoices - For non-MPP invoices, accept only one fulfilled TLC and reject all others This refactoring ensures that, in the presence of multiple payments to the same invoice, at most one payment succeeds. However, due to the lack of database transaction support, there remains a small window where the total accepted amount may exceed the invoice amount. Additionally, in the case of two concurrent MPP payments, it's possible to partially accept TLCs from both without either payment fully succeeding. This is an inherent limitation of MPP and cannot be avoided on the recipient side. Closes nervosnetwork#965
doitian
force-pushed
the
fix-over-paid-invoice
branch
from
2e9bd7a to
ff00467
Compare
3 participants
Refactor hold TLC settlement logic to improve payment handling:
RemoveTlcis called for invalid TLCsKey Changes
SettleTlcSetCommandmodule: Encapsulates TLC settlement logic with clear verification steps and settlement actionsSettleTlcSetinto two distinct commands:SettleTlcSet(payment_hash, tlc_ids)- Settle TLCs by given list of(channel_id, tlc_id)SettleHoldTlcSet(payment_hash)- Settle hold TLC set saved for a payment hashis_mppfield fromPendingNotifySettleTlcand simplified the settlement flowBehavior Notes
This refactoring ensures that, in the presence of multiple payments to the same invoice, at most one payment succeeds. However, due to the lack of database transaction support, there remains a small window where the total accepted amount may exceed the invoice amount.
Additionally, in the case of two concurrent MPP payments, it's possible to partially accept TLCs from both without either payment fully succeeding. This is an inherent limitation of MPP and cannot be avoided on the recipient side.
Related Issues
Closes #965