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

Add liquidity checks and improve payment error handling #3175

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
+228 −43
Open

Add liquidity checks and improve payment error handling #3175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b2c3ddb
Refactor amount validation and relocate currency check
slanesuke Jul 18, 2024
e7edc86
f Remove currency validation from parsing code
slanesuke Aug 26, 2024
2f029a9
Move currency check to `OffersMessageHandler`
slanesuke Aug 26, 2024
61f7348
Add `Bolt12CreationError` error type
slanesuke Jul 19, 2024
5c8be84
f Revert `Bolt12CreationError` to `Bolt12SemanticError`
slanesuke Aug 23, 2024
6f9ac11
Add a `Bolt12RequestError` error type
slanesuke Aug 7, 2024
d38acd3
Abort payment if insufficient liquidity in `pay_for_offer`
slanesuke Jul 12, 2024
7dfcbc5
Add test for insufficient liquidity in `pay_for_offer`
slanesuke Jul 23, 2024
f6f752f
Add tests for `Offers` with fiat amount
slanesuke Jul 25, 2024
15653e8
Update docs for `InvoiceRequestBuilder::amount_msats`
slanesuke Aug 8, 2024
b22a843
Abort `RefundBuilder` creation on insufficient liquidity
slanesuke Aug 7, 2024
7b465f9
Add test for refund creation with insufficient liquidity
slanesuke Aug 24, 2024
f15db05
Validate amount_msats against invoice and refund amounts
slanesuke Aug 8, 2024
35e3daa
Update docs for `Bolt12SemanticError::InvalidAmount`
slanesuke Aug 24, 2024
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
138 changes: 115 additions & 23 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ use crate::offers::invoice::{Bolt12Invoice, DEFAULT_RELATIVE_EXPIRY, DerivedSign
use crate::offers::invoice_error::InvoiceError;
use crate::offers::invoice_request::{DerivedPayerSigningPubkey, InvoiceRequest, InvoiceRequestBuilder};
use crate::offers::nonce::Nonce;
use crate::offers::offer::{Offer, OfferBuilder};
use crate::offers::offer::{Amount, Offer, OfferBuilder};
use crate::offers::parse::Bolt12SemanticError;
use crate::offers::refund::{Refund, RefundBuilder};
use crate::offers::signer;
Expand Down Expand Up @@ -1844,10 +1844,9 @@ where
///
/// ```
/// # use lightning::events::{Event, EventsProvider, PaymentPurpose};
/// # use lightning::ln::channelmanager::AChannelManager;
/// # use lightning::offers::parse::Bolt12SemanticError;
/// # use lightning::ln::channelmanager::{AChannelManager, Bolt12CreationError};
/// #
/// # fn example<T: AChannelManager>(channel_manager: T) -> Result<(), Bolt12SemanticError> {
/// # fn example<T: AChannelManager>(channel_manager: T) -> Result<(), Bolt12CreationError> {
slanesuke marked this conversation as resolved.
Show resolved Hide resolved
/// # let channel_manager = channel_manager.get_cm();
/// # let absolute_expiry = None;
/// let offer = channel_manager
Expand Down Expand Up @@ -1947,13 +1946,12 @@ where
/// ```
/// # use core::time::Duration;
/// # use lightning::events::{Event, EventsProvider};
/// # use lightning::ln::channelmanager::{AChannelManager, PaymentId, RecentPaymentDetails, Retry};
/// # use lightning::offers::parse::Bolt12SemanticError;
/// # use lightning::ln::channelmanager::{AChannelManager, Bolt12RequestError, PaymentId, RecentPaymentDetails, Retry};
/// #
/// # fn example<T: AChannelManager>(
/// # channel_manager: T, amount_msats: u64, absolute_expiry: Duration, retry: Retry,
/// # max_total_routing_fee_msat: Option<u64>
/// # ) -> Result<(), Bolt12SemanticError> {
/// # ) -> Result<(), Bolt12RequestError> {
/// # let channel_manager = channel_manager.get_cm();
/// let payment_id = PaymentId([42; 32]);
/// let refund = channel_manager
Expand Down Expand Up @@ -2685,6 +2683,40 @@ pub enum RecentPaymentDetails {
},
}

/// Error during creation and handling of BOLT 12 related payments.
Copy link
Contributor

