Add DeleteInvoice gRPC call

[MPins]

Fixes #8161

Steps to Test

lncli deleteinvoices --all lncli deleteinvoices --invoice_hashes hash1,hash2...

This change adds the delete invoices gRPC call and the lncli functionality. Is is splited in two rpc calls:

1. Delete all cancelled invoices
2. Delete specific cancelled invoices by their payment hashes

[coderabbitai[bot]]

[!IMPORTANT]

Review skipped

Auto reviews are limited to specific labels.

:label: Labels to auto review (1)

• llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[Abdulkbk]

Thanks for the PR @MPins. I tested the changes and have a few early observations.

The --all arg returns "status": "all cancelled invoices deleted"; it would be nice to specify the number of invoices deleted, just like how deletepayments --all returns something like X payments deleted....

Also, rather than using --hash_list, wouldn't naming it --invoice_hashes or something else be more descriptive?

I also noticed that the first_ and last_ indices do not readjust after deleting some invoices. However, I think that's the correct behavior (similar to deletepayments) because readjusting the indices would be expensive and not necessarily useful. Also, the check preventing using --all and --hash_list simultaneously works.

Rebase and push so I can take a look at the code as well

[Abdulkbk]

It took me some time to find a way to cancel an invoice. I'm sharing this to help anyone who wants to test this change.

Steps

1. Install LND using make install tags=invoicesrpc (to enable cancelinvoice rpc call)
2. Create some invoices lncli addinvoice --amt=1000
3. Verify the created invoices lncli listinvoices
4. Cancel an invoice lncli cancelinvoice <invoice-hash>
5. Delete invoice lncli deleteinvoices --all or lncli deleteinvoices --hash_list=<inv-hash1>,<inv-hash2>
6. Verify that the canceled invoice was deleted lncli listinvoices or lncli lookupinvoice <invoice-hash>

[MPins]

It took me some time to find a way to cancel an invoice. I'm sharing this to help anyone who wants to test this change.

Steps

1. Install LND using `make install tags=invoicesrpc` (to enable `cancelinvoice` rpc call)

2. Create some invoices `lncli addinvoice --amt=1000`

3. Verify the created invoices `lncli listinvoices`

4. Cancel an invoice `lncli cancelinvoice <invoice-hash>`

5. Delete invoice `lncli deleteinvoices --all` or `lncli deleteinvoices --hash_list=<inv-hash1>,<inv-hash2>`

6. Verify that the canceled invoice was deleted `lncli listinvoices` or `lncli lookupinvoice <invoice-hash>`

You can also create an invoice with a very short expiration time, e.g., lncli addinvoice --expiry=1 --amt=1000

[MPins]

Thanks for the PR @MPins. I tested the changes and have a few early observations.

The --all arg returns "status": "all cancelled invoices deleted"; it would be nice to specify the number of invoices deleted, just like how deletepayments --all returns something like X payments deleted....

Also, rather than using --hash_list, wouldn't naming it --invoice_hashes or something else be more descriptive?

I also noticed that the first_ and last_ indices do not readjust after deleting some invoices. However, I think that's the correct behavior (similar to deletepayments) because readjusting the indices would be expensive and not necessarily useful. Also, the check preventing using --all and --hash_list simultaneously works.

Rebase and push so I can take a look at the code as well

Thank you, @Abdulkbk! I’ll address your earlier observations, rebase, and push the updates shortly.

[Abdulkbk]

You can also create an invoice with a very short expiration time, e.g., lncli addinvoice --expiry=1 --amt=1000

Cool, it wasn't working for me yesterday but tried with short expiration today and it worked 👍

[MPins]

Thanks for the PR @MPins. I tested the changes and have a few early observations. The --all arg returns "status": "all cancelled invoices deleted"; it would be nice to specify the number of invoices deleted, just like how deletepayments --all returns something like X payments deleted....

The func DeleteCanceledInvoices (which we are using for deleting all the canceled invoices) does not return the number of deleted invoices, changing this function would impact several parts of the code that rely on this function. At first glance, it doesn't seem worth it.

That said, when deleting a specific list of invoices, the number of deleted items is now reported. Since we delete them individually, using a counter was appropriate.

