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

feat: add ldk gateway #5554

Merged
merged 1 commit into from
Jul 23, 2024
Merged

feat: add ldk gateway #5554

merged 1 commit into from
Jul 23, 2024

Conversation

tvolk131
Copy link
Member

@tvolk131 tvolk131 commented Jul 1, 2024

Add a third lightning node type to the gateway, a lightning node powered by ldk-node which runs in the same process as the gateway. The gateway can be run in ldk mode by setting the following environment variables:

FM_LDK_ESPLORA_SERVER_URL: Esplora server where blockchain data is accessed
FM_LDK_NETWORK: The bitcoin network being used (e.g. mainnet/testnet)
FM_PORT_LDK: The port that the lightning server is running on, where other lightning nodes can access it

The node's data is stored in a subdirectory of the gateway's data folder

This gateway mode should still be considered experimental!!! None of the behavior is tested or has locked-in backwards compatibility yet!

@tvolk131 tvolk131 marked this pull request as ready for review July 1, 2024 21:09
@tvolk131 tvolk131 requested review from a team as code owners July 1, 2024 21:09
/// The HTLC stream, until it is taken by calling
/// `ILnRpcClient::route_htlcs`.
htlc_stream_receiver_or:
Option<tokio::sync::mpsc::Receiver<Result<InterceptHtlcRequest, Status>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems really awkward to me. Does GatewayLdkClient need to own this? Can it be passed in as an argument to a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

The gateway lightning client can be in two states - either it's inactive (not intercepting HTLCs) or active (intercepting HTLCs), and calling ILnRpcClient::route_htlcs spins up the HTLC interception, switching from the former state to the latter. Currently these two states are internally represented in the following way:

Inactive: GatewayLdkClient.htlc_stream_seeder_task_handle is already running and passing incoming HTLCs to the HTLC stream receiver. The HTLC stream receiver is held in GatewayLdkClient.htlc_stream_receiver_or until route_htlcs is called, at which point this receiver is taken and returned.

Active: GatewayLdkClient.htlc_stream_seeder_task_handle is still running. But the HTLC stream receiver has been passed to the caller of route_htlcs and so GatewayLdkClient.htlc_stream_receiver_or is None.

What if we changed GatewayLdkClient.htlc_stream_seeder_task_handle to be an Option<tokio::task::JoinHandle<()>>, removed GatewayLdkClient.htlc_stream_receiver_or altogether, and represented the states like this:

Inactive: GatewayLdkClient.htlc_stream_seeder_task_handle_or is None and ldk-node events are not being handled yet. The HTLC stream receiver doesn't exist yet.

Active: The HTLC stream sender and receiver are created when route_htlcs is called. The sender is passed into the htlc stream seeder task, which is started up and assigned to GatewayLdkClient.htlc_stream_seeder_task_handle_or. The receiver is returned by route_htlcs and never held by GatewayLdkClient.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's chat about this offline


// TODO: Find a way to avoid looping/polling to find out when a payment is
// completed. `ldk-node` provides `PaymentSuccessful` and `PaymentFailed`
// events, but routing them to specific payment IDs isn't straightforward.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "routing them" mean here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, that was a strange wording. What I mean is we should only be handling events from ldk-node's event queue in one place to avoid having two threads stomping/skipping over events. Right now, event handling is done in GatewayLdkClient::event_handler_task_handle, so we'd need to add functionality there to handle PaymentSuccessful and PaymentFailed events and somehow be able to tell GatewayLdkClient::pay when a payment has succeeded or failed. This can definitely be done, but we'd need some sort of per-paymenthash queue or something.

I cleaned up the comment here

