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

Hold-Invoices over grpc #6358

Closed

Conversation

daywalker90
Copy link
Contributor

Hi,

Motivation

Hold-Invoices are used today on different projects for different purposes like return-payments for merchants or security bonds for exchanges.

This PR

This pr adds a stripped-down copy of cln-grpc that enables cln to have Hold-Invoices like lnd.

For this i tried to mimic lnds grpc methods by introducing these new methods:

  • HoldInvoice is just a wrapper for Invoice that will make a database entry to mark it as a hold invoice.
  • HoldInvoiceSettle marks the invoice related htlcs to be settled
  • HoldInvoiceCancel marks the invoice related htlcs to be rejected
  • HoldInvoiceLookup returns the status of a hold invoice, which has more possible states than a regular invoice. Will wait with a timeout to confirm final settlement/rejection.

The most notable difference for users of Hold-Invoices between lnd and cln is that if an invoice is close (60s) to expiry, this plugin will reject all related htlcs and mark it as CANCELED since cln can't accept htlcs for expired invoices.

Some technical stuff:

  1. the plugin saves the payment hashes of Hold-Invoices in the cln database and is hooked into htlc_accepted. If the payment_hash of the htlc is found by the plugin the htlc is held in a loop. This makes the Hold-Invoices restart-persistent but requires the plugin to be loaded as important (can i enforce this?)!

  2. the plugin also rejects htlcs if they get close to expiry to prevent force closes, but looking at the values i get from the hook and looking at this i got a little confused and chose 6 blocks before "htlc_hook_payload".htlc.cltv_expiry see here

What this pr is missing:

  • all the integration stuff with automated tests on github, building with the Makefiles?, copying files after building and making it the same as cln-grpc so users only have to specify the grpc-hold-port

Performance:

I tested this plugin on regtest and an i7-6700k and held ~500 htlcs on 80 invoices with top showing 3-7% cpu for the plugin and a little less ram than lightningd

Finally:

This plugin is running (in a slightly older version) on testnet in combination with Robosats.

Thank you @cdecker for helping me make this plugin possible. He told me to make it a separate plugin but ideally i would like to see it merged into cln-grpc.

If you have any questions/feedback/improvement proposals please feel free to answer here :)

@vincenzopalazzo
Copy link
Collaborator

Would this plugin stay in a separate repository and use tools like coffee to manage it in the system that it is planning to use this plugin?

I see some pattern in cln, and it is the correct thing to do imho, to implement this to the core level (also with a rust plugin) and expose low-level RPC call. Then use the GRPC/Rest/Command to expose these commands through another protocol. So maybe it something missing in this PR to be included at the core level?

@daywalker90
Copy link
Contributor Author

Would this plugin stay in a separate repository and use tools like coffee to manage it in the system that it is planning to use this plugin?

Imo it shouldn't be a separate plugin at all, but i'm following you guys lead on how hold-invoices are made available in cln. If it has to be a separate plugin i'd like to see it installed automatically with cln and only if these options are out make it available in a separate repo.

I see some pattern in cln, and it is the correct thing to do imho, to implement this to the core level (also with a rust plugin) and expose low-level RPC call.

What do you mean by this? To make hold-invoice methods available like regular invoice is, directly from cln? I wouldn't know how to do this.

Then use the GRPC/Rest/Command to expose these commands through another protocol. So maybe it something missing in this PR to be included at the core level?

Do you mean i should put this code into cln-rpc and others use those methods?

@vincenzopalazzo
Copy link
Collaborator

If I understand correctly, your plugin currently works only with gRPC, but theoretically, it could also function as a regular plugin without gRPC.

So, why not develop the plugin in Rust and then expose the API via gRPC?

@daywalker90
Copy link
Contributor Author

It can definitely work as a regular plugin.

So, why not develop the plugin in Rust and then expose the API via gRPC?

Can you clarify this a bit more? Do you want me to edit cln-rpc and cln-grpc for this or make a "cln-rpc-hold" and a "cln-grpc-hold" that exposes the API from "cln-rpc-hold".

Currently this pr has just a "cln-grpc-hold" and instead of exposing something else it is all self-contained.

@daywalker90
Copy link
Contributor Author

daywalker90 commented Jul 25, 2023

cleaned up some clippy complains + rebase

@niftynei
Copy link
Collaborator

Wow this is great and really needed. Thanks for getting this started.

In terms of fitting into existing infrastructure, I think we can cut down on what this PR is doing by a lot.

Ideally this PR would just:

  • add a new plugin that implements the following methods
    • hodlinvoice: creates an invoice, adds the payment_hash to the list of invoices to hodl
    • hodlinvoicesettle: allows htlcs to this payment_hash to settle
    • hodlinvoicecancel: fails back all htlcs for this payment_hash
    • hodlinvoicelookup: reports back information about a payment-hash/invoice that's being held

Once this plugin is tested + added to CLN, we can then work with @cdecker on how to expose these methods via the existing grpc transport code. (There's another todo currently for other built-in plugins)

In summary, the biggest TODO here is to remove all the independent grpc server code and trim this down significantly to just add a single plugin that runs on startup and offers all the new RPC methods you need to make this work.

That'll make this PR much easier to review + test; and allows us to re-use existing grpc code to expose it in a follow on PR.

@daywalker90
Copy link
Contributor Author

ok @niftynei , i've done it here: #6611

@daywalker90
Copy link
Contributor Author

i'll close this in favor of #6611

@daywalker90 daywalker90 deleted the cln-grpc-hodl-clean branch July 31, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants