Skip to content

Add CLN support #713

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

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Conversation

macgyver13
Copy link
Contributor

@macgyver13 macgyver13 commented Apr 23, 2025

Replaces #685 (Continues work started by Dan, but only focuses on CLN support at this time, he faced stability issues with eclair, will revisit that integration in the future)

This PR adds CLN pod support to warnet and SimLN plugin. Noteable changes:

  1. commander/templates/rbac.yaml now requires exec rule
  2. SimLN plugin copies certs/key from source cln pod to simln pod
  3. ln_framework/ln.py::run_command has to determine if being executed inside or outside cluster to make connection to cln node correctly via kubernetes.stream
  4. added sleep(0.25) before most thread.start() to greatly increase reliability of ln_init.py
  5. deploy.py now searches node-defaults.yaml to determine if ln_init is required
  6. lnd nodes need to be indexed in the same block before cln nodes, otherwise the expected channel id will not be correct

There is a new test ln_mixed_test.py that is partially implemented until the following decision is determined - how should LNNode access LND nodes from testing instance outside of cluster?:

  • port forward? - seems like a lot of overhead / clunky
  • use rpc instead of REST? - duplicates warnet/ln.py
  • use kubectl to issue curl commands? - messy / clunky
  • use simln plugin which will act inside cluster avoiding all the issues? - a drawback for simln is the activity is sendkey based instead of invoice / pay

danvergara and others added 16 commits March 23, 2025 21:07
issues:
- restoring wallet produces same spend address on two nodes
- lnd node connecting to cln produces infinite reconnect issue - system resource issue
- two cln nodes can not find each other - gossip error
Tune sleep / pauses down
Set simln container restartPolicy: Never
Remove all unnecessary thread sleeps
* main:
  allow scenarios to run locally without a cluster (for --help)
  control: add "please install helm"
  add metricsExport:true
  Update apidocs on
  replace master build with 28.1
  add example to build image docs
  TypeError: build() missing 1 required positional argument: 'registry'
  handle missing key in base config
  lint: action all ruff fixes
  ci: bump actions, and ruff version. Use action
  remove unused docker_registry param
  build: update image build for cmake
  check for None, replace with []
  Update install.md with a link to the scaling tips
  remove added print
  remove unused import
  reverse graph_test change did not improve stability make local_test_runner executable
  Resolve ruff formatting issues
  - Add local_test_runner script - add delay to graph_test expect - increases reliability

# Conflicts:
#	resources/scenarios/commander.py
#	resources/scenarios/ln_init.py
migrate references from -ln to -lnd
remove eclair references for now
support ln: type: True in node-defaults.yaml
update cln test data
reorder channel index for stability
LNNode shares commander logger
Update documentation to address CLN support
Make ln_framework/ln.py work in commander or local device
Add CLN support to warnet/ln.py
Add short sleep between thread starts to improve stability
@bdp-DrahtBot
Copy link
Collaborator

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #686 (cli: add 'port_forward' subcommand for ln by f3r10)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

Starting to review this, will add more comments and address your questions once ive got my head around it

Copy link
Contributor

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

really really REALLY great work, thank you. I reviewed all the changes and left comments throughout.

Next I'm going to try answer your open questions in the PR description, and then try to figure out whats up with the test failures.

Comment on lines +56 to +57
if "cln" in pod.metadata.labels["app.kubernetes.io/name"]:
pubkey_key = "id"
Copy link
Contributor

Choose a reason for hiding this comment

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

this kind of thing is ok for now and probably will be ok for eclair as well -- but its a tad codesmelly. feels like implementation-specific methods should stay in their classes, even as jsut static functions

Comment on lines +5 to +6
ln:
cln: true
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondered what would happen in the edge case

ln:
  cln: true
  lnd: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bet helm would add both charts 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tested this, lnd started when both were defined

@pinheadmz
Copy link
Contributor

Now that i've read through the entire patch i think i might have opinions about these questions, lets see...


commander/templates/rbac.yaml now requires exec rule

I think you only needed this to use lightning-cli inside cln containers, and im hoping we dont need to do that, but this isn't a bad change necessarily


SimLN plugin copies certs/key from source cln pod to simln pod

yes I think this is the only way to do it for now


ln_framework/ln.py::run_command has to determine if being executed inside or outside cluster to make connection to cln node correctly via kubernetes.stream

everything inside scenarios/ should only ever be executed inside the cluster. The only exception is if a user needs help: warnet run ... -- --help which requires the scenario file to be run locally just to gather the argument parser comments.


added sleep(0.25) before most thread.start() to greatly increase reliability of ln_init.py

Not sure why you needed this? What happens otherwise?


deploy.py now searches node-defaults.yaml to determine if ln_init is required

yes thank you yes!


lnd nodes need to be indexed in the same block before cln nodes, otherwise the expected channel id will not be correct

Hm I didn't notice this. Did you see how we use decreasing transaction fees in the channel opens to ensure they are in the expected order in a block? Hopefully when you open a channel from cln you can also set the fee rate, and the process will continue to be implementation-agnostic


There is a new test ln_mixed_test.py that is partially implemented until the following decision is determined - how should LNNode access LND nodes from testing instance outside of cluster?:

port forward? - seems like a lot of overhead / clunky
use rpc instead of REST? - duplicates warnet/ln.py
use kubectl to issue curl commands? - messy / clunky
use simln plugin which will act inside cluster avoiding all the issues? - a drawback for simln is the activity is sendkey based instead of invoice / pay

I don't think this should ever happen - was it happening before? Scenarios are executed entirely inside the cluster.

reduce ln_init changes
start migration of CLN to REST
other clean up for comments
remove rbac exec change
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.

4 participants