Skip to content

Commit e60acde

Browse files
valentinewallaceTheBlueMatt
authored andcommitted
Set PaymentSent::fee_paid_msat in abandoned case
If an outbound payment was abandoned with htlcs in-flight and later claimed, we would previously have the PaymentSent::fee_paid_msat be set to None. This contradicted some docs on the event that stated the field would always be Some after 0.0.103. Backport of 3e9e6e9 Conflicts resolved in: * lightning/src/ln/functional_tests.rs * lightning/src/ln/outbound_payment.rs * lightning/src/ln/payment_tests.rs
1 parent ed0ba30 commit e60acde

4 files changed

Lines changed: 40 additions & 2 deletions

File tree

lightning/src/events/mod.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -930,7 +930,8 @@ pub enum Event {
930930
/// If the recipient or an intermediate node misbehaves and gives us free money, this may
931931
/// overstate the amount paid, though this is unlikely.
932932
///
933-
/// This is only `None` for payments initiated on LDK versions prior to 0.0.103.
933+
/// This is only `None` for payments abandoned but ultimately claimed when using LDK versions
934+
/// prior to 0.3, 0.2.3, or 0.1.10.
934935
///
935936
/// [`Route::get_total_fees`]: crate::routing::router::Route::get_total_fees
936937
fee_paid_msat: Option<u64>,

lightning/src/ln/functional_tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10281,7 +10281,7 @@ fn test_inconsistent_mpp_params() {
1028110281
do_claim_payment_along_route(
1028210282
ClaimAlongRouteArgs::new(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], our_payment_preimage)
1028310283
);
10284-
expect_payment_sent(&nodes[0], our_payment_preimage, Some(None), true, true);
10284+
expect_payment_sent(&nodes[0], our_payment_preimage, Some(Some(2000)), true, true);
1028510285
}
1028610286

1028710287
#[test]

lightning/src/ln/outbound_payment.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,9 @@ pub(crate) enum PendingOutboundPayment {
133133
/// Will be `None` if the payment was serialized before 0.0.115 or if downgrading to 0.0.124
134134
/// or later with a reason that was added after.
135135
reason: Option<PaymentFailureReason>,
136+
/// Preserved from `Retryable` so we can still report `fee_paid_msat` if an HTLC succeeds after
137+
/// the payment was abandoned. Added in 0.3/0.1.10.
138+
pending_fee_msat: Option<u64>,
136139
},
137140
}
138141

@@ -209,6 +212,7 @@ impl PendingOutboundPayment {
209212
fn get_pending_fee_msat(&self) -> Option<u64> {
210213
match self {
211214
PendingOutboundPayment::Retryable { pending_fee_msat, .. } => pending_fee_msat.clone(),
215+
PendingOutboundPayment::Abandoned { pending_fee_msat, .. } => pending_fee_msat.clone(),
212216
_ => None,
213217
}
214218
}
@@ -251,6 +255,7 @@ impl PendingOutboundPayment {
251255
},
252256
_ => new_hash_set(),
253257
};
258+
let pending_fee_msat = self.get_pending_fee_msat();
254259
match self {
255260
Self::Retryable { payment_hash, .. } |
256261
Self::InvoiceReceived { payment_hash, .. } |
@@ -260,6 +265,7 @@ impl PendingOutboundPayment {
260265
session_privs,
261266
payment_hash: *payment_hash,
262267
reason: Some(reason),
268+
pending_fee_msat,
263269
};
264270
},
265271
_ => {}
@@ -2409,6 +2415,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
24092415
(0, session_privs, required),
24102416
(1, reason, upgradable_option),
24112417
(2, payment_hash, required),
2418+
(5, pending_fee_msat, option),
24122419
},
24132420
(5, AwaitingInvoice) => {
24142421
(0, expiration, required),

lightning/src/ln/payment_tests.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1751,6 +1751,36 @@ fn abandoned_send_payment_idempotent() {
17511751
claim_payment(&nodes[0], &[&nodes[1]], second_payment_preimage);
17521752
}
17531753

1754+
#[test]
1755+
fn abandoned_payment_fulfilled_preserves_fee_paid_msat() {
1756+
// Previously, if we abandoned a payment with HTLCs in-flight and the payment eventually
1757+
// succeeded, we would set the `Event::PaymentSent::fee_paid_msat` to None, even though we had
1758+
// docs guaranteeing that it would always be Some after 0.0.103.
1759+
let chanmon_cfgs = create_chanmon_cfgs(3);
1760+
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
1761+
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
1762+
let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
1763+
1764+
create_announced_chan_between_nodes(&nodes, 0, 1);
1765+
create_announced_chan_between_nodes(&nodes, 1, 2);
1766+
1767+
let amt_msat = 5_000_000;
1768+
let (route, payment_hash, payment_preimage, payment_secret) =
1769+
get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat);
1770+
let payment_id = PaymentId(payment_hash.0);
1771+
let onion = RecipientOnionFields::secret_only(payment_secret);
1772+
nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap();
1773+
check_added_monitors(&nodes[0], 1);
1774+
1775+
let path: &[&Node] = &[&nodes[1], &nodes[2]];
1776+
pass_along_route(&nodes[0], &[path], amt_msat, payment_hash, payment_secret);
1777+
1778+
nodes[0].node.abandon_payment(payment_id);
1779+
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
1780+
1781+
claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path], payment_preimage));
1782+
}
1783+
17541784
#[derive(PartialEq)]
17551785
enum InterceptTest {
17561786
Forward,

0 commit comments

Comments
 (0)