Skip to content
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

Add counterparty_skimmed_fee_msat field to PaymentKind::Bolt11Jit #497

Merged
merged 2 commits into from
Mar 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bindings/ldk_node.udl
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ interface ClosureReason {
interface PaymentKind {
Onchain(Txid txid, ConfirmationStatus status);
Bolt11(PaymentHash hash, PaymentPreimage? preimage, PaymentSecret? secret);
Bolt11Jit(PaymentHash hash, PaymentPreimage? preimage, PaymentSecret? secret, LSPFeeLimits lsp_fee_limits);
Bolt11Jit(PaymentHash hash, PaymentPreimage? preimage, PaymentSecret? secret, u64? counterparty_skimmed_fee_msat, LSPFeeLimits lsp_fee_limits);
Bolt12Offer(PaymentHash? hash, PaymentPreimage? preimage, PaymentSecret? secret, OfferId offer_id, UntrustedString? payer_note, u64? quantity);
Bolt12Refund(PaymentHash? hash, PaymentPreimage? preimage, PaymentSecret? secret, UntrustedString? payer_note, u64? quantity);
Spontaneous(PaymentHash hash, PaymentPreimage? preimage);
Expand Down
20 changes: 20 additions & 0 deletions src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,26 @@ where
};
}

// If the LSP skimmed anything, update our stored payment.
if counterparty_skimmed_fee_msat > 0 {
match info.kind {
PaymentKind::Bolt11Jit { .. } => {
let update = PaymentDetailsUpdate {
counterparty_skimmed_fee_msat: Some(Some(counterparty_skimmed_fee_msat)),
..PaymentDetailsUpdate::new(payment_id)
};
match self.payment_store.update(&update) {
Ok(_) => (),
Err(e) => {
log_error!(self.logger, "Failed to access payment store: {}", e);
return Err(ReplayEvent());
},
};
}
_ => debug_assert!(false, "We only expect the counterparty to get away with withholding fees for JIT payments."),
}
}

