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.

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

[feature]: fail AddInvoice if not enough capacity to actually receive the payment #9380

Open
ZZiigguurraatt opened this issue Dec 20, 2024 · 13 comments
Labels
enhancement Improvements to existing features / behaviour feature request Requests for new features P2 should be fixed if one has time

Comments

@ZZiigguurraatt
Copy link

Is your feature request related to a problem? Please describe.
Avoiding payment we know will fail before the customer tries to pay.

Describe the solution you'd like
Make an option for AddInvoice that will fail if the node does not have enough inbound capacity to actually receive the payment. This check should consider all open invoices that have not been paid and not been expired.

This can then allow the node operator to catch the error and get some new channels opened (then re-request an invoice to be created), or not try to do the business deal and tell the customer gracefully that they currently can't receive payments.

Describe alternatives you've considered

  • Let the payment fail and have a bad user experience
  • Have another daemon running that keeps checking to see if new invoices are generated and try and quickly get new channels opened before the customer makes payment.

Additional context
We could proactively check if we think there is going to be enough channel capacity before requesting an invoice to be generated, HOWEVER, this can't be done if the point of sale device only has an invoice macaroon. Also, even if we were somehow okay having a more privileged macaroon on the point of sale device, it would be better if every application developer didn't have to reinvent this wheel.

I think this feature should be optional, but on by default. The reason it should be optional is because some people may have an LSP that automatically creates an inbound channel if not enough liquidity is available. For others that don't, this feature can be essential.

This should also be done for AddHoldInvoice.

@ZZiigguurraatt ZZiigguurraatt added the enhancement Improvements to existing features / behaviour label Dec 20, 2024
@yyforyongyu yyforyongyu added the feature request Requests for new features label Dec 20, 2024
@ZZiigguurraatt
Copy link
Author

Related: I'm not sure what currently happens, but if private route hints are enabled, it might be good to also check those for sufficient capacity before adding to an invoice. This consideration is different though because we may not fail the entire AddInvoice command, but we may fail to add some of the route hints that we don't want to waste the payer's time trying to pay that we know they won't work.

@saubyk saubyk added the P2 should be fixed if one has time label Dec 20, 2024
@starius
Copy link
Collaborator

starius commented Dec 20, 2024

I propose warning the user that the invoice amount is larger than the inbound liquidity, instead of failing invoice creation. Maybe the user knows what they are doing. For example, they might plan to pay the invoice after increasing inbound liquidity (by receiving a channel or sending a Lightning payment).

This check should consider all open invoices that have not been paid and not been expired.

Sometimes I create an invoice and then decide not to use it, creating another one instead, e.g., if the amount changes. Also, sometimes my invoice fails, and I create another one with a lower amount. If this feature is implemented, the new invoice may fail because inbound liquidity is "assigned" to a previous invoice that I abandoned.

@ZZiigguurraatt
Copy link
Author

Sometimes I create an invoice and then decide not to use it, creating another one instead, e.g., if the amount changes. Also, sometimes my invoice fails, and I create another one with a lower amount. If this feature is implemented, the new invoice may fail because inbound liquidity is "assigned" to a previous invoice that I abandoned.

Why don't you cancel those invoices after you've decided you don't need them? I know they are auto garbage collected after they expire, but seems in the case you described, you should manually cancel them.

@ZZiigguurraatt
Copy link
Author

I propose warning the user that the invoice amount is larger than the inbound liquidity, instead of failing invoice creation. Maybe the user knows what they are doing. For example, they might plan to pay the invoice after increasing inbound liquidity (by receiving a channel or sending a Lightning payment).

A warning message could also work instead of a failure. This may be the more flexible option as the application developer can choose to fail after seeing the warning. HOWEVER, if application 1 gets the warning, then requests more inbound liquidity, then while inbound liquidity is being received, application 2 requests an invoice before application 1 sees the new liquidity arrives, application 2 might not get the warning and then application 1 and application 2's invoices are both depending on the same liquidity. Also, if we want our applications to use invoice macaroons because they are POS devices out in the field, they shouldn't be able to check to add more liquidity themselves and they also shouldn't be able to check liquidity changes.

So, I think we need to have the option of a warning message OR failure to generate an invoice.

Maybe we also need some capability in LND when there is a failure to allow some other "hook" to request a more privileged process to secure the new inbound liquidity so the unprivileged POS devices don't have to do anything. Even better might be for AddInvoice to optionally block rather than fail while this "hook" is securing the new inbound liquidity. I guess this is starting to emulate an LSP though?

@starius
Copy link
Collaborator

starius commented Dec 20, 2024

Why don't you cancel those invoices after you've decided you don't need them?

Probably I should. I just abandon them because they don't get in the way.

