Add support of ldk-node#307
Conversation
Feel free to bump this in a preparatory commit in this PR! |
83e2d28 to
526eb15
Compare
|
Initially I was trying to wire up ldk-node, which is not a daemon and I thought sim-ln could orchestrate ldk-node building, initialization. However it turned out that we want already running and funded nodes with opened channels, so this wasn't an option. Added support of https://github.com/lightningdevkit/ldk-server, which is a reference daemon implementation with external API, it uses ldk-node internally. Please note, that ldk-server has not yet been released, therefore a git reference and a revision is used in Cargo.toml The configuration for ldk-server looks in the following way: Please note that currently it's not possible to guess the network via RPC calls, that's why we need to provide it in the config. This might change in future. |
Happy to go with server, I believe a release is coming soon ™️ ! |
|
Promise to get to review this week - sorry I've been a bit slow but it's on my list! |
chuksys
left a comment
There was a problem hiding this comment.
Thanks for working on this! I've done a quick initial code/design review. It looks good so far, but I still need to run some local tests. I'll do that ASAP and get back to you with final feedback.
8337320 to
0c69012
Compare
0c69012 to
590d5ae
Compare
chuksys
left a comment
There was a problem hiding this comment.
If CI is happy, this LGTM.
|
Should I ping someone, for example @carlaKC, to merge? |
carlaKC
left a comment
There was a problem hiding this comment.
So sorry that this took so long!
| Ok(NodeInfo { | ||
| pubkey: *node_id, | ||
| alias, | ||
| features: NodeFeatures::empty(), |
There was a problem hiding this comment.
Do we need to set keysend here?
I think that running this with random activity would fail - did you test this out?
There was a problem hiding this comment.
I think this should follow the same pattern as the LND and CLN implementations: query the backend for the requested node and populate NodeInfo.features from the returned feature set.
Returning NodeFeatures::empty() here means sim-ln will treat the node as not supporting keysend, even if it does. Now that ldk-server exposes feature bits through GetNodeInfo, we should use that API and construct NodeInfo from the actual returned features instead of manual default feature support.
There was a problem hiding this comment.
Unfortunately ldk-server does not show feature bits for other nodes, it only shows them for itself. In fact there are two methods get-node-info about itself and graph-get-node which returns information about other nodes, its example response is the following:
{
"node": {
"channels": [
3173352185970884608,
3173346688412745728,
3173354384994140160,
3336995800067670017
],
"announcement_info": {
"last_update": 1782471184,
"alias": "lnd-signet-node",
"rgb": "ff0000",
"addresses": []
}
}
}
I've tested random activity. it works fine. Also when we call get_node_info it checks whether we ask about this node, or another one. For this node we provide correct NodeInfo that was populated in new().
There was a problem hiding this comment.
Unfortunately ldk-server does not show feature bits for other nodes, it only shows them for itself.
This is surprising. I've open a PR lightningdevkit/ldk-server#237 to address this. If merged, we should get access to feature bits from other nodes like shown below.
{
"node": {
"channels": [
468391953498113
],
"announcement_info": {
"last_update": 1782481522,
"alias": "ldk_server_2",
"rgb": "000000",
"addresses": [
"54.3.7.81:9745"
],
"features": {
"0": {
"name": "DataLossProtect",
"is_required": true
},
...
}
}
}
}There was a problem hiding this comment.
Not putting keysend in other nodes features will mean that the simulation will fail if we are sending payments to a node that we don't have control over.
Eg for network A - B:
nodes: [A]
activity: [A -> B]
This is kinda niche-ey, but it is something people wanted to regularly shoot payments out to strangeres on signet, so I'd like to preserve it. Happy to hard-code if they're not surfaced by LDK-server, but if it's newly added let's use their API.
I've tested random activity. it works fine.
(my bad, thought this hit is this case)
There was a problem hiding this comment.
Fixed. Previously I tested random activity with all owned nodes, so I think your original comment is valid.
| // ldk-server doesn't expose feature bits, but it always supports keysend | ||
| // via `spontaneous_send`, so advertise it so sim-ln's keysend checks pass. | ||
| let mut features = NodeFeatures::empty(); | ||
| features.set_keysend_optional(); |
There was a problem hiding this comment.
Feature bit exposure is now supported by ldk-server following lightningdevkit/ldk-server#231, so we should be able to query the node’s actual features instead of creating an empty feature set and manually setting keysend.
Manually setting it assumes the node advertises keysend support. That matches default LDK behavior today, but it can be wrong for a fork that changes feature support/requirements, in which case sim-ln would incorrectly treat the node as keysend-capable.
There was a problem hiding this comment.
Fixed, now we return real feature list.
| Ok(NodeInfo { | ||
| pubkey: *node_id, | ||
| alias, | ||
| features: NodeFeatures::empty(), |
There was a problem hiding this comment.
I think this should follow the same pattern as the LND and CLN implementations: query the backend for the requested node and populate NodeInfo.features from the returned feature set.
Returning NodeFeatures::empty() here means sim-ln will treat the node as not supporting keysend, even if it does. Now that ldk-server exposes feature bits through GetNodeInfo, we should use that API and construct NodeInfo from the actual returned features instead of manual default feature support.
carlaKC
left a comment
There was a problem hiding this comment.
Typically would want to fixup and squash things like clippy fixes and node feature setting, but I left this hanging unacceptably long so happy to go in as-is. Will merge once CI is clean - thanks for the patience here and the cool new feature!
|
I'll squash commits. |
Ready and waiting with the merge button 🫡 |
Add `LdkServerNode` as a new `LightningNode` implementation ldk-server-client over gRPC-over-HTTPS. Supports keysend via `spontaneous_send`, payment tracking via polling `get_payment_details`, graph queries, and channel capacity reporting. Wire up `NodeConnection::LdkServer` in sim-cli parsing so ldk-server nodes can be specified in sim.json alongside LND, CLN, and Eclair nodes.
8db2568 to
69afaab
Compare

This PR adds support of ldk-node to sim-ln.
Closes: #26
As ldk-node is not a daemon, before we start we parse the simulation config file and extract all ldk-nodes, build them and start. For that an additional chain source (bitcoind/esplora/electrum) is needed to start.
Before this PR can be merged, an update of used lighting libraries should be merged, as the current ldk-node has newer versions.