Also, rather than using --hash_list, wouldn't naming it --invoice_hashes or something else be more descriptive? I also noticed that the first_ and last_ indices do not readjust after deleting some invoices. However, I think that's the correct behavior (similar to deletepayments) because readjusting the indices would be expensive and not necessarily useful. Also, the check preventing using --all and --hash_list simultaneously works. Rebase and push so I can take a look at the code as well

Thank you, @Abdulkbk! I’ll address your earlier observations, rebase, and push the updates shortly.

Rebase and push done.

For the moment, no iTest has been implemented — following the approach of the deletepayments command, which also doesn't have a dedicated iTest. I'd appreciate hearing your thoughts on this, as well as feedback from others.

[MPins]

Hello @Abdulkbk, thank you for reviewing it! :+1: I’ve addressed your feedback — it’s now ready for another round.

[MPins]

Thank you again @Abdulkbk for your careful review! :pray: I've addressed your comments.

[ziggie1984]

How is the progress on this issue ? You can mute the review bot and restart it if you have a new version ready @MPins

[MPins]

How is the progress on this issue ? You can mute the review bot and restart it if you have a new version ready @MPins

Hello, I’m pushing a new version today.

[MPins]

Did a first pass, I still think we should go with just one RPC, but if another review favours your approach I am also ok with having two rpc endpoints, but seems over the top with two.

Left some comments.

Thank you! I’m now convinced that having a single RPC is the better approach. The changes are ready for your next review.

[saubyk]

This feedback might be coming a little in development cycle, but I am wondering if we can add invoices with expired status as well to the scope?

In that case the rpc can be deleteinvoices instead of deletecanceledinvoices

[MPins]

This feedback might be coming a little in development cycle, but I am wondering if we can add invoices with expired status as well to the scope?

In that case the rpc can be deleteinvoices instead of deletecanceledinvoices

In any case, the idea was somewhat brought up by @ziggie1984 a few weeks ago. https://github.com/lightningnetwork/lnd/pull/9625#discussion_r2131717867

Expired invoices are automatically transitioned to the canceled state by the InvoiceExpiryWatcher, so we probably don’t need to worry about them.

https://github.com/lightningnetwork/lnd/blob/5f961e434940b943de7067a65e1d5f76b8271c97/invoices/invoice_expiry_watcher.go#L61

[ziggie1984]

/gemini review

[lightninglabs-deploy]

@hieblmi: review reminder @mpins, remember to re-request review from reviewers when ready

[saubyk]

lncli deletecanceledinvoices --all lncli deletecanceledinvoices --invoice_hashes hash1,hash2...

I am a little vary of the option to enable deletion of "all" canceled invoices. Especially for nodes with large dbs, this can potentially result in table scans which can impact the node performance. It's precisely for this reason the garbage collection of canceled invoices is enabled at startup, so that bulk delete doesn't impact a live node's performance.

Do we want to reconsider enabling the "all" option?

Also, why enable deletion of more than one invoice in the arguments? IMO it's better to keep it simple and enable a simple delete of one invoice. The rpc can be called in a loop if a user is interested in deleting more than one invoice.

[MPins]

lncli deletecanceledinvoices --all lncli deletecanceledinvoices --invoice_hashes hash1,hash2...

I am a little vary of the option to enable deletion of "all" canceled invoices. Especially for nodes with large dbs, this can potentially result in table scans which can impact the node performance. It's precisely for this reason the garbage collection of canceled invoices is enabled at startup, so that bulk delete doesn't impact a live node's performance.

Do we want to reconsider enabling the "all" option?

IMO, the "all" option shouldn’t be a concern because it is done with just a single pass, for both kvdb and SQL.

Also, why enable deletion of more than one invoice in the arguments? IMO it's better to keep it simple and enable a simple delete of one invoice. The rpc can be called in a loop if a user is interested in deleting more than one invoice.

I think the list of invoices hashes to be deleted is a good approach to support typical node operators as well, who would likely use the command line to delete some invoices. If the other reviewers also find it appropriate, we could limit the maximum number of invoices to be deleted, or at least highlight in the documentation that deleting a very large list of invoices may have some performance impact.

[ziggie1984]