// If this is known by the store but ChannelManager doesn't know the preimage,
// the payment has been registered via `_for_hash` variants and needs to be manually claimed via
// user interaction.
Expand Down
1 change: 1 addition & 0 deletions src/payment/bolt11.rs
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,7 @@ impl Bolt11Payment {
hash: payment_hash,
preimage,
secret: Some(payment_secret.clone()),
counterparty_skimmed_fee_msat: None,
lsp_fee_limits,
};
let payment = PaymentDetails::new(
Expand Down
49 changes: 47 additions & 2 deletions src/payment/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ pub struct PaymentDetails {
/// The kind of the payment.
pub kind: PaymentKind,
/// The amount transferred.
///
/// Will be `None` for variable-amount payments until we receive them.
pub amount_msat: Option<u64>,
/// The fees that were paid for this payment.
///
Expand Down Expand Up @@ -165,6 +167,18 @@ impl PaymentDetails {
update_if_necessary!(self.fee_paid_msat, fee_paid_msat_opt);
}

if let Some(skimmed_fee_msat) = update.counterparty_skimmed_fee_msat {
match self.kind {
PaymentKind::Bolt11Jit { ref mut counterparty_skimmed_fee_msat, .. } => {
update_if_necessary!(*counterparty_skimmed_fee_msat, skimmed_fee_msat);
},
_ => debug_assert!(
false,
"We should only ever override counterparty_skimmed_fee_msat for JIT payments"
),
}
}

if let Some(status) = update.status {
update_if_necessary!(self.status, status);
}
Expand Down Expand Up @@ -257,7 +271,14 @@ impl Readable for PaymentDetails {

if secret.is_some() {
if let Some(lsp_fee_limits) = lsp_fee_limits {
PaymentKind::Bolt11Jit { hash, preimage, secret, lsp_fee_limits }
let counterparty_skimmed_fee_msat = None;
PaymentKind::Bolt11Jit {
hash,
preimage,
secret,
counterparty_skimmed_fee_msat,
lsp_fee_limits,
}
} else {
PaymentKind::Bolt11 { hash, preimage, secret }
}
Expand Down Expand Up @@ -346,6 +367,12 @@ pub enum PaymentKind {
preimage: Option<PaymentPreimage>,
/// The secret used by the payment.
secret: Option<PaymentSecret>,
/// The value, in thousands of a satoshi, that was deducted from this payment as an extra
/// fee taken by our channel counterparty.
///
/// Will only be `Some` once we received the payment. Will always be `None` for LDK Node
/// v0.4 and prior.
counterparty_skimmed_fee_msat: Option<u64>,
/// Limits applying to how much fee we allow an LSP to deduct from the payment amount.
///
/// Allowing them to deduct this fee from the first inbound payment will pay for the LSP's
Expand Down Expand Up @@ -423,6 +450,7 @@ impl_writeable_tlv_based_enum!(PaymentKind,
},
(4, Bolt11Jit) => {
(0, hash, required),
(1, counterparty_skimmed_fee_msat, option),
(2, preimage, option),
(4, secret, option),
(6, lsp_fee_limits, required),
Expand Down Expand Up @@ -501,6 +529,7 @@ pub(crate) struct PaymentDetailsUpdate {
pub secret: Option<Option<PaymentSecret>>,
pub amount_msat: Option<Option<u64>>,
pub fee_paid_msat: Option<Option<u64>>,
pub counterparty_skimmed_fee_msat: Option<Option<u64>>,
pub direction: Option<PaymentDirection>,
pub status: Option<PaymentStatus>,
pub confirmation_status: Option<ConfirmationStatus>,
Expand All @@ -515,6 +544,7 @@ impl PaymentDetailsUpdate {
secret: None,
amount_msat: None,
fee_paid_msat: None,
counterparty_skimmed_fee_msat: None,
direction: None,
status: None,
confirmation_status: None,
Expand All @@ -538,13 +568,21 @@ impl From<&PaymentDetails> for PaymentDetailsUpdate {
_ => None,
};

let counterparty_skimmed_fee_msat = match value.kind {
PaymentKind::Bolt11Jit { counterparty_skimmed_fee_msat, .. } => {
Some(counterparty_skimmed_fee_msat)
},
_ => None,
};

Self {
id: value.id,
hash: Some(hash),
preimage: Some(preimage),
secret: Some(secret),
amount_msat: Some(value.amount_msat),
fee_paid_msat: Some(value.fee_paid_msat),
counterparty_skimmed_fee_msat,
direction: Some(value.direction),
status: Some(value.status),
confirmation_status,
Expand Down Expand Up @@ -841,10 +879,17 @@ mod tests {
);

match bolt11_jit_decoded.kind {
PaymentKind::Bolt11Jit { hash: h, preimage: p, secret: s, lsp_fee_limits: l } => {
PaymentKind::Bolt11Jit {
hash: h,
preimage: p,
secret: s,
counterparty_skimmed_fee_msat: c,
lsp_fee_limits: l,
} => {
assert_eq!(hash, h);
assert_eq!(preimage, p);
assert_eq!(secret, s);
assert_eq!(None, c);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we test with Some() as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No? This test checks that we're able to deserialize a (by now ancient) version of PaymentDetails. As we're just adding the new field, it always has to be None for any PaymentDetails deserialized from the old version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this test is for old payments deser, but this comment was specifically if we should test with deser of new field in another test such as payment_info_is_persisted

Copy link
Collaborator Author

@tnull tnull Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the goal of payment_info_is_persisted is not to cover all possible edge cases and to re-test upstream's serialization macros. If we think we'd want to cover that, I'd rather write proptests for it, which is out-of-scope for this PR though.

assert_eq!(lsp_fee_limits, Some(l));
},
_ => {
Expand Down
10 changes: 9 additions & 1 deletion tests/integration_tests_rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1137,7 +1137,15 @@ fn lsps2_client_service_integration() {
let service_fee_msat = (jit_amount_msat * channel_opening_fee_ppm as u64) / 1_000_000;
let expected_received_amount_msat = jit_amount_msat - service_fee_msat;
expect_payment_successful_event!(payer_node, Some(payment_id), None);
expect_payment_received_event!(client_node, expected_received_amount_msat);
let client_payment_id =
expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap();
let client_payment = client_node.payment(&client_payment_id).unwrap();
match client_payment.kind {
PaymentKind::Bolt11Jit { counterparty_skimmed_fee_msat, .. } => {
assert_eq!(counterparty_skimmed_fee_msat, Some(service_fee_msat));
},
_ => panic!("Unexpected payment kind"),
}

let expected_channel_overprovisioning_msat =
(expected_received_amount_msat * channel_over_provisioning_ppm as u64) / 1_000_000;
Expand Down