Add liquidity checks and improve payment error handling #3175

slanesuke · 2024-07-13T00:19:37Z

This PR introduces a few improvements including liquidity checks in the pay_for_offer and create_refund_builder, updated error handling, and support for fiat-denominated offers.

Changes:

Liquidity Checks:
- Added early aborts in pay_for_offer and create_refund_builder if channel liquidity is insufficient.
- Added tests to validate the new liquidity checks.
Error Handling:
- Introduced Bolt12RequestError for errors related to BOLT12 payment/refund requests.
- Introduced Bolt12CreationError for errors occurring during the creation of BOLT12 offers/refunds.
Fiat-Denominated Offers:
- Improved support for offers specifying an amount in fiat currencies.
- Added tests for InvoiceRequest creation with currency-based amounts.
Docs updates:
- Updated the docs for InvoiceRequestBuilder::amount_msats to clarify behavior when handling currency-based offers.
- Updated the docs for Bolt12SemanticError::InvalidAmount to include cases where the invoice amount does not match the expected amount.

Fixes #3174

lightning/src/ln/channelmanager.rs

codecov-commenter · 2024-07-15T14:17:41Z

Codecov Report

Attention: Patch coverage is 88.41463% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.59%. Comparing base (43e28fe) to head (35e3daa).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	82.43%	9 Missing and 4 partials ⚠️
lightning/src/ln/offers_tests.rs	96.49%	2 Missing ⚠️
lightning/src/offers/invoice.rs	77.77%	2 Missing ⚠️
lightning/src/offers/invoice_request.rs	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3175      +/-   ##
==========================================
- Coverage   89.62%   89.59%   -0.04%     
==========================================
  Files         127      127              
  Lines      103531   103664     +133     
  Branches   103531   103664     +133     
==========================================
+ Hits        92787    92874      +87     
- Misses       8050     8084      +34     
- Partials     2694     2706      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/ln/channelmanager.rs

tnull

LGTM, but will let @jkczyz take another look.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-07-16T15:45:31Z