Choose a reason for hiding this comment

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

The roles of Bolt12CreationError, and Bolt12Response feels a little confusing to me.

For example

It reads during creation and handling of BOLT 12 related payments but technically we are not creating any payment when we are creating an offer.

Maybe we can simplify the enums, by introducing only one new enum called Bolt12CreationError that contains "Error during creation of BOLT12 Messages"?

#[derive(Debug, Clone, PartialEq)]
pub enum Bolt12CreationError {
/// Error from BOLT 12 semantic checks.
InvalidSemantics(Bolt12SemanticError),
/// Failed to create a blinded path.
BlindedPathCreationFailed,
}

impl From<Bolt12SemanticError> for Bolt12CreationError {
fn from(err: Bolt12SemanticError) -> Self {
Bolt12CreationError::InvalidSemantics(err)
}
}

/// Error during requesting a BOLT 12 related payment.
#[derive(Debug, Clone, PartialEq)]
pub enum Bolt12RequestError {
/// Error from BOLT 12 semantic checks.
InvalidSemantics(Bolt12SemanticError),
/// The payment id for a refund or request is already in use.
DuplicatePaymentId,
/// There is insufficient liquidity to complete the payment.
InsufficientLiquidity,
/// Failed to create a blinded path.
BlindedPathCreationFailed,
}

impl From<Bolt12SemanticError> for Bolt12RequestError {
fn from(err: Bolt12SemanticError) -> Self {
Bolt12RequestError::InvalidSemantics(err)
}
}

/// Route hints used in constructing invoices for [phantom node payents].
///
/// [phantom node payments]: crate::sign::PhantomKeysManager
Expand Down Expand Up @@ -9114,7 +9146,7 @@ macro_rules! create_offer_builder { ($self: ident, $builder: ty) => {
/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
pub fn create_offer_builder(
&$self, absolute_expiry: Option<Duration>
) -> Result<$builder, Bolt12SemanticError> {
) -> Result<$builder, Bolt12CreationError> {
let node_id = $self.get_our_node_id();
let expanded_key = &$self.inbound_payment_key;
let entropy = &*$self.entropy_source;
Expand All @@ -9124,7 +9156,7 @@ macro_rules! create_offer_builder { ($self: ident, $builder: ty) => {
let context = OffersContext::InvoiceRequest { nonce };
let path = $self.create_blinded_paths_using_absolute_expiry(context, absolute_expiry)
.and_then(|paths| paths.into_iter().next().ok_or(()))
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
.map_err(|()| Bolt12CreationError::BlindedPathCreationFailed)?;
let builder = OfferBuilder::deriving_signing_pubkey(node_id, expanded_key, nonce, secp_ctx)
.chain_hash($self.chain_hash)
.path(path);
Expand Down Expand Up @@ -9187,7 +9219,7 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
pub fn create_refund_builder(
&$self, amount_msats: u64, absolute_expiry: Duration, payment_id: PaymentId,
retry_strategy: Retry, max_total_routing_fee_msat: Option<u64>
) -> Result<$builder, Bolt12SemanticError> {
) -> Result<$builder, Bolt12RequestError> {
let node_id = $self.get_our_node_id();
let expanded_key = &$self.inbound_payment_key;
let entropy = &*$self.entropy_source;
Expand All @@ -9197,7 +9229,28 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
let context = OffersContext::OutboundPayment { payment_id, nonce, hmac: None };
let path = $self.create_blinded_paths_using_absolute_expiry(context, Some(absolute_expiry))
.and_then(|paths| paths.into_iter().next().ok_or(()))
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
.map_err(|()| Bolt12RequestError::BlindedPathCreationFailed)?;

let total_liquidity: u64 = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkczyz

Should we add a similar liquidity check for inbound liquidity? This would be helpful when we are creating an offer, or calling request_refund_payment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Offers can be long-lived and be paid more than once. So it would be only be accurate for short-lived, single-use offers. For those and refund payment request, I wonder if that approach is preferable as it allows us to give early indication of a failure to the user. Drawback is it would prevent creating an offer / sending an invoice even if liquidity is soon-but-not-yet available (e.g., future JIT channel work). @tnull Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ISTM we should consider it for refunds since those are also outbound liquidity (and likely to be claimed ~immediately), but for inbound liquidity yea I'd be kinda skeptical of it given JIT channels can come in many forms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So IIUC, the outbound liquidity check for refunds makes sense since they’re likely to be claimed quickly. But for inbound liquidity, JIT channels could complicate things. Should we hold off on adding an inbound liquidity check in this PR? I’d appreciate any thoughts on how to proceed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's hold off.

let per_peer_state = &$self.per_peer_state.read().unwrap();
let mut sum: u64 = 0;

per_peer_state.iter().for_each(|(_, peer_state_mutex)| {
let peer_state_lock = peer_state_mutex.lock().unwrap();
let peer_state = &*peer_state_lock;

sum += peer_state.channel_by_id
.iter()
.map(|(_, phase)| phase.context().get_available_balances(&$self.fee_estimator).outbound_capacity_msat)
.sum::<u64>();
});
sum
};

if amount_msats > total_liquidity {
log_error!($self.logger, "Insufficient liquidity for payment with payment id: {}", payment_id);
return Err(Bolt12RequestError::InsufficientLiquidity);
}

let builder = RefundBuilder::deriving_signing_pubkey(
node_id, expanded_key, nonce, secp_ctx, amount_msats, payment_id
Expand All @@ -9213,7 +9266,7 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
.add_new_awaiting_invoice(
payment_id, expiration, retry_strategy, max_total_routing_fee_msat, None,
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;
.map_err(|()| Bolt12RequestError::DuplicatePaymentId)?;

Ok(builder.into())
}
Expand Down Expand Up @@ -9305,7 +9358,7 @@ where
&self, offer: &Offer, quantity: Option<u64>, amount_msats: Option<u64>,
payer_note: Option<String>, payment_id: PaymentId, retry_strategy: Retry,
max_total_routing_fee_msat: Option<u64>
) -> Result<(), Bolt12SemanticError> {
) -> Result<(), Bolt12RequestError> {
let expanded_key = &self.inbound_payment_key;
let entropy = &*self.entropy_source;
let secp_ctx = &self.secp_ctx;
Expand Down Expand Up @@ -9335,7 +9388,38 @@ where
OffersContext::OutboundPayment { payment_id, nonce, hmac: Some(hmac) }
);
let reply_paths = self.create_blinded_paths(context)
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
.map_err(|()| Bolt12RequestError::BlindedPathCreationFailed)?;

let total_liquidity: u64 = {
let per_peer_state = &self.per_peer_state.read().unwrap();
let mut sum: u64 = 0;

per_peer_state.iter().for_each(|(_, peer_state_mutex)| {
let peer_state_lock = peer_state_mutex.lock().unwrap();
let peer_state = &*peer_state_lock;

sum += peer_state.channel_by_id
.iter()
.map(|(_, phase)| phase.context().get_available_balances(&self.fee_estimator).outbound_capacity_msat)
.sum::<u64>();
});
sum
};

let requested_amount_msats = match invoice_request.amount_msats() {
Some(amount_msats) => Some(amount_msats),
None => match offer.amount() {
Some(Amount::Bitcoin { amount_msats }) => Some(amount_msats),
_ => None,
},
};

if let Some(amount) = requested_amount_msats {
if amount > total_liquidity {
log_error!(self.logger, "Insufficient liquidity for payment with payment id: {}", payment_id);
return Err(Bolt12RequestError::InsufficientLiquidity);
}
}
Comment on lines +9417 to +9422
Copy link
Contributor

Choose a reason for hiding this comment

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

@tnull Should we consider outstanding invoice requests / refunds?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, remind me, what would be the timeout for pending invoice requests, i.e., when would we abort and clear them? Is it one timer tick? Given that we wouldn't consider them for other payment types (BOLT11), it might come as a surprise that you're able to send legacy payments but BOLT12 fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Invoice requests are one tick (so max ~2 ticks if called right after a tick), while refunds are based on the refund's expiration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, no super strong opinion, but maybe we should roughly align the failure cases across different payment types, so maybe not worth?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand the point. For BOLT11, if we initiate a bunch of payments, each subsequent one will have less liquidity to consider, IIUC. But for BOLT12, we can similarly initiate more than one payment but each subsequent one will consider the same liquidity unless an invoice is received for a prior one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply! Since outstanding invoice requests and refunds can tie up liquidity, do you think we should check for any outstanding invoices and see how they affect available liquidity before moving forward with the payment? And should I address it in this PR? I want to make sure to cover all our bases.

Copy link
Contributor

Choose a reason for hiding this comment

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

So they can potentially use liquidity if an invoice is received in response. But they don't tie up liquidity since a payment hasn't been made yet. That said, we can consider a request for payment as an intention to tie up liquidity in the hopefully near future. I'd lean towards including it but will defer to @tnull.

FWIW, there's also some edge cases to consider. If we have a fiat-denominated offer and the users doesn't give an amount, how much liquidity should we considered unavailable?


let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);

Expand All @@ -9349,16 +9433,17 @@ where
payment_id, expiration, retry_strategy, max_total_routing_fee_msat,
Some(retryable_invoice_request)
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;
.map_err(|()| Bolt12RequestError::DuplicatePaymentId)?;

self.enqueue_invoice_request(invoice_request, reply_paths)
self.enqueue_invoice_request(invoice_request, reply_paths)?;
Ok(())
}

fn enqueue_invoice_request(
&self,
invoice_request: InvoiceRequest,
reply_paths: Vec<BlindedMessagePath>,
) -> Result<(), Bolt12SemanticError> {
) -> Result<(), Bolt12RequestError> {
let mut pending_offers_messages = self.pending_offers_messages.lock().unwrap();
if !invoice_request.paths().is_empty() {
reply_paths
Expand All @@ -9384,7 +9469,7 @@ where
}
} else {
debug_assert!(false);
return Err(Bolt12SemanticError::MissingIssuerSigningPubkey);
return Err(Bolt12RequestError::InvalidSemantics(Bolt12SemanticError::MissingIssuerSigningPubkey));
}

Ok(())
Expand Down Expand Up @@ -9414,7 +9499,7 @@ where
/// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice
pub fn request_refund_payment(
&self, refund: &Refund
) -> Result<Bolt12Invoice, Bolt12SemanticError> {
) -> Result<Bolt12Invoice, Bolt12CreationError> {
let expanded_key = &self.inbound_payment_key;
let entropy = &*self.entropy_source;
let secp_ctx = &self.secp_ctx;
Expand All @@ -9423,7 +9508,7 @@ where
let relative_expiry = DEFAULT_RELATIVE_EXPIRY.as_secs() as u32;

if refund.chain() != self.chain_hash {
return Err(Bolt12SemanticError::UnsupportedChain);
return Err(Bolt12CreationError::InvalidSemantics(Bolt12SemanticError::UnsupportedChain));
}

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
Expand All @@ -9434,7 +9519,7 @@ where
let payment_paths = self.create_blinded_payment_paths(
amount_msats, payment_secret, payment_context
)
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
.map_err(|()| Bolt12CreationError::BlindedPathCreationFailed)?;

#[cfg(feature = "std")]
let builder = refund.respond_using_derived_keys(
Expand All @@ -9457,7 +9542,7 @@ where
payment_hash: invoice.payment_hash(), nonce, hmac
});
let reply_paths = self.create_blinded_paths(context)
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
.map_err(|()| Bolt12CreationError::BlindedPathCreationFailed)?;

let mut pending_offers_messages = self.pending_offers_messages.lock().unwrap();
if refund.paths().is_empty() {
Expand Down Expand Up @@ -9486,7 +9571,7 @@ where

Ok(invoice)
},
Err(()) => Err(Bolt12SemanticError::InvalidAmount),
Err(()) => Err(Bolt12CreationError::InvalidSemantics(Bolt12SemanticError::InvalidAmount)),
}
}

Expand Down Expand Up @@ -11135,6 +11220,13 @@ where
None => return None,
};

if let Some(amount) = invoice_request.amount() {
if let Amount::Currency { .. } = amount {
let error = Bolt12SemanticError::UnsupportedCurrency;
return Some((OffersMessage::InvoiceError(error.into()), responder.respond()));
}
}

let nonce = match context {
None if invoice_request.metadata().is_some() => None,
Some(OffersContext::InvoiceRequest { nonce }) => Some(nonce),
Expand Down
Loading
Loading