-
Notifications
You must be signed in to change notification settings - Fork 385
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
Include RecipientOnionFields
in Event::PaymentClaimed
#3084
Changes from all commits
101af71
3ab45d8
a4a34f9
b745047
b6ff46d
ac15105
8499214
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -680,13 +680,15 @@ struct ClaimingPayment { | |||||||||||||||||||||||||
receiver_node_id: PublicKey, | ||||||||||||||||||||||||||
htlcs: Vec<events::ClaimedHTLC>, | ||||||||||||||||||||||||||
sender_intended_value: Option<u64>, | ||||||||||||||||||||||||||
onion_fields: Option<RecipientOnionFields>, | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
impl_writeable_tlv_based!(ClaimingPayment, { | ||||||||||||||||||||||||||
(0, amount_msat, required), | ||||||||||||||||||||||||||
(2, payment_purpose, required), | ||||||||||||||||||||||||||
(4, receiver_node_id, required), | ||||||||||||||||||||||||||
(5, htlcs, optional_vec), | ||||||||||||||||||||||||||
(7, sender_intended_value, option), | ||||||||||||||||||||||||||
(9, onion_fields, option), | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
struct ClaimablePayment { | ||||||||||||||||||||||||||
|
@@ -6324,19 +6326,27 @@ where | |||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
let htlcs = payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(); | ||||||||||||||||||||||||||
let sender_intended_value = payment.htlcs.first().map(|htlc| htlc.total_msat); | ||||||||||||||||||||||||||
let dup_purpose = claimable_payments.pending_claiming_payments.insert(payment_hash, | ||||||||||||||||||||||||||
ClaimingPayment { amount_msat: payment.htlcs.iter().map(|source| source.value).sum(), | ||||||||||||||||||||||||||
payment_purpose: payment.purpose, receiver_node_id, htlcs, sender_intended_value | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
if dup_purpose.is_some() { | ||||||||||||||||||||||||||
debug_assert!(false, "Shouldn't get a duplicate pending claim event ever"); | ||||||||||||||||||||||||||
log_error!(self.logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug", | ||||||||||||||||||||||||||
&payment_hash); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
let claiming_payment = claimable_payments.pending_claiming_payments | ||||||||||||||||||||||||||
.entry(payment_hash) | ||||||||||||||||||||||||||
.and_modify(|_| { | ||||||||||||||||||||||||||
debug_assert!(false, "Shouldn't get a duplicate pending claim event ever"); | ||||||||||||||||||||||||||
log_error!(self.logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug", | ||||||||||||||||||||||||||
&payment_hash); | ||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||
Comment on lines
+6331
to
+6335
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted that there is a small change of behavior here. When this case is hit, the entry is not overridden. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems fine as it should be a duplicate anyway. Should the very unlikely error message above also state that the old event is not overridden? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mhh, while claiming duplicates isn't super nice as it indicates a bug on the sender side, do we still need to support it? If not, should we enforce the failure of duplicates rather than leaving it up to the user? At least currently our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, this is actually an invariant check and should never happen. We'll only insert into rust-lightning/lightning/src/ln/channelmanager.rs Lines 5455 to 5466 in df01208
So I think we don't allow two outstanding payments for the same payment hash, but we will allow consecutive ones. cc: @TheBlueMatt for confirmation. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we debug-panic and log an error because this absolutely shouldn't happen. I'm not sure it matters too much if we overwrite vs keeping as-is - either way we're gonna give the user spurious PaymentClaimed events, whether too early too late or not at all. |
||||||||||||||||||||||||||
.or_insert_with(|| { | ||||||||||||||||||||||||||
let htlcs = payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(); | ||||||||||||||||||||||||||
let sender_intended_value = payment.htlcs.first().map(|htlc| htlc.total_msat); | ||||||||||||||||||||||||||
ClaimingPayment { | ||||||||||||||||||||||||||
amount_msat: payment.htlcs.iter().map(|source| source.value).sum(), | ||||||||||||||||||||||||||
payment_purpose: payment.purpose, | ||||||||||||||||||||||||||
receiver_node_id, | ||||||||||||||||||||||||||
htlcs, | ||||||||||||||||||||||||||
sender_intended_value, | ||||||||||||||||||||||||||
onion_fields: payment.onion_fields, | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if let Some(RecipientOnionFields { ref custom_tlvs, .. }) = payment.onion_fields { | ||||||||||||||||||||||||||
if let Some(RecipientOnionFields { ref custom_tlvs, .. }) = claiming_payment.onion_fields { | ||||||||||||||||||||||||||
if !custom_tlvs_known && custom_tlvs.iter().any(|(typ, _)| typ % 2 == 0) { | ||||||||||||||||||||||||||
log_info!(self.logger, "Rejecting payment with payment hash {} as we cannot accept payment with unknown even TLVs: {}", | ||||||||||||||||||||||||||
&payment_hash, log_iter!(custom_tlvs.iter().map(|(typ, _)| typ).filter(|typ| *typ % 2 == 0))); | ||||||||||||||||||||||||||
|
@@ -6743,6 +6753,7 @@ where | |||||||||||||||||||||||||
receiver_node_id, | ||||||||||||||||||||||||||
htlcs, | ||||||||||||||||||||||||||
sender_intended_value: sender_intended_total_msat, | ||||||||||||||||||||||||||
onion_fields, | ||||||||||||||||||||||||||
}) = payment { | ||||||||||||||||||||||||||
self.pending_events.lock().unwrap().push_back((events::Event::PaymentClaimed { | ||||||||||||||||||||||||||
payment_hash, | ||||||||||||||||||||||||||
|
@@ -6751,6 +6762,7 @@ where | |||||||||||||||||||||||||
receiver_node_id: Some(receiver_node_id), | ||||||||||||||||||||||||||
htlcs, | ||||||||||||||||||||||||||
sender_intended_total_msat, | ||||||||||||||||||||||||||
onion_fields, | ||||||||||||||||||||||||||
}, None)); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
|
@@ -12261,6 +12273,7 @@ where | |||||||||||||||||||||||||
amount_msat: claimable_amt_msat, | ||||||||||||||||||||||||||
htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), | ||||||||||||||||||||||||||
sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), | ||||||||||||||||||||||||||
onion_fields: payment.onion_fields, | ||||||||||||||||||||||||||
}, None)); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
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.
We should write when it was added/that its always filled in in a comment.