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.

Sign up for GitHub

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

Jump to bottom

Run gateway as Core-Lightning plugin #174

Merged
merged 1 commit into from
Jul 11, 2022
Merged

Run gateway as Core-Lightning plugin #174

merged 1 commit into from
Jul 11, 2022

Conversation

justinmoon
Copy link
Contributor

@justinmoon justinmoon commented Jun 24, 2022 •
edited
Loading

With this PR user can generate a lightning invoice and get paid in via lightning gateway.

Still need to:

  • Figure out all the parameters for route hints. I think we can do that in future PRs.
  • We should update cln-plugin to allow plugin CLI arguments (minimint-cfg in our case) that don't have default values Plugin config options with no defaults ElementsProject/lightning#5369.
  • This PR basically makes running the gateway mandatory. Previously you didn't have to run it because we always generated a fake node pubkey in client.json and it was never used so that didn't matter. So I just made it required for this PR because all the data structures assume the gateway is always there. Make lightning gateway optional #200 makes it actually optional but that's a deeper change so I separated it.

@justinmoon justinmoon force-pushed the cln-plugin branch 8 times, most recently from dc51809 to 591353a Compare July 1, 2022 01:48
@justinmoon justinmoon mentioned this pull request Jul 1, 2022
Closed
@justinmoon justinmoon marked this pull request as ready for review July 1, 2022 02:09
justinmoon
justinmoon commented Jul 1, 2022
View reviewed changes
ln-gateway/src/lib.rs Outdated Show resolved Hide resolved
jkitman
jkitman previously requested changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@jkitman jkitman left a comment

Choose a reason for hiding this comment

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

Regarding the integration tests, does this change any behavior that would need testing?

If not, perhaps you can just modify the integrationtests/README.md so that users can run the LN plugin locally when testing with the real fixtures.

Additionally scripts/latencytest.sh probably needs a small update.

@justinmoon
Copy link
Contributor Author

Regarding the integration tests, does this change any behavior that would need testing?

This doesn't change any behavior W.R.T. existing integration tests likereceive_lightning_payment_valid_preimage and receive_lightning_payment_invalid_preimage. The current tests do a lot of work calling internal functions in the right order and everything. It would be nice if a new integration test just (1) generated invoice, (2) had "ln2" pay it, and (3) asserted that balances eventually updated as expected. A true "integration test".

If not, perhaps you can just modify the integrationtests/README.md so that users can run the LN plugin locally when testing with the real fixtures.

One tricky thing here is that we'd need to pass the --minimint-cfg argument to the plugin which points to the directory with all the JSON configs and sled db files. AFAIK the integration tests just do everything in-memory?

Another tricky thing is that the plugin would really need to be reloadable per-test, because the federation details might change every test.

I think the best option here would be to add a --minimint-testing flag to the plugin which causes the plugin to run without running a gateway. Then add a new RPC command to the plugin to run a gateway based on JSON federation config pass as argument to this RPC command. This way the plugin could start before the integration tests have even started. Each test that needs to exercise the plugin can send over the federation configs used by that test.

This could happen in a follow-on PR.

Additionally scripts/latencytest.sh probably needs a small update.

I didn't even know about latencytest.sh! Will do.

@jkitman
Copy link
Contributor

jkitman commented Jul 1, 2022

This doesn't change any behavior W.R.T. existing integration tests likereceive_lightning_payment_valid_preimage and receive_lightning_payment_invalid_preimage. The current tests do a lot of work calling internal functions in the right order and everything. It would be nice if a new integration test just (1) generated invoice, (2) had "ln2" pay it, and (3) asserted that balances eventually updated as expected. A true "integration test".

Perhaps it's sufficient that is handled via the shell scripts, although I guess we need to expose a new CLI command for that.

One tricky thing here is that we'd need to pass the --minimint-cfg argument to the plugin which points to the directory with all the JSON configs and sled db files. AFAIK the integration tests just do everything in-memory?

Ah, yes the current tests only use an in-memory DB, although I was meaning to enable a real DB for when running with the real fixtures.

Another tricky thing is that the plugin would really need to be reloadable per-test, because the federation details might change every test.
I think the best option here would be to add a --minimint-testing flag to the plugin which causes the plugin to run without running a gateway. Then add a new RPC command to the plugin to run a gateway based on JSON federation config pass as argument to this RPC command. This way the plugin could start before the integration tests have even started. Each test that needs to exercise the plugin can send over the federation configs used by that test.

Agreed, tearing down the LN fixtures and funding a channel would make the tests take forever, much faster just to reload the configs.

@justinmoon justinmoon force-pushed the cln-plugin branch 4 times, most recently from 4761790 to 2aaf4c5 Compare July 3, 2022 19:04
@justinmoon justinmoon force-pushed the cln-plugin branch 8 times, most recently from aff61c9 to 65d37ef Compare July 11, 2022 10:54
@justinmoon
Run gateway as Core-Lightning plugin
92f832d 
- mint-clint-cli gets command to generate invoices
- Gateway plugin automatically generates gateway.json if it isn't
there. Removed gw_configgen bin script.
- Add route hint to invoices we generate so HTLCs reach gateway
- Gateway uses channels to manage concurrency between webserver,
plugin and background fetching
- Had to add more features to tracing dependency for the gateway
crate because Core Lightning communicates to plugins via stdin/
stdout. Logging is routed via JSON-RPC. These feature flags helped
make that work.
- Move cln-rpc to git dependency
@elsirion elsirion dismissed jkitman’s stale review July 11, 2022 12:07

Just went throgh everyting with Justin, will fix some minor annoyances in follow up PRs

@elsirion elsirion merged commit 563c600 into fedimint:master Jul 11, 2022
@justinmoon justinmoon mentioned this pull request Jul 12, 2022
Closed
@justinmoon justinmoon deleted the cln-plugin branch July 31, 2022 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elsirion elsirion elsirion approved these changes

@jkitman jkitman jkitman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justinmoon @jkitman @elsirion