Looks like all CI is failing here :(

jkczyz

Drop the fixup message from the commit message.

lightning/src/ln/channelmanager.rs

lightning/src/offers/parse.rs

lightning/src/ln/channelmanager.rs

slanesuke · 2024-07-18T19:30:44Z

@jkczyz I added a test to check the Insufficient Liquidity error in offers_tests. Moved the currency check to the parsing code and modified the amount check to trust the user-provided msats amount (I think I did this correctly).

And regarding the InsufficientLiquidity error type, I left it as is. I just saw the new comments so I'll add the Bolt12CreationError now!

slanesuke · 2024-07-18T19:36:25Z

@jkczyz I added a test to check the Insufficient Liquidity error in offers_tests. Moved the currency check to the parsing code and modified the amount check to trust the user-provided msats amount (I think I did this correctly).

And regarding the InsufficientLiquidity error type, I left it as is. I just saw the new comments so I'll add the Bolt12CreationError now!

Hmmm... looks like a few different test are failing now.

tnull · 2024-07-19T11:04:36Z

Hmmm... looks like a few different test are failing now.

Two of the failures are expected as they test the failure case you just removed 🙂

The ones failing with InsufficientAmount fail because you changed it to check check the offer amount (i.e., the amount for a single item), not the full amount to be sent (which is expected to be offer amount times quantity).

slanesuke · 2024-07-19T19:19:03Z

The variant wrapping Bolt12SemanticError could use a better name and the docs for Bolt12CreationError may need some fine-tuning. But this should be close to finished.

Also, let me know if I need to move any other error variants from Bolt12SemanticErrors!

lightning/src/offers/invoice_request.rs

lightning/src/ln/channelmanager.rs

lightning/src/offers/invoice_request.rs

slanesuke · 2024-07-22T20:51:44Z

@jkczyz ready for review.

lightning/src/ln/offers_tests.rs

lightning/src/offers/invoice_request.rs

lightning/src/ln/channelmanager.rs

lightning/src/offers/invoice_request.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/offers_tests.rs

lightning/src/offers/parse.rs

lightning/src/ln/offers_tests.rs

lightning/src/offers/invoice_request.rs

lightning/src/offers/offer.rs

lightning/src/ln/channelmanager.rs

jkczyz · 2024-08-20T22:28:19Z

I’ve made the changes to add both Bolt12CreationError and Bolt12RequestError as separate error types. Since both share a couple common variants (InvalidSemantics and BlindedPathCreationFailed), I’m wondering about redundancy. Would you guys recommend keeping the current approach with these duplicated variants, or do you think there’s a better way to handle this to avoid redundancy?

Nah, the redundancy is fine here, IMO.

Also, I’ve updated the approach to calculate the total channel liquidity by (hopefully) iterating directly over the channels themselves. Would this be correct/efficient, or are there better ways to accomplish this!?

(cc @jkczyz @TheBlueMatt)

Yeah, I think this is what @TheBlueMatt had in mind.

lightning/src/ln/channelmanager.rs

lightning/src/offers/invoice.rs

lightning/src/offers/invoice_request.rs

slanesuke · 2024-08-22T02:13:24Z

I’ve made the changes to add both Bolt12CreationError and Bolt12RequestError as separate error types. Since both share a couple common variants (InvalidSemantics and BlindedPathCreationFailed), I’m wondering about redundancy. Would you guys recommend keeping the current approach with these duplicated variants, or do you think there’s a better way to handle this to avoid redundancy?

Nah, the redundancy is fine here, IMO.

Also, I’ve updated the approach to calculate the total channel liquidity by (hopefully) iterating directly over the channels themselves. Would this be correct/efficient, or are there better ways to accomplish this!?
(cc @jkczyz @TheBlueMatt)

Yeah, I think this is what @TheBlueMatt had in mind.

Sweet, thanks for the feedback! Just pushed the updates.

jkczyz

Regarding commit messages, please state not only the what but also the why (but not the how). Also, use the imperative instead mood rather than first person (e.g., "Moved" vs "I moved"). See the following guidelines: https://cbea.ms/git-commit/.

lightning/src/offers/invoice_request.rs

jkczyz · 2024-08-22T22:15:07Z

lightning/src/offers/invoice_request.rs

- offer.check_quantity(quantity)?;
- offer.check_amount_msats_for_quantity(amount, quantity)?;
+ match offer.amount() {
+ Some(Amount::Currency {..}) => return Err(Bolt12SemanticError::UnsupportedCurrency),


Unfortunately, it looks like we can't fail like this because this TryFrom implementation is also used when parsing a Bolt12Invoice. So if we send an amount-less request for an Offer with a currency, we'll fail to parse the invoice that is sent back. Here's a branch with a test that demonstrates this. Feel free to cherrypick the commit.

I guess we need to move the these checks higher up in the parsing code, after the contents has been created. Haven't checked if that will have any unexpected behavior.

@TheBlueMatt @tnull I wonder if we should allow parsing an InvoiceRequest with any amount and rather check the amount when handling it (i.e., in ChannelManager). Otherwise, once currency support is added, it would require looking up exchange rates at parsing time, which seems wrong. Also, if no amount is specified in the request, then that is where the issuer would convert from the offer amount. Thoughts?

@jkczyz I moved the checks higher up in the parsing code as you suggested. But, after cherry-picking the commit and running the tests, the parses_invoice_with_currency test fails. Given the context, it looks like checking amounts during handling might be more fitting. If so, I can include this change in this PR.

Sure, let's go ahead and do that. Longer term it makes more sense, IMO.

lightning/src/offers/invoice.rs

lightning/src/offers/invoice_request.rs

lightning/src/ln/offers_tests.rs

lightning/src/ln/channelmanager.rs

jkczyz · 2024-08-22T22:35:55Z

lightning/src/ln/channelmanager.rs

+ 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);
+ }
+ }


@tnull Should we consider outstanding invoice requests / refunds?

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?

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

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

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.

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.

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?

slanesuke · 2024-08-23T14:02:08Z

Regarding commit messages, please state not only the what but also the why (but not the how). Also, use the imperative instead mood rather than first person (e.g., "Moved" vs "I moved"). See the following guidelines: https://cbea.ms/git-commit/.

Okay, gotcha. Atomic commits and commit messages have been a bit of a weak spot for me, but I’ll make sure to get it right! Thanks again

slanesuke · 2024-09-05T14:32:02Z

rebased

shaavan

Generally, the PR looks good to me!

Some small suggestions from my side

shaavan · 2024-09-24T11:34:29Z

lightning/src/ln/offers_tests.rs

- .amount_msats(10_000_000)
- .build().unwrap();
+ .create_offer_builder(None).unwrap()
+ .amount(Amount::Currency { iso4217_code: *b"USD", amount: 6_000 })


Since we are creating offers in fiat denomination in Add tests for Offers with fiat amount commit, maybe we can remove this change and the change below to simplify the PR?

Ah good point! I'll update the PR shortly

shaavan · 2024-09-24T11:38:44Z

lightning/src/ln/channelmanager.rs

@@ -2548,6 +2546,40 @@ pub enum RecentPaymentDetails {
 },
 }