Another scenario to consider is when invoices are created automatically by a service based on user requests. For example, when a Lightning address is used, a user could generate multiple invoices for the maximum amount without paying them. This could disrupt the workflow for other users, given the proposal is implemented.

application 2 requests an invoice before application 1 sees the new liquidity arrives

If the warning is triggered when the total amount of all open invoices exceeds the inbound liquidity, then app 2 will also see a warning. If a warning is implemented instead of a failure, it might make sense to sum up all open invoices rather than only comparing the current invoice amount with the available inbound liquidity.

Maybe we also need some capability in LND when there is a failure to allow some other "hook" to request a more privileged process to secure the new inbound liquidity so the unprivileged POS devices don't have to do anything.

This might be slightly outside the scope of the original issue. Also, I’m not entirely sure how this problem would be detected. Could it be that the payment got stuck somewhere along the route? In that case, LND might have sufficient inbound liquidity, but its peers might not.

@ZZiigguurraatt
Copy link
Author

it might make sense to sum up all open invoices rather than only comparing the current invoice amount with the available inbound liquidity.

That's why I said "This check should consider all open invoices that have not been paid and not been expired." in my original issue.

@ZZiigguurraatt
Copy link
Author

ZZiigguurraatt commented Dec 20, 2024

This might be slightly outside the scope of the original issue. Also, I’m not entirely sure how this problem would be detected. Could it be that the payment got stuck somewhere along the route? In that case, LND might have sufficient inbound liquidity, but its peers might not.

I don't think we can ever know what our peer's inbound liquidity is, but if we can hope they are routing nodes which want to profit from successful payments to us, then it's a reasonable assumption that they should always have enough. It's likely their liquidity is orders of magnitude greater so they will be managing liquidity in other ways. I think this issue assumes the user is using LND as a point of sale node and not a routing node.

@ZZiigguurraatt
Copy link
Author

If the warning is triggered when the total amount of all open invoices exceeds the inbound liquidity, then app 2 will also see a warning

OK, maybe you are right!

@ZZiigguurraatt
Copy link
Author

Another scenario to consider is when invoices are created automatically by a service based on user requests. For example, when a Lightning address is used, a user could generate multiple invoices for the maximum amount without paying them. This could disrupt the workflow for other users, given the proposal is implemented.

Seems like the lightning address server should have some rate limiting and also have short invoice expiry times? Let's say their are 10 open invoices and a limit of 10 and they have a expiry of 120 seconds, the LN address server should just block until one expires before returning a new invoice.

@ZZiigguurraatt
Copy link
Author

This might be slightly outside the scope of the original issue. Also, I’m not entirely sure how this problem would be detected. Could it be that the payment got stuck somewhere along the route?

I guess I meant that if this new feature fails to generate an invoice (because it does not think there is enough inbound liquidity, not that the payment fails somewhere else along the route after generating and sharing an invoice), then fire the "hook" to request new inbound liquidity.

@starius
Copy link
Collaborator

starius commented Dec 20, 2024

I guess I meant that if this new feature fails to generate an invoice (because it does not think there is enough inbound liquidity, not that the payment fails somewhere else along the route after generating and sharing an invoice), then fire the "hook" to request new inbound liquidity.

Got it! In my opinion, this logic should be handled by the calling application rather than being implemented directly within LND. The flow could be something like: "If a warning is received when creating an invoice, then request additional inbound liquidity." The warning itself could act as the hook to trigger the request for more liquidity.

At the RPC level, this warning could simply be a new field in the AddInvoiceResponse. The field might include a proto message with details about the situation, such as the amount of inbound liquidity to request or other relevant information. lncli could display a warning if it detects this message, while other applications could handle it in their own way.

@ZZiigguurraatt
Copy link
Author

Got it! In my opinion, this logic should be handled by the calling application rather than being implemented directly within LND. The flow could be something like: "If a warning is received when creating an invoice, then request additional inbound liquidity." The warning itself could act as the hook to trigger the request for more liquidity.

Why do you want to require your calling application to have permissions to do channel management? A POS with a low level employee or overnight burglar access, a web server, or unattended machine in public can't be trusted with that.

@starius
Copy link
Collaborator

starius commented Dec 20, 2024

A POS with a low level employee or overnight burglar access, a web server, or unattended machine in public can't be trusted with that.

👍

I’m not entirely sure whether the best place for this code is within LND itself or in a separate trusted application with access to the LND API (e.g., ChannelBalance) and the liquidity provider to request a channel. In the latter case, the external app could implement more customized logic, suited to specific business needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour feature request Requests for new features P2 should be fixed if one has time
Projects
None yet
Development

No branches or pull requests

4 participants