Introduce Retry InvoiceRequest Flow #3010
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3010 +/- ##
==========================================
- Coverage 89.64% 89.64% -0.01%
==========================================
Files 126 126
Lines 102251 102383 +132
Branches 102251 102383 +132
==========================================
+ Hits 91666 91781 +115
- Misses 7863 7874 +11
- Partials 2722 2728 +6 ☔ View full report in Codecov by Sentry. |
shaavan
changed the title
|
Updated from pr3010.04 to pr3010.05 (diff): Addressed @TheBlueMatt comments Updates:
|
|
I don't buy that we need a new counter. There's a lot of things that hang on the timer rate, but this is a great opportunity to redefine the constants in seconds and multiply out a rate. Looking at all the things we do in the timer tick, as far as I can tell the only one that doesn't have a counter and a limit constant already is |
|
Hi Matt! I think this will definitely be a more maintainable way to handle this change than introducing a counter! Thanks a lot! |
|
Updated from pr3010.08 to pr3010.09 (diff): Update:
Details:
|
TheBlueMatt
reviewed
Jun 11, 2024
|
Updated from pr3010.09 to pr3010.10 (diff): Changes:
|
|
@TheBlueMatt |
TheBlueMatt
reviewed
Jun 25, 2024
|
Updated from pr3010.11 to pr3010.12 (diff): Changes:
|
jkczyz
reviewed
Aug 16, 2024
|
Updated from pr3010.22 to pr3010.23 (diff): Changes:
Range-Diff:1: 115fab41 ! 1: 8abd02fd Add InvoiceRequest and Context Fields in AwaitingInvoice
@@ Metadata
Author: shaavan <shaavan.github@gmail.com>
## Commit message ##
- Add InvoiceRequest and Context Fields in AwaitingInvoice
+ Add InvoiceRequest and Nonce Fields in AwaitingInvoice
- - Introduced `InvoiceRequest` and `context` fields in the `AwaitingInvoice`
+ - Introduced `InvoiceRequest` and `nonce` fields in the `AwaitingInvoice`
to enable recreation of the `InvoiceRequest` message for retries if the
corresponding invoice is not received in time.
- Added `awaiting_invoice` flag to track pending outbound payments with
@@ lightning/src/ln/channelmanager.rs: where
@@ lightning/src/ln/channelmanager.rs: macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
let nonce = Nonce::from_entropy_source(entropy);
- let context = OffersContext::OutboundPayment { payment_id, nonce };
+ let context = OffersContext::OutboundPayment { payment_id, nonce, hmac: None };
- let path = $self.create_blinded_paths_using_absolute_expiry(context, Some(absolute_expiry))
+ let path = $self.create_blinded_paths_using_absolute_expiry(context.clone(), Some(absolute_expiry))
.and_then(|paths| paths.into_iter().next().ok_or(()))
@@ lightning/src/ln/channelmanager.rs: macro_rules! create_refund_builder { ($self:
$self.pending_outbound_payments
.add_new_awaiting_invoice(
- payment_id, expiration, retry_strategy, max_total_routing_fee_msat,
-+ payment_id, expiration, retry_strategy, max_total_routing_fee_msat, None, context
++ payment_id, expiration, retry_strategy, max_total_routing_fee_msat, None, nonce
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;
-@@ lightning/src/ln/channelmanager.rs: where
- let invoice_request = builder.build_and_sign()?;
-
- let context = OffersContext::OutboundPayment { payment_id, nonce };
-- let reply_paths = self.create_blinded_paths(context)
-+ let reply_paths = self.create_blinded_paths(context.clone())
- .map_err(|_| Bolt12SemanticError::MissingPaths)?;
-
- let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
@@ lightning/src/ln/channelmanager.rs: where
let expiration = StaleExpiration::TimerTicks(1);
self.pending_outbound_payments
.add_new_awaiting_invoice(
- payment_id, expiration, retry_strategy, max_total_routing_fee_msat
+ payment_id, expiration, retry_strategy, max_total_routing_fee_msat,
-+ Some(invoice_request.clone()), context,
++ Some(invoice_request.clone()), nonce,
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;
@@ lightning/src/ln/outbound_payment.rs: use bitcoin::hashes::Hash;
+use crate::blinded_path::message::OffersContext;
use crate::blinded_path::{IntroductionNode, NodeIdLookUp};
- use crate::blinded_path::payment::advance_path_by_one;
use crate::events::{self, PaymentFailureReason};
-@@ lightning/src/ln/outbound_payment.rs: use crate::ln::channelmanager::{EventCompletionAction, HTLCSource, PaymentId};
+ use crate::ln::types::{PaymentHash, PaymentPreimage, PaymentSecret};
+@@ lightning/src/ln/outbound_payment.rs: use crate::ln::features::Bolt12InvoiceFeatures;
use crate::ln::onion_utils;
use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason};
use crate::offers::invoice::Bolt12Invoice;
+use crate::offers::invoice_request::InvoiceRequest;
++use crate::offers::nonce::Nonce;
use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router};
use crate::sign::{EntropySource, NodeSigner, Recipient};
use crate::util::errors::APIError;
@@ lightning/src/ln/outbound_payment.rs: pub(crate) enum PendingOutboundPayment {
retry_strategy: Retry,
max_total_routing_fee_msat: Option<u64>,
+ invoice_request: Option<InvoiceRequest>,
-+ context: OffersContext,
++ nonce: Option<Nonce>,
},
InvoiceReceived {
payment_hash: PaymentHash,
@@ lightning/src/ln/outbound_payment.rs: impl OutboundPayments {
&self, payment_id: PaymentId, expiration: StaleExpiration, retry_strategy: Retry,
- max_total_routing_fee_msat: Option<u64>
+ max_total_routing_fee_msat: Option<u64>, invoice_request: Option<InvoiceRequest>,
-+ context: OffersContext,
++ nonce: Nonce,
) -> Result<(), ()> {
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
match pending_outbounds.entry(payment_id) {
@@ lightning/src/ln/outbound_payment.rs: impl OutboundPayments {
retry_strategy,
max_total_routing_fee_msat,
+ invoice_request,
-+ context,
++ nonce: Some(nonce),
});
+ self.awaiting_invoice.store(true, Ordering::Release);
@@ lightning/src/ln/outbound_payment.rs: impl OutboundPayments {
+ }
+
+ let mut pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
-+ let invoice_requests = pending_outbound_payments.iter_mut()
-+ .filter_map(|(_, payment)| match payment {
-+ PendingOutboundPayment::AwaitingInvoice { invoice_request, context, ..} => {
-+ invoice_request.take().map(|req| (context.clone(), req))
++ let invoice_requests = pending_outbound_payments
++ .iter_mut()
++ .filter_map(|(payment_id, payment)| {
++ if let PendingOutboundPayment::AwaitingInvoice {
++ invoice_request, nonce: Some(nonce), ..
++ } = payment {
++ let context = OffersContext::OutboundPayment {
++ payment_id: *payment_id,
++ nonce: *nonce,
++ // TODO: hmac to be incorporated.
++ hmac: None,
++ };
++ invoice_request.take().map(|req| (context, req))
++ } else {
++ None
+ }
-+ _ => None,
+ })
+ .collect();
+
@@ lightning/src/ln/outbound_payment.rs: impl_writeable_tlv_based_enum_upgradable!(
(2, retry_strategy, required),
(4, max_total_routing_fee_msat, option),
+ (5, invoice_request, option),
-+ (6, context, required),
++ (7, nonce, option),
},
(7, InvoiceReceived) => {
(0, payment_hash, required),
@@ lightning/src/ln/outbound_payment.rs: mod tests {
-
- use core::time::Duration;
-
-+ use crate::blinded_path::message::OffersContext;
- use crate::blinded_path::EmptyNodeIdLookUp;
- use crate::events::{Event, PathFailure, PaymentFailureReason};
- use crate::ln::types::PaymentHash;
-@@ lightning/src/ln/outbound_payment.rs: mod tests {
+ use crate::ln::outbound_payment::{Bolt12PaymentError, OutboundPayments, Retry, RetryableSendFailure, StaleExpiration};
+ #[cfg(feature = "std")]
+ use crate::offers::invoice::DEFAULT_RELATIVE_EXPIRY;
++ use crate::offers::nonce::Nonce;
+ use crate::offers::offer::OfferBuilder;
+ use crate::offers::test_utils::*;
+ use crate::routing::gossip::NetworkGraph;
use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteHop, RouteParameters};
use crate::sync::{Arc, Mutex, RwLock};
use crate::util::errors::APIError;
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_err()
);
}
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_err()
);
}
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), None
-+ payment_id, expiration, Retry::Attempts(0), None, None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), None, None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
@@ lightning/src/ln/outbound_payment.rs: mod tests {
outbound_payments.add_new_awaiting_invoice(
payment_id, expiration, Retry::Attempts(0),
- Some(invoice.amount_msats() / 100 + 50_000)
-+ Some(invoice.amount_msats() / 100 + 50_000), None, OffersContext::Unknown {}
++ Some(invoice.amount_msats() / 100 + 50_000), None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
@@ lightning/src/ln/outbound_payment.rs: mod tests {
assert!(
outbound_payments.add_new_awaiting_invoice(
- payment_id, expiration, Retry::Attempts(0), Some(1234)
-+ payment_id, expiration, Retry::Attempts(0), Some(1234), None, OffersContext::Unknown {}
++ payment_id, expiration, Retry::Attempts(0), Some(1234), None, Nonce::try_from(&[0u8; 16][..]).unwrap()
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
2: bf1d03b9 < -: -------- f: Include Nonce in AwaitingInvoice for context recreation
3: 9b1e056c ! 2: 1cc365ce Introduce enqueue_invoice_request Function
@@ Commit message
## lightning/src/ln/channelmanager.rs ##
@@ lightning/src/ln/channelmanager.rs: use crate::ln::outbound_payment::{OutboundPayments, PaymentAttempts, PendingOutb
use crate::ln::wire::Encode;
- use crate::offers::invoice::{BlindedPayInfo, Bolt12Invoice, DEFAULT_RELATIVE_EXPIRY, DerivedSigningPubkey, ExplicitSigningPubkey, InvoiceBuilder, UnsignedBolt12Invoice};
+ use crate::offers::invoice::{Bolt12Invoice, DEFAULT_RELATIVE_EXPIRY, DerivedSigningPubkey, ExplicitSigningPubkey, InvoiceBuilder, UnsignedBolt12Invoice};
use crate::offers::invoice_error::InvoiceError;
-use crate::offers::invoice_request::{DerivedPayerId, InvoiceRequestBuilder};
+use crate::offers::invoice_request::{DerivedPayerId, InvoiceRequest, InvoiceRequestBuilder};
@@ lightning/src/ln/channelmanager.rs: where
+ fn enqueue_invoice_request(
+ &self,
+ invoice_request: InvoiceRequest,
-+ reply_paths: Vec<BlindedPath>,
++ reply_paths: Vec<BlindedMessagePath>,
+ ) -> Result<(), Bolt12SemanticError> {
let mut pending_offers_messages = self.pending_offers_messages.lock().unwrap();
- if !offer.paths().is_empty() {
4: e9fbabc9 = 3: 8a17319a Introduce handle_message_received in ChannelMessageHandler
5: 656dd95d ! 4: 6a9e9134 Introduce handle_message_received test
@@ lightning/src/ln/offers_tests.rs: fn creates_and_pays_for_offer_using_one_hop_bl
+ assert_ne!(offer.signing_pubkey(), Some(alice_id));
+ assert!(!offer.paths().is_empty());
+ for path in offer.paths() {
-+ assert_eq!(path.introduction_node, IntroductionNode::NodeId(alice_id));
++ assert_eq!(path.introduction_node(), &IntroductionNode::NodeId(alice_id));
+ }
+ let payment_id = PaymentId([1; 32]);
+ bob.node.pay_for_offer(&offer, None, None, None, payment_id, Retry::Attempts(0), None).unwrap();
@@ lightning/src/ln/offers_tests.rs: fn creates_and_pays_for_offer_using_one_hop_bl
+ });
+ assert_eq!(invoice_request.amount_msats(), None);
+ assert_ne!(invoice_request.payer_id(), bob_id);
-+ assert_eq!(reply_path.introduction_node, IntroductionNode::NodeId(bob_id));
++ assert_eq!(reply_path.introduction_node(), &IntroductionNode::NodeId(bob_id));
+ let onion_message = alice.onion_messenger.next_onion_message_for_peer(bob_id).unwrap();
+ bob.onion_messenger.handle_onion_message(&alice_id, &onion_message);
+
@@ lightning/src/ln/offers_tests.rs: fn creates_and_pays_for_offer_using_one_hop_bl
+ assert_eq!(invoice.amount_msats(), 10_000_000);
+ assert_ne!(invoice.signing_pubkey(), alice_id);
+ assert!(!invoice.payment_paths().is_empty());
-+ for (_, path) in invoice.payment_paths() {
-+ assert_eq!(path.introduction_node, IntroductionNode::NodeId(alice_id));
++ for path in invoice.payment_paths() {
++ assert_eq!(path.introduction_node(), &IntroductionNode::NodeId(alice_id));
+ }
+ route_bolt12_payment(bob, &[alice], &invoice);
+ expect_recent_payment!(bob, RecentPaymentDetails::Pending, payment_id); |
|
Updated from pr3010.23 to pr3010.24 (diff): Changes:
|
jkczyz
reviewed
Aug 21, 2024
|
Updated from pr3010.24 to pr3010.25 (diff): Changes:
|
TheBlueMatt
reviewed
Sep 6, 2024
|
Updated from pr3010.25 to pr3010.26 (diff):
|
jkczyz
reviewed
Sep 9, 2024
jkczyz
reviewed
Sep 10, 2024
1. To enable the retry of the Invoice Request message, it's necessary to store the essential data required to recreate the message. 2. A new struct is introduced to manage this data, ensuring the InvoiceRequest message can be reliably recreated for retries. 3. The addition of an `awaiting_invoice` flag allows tracking of retryable invoice requests, preventing the need to lock the `pending_outbound_payment` mutex.
1. Separate the logic of forming `invoice_request` messages from `invoice_request` and `reply_paths` and enqueueing them into a separate function. 2. This logic will be reused in the following commit when reforming `invoice_request` messages for retrying.
- Introduce the `message_received` function to manage the behavior when a message is received from any peer. - This function is used within `ChannelManager` to retry `InvoiceRequest` messages if we haven't received the corresponding invoice yet. - This change makes the offer communication robust against sudden connection drops where the initial attempt to send the message might have failed.
- Add a test to verify the functionality of the handle_message_received function. - Ensure the test covers scenarios where InvoiceRequest messages are retried for PendingOutboundPayments after a simulated connection loss.
jkczyz
approved these changes
Sep 12, 2024
TheBlueMatt
approved these changes
Sep 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #2836
Description:
invoice_requestmessages on new reply_paths that are still awaiting invoices.Changes:
PendingOutboundPayments::AwaitingInvoicevariant to accommodate instances without invoice requests.pay_for_offerto create invoice request messages into a separate function for reuse with retry message flow.retry_tick_occurredfunction in ChannelManager to handle generating invoice request messages for AwaitingInvoice payments and enqueueing them.retry_tick_occurredtoln_background_processorwith a timer duration of 5 seconds for timely retries without overwhelming the system with too many onion messages.