+/// Error during creation and handling of BOLT 12 related payments.


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"?

shaavan · 2024-09-24T11:40:12Z

lightning/src/ln/channelmanager.rs

- .map_err(|_| Bolt12SemanticError::MissingPaths)?;
+ .map_err(|()| Bolt12RequestError::BlindedPathCreationFailed)?;
+
+ let total_liquidity: u64 = {


@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.

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?

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.

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!

Yeah, let's hold off.

Update `InvoiceRequest` parsing to handle currency amounts in offers. The currency check has been moved from `OfferContents::check_amount_msats_for_quantity` to the `InvoiceRequest` parsing code. This makes sure that an error is returned early if an offer specifies an unsupported currency, preventing issues with parsing a `Bolt12Invoice`. This change also simplifies `OfferContents::check_amount_msats_for_quantity` by removing redundant currency validation, allowing it to trust the sender-provided amount.

This fixup commit removes the currency validation from the `InvoiceRequest` parsing code. Additionally, updates a test that previously expected a failure due to an unsupported currency. Since the logic now parses any amount the test no longer triggers the `UnsupportedCurrency` error during parsing. The next commit adds the currency validation during handling in `ChannelManager` instead. This way, when currency support is added, we avoid the need for exchange rate lookups at parsing time.

Add and move the check for unsupported currencies from the parsing code to `handle_message` in `ChannelManager`. This change ensures that we don't fail when parsing an `InvoiceRequest` for an Offer with a currency.

Introduce the `Bolt12CreationError` error type for handling BOLT12-related errors in the `ChannelManager`. This error type consolidates various variants, which were previously part of `Bolt12SemanticError`. Additionally, updated the relevant code to replace `Bolt12SemanticError` with `Bolt12CreationError` throughout the affected files. Note: The following commit will revert the changes in the `create_refund_builder` and `pay_for_offer` methods, switching back to `Bolt12SemanticError` temporarily. A future commit will introduce a `Bolt12RequestError` to handle these scenarios more accurately.

Revert the return type of `create_refund_builder` and `pay_for_offer` from `Bolt12CreationError` back to `Bolt12SemanticError`. This change is temporary, as a future commit will introduce `Bolt12RequestError` for these functions.

Introduced the `Bolt12RequestError` type to handle errors associated with BOLT12 payment and refund requests. This type includes variants for handling `InvalidSemantics`, `DuplicatePaymentId`, `InsufficientLiquidity`, and `BlindedPathCreationFailed`.

Check for sufficient channel liquidity in `pay_for_offer` and abort the payment if there is insufficient liquidity to fulfill the payment. This makes sure that the payment attempt is only made when there is enough outbound liquidity available, preventing failed payment attempts due to liquidity issues.

Adds the `fails_paying_offer_with_insufficient_liquidity` test to ensure that the `pay_for_offer` method correctly handles cases where the liquidity is insufficient. The test verifies that the method returns the `Bolt12RequestError::InsufficientLiquidity` error when the requested payment amount exceeds the available liquidity.

Introduces tests to ensure the successful creation of `InvoiceRequest` when specifying a currency amount with no amount_msats. And verifies that when both a currency amount and amount_msats are specified, the values are handled correctly. This ensures that `InvoiceRequest` behaves as expected with various amount configs.

Improves documentation for the amount_msats method in `InvoiceRequestBuilder`. The update describes the behavior when an `Offer` specifies a currency.

Add a liquidity check in `create_refund_builder` to ensure that refund creation is aborted if there is insufficient outbound liquidity. This change prevents attempts to create a refund when the available liquidity is below the required amount, ensuring that refunds are only created when there is adequate liquidity.

Adds the `fails_creating_refund_with_insufficient_liquidity` test to verify that the `create_refund_builder` method correctly handles cases where there is insufficient channel liquidity. The test verifies that the method returns the `Bolt12RequestError::InsufficientLiquidity` error when the refund amount exceeds the available liquidity.

Add a check to ensure that the amount_msats in an invoice matches the amount_msats specified in the invoice_request or refund. Reject the invoice as invalid if there is a mismatch between these amounts. This validation ensures consistency in invoice handling.

Added doc update to clarify that `InvalidAmount` now also covers cases where the amount in an invoice does not match the expected amount specified in the associated `InvoiceRequest` or `Refund`.

slanesuke · 2024-10-10T18:54:24Z

rebased

slanesuke changed the title ~~Add Liquidity Check to BOLT12 pay_for_offer~~ Add liquidity check to pay_for_offer Jul 13, 2024

tnull self-requested a review July 13, 2024 11:10

tnull reviewed Jul 15, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from debf857 to 389c7a5 Compare July 15, 2024 14:03

jkczyz reviewed Jul 15, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from 389c7a5 to 0dca193 Compare July 15, 2024 21:17

tnull previously approved these changes Jul 16, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

slanesuke dismissed tnull’s stale review via 807af19 July 16, 2024 14:43

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from 0dca193 to 807af19 Compare July 16, 2024 14:43

jkczyz reviewed Jul 16, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/offers/parse.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from 807af19 to 13c3c06 Compare July 18, 2024 19:29

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from 13c3c06 to 7ce02a6 Compare July 18, 2024 21:02

jkczyz reviewed Jul 19, 2024

View reviewed changes

jkczyz reviewed Jul 22, 2024

View reviewed changes

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch 2 times, most recently from f2bc100 to 99b949b Compare July 24, 2024 14:47

jkczyz reviewed Jul 24, 2024

View reviewed changes

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch 3 times, most recently from 2731746 to 23d37fb Compare July 31, 2024 13:36

TheBlueMatt reviewed Aug 6, 2024

View reviewed changes

lightning/src/offers/offer.rs Show resolved Hide resolved

lightning/src/offers/offer.rs Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch 2 times, most recently from 05fc9fd to 8ca69cb Compare August 14, 2024 19:16

jkczyz reviewed Aug 20, 2024

View reviewed changes

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from c69ef2d to 454615d Compare August 22, 2024 02:10

jkczyz reviewed Aug 22, 2024

View reviewed changes

slanesuke changed the title ~~Add liquidity check to pay_for_offer~~ Add liquidity checks and improve payment error handling Aug 25, 2024

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch 3 times, most recently from ef2a15f to 5bbebea Compare August 27, 2024 02:22

jkczyz mentioned this pull request Aug 29, 2024

Support paying static invoices #3140

Merged

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from 5bbebea to a078216 Compare September 5, 2024 14:31

shaavan reviewed Sep 24, 2024

View reviewed changes

slanesuke added 14 commits October 10, 2024 12:00

Move currency check to OffersMessageHandler

2f029a9

Add and move the check for unsupported currencies from the parsing code to `handle_message` in `ChannelManager`. This change ensures that we don't fail when parsing an `InvoiceRequest` for an Offer with a currency.

f Revert Bolt12CreationError to Bolt12SemanticError

5c8be84

Revert the return type of `create_refund_builder` and `pay_for_offer` from `Bolt12CreationError` back to `Bolt12SemanticError`. This change is temporary, as a future commit will introduce `Bolt12RequestError` for these functions.

Add a Bolt12RequestError error type

6f9ac11

Introduced the `Bolt12RequestError` type to handle errors associated with BOLT12 payment and refund requests. This type includes variants for handling `InvalidSemantics`, `DuplicatePaymentId`, `InsufficientLiquidity`, and `BlindedPathCreationFailed`.

Update docs for InvoiceRequestBuilder::amount_msats

15653e8

Improves documentation for the amount_msats method in `InvoiceRequestBuilder`. The update describes the behavior when an `Offer` specifies a currency.

Update docs for Bolt12SemanticError::InvalidAmount

35e3daa

Added doc update to clarify that `InvalidAmount` now also covers cases where the amount in an invoice does not match the expected amount specified in the associated `InvoiceRequest` or `Refund`.

slanesuke force-pushed the 2024-07-abort-insufficient-funds-bolt12 branch from a078216 to 35e3daa Compare October 10, 2024 18:54

Add liquidity checks and improve payment error handling #3175

Are you sure you want to change the base?

Add liquidity checks and improve payment error handling #3175

Conversation

slanesuke commented Jul 13, 2024 • edited Loading

Changes:

codecov-commenter commented Jul 15, 2024 • edited by codecov bot Loading

Codecov Report

tnull left a comment • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Jul 16, 2024

jkczyz left a comment

Choose a reason for hiding this comment

slanesuke commented Jul 18, 2024

slanesuke commented Jul 18, 2024

tnull commented Jul 19, 2024

slanesuke commented Jul 19, 2024 • edited Loading

slanesuke commented Jul 22, 2024

jkczyz commented Aug 20, 2024

slanesuke commented Aug 22, 2024

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slanesuke commented Aug 23, 2024

slanesuke commented Sep 5, 2024

shaavan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slanesuke commented Oct 10, 2024

slanesuke commented Jul 13, 2024 •

edited

Loading

codecov-commenter commented Jul 15, 2024 •

edited by codecov bot

Loading

tnull left a comment •

edited

Loading

slanesuke commented Jul 19, 2024 •

edited

Loading