Include `RecipientOnionFields` in `Event::PaymentClaimed` #3084

jkczyz · 2024-05-24T21:48:54Z

RecipientOnionFields is included in Event::PaymentClaimable. For completeness, include it in Event::PaymentClaimed as well.

Fixes #2988.

This allows for obtaining the value without needing to re-look it up. An upcoming commit will include RecipientOnionFields in the inserted value. Having it available afterwards prevents needing to clone it.

This will be used to include the same field in Event::PaymentClaimed.

jkczyz · 2024-05-24T21:50:07Z

lightning/src/ln/channelmanager.rs

+					.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);
+					})


Noted that there is a small change of behavior here. When this case is hit, the entry is not overridden.

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?

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 PaymentClaimable docs state "Note that LDK will not stop you from registering duplicate payment hashes for inbound payments."

IIUC, this is actually an invariant check and should never happen. We'll only insert into ClaimablePayments::claimable_payments if ClaimablePayments::pending_claiming_payments doesn't contain the same payment hash.

rust-lightning/lightning/src/ln/channelmanager.rs

Lines 5455 to 5466 in df01208

if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) {

fail_htlc!(claimable_htlc, payment_hash);

}

let ref mut claimable_payment = claimable_payments.claimable_payments

.entry(payment_hash)

// Note that if we insert here we MUST NOT fail_htlc!()

.or_insert_with(|| {

committed_to_claimable = true;

ClaimablePayment {

purpose: $purpose.clone(), htlcs: Vec::new(), onion_fields: None,

}

});

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 ClaimablePayments should have been one map storing an enum?

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.

RecipientOnionFields are included in Event::PaymentClaimable, so naturally they should be included in Event::PaymentClaimed, too.

codecov-commenter · 2024-05-24T22:00:42Z

Codecov Report

Attention: Patch coverage is 94.61078% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 89.88%. Comparing base (a95338a) to head (8499214).
Report is 2 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	78.26%	3 Missing and 2 partials ⚠️
lightning/src/events/mod.rs	0.00%	4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3084    +/-   ##
========================================
  Coverage   89.87%   89.88%            
========================================
  Files         117      117            
  Lines       96952    97130   +178     
  Branches    96952    97130   +178     
========================================
+ Hits        87134    87301   +167     
- Misses       7273     7285    +12     
+ Partials     2545     2544     -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dunxen

Thorough review and LGTM.

dunxen · 2024-05-28T11:59:42Z

lightning/src/ln/channelmanager.rs

+					.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);
+					})


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?

tnull

Generally LGTM, although I'm not super sure about whether we should or shouldn't support claiming duplicate payments.

tnull · 2024-05-29T10:10:16Z

lightning/src/ln/channelmanager.rs

+					.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);
+					})


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 PaymentClaimable docs state "Note that LDK will not stop you from registering duplicate payment hashes for inbound payments."

TheBlueMatt

Gonna go ahead and land this. We can do a followup if there's a need.

TheBlueMatt · 2024-05-30T19:26:29Z

lightning/src/ln/channelmanager.rs

@@ -680,13 +680,15 @@ struct ClaimingPayment {
 	receiver_node_id: PublicKey,
 	htlcs: Vec<events::ClaimedHTLC>,
 	sender_intended_value: Option<u64>,
+	onion_fields: Option<RecipientOnionFields>,


We should write when it was added/that its always filled in in a comment.

TheBlueMatt · 2024-05-30T19:34:25Z

lightning/src/ln/channelmanager.rs

+					.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);
+					})


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.

jkczyz added 3 commits May 24, 2024 11:30

Format HashMap::insert call

101af71

Use HashMap::entry instead of HashMap::insert

3ab45d8

This allows for obtaining the value without needing to re-look it up. An upcoming commit will include RecipientOnionFields in the inserted value. Having it available afterwards prevents needing to clone it.

Add RecipientOnionFields to ClaimingPayment

a4a34f9

This will be used to include the same field in Event::PaymentClaimed.

jkczyz commented May 24, 2024

View reviewed changes

jkczyz added 4 commits May 24, 2024 16:51

Include RecipientOnionFields in PaymentClaimed

b745047

RecipientOnionFields are included in Event::PaymentClaimable, so naturally they should be included in Event::PaymentClaimed, too.

Pass ClaimAlongRouteArgs to do_claim_payment_along_route

b6ff46d

Pass ClaimAlongRouteArgs to claim_payment_along_route

ac15105

Check custom_tlvs in Event::PaymentClaimed

8499214

jkczyz force-pushed the 2024-05-onion-fields branch from 2bad99a to 8499214 Compare May 24, 2024 21:53

dunxen approved these changes May 28, 2024

View reviewed changes

tnull reviewed May 29, 2024

View reviewed changes

TheBlueMatt approved these changes May 30, 2024

View reviewed changes

TheBlueMatt merged commit 7d2d047 into lightningdevkit:main May 30, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include `RecipientOnionFields` in `Event::PaymentClaimed` #3084

Include `RecipientOnionFields` in `Event::PaymentClaimed` #3084

jkczyz commented May 24, 2024

jkczyz May 24, 2024

dunxen May 28, 2024

tnull May 29, 2024

jkczyz May 29, 2024

TheBlueMatt May 30, 2024

codecov-commenter commented May 24, 2024

dunxen left a comment

dunxen May 28, 2024

tnull left a comment

tnull May 29, 2024

TheBlueMatt left a comment

TheBlueMatt May 30, 2024

TheBlueMatt May 30, 2024

	if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) {
	fail_htlc!(claimable_htlc, payment_hash);
	}
	let ref mut claimable_payment = claimable_payments.claimable_payments
	.entry(payment_hash)
	// Note that if we insert here we MUST NOT fail_htlc!()
	.or_insert_with(\|\| {
	committed_to_claimable = true;
	ClaimablePayment {
	purpose: $purpose.clone(), htlcs: Vec::new(), onion_fields: None,
	}
	});

Include RecipientOnionFields in Event::PaymentClaimed #3084

Include RecipientOnionFields in Event::PaymentClaimed #3084

Conversation

jkczyz commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 24, 2024

Codecov Report

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Include `RecipientOnionFields` in `Event::PaymentClaimed` #3084

Include `RecipientOnionFields` in `Event::PaymentClaimed` #3084