Skip to content

Commit cc2f5aa

Browse files
committed
Only avoid duplicate payments if we failed sending
Previously, we denied retrying any payment for which we already had seen the payment hash. However, this is rather invasive as payments might fail due to local connections issues or unavailability of usable channels, in which case the invoices/payment hashes would be 'burnt' and couldn't get retried. Here, we introduce a new `PaymentStatus::SendingFailed` to differentiate send failures and allow retrying tracked payments if they failed to send immediately. We also rename some error types accordingly for further clarification.
1 parent c9d3bb3 commit cc2f5aa

File tree

5 files changed

+54
-25
lines changed

5 files changed

+54
-25
lines changed

bindings/ldk_node.udl

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ enum NodeError {
6969
"OnchainTxCreationFailed",
7070
"ConnectionFailed",
7171
"InvoiceCreationFailed",
72-
"PaymentFailed",
72+
"PaymentSendingFailed",
7373
"PeerInfoParseFailed",
7474
"ChannelCreationFailed",
7575
"ChannelClosingFailed",
@@ -107,6 +107,7 @@ enum PaymentDirection {
107107

108108
enum PaymentStatus {
109109
"Pending",
110+
"SendingFailed",
110111
"Succeeded",
111112
"Failed",
112113
};

src/error.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ pub enum Error {
1313
ConnectionFailed,
1414
/// Invoice creation failed.
1515
InvoiceCreationFailed,
16-
/// An attempted payment has failed.
17-
PaymentFailed,
16+
/// Sending a payment has failed.
17+
PaymentSendingFailed,
1818
/// A given peer info could not be parsed.
1919
PeerInfoParseFailed,
2020
/// A channel could not be opened.
@@ -63,7 +63,7 @@ impl fmt::Display for Error {
6363
}
6464
Self::ConnectionFailed => write!(f, "Network connection closed."),
6565
Self::InvoiceCreationFailed => write!(f, "Failed to create invoice."),
66-
Self::PaymentFailed => write!(f, "Failed to send the given payment."),
66+
Self::PaymentSendingFailed => write!(f, "Failed to send the given payment."),
6767
Self::PeerInfoParseFailed => write!(f, "Failed to parse the given peer information."),
6868
Self::ChannelCreationFailed => write!(f, "Failed to create channel."),
6969
Self::ChannelClosingFailed => write!(f, "Failed to close channel."),
@@ -80,7 +80,9 @@ impl fmt::Display for Error {
8080
Self::InvalidInvoice => write!(f, "The given invoice is invalid."),
8181
Self::InvalidChannelId => write!(f, "The given channel ID is invalid."),
8282
Self::InvalidNetwork => write!(f, "The given network is invalid."),
83-
Self::DuplicatePayment => write!(f, "A payment with the given hash has already been initiated."),
83+
Self::DuplicatePayment => {
84+
write!(f, "A payment with the given hash has already been initiated.")
85+
}
8486
Self::InsufficientFunds => {
8587
write!(f, "There are insufficient funds to complete the given operation.")
8688
}

src/lib.rs

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,9 +1143,11 @@ impl Node {
11431143

11441144
let payment_hash = PaymentHash((*invoice.payment_hash()).into_inner());
11451145

1146-
if self.payment_store.contains(&payment_hash) {
1147-
log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
1148-
return Err(Error::DuplicatePayment);
1146+
if let Some(payment) = self.payment_store.get(&payment_hash) {
1147+
if payment.status != PaymentStatus::SendingFailed {
1148+
log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
1149+
return Err(Error::DuplicatePayment);
1150+
}
11491151
}
11501152

11511153
let payment_secret = Some(*invoice.payment_secret());
@@ -1185,11 +1187,16 @@ impl Node {
11851187
secret: payment_secret,
11861188
amount_msat: invoice.amount_milli_satoshis(),
11871189
direction: PaymentDirection::Outbound,
1188-
status: PaymentStatus::Failed,
1190+
status: PaymentStatus::SendingFailed,
11891191
};
11901192
self.payment_store.insert(payment)?;
11911193

1192-
Err(Error::PaymentFailed)
1194+
match e {
1195+
channelmanager::RetryableSendFailure::DuplicatePayment => {
1196+
Err(Error::DuplicatePayment)
1197+
}
1198+
_ => Err(Error::PaymentSendingFailed),
1199+
}
11931200
}
11941201
}
11951202
}
@@ -1218,9 +1225,11 @@ impl Node {
12181225
}
12191226

12201227
let payment_hash = PaymentHash((*invoice.payment_hash()).into_inner());
1221-
if self.payment_store.contains(&payment_hash) {
1222-
log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
1223-
return Err(Error::DuplicatePayment);
1228+
if let Some(payment) = self.payment_store.get(&payment_hash) {
1229+
if payment.status != PaymentStatus::SendingFailed {
1230+
log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
1231+
return Err(Error::DuplicatePayment);
1232+
}
12241233
}
12251234

12261235
let payment_id = PaymentId(invoice.payment_hash().into_inner());
@@ -1279,11 +1288,16 @@ impl Node {
12791288
secret: payment_secret,
12801289
amount_msat: Some(amount_msat),
12811290
direction: PaymentDirection::Outbound,
1282-
status: PaymentStatus::Failed,
1291+
status: PaymentStatus::SendingFailed,
12831292
};
12841293
self.payment_store.insert(payment)?;
12851294

1286-
Err(Error::PaymentFailed)
1295+
match e {
1296+
channelmanager::RetryableSendFailure::DuplicatePayment => {
1297+
Err(Error::DuplicatePayment)
1298+
}
1299+
_ => Err(Error::PaymentSendingFailed),
1300+
}
12871301
}
12881302
}
12891303
}
@@ -1300,6 +1314,13 @@ impl Node {
13001314
let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
13011315
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
13021316

1317+
if let Some(payment) = self.payment_store.get(&payment_hash) {
1318+
if payment.status != PaymentStatus::SendingFailed {
1319+
log_error!(self.logger, "Payment error: must not send duplicate payments.");
1320+
return Err(Error::DuplicatePayment);
1321+
}
1322+
}
1323+
13031324
let route_params = RouteParameters {
13041325
payment_params: PaymentParameters::from_node_id(
13051326
node_id,
@@ -1338,13 +1359,18 @@ impl Node {
13381359
hash: payment_hash,
13391360
preimage: Some(payment_preimage),
13401361
secret: None,
1341-
status: PaymentStatus::Failed,
1362+
status: PaymentStatus::SendingFailed,
13421363
direction: PaymentDirection::Outbound,
13431364
amount_msat: Some(amount_msat),
13441365
};
13451366
self.payment_store.insert(payment)?;
13461367

1347-
Err(Error::PaymentFailed)
1368+
match e {
1369+
channelmanager::RetryableSendFailure::DuplicatePayment => {
1370+
Err(Error::DuplicatePayment)
1371+
}
1372+
_ => Err(Error::PaymentSendingFailed),
1373+
}
13481374
}
13491375
}
13501376
}

src/payment_store.rs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,16 +57,19 @@ impl_writeable_tlv_based_enum!(PaymentDirection,
5757
pub enum PaymentStatus {
5858
/// The payment is still pending.
5959
Pending,
60+
/// The sending of the payment failed and is safe to be retried.
61+
SendingFailed,
6062
/// The payment suceeded.
6163
Succeeded,
62-
/// The payment failed.
64+
/// The payment failed and is not retryable.
6365
Failed,
6466
}
6567

6668
impl_writeable_tlv_based_enum!(PaymentStatus,
6769
(0, Pending) => {},
68-
(1, Succeeded) => {},
69-
(2, Failed) => {};
70+
(2, SendingFailed) => {},
71+
(4, Succeeded) => {},
72+
(6, Failed) => {};
7073
);
7174

7275
#[derive(Clone, Debug, PartialEq, Eq)]
@@ -141,10 +144,6 @@ where
141144
self.payments.lock().unwrap().get(hash).cloned()
142145
}
143146

144-
pub(crate) fn contains(&self, hash: &PaymentHash) -> bool {
145-
self.payments.lock().unwrap().contains_key(hash)
146-
}
147-
148147
pub(crate) fn update(&self, update: &PaymentDetailsUpdate) -> Result<bool, Error> {
149148
let mut updated = false;
150149
let mut locked_payments = self.payments.lock().unwrap();
@@ -237,7 +236,7 @@ mod tests {
237236
let payment_store = PaymentStore::new(Vec::new(), Arc::clone(&store), logger);
238237

239238
let hash = PaymentHash([42u8; 32]);
240-
assert!(!payment_store.contains(&hash));
239+
assert!(!payment_store.get(&hash).is_some());
241240

242241
let payment = PaymentDetails {
243242
hash,

src/test/functional_tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ fn channel_full_cycle() {
9595

9696
println!("\nA send_payment");
9797
let payment_hash = node_a.send_payment(&invoice).unwrap();
98+
assert_eq!(node_a.send_payment(&invoice), Err(Error::DuplicatePayment));
9899

99100
let outbound_payments_a =
100101
node_a.list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound);

0 commit comments

Comments
 (0)