{
description
} else {
""
Copy link
Contributor

Choose a reason for hiding this comment

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

does ldk node support setting the description hash in receive_for_hash?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. Drilling down into the function calls here, we end up at this:

https://github.com/lightningdevkit/rust-lightning/blob/f48a273211d5235e1ff335b2c25f8aacf3642f24/lightning-invoice/src/utils.rs#L431-L453

Where _create_invoice_from_channelmanager_and_duration_since_epoch() accepts a Bolt11InvoiceDescription‎ (which can be a direct description or a hash) but is hardcoded to always be a direct description.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably be good to make an issue upstream

Copy link
Member Author

Choose a reason for hiding this comment

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

@tvolk131 tvolk131 force-pushed the ldk_node_gateway branch 2 times, most recently from 813d002 to 01d4751 Compare July 9, 2024 14:36
@tvolk131 tvolk131 requested a review from m1sterc001guy July 9, 2024 14:44
@tvolk131 tvolk131 force-pushed the ldk_node_gateway branch 2 times, most recently from b081aa3 to 2eadef4 Compare July 12, 2024 22:12
Copy link
Contributor

@dpc dpc left a comment

Choose a reason for hiding this comment

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

LGTM, but someone from @fedimint/lightning should approve.

@tvolk131 tvolk131 force-pushed the ldk_node_gateway branch 2 times, most recently from f152040 to c37d607 Compare July 14, 2024 23:28
// TODO: Find a way to avoid looping/polling to know when a payment is
// completed. `ldk-node` provides `PaymentSuccessful` and `PaymentFailed`
// events, but interacting with the node event queue here isn't
// straightforward.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to implement this, you could potentially use a channel to communicate from the event queue thread to here. Can be done in a follow up

}),
}?;

Ok((route_htlc_stream, Arc::new(*self)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This will move self rather than clone it, which is what we do in the other implementations. As far as I can tell this won't cause any issues (whereas with LND we need to clone it). I assume this was intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! This was intentional. The reason that ILnRpcClient::route_htlcs consumes self and returns Arc<Self> (IIUC) is so that the type checker can enforce that it cannot be called twice. That should still be enforced here.

@Kodylow
Copy link
Member

Kodylow commented Jul 16, 2024

Blockers on this, waiting until after 0.4 to merge since we can't get this in this week and need to cut release with the DNS fix. This will be the next big merge.

  • Liquidity API and other lightning functionality moved into gateway-cli and gateway api: The only way to interface with the underlying ldk-node is the gateway cli. Normally we use the node's api. For this, we need the gateway-cli to expose api functionality we'd normally just have the node use (open channel, close channel, sweep funds, etc) and add that into the gateway-cli. Instead of only doing this for the ldk node, we're going to implement this generically for all the supported node implementations across the gateway api, which means we can also do lightning side management from the gateway ui in https://github.com/fedimint/ui

    • We already support the liquidity ones for all node impls, we need to add the channel management ones in
  • LNv2 has multi-mint support, but we're dropping that as part of the bolt12 refactor: 0.4 will still use LNv1, but ship with LNv2 binaries that optionally run with an env variable, then for 0.5 both v1 and v2 will run side by side, then 0.6 we retire lnv1.

LNv2 requires an implementation of pay_private for multi-mint payments which requires upstream changes to ldk-node (ldk-node doesn't let us do partial payments of an invoice).

  • @joschisan : the way to go here is not to require multi-mint payments generally. We have pruned invoices which allow us to do partial payments, which are already supported by LND and CLN, so that made multi-mint payments easy. We intend to drop the pruned-invoices struct because they're causing a bunch of problems elsewhere in the codebase as we prepare for bolt12 support. So we're going to remove that multi-mint payment as part of LNv2 for now, which removes this blocker.

  • LNv2 testing: waiting on ldk to be added to this list of gateway tests): https://github.com/fedimint/fedimint/pull/5651/files

Comment on lines +251 to +256
// TODO: Respect `max_delay` and `max_fee` parameters.
async fn pay(
&self,
invoice: Bolt11Invoice,
_max_delay: u64,
_max_fee: Amount,
Copy link
Contributor

Choose a reason for hiding this comment

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

Respecting this parameters is security critical!

@tvolk131 tvolk131 force-pushed the ldk_node_gateway branch 3 times, most recently from d521327 to 80250f8 Compare July 23, 2024 13:52
@joschisan joschisan added this pull request to the merge queue Jul 23, 2024
Merged via the queue into fedimint:master with commit a0e29e8 Jul 23, 2024
23 checks passed
@tvolk131 tvolk131 deleted the ldk_node_gateway branch July 23, 2024 19:47
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.

5 participants