After discussing this PR with @bhandras, I have some concerns about the potential for performance degradation.

The Problem with Bulk Deletion

During a recent refactoring of the payments lifecycle, I observed that deletepayments --all causes a significant performance hit on LND nodes with large databases. We should avoid introducing the same vulnerability for invoice deletion.

If we were to implement a "delete all" feature, it should be done carefully. For example, we could trigger a background garbage collection job that deletes invoices in small batches (e.g., 1000 at a time) to avoid overwhelming the node.

Recommendation for This PR

I propose we take a more conservative approach for now:

Limit this PR to deleting a single invoice. LND processes invoice deletions one by one anyway. Exposing only a single-invoice deletion API is the safest approach, as it gives the caller control while minimizing the risk of them accidentally degrading the daemon's performance.

Postpone advanced bulk deletion. We can revisit more powerful deletion capabilities in the future, particularly for the SQL implementation, where we could introduce filtering by criteria like creation date.

Let's not rush this functionality at the cost of stability. A single-invoice deletion API provides core functionality without the associated performance risks.

[MPins]

After discussing this PR with @bhandras, I have some concerns about the potential for performance degradation.

The Problem with Bulk Deletion

During a recent refactoring of the payments lifecycle, I observed that deletepayments --all causes a significant performance hit on LND nodes with large databases. We should avoid introducing the same vulnerability for invoice deletion.

If we were to implement a "delete all" feature, it should be done carefully. For example, we could trigger a background garbage collection job that deletes invoices in small batches (e.g., 1000 at a time) to avoid overwhelming the node.

Recommendation for This PR

I propose we take a more conservative approach for now:

Limit this PR to deleting a single invoice. LND processes invoice deletions one by one anyway. Exposing only a single-invoice deletion API is the safest approach, as it gives the caller control while minimizing the risk of them accidentally degrading the daemon's performance.

Postpone advanced bulk deletion. We can revisit more powerful deletion capabilities in the future, particularly for the SQL implementation, where we could introduce filtering by criteria like creation date.

Let's not rush this functionality at the cost of stability. A single-invoice deletion API provides core functionality without the associated performance risks.

Thank you @ziggie1984 and @bhandras for the thorough review and for pointing out the potential performance risks. Let's keep it simple as suggested by @saubyk — it's not worth running the risk of compromising LND's stability.

[MPins]

@ziggie1984, @bhandras , @hieblmi and @GustavoStingelin ready for another review! Thanks.

[MPins]

Looking really good final grammar nits here:

  cmd/commands/cmd_invoice.go
   * Line 445: This command delete a canceled invoice...
       * Suggestion: This command deletes a canceled invoice... (Verb tense)

 docs/release-notes/release-notes-0.20.0.md
   * Line 74: ...to allow removal of canceled invoice.
       * Suggestion: ...to allow the removal of a canceled invoice. (Article usage)
   * Line 75: Supports deleting canceled invoice...
       * Suggestion: Supports deleting a canceled invoice... (Article usage)

  itest/lnd_delete_canceled_invoice_test.go
   * Line 39: // Let's assert an error while providing a invalid invoice hash.
       * Suggestion: // Let's assert an error while providing an invalid invoice hash. (Article usage: "an" before a vowel sound)
   * Line 54: // Let's assert an error while deleting a non existing invoice.
       * Suggestion: // Let's assert an error while deleting a non-existent invoice. (More standard phrasing)
   * Line 70: // Assert that just one invoice exist.
       * Suggestion: // Assert that just one invoice exists. (Verb tense)

  lnrpc/lightning.proto
   * Line 344: // DeleteCanceledInvoice deletes canceled invoice from DB.
       * Suggestion: // DeleteCanceledInvoice deletes a canceled invoice from the DB. (Article usage)

  rpcserver.go
   * Line 7552: // DeleteCanceledInvoice remove canceled invoice from the database.
       * Suggestion: // DeleteCanceledInvoice removes a canceled invoice from the database. (Verb tense and article usage)

Thank you @ziggie1984! The comments were addressed.

[MPins]

Had some additional formating things while comparing the rules in the guide ;)

Thank you, I think I'm finally getting the hang of these formatting things 😉