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.

Sign up for GitHub

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

Jump to bottom

Payment Retries #1059

Merged
merged 9 commits into from
Oct 27, 2021
Merged

Payment Retries #1059

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a3a4e61
Expose log_bytes! macro for use in other crates
jkczyz Aug 24, 2021
3410f18
Add PaymentId to PaymentSent event
jkczyz Oct 17, 2021
73f601f
Add PaymentId to PaymentPathFailed event
jkczyz Sep 30, 2021
5feb4dc
Rewrite Invoice's interface in terms of msats
jkczyz Oct 4, 2021
2d102a3
Unify route finding methods
jkczyz Oct 25, 2021
ad4f16b
Add InvoicePayer for retrying failed payments
jkczyz Aug 24, 2021
e523e58
Support paying zero-value invoices
jkczyz Oct 4, 2021
d9b9916
Fail payment retry if Invoice is expired
jkczyz Sep 30, 2021
010436d
Implement Payer and Router for lightning crate
jkczyz Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
let events_3 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_3.len(), 1);
match events_3[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down Expand Up @@ -397,7 +397,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
let events_3 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_3.len(), 1);
match events_3[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down Expand Up @@ -1399,7 +1399,7 @@ fn claim_while_disconnected_monitor_update_fail() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down Expand Up @@ -1806,7 +1806,7 @@ fn monitor_update_claim_fail_no_response() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down
4 changes: 3 additions & 1 deletion lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3475,6 +3475,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
self.pending_events.lock().unwrap().push(
events::Event::PaymentSent {
payment_id: Some(payment_id),
payment_preimage,
payment_hash: payment_hash
}
Expand Down Expand Up @@ -6256,7 +6257,8 @@ mod tests {
// further events will be generated for subsequence path successes.
let events = nodes[0].node.get_and_clear_pending_events();
match events[0] {
Event::PaymentSent { payment_preimage: ref preimage, payment_hash: ref hash } => {
Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash } => {
assert_eq!(Some(payment_id), *id);
assert_eq!(payment_preimage, *preimage);
assert_eq!(our_payment_hash, *hash);
},
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,7 @@ macro_rules! expect_payment_sent {
let expected_payment_hash = PaymentHash(Sha256::hash(&$expected_payment_preimage.0).into_inner());
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!($expected_payment_preimage, *payment_preimage);
assert_eq!(expected_payment_hash, *payment_hash);
},
Expand Down
16 changes: 8 additions & 8 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2659,7 +2659,7 @@ fn test_htlc_on_chain_success() {
let mut first_claimed = false;
for event in events {
match event {
Event::PaymentSent { payment_preimage, payment_hash } => {
Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
if payment_preimage == our_payment_preimage && payment_hash == payment_hash_1 {
assert!(!first_claimed);
first_claimed = true;
Expand Down Expand Up @@ -3350,7 +3350,7 @@ fn test_simple_peer_disconnect() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 2);
match events[0] {
Event::PaymentSent { payment_preimage, payment_hash } => {
Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
assert_eq!(payment_preimage, payment_preimage_3);
assert_eq!(payment_hash, payment_hash_3);
},
Expand Down Expand Up @@ -3514,7 +3514,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
let events_4 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_4.len(), 1);
match events_4[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(payment_preimage_1, *payment_preimage);
assert_eq!(payment_hash_1, *payment_hash);
},
Expand Down Expand Up @@ -3555,7 +3555,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
let events_4 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_4.len(), 1);
match events_4[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(payment_preimage_1, *payment_preimage);
assert_eq!(payment_hash_1, *payment_hash);
},
Expand Down Expand Up @@ -3790,7 +3790,7 @@ fn test_drop_messages_peer_disconnect_dual_htlc() {
let events_3 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_3.len(), 1);
match events_3[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down Expand Up @@ -5059,7 +5059,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {

let events = nodes[0].node.get_and_clear_pending_events();
match events[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(*payment_preimage, our_payment_preimage);
assert_eq!(*payment_hash, duplicate_payment_hash);
}
Expand Down Expand Up @@ -5572,7 +5572,7 @@ fn do_htlc_claim_local_commitment_only(use_dust: bool) {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { payment_preimage, payment_hash } => {
Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
assert_eq!(payment_preimage, our_payment_preimage);
assert_eq!(payment_hash, our_payment_hash);
},
Expand Down Expand Up @@ -6000,7 +6000,7 @@ fn test_free_and_fail_holding_cell_htlcs() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
}
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/shutdown_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ fn updates_shutdown_wait() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(our_payment_preimage, *payment_preimage);
assert_eq!(our_payment_hash, *payment_hash);
},
Expand Down Expand Up @@ -309,7 +309,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
assert_eq!(our_payment_preimage, *payment_preimage);
assert_eq!(our_payment_hash, *payment_hash);
},
Expand Down
13 changes: 12 additions & 1 deletion lightning/src/util/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//! few other things.

use chain::keysinterface::SpendableOutputDescriptor;
use ln::channelmanager::PaymentId;
use ln::msgs;
use ln::msgs::DecodeError;
use ln::{PaymentPreimage, PaymentHash, PaymentSecret};
Expand Down Expand Up @@ -178,6 +179,12 @@ pub enum Event {
/// Note for MPP payments: in rare cases, this event may be preceded by a `PaymentPathFailed`
/// event. In this situation, you SHOULD treat this payment as having succeeded.
PaymentSent {
/// The id returned by [`ChannelManager::send_payment`] and used with
/// [`ChannelManager::retry_payment`].
///
/// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I just noticed that documentation for payment_id seems to be missing from ChannelMannager::send_payment. It would be nice to (a) document that the id should be stored for a retry, and (b) that it's needed because technically PaymentHashs can be non-unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving unresolved pending discussions around changes to send_payment.

/// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment
payment_id: Option<PaymentId>,
/// The preimage to the hash given to ChannelManager::send_payment.
/// Note that this serves as a payment receipt, if you wish to have such a thing, you must
/// store it somehow!
Expand Down Expand Up @@ -322,11 +329,12 @@ impl Writeable for Event {
(8, payment_preimage, option),
});
},
&Event::PaymentSent { ref payment_preimage, ref payment_hash} => {
&Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash} => {
2u8.write(writer)?;
write_tlv_fields!(writer, {
(0, payment_preimage, required),
(1, payment_hash, required),
(3, payment_id, option),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be a little nicer public API-wise if payment_id were a non-Option, and set to 0 if serialized before 0.0.104 (or wtv). Similar for the next commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we leave it an Option for now and then just do a hard-break on our serialization backwards compat in like 2-3 versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me! 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, maybe that is why we decided to key by payment_hash for now? I guess we could just not retry if PaymentId is None, though.

});
},
&Event::PaymentPathFailed {
Expand Down Expand Up @@ -435,14 +443,17 @@ impl MaybeReadable for Event {
let f = || {
let mut payment_preimage = PaymentPreimage([0; 32]);
let mut payment_hash = None;
let mut payment_id = None;
read_tlv_fields!(reader, {
(0, payment_preimage, required),
(1, payment_hash, option),
(3, payment_id, option),
});
if payment_hash.is_none() {
payment_hash = Some(PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()));
}
Ok(Some(Event::PaymentSent {
payment_id,
payment_preimage,
payment_hash: payment_hash.unwrap(),
}))
Expand Down
12 changes: 12 additions & 0 deletions lightning/src/util/logger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,18 @@ pub trait Logger {
fn log(&self, record: &Record);
}

/// Wrapper for logging byte slices in hex format.
#[doc(hidden)]
pub struct DebugBytes<'a>(pub &'a [u8]);
impl<'a> core::fmt::Display for DebugBytes<'a> {
fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
for i in self.0 {
write!(f, "{:02x}", i)?;
}
Ok(())
}
}

#[cfg(test)]
mod tests {
use util::logger::{Logger, Level};
Expand Down
15 changes: 5 additions & 10 deletions lightning/src/util/macro_logger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use bitcoin::secp256k1::key::PublicKey;

use routing::router::Route;
use ln::chan_utils::HTLCType;
use util::logger::DebugBytes;

pub(crate) struct DebugPubKey<'a>(pub &'a PublicKey);
impl<'a> core::fmt::Display for DebugPubKey<'a> {
Expand All @@ -32,18 +33,11 @@ macro_rules! log_pubkey {
}
}

pub(crate) struct DebugBytes<'a>(pub &'a [u8]);
impl<'a> core::fmt::Display for DebugBytes<'a> {
fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
for i in self.0 {
write!(f, "{:02x}", i)?;
}
Ok(())
}
}
/// Logs a byte slice in hex format.
#[macro_export]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we actually #[doc(hidden)] this and all the other macros in this file that really shoulnd't be used by downstream crates but only LDK crates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could see log_internal needing that. But which other ones? Presumably users may want to log within their trait implementations, possibly using log_bytes, too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I guess I was mostly figuring the logging stuff is a one-way street - the lightning crates log stuff, and users consume it and put it somewhere. I suppose there's no real reason to require that, though, so, ok, fair enough. Please do add it to the internal one, though.

macro_rules! log_bytes {
($obj: expr) => {
::util::macro_logger::DebugBytes(&$obj)
$crate::util::logger::DebugBytes(&$obj)
}
}

Expand Down Expand Up @@ -157,6 +151,7 @@ macro_rules! log_spendable {

/// Create a new Record and log it. You probably don't want to use this macro directly,
/// but it needs to be exported so `log_trace` etc can use it in external crates.
#[doc(hidden)]
#[macro_export]
macro_rules! log_internal {
($logger: expr, $lvl:expr, $($arg:tt)+) => (
Expand Down