Adds curated clightning plugins

[GambolingPangolin]

This change adds an option to the clightning module to configure plugins and makes most of the plugins in the lightningd/plugins repository available in the attribute set pkgs.nix-bitcoin.clightning-plugins (see example/configuration.nix).

To configure clightning to use a new plugin with nix derivation somePlugin, the admin would simply add (somePlugin + "/bin/plugin") or whatever to the list of enabled plugins.

Instead of relying on the curated Python package set, I used mach-nix to build a closure from each plugin's requirements.txt. (This might be useful elsewhere in nix-bitcoin; I haven't checked.)

I tested that all of the plugins build successfully and that a subset of them run correctly with clighting using the intended configuration syntax forms.

This could be considered to close #71.

By the way this is a really fantastic project and I learned a lot about nix by poking around!

[jonasnick]

By the way this is a really fantastic project and I learned a lot about nix by poking around!

Hi, me too. Thank you for the PR! The plugins look quite interesting, looking forward to trying them out

How do you build a plugin with nix build? clightning-plugins is an attrset with paths that don't exist on my system instead of a derivation.

There are a few things we should add before merging this:

• Add clightning-plugins build test to travis (similar to the other packages)
• Add functional test by enabling the plugins in tests.nix and checking whether lightning-cli plugins list shows the plugins
• Usually we also have scripts that print the hash of the latest version of a package and verify signature. They're called get-sha256.sh in other packages. This would be less useful here, because I don't see any signatures that we could verify. But it could help figuring out what the latest version is without using a browser.

By the way mach-nix looks interesting - nix-based alternative to poetry? yes please. Sent them donation sats (with my nix-bitcoin clightning node).

[GambolingPangolin]

How do you build a plugin with nix build? clightning-plugins is an attrset with paths that don't exist on my system instead of a derivation.

The way that I have currently abstracted things, there is nothing you can pass into nix-build. However, constructing these paths involves casting a derivation to a string, so if you include the plugin path in a script it will build the plugin. For example passing

pkgs.writeScript "run-plugin" "${pkgs.nix-bitcoin.clightning-plugin.drain}"

to nix-build will give you a ./result which runs the plugin (assuming that pkgs has the nix-bitcoin attributes). I chose to structure things this way because each plugin script has a different name and I thought it would be more user friendly to expose the path to the executable rather than the derivation itself. If we bound clightning-plugins.plugin to the derivation then either users would have to write things like clightning-plugins.donations + "/donations.py" or we would have to export the complete paths somewhere else. What do you think of changing clightning-plugins/default.nix to export the derivations, and then attaching these plugins to the configuration in modules/clightning.nix:

# modules/clightning.nix
{ configuration.nix-bitcoin.clightning.plugins = { 
    autopilot = pkgs.nix-bitcoin.clightning-plugins.autopilot + "/autopilot.py"; 
    # etc
  }; 
}

# configuration.nix
{ services.clightning.plugins = with configuration.nix-bitcoin.clightning.plugins; [ autopilot donation ]; etc
}

Another possibility is we could configure clightning to run in an environment where all of the plugins are on the path. That seems like it would be hard to do in such a way that users can easily roll out plugins that are not part of nix-bitcoin.

Are the plugins configured with clightning extraConfig? Perhaps would be helpful to mention this here.

No. I should spell this out a bit more. I added a configuration option plugins which is a list of paths to the plugins. Users can add plugin=path or plugin-dir=path lines to extraConfig. However, in nix I believe it is common to have derivations in scope in a variable and use this for configuration. Following that line of thinking, to configure via extraConfig you would add a line like

{ services.clightning.extraConfig = "plugin=${myPlugin}"; } # or "plugin=${myPluginDrv + "/myPlugin.py"}"

The plugins key is just syntactic sugar for this.

If there's no simpler way for the user to figure out which plugins are available, we should mention which plugins are available.

I will add the list of plugins to the readme.

Your testing suggestions seem reasonable. I'll work on adding that missing stuff.

[GambolingPangolin]

I just realized that by using the plugin-dir configuration key instead of plugin in the clightning config, we could get the same user-friendly API and a more standard integration of the plugin definitions with the rest of the code. I'll put that together and see if it works out.

[GambolingPangolin]

I tried to run the test suite myself using test/run-tests.sh but several derivations failed to build for me: elementsd, joinmarket (problem with coincurve), and lightning-charge. I gave up on making the test work for tonight. However I did prune the set of plugins down to the ones which activate properly on my own testing node.

[GambolingPangolin]

What formatting tool does the project use for python scripts?

[jonasnick]

I chose to structure things this way because each plugin script has a different name and I thought it would be more user friendly to expose the path to the executable rather than the derivation

I didn't see anything wrong with your approach. Seemed to work. Nice that you found the plugin-dir option. nix-build is straightforward now.

No. I should spell this out a bit more.

Sorry, I wasn't referring to enabling plugins but to configuring plugins themselves. For example autopilot adds new command line options. I'm wondering whether these command line options can just as well be added to the clightning config.

I tried to run the test suite myself using test/run-tests.sh but several derivations failed to build for me: elementsd, joinmarket (problem with coincurve), and lightning-charge

That shouldn't happen. What error do you get?

What formatting tool does the project use for python scripts?

Not exaclty sure. We inherit it as part of the NixOS test framewor.

[nixbitcoin]

This should either

a) yield two sha256's that can be used with fetchFromGitHub for both mach-nix and plugins

OR

b) switch mach-nix and plugins to fetchurl in default.nix so they can use the sha256 hashes generated by this get-sha256.sh.

[nixbitcoin]

I tested the basic functionality by enabling and functionally testing both summary and monitor. Both work. I like the plugin-dir method. I also checked the sha256's with nix-prefetch-git.

For example autopilot adds new command line options. I'm wondering whether these command line options can just as well be added to the clightning config.

I agree, all plugins with relevant config options should have corresponding nix-bitcoin options with sane default values. When the plugin is enabled by the user, it should automatically insert these config options into the clightning config.

I tried to run the test suite myself using test/run-tests.sh but several derivations failed to build for me: elementsd, joinmarket (problem with coincurve), and lightning-charge. I gave up on making the test work for tonight. However I did prune the set of plugins down to the ones which activate properly on my own testing node.

The test you added ran for me. I like the test mechanism.

I don't think all the TODO's need to be implemented before we can merge. The currently integrated plugins already provide a great amount of additional functionality to the user. We should document the available plugins for the user and how to enable them in docs/usage.md.

[GambolingPangolin]

I just pushed a new version taking your comments into consideration.

• Each plugin gets its own module so that we can define and set options according to nix idioms (for example)
• I fixed clightning-plugins/get-sha256.sh so that it satisfies option (a) in the comment above.
• Added some documentation to usage.md

My problem running the test suite comes down to a permission problem with my workstation installation. I haven't tried to work it out yet but I can run the tests on a separate nixos system.

I added some code for autopilot but did not add it to usage.md because it fails without producing log output. I would prefer to triage that. I can add an issue for it if you agree.

[erikarvstedt]

Great contribution, welcome to nix-bitcoin!

mach-nix has serious downsides which, as far as I'm concerned, preclude its use in nix-bitcoin. (But I can offer a nice alternative at the end.)

• Downloading and unpacking the mach-nix sources adds > 30 s of evaluation overhead on my desktop system.
  Because gcroots are not created for evaluation sources, this overhead is incurred after each nix garbage
  collection run.
• pypi-deps-db, the main package source, has an uncompressed size of 567 MB and is extracted to a store path. Due to limitations of nix this can exhaust available memory on low-end nodes.
• Extremely slow evaluation:
  Eval time of the default test without plugins: 3.7 s. With plugins: 11 s.
  (On my desktop system, with warm caches.)
  This overhead of ~7 s is almost exclusively due to mach-nix.
  Build times are also notably slow due to dependency resolution, but this is less relevant.
• Auditing the contents of mach-nix is infeasible to achieve in a practical way.
  Also, there's no systematic review from a wider audience of open source contributors.

Luckily, most plugin dependencies are readily available in nixpkgs and our own joinmarket Python packages (thanks @nixbitcoin).
In this branch, I've bundled all our Python pkgs into a common pythonPackages attrset, added a few extra python pkgs (mostly clightning-related, reusing pkgs.clightning.src) and implemented the clightning plugins on top of it.
With this approach, it was also easy to make the noise plugin work.

Downside: The Python pkg versions don't always exactly match the plugin requirements, but the test suite runs fine. Could you do some manual checks?

I've also added these minor changes to clightning-plugins/default.nix:

• Use short prefix instead of full SHA1 rev as the plugin derivation version, to avoid overlong store paths.
• Don't copy the whole plugin src dir when building a plugin. Also, simplify the build cmd.
• Use buildInputs instead of propagatedBuildInputs, which is the correct variant in this case.

This PR needs some more fixes, but let's tackle the mach-nix issue first.

More questions

Why is the autopilot plugin not tested?
What exactly is the "conflict between sauron and bcli plugins"?

[erikarvstedt]

@GambolingPangolin The nix-bitcoin tests should work out-of-the-box on all x86 Linux platforms where nix is supported. It would be great if you could share the output of the failing tests and a description of your system environment.

[erikarvstedt]

I've noticed that Travis sometimes fails with error

machine # [  112.914639] lightningd[1156]: 2020-11-10T22:35:27.718Z INFO    plugin-cl-zmq.py: Killing plugin: failed to respond to 'getmanifest' in time, terminating.

This only happens on slow CI nodes and is due to clightning's fixed 60 ms plugin response timeout. We could work around this by disabling some plugins in the test.
@jonasnick, could you ask the clightning folks to add a configurable timeout?
Edit: Sorry, I've misread that, the timeout is 60 seconds. I can't explain this CI-only failure then.

[jonasnick]

The nice thing about the mach-nix approach was that it didn't (seem to) require a lot of maintenance work, but I agree that it apparently has serious downsides. When we go forward with @erikarvstedt's suggestions we should only add plugins that people actually want to use. Many of the plugins are just proof of concepts, not actively maintained or very unlikely to be useful to anyone (imo). Not adding those would be more appropriate given the resources of the nix-bitcoin project.

The Python pkg versions don't always exactly match the plugin requirements, but the test suite runs fine

I'm worried about this because one can imagine how an exception can result in loss of funds (for example with the rebalance or jitrebalance plugins).

[GambolingPangolin]

@erikarvstedt I agree with your critique of mach-nix. My main reason for
using mach-nix was that it would automatically handle the dependencies,
including version constraints. The work you did reorganizing the Python package
definitions looks good to me.

I agree with @jonasnick that we should take care to satisfy the version
constraints. For a small set of Python packages it will be possible to do by
hand, but I don't see a simple way to automate the process. I don't know what
form the result of buildPythonPackage has but if it exposes the version in
some way we could add some assertions for the constraints in various
requirements.txt at least.

Why is the autopilot plugin not tested?

The autopilot plugin dies immediately on my system without producing meaningful
log messages. I should have removed the module, but I thought it might be
worthwhile to leave the code in place. Perhaps that is misguided and in any
case, we should either get it to work or remove autopilot from
modules/clightning-plugins/default.nix.

What exactly is the "conflict between sauron and bcli plugins"?

There are method namespace collisions including getchaininfo, getutxout,
etc. Presumably, this is intentional because they provide the same
functionality but with different trusted data sources. We could make sauron
available by using the disable-plugin config. flag to disable bcli.

The nix-bitcoin tests should work out-of-the-box on all x86 Linux platforms
where nix is supported. It would be great if you could share the output of the
failing tests and a description of your system environment.

I really appreciate any advice you might have! Here is the output:
https://gist.github.com/GambolingPangolin/db59d05aaea4c65ba52fbd078082ba4d . I'm
running nix 2.3.7 on Arch Linux (kernel 5.9.6).

@jonasnick I think that supporting only the plugins that people want to use is
the appropriate strategy. I am interested in prometheus and rebalance and
will dogfood those. The plugins helpme, monitor, and summary should be
pretty safe to ship as well.

[erikarvstedt]

@GambolingPangolin, let's continue fixing your build failure here.

I've rebased and updated my branch to ensure that plugin requirements are met.
Please continue with removing plugins as you see fit.

[nixbitcoin]

Why are we still calculating the sha256 for mach-nix?

[GambolingPangolin]

My bad! This is an oversight.

[nixbitcoin]

AFAICT backup plugin is no longer available

[GambolingPangolin]

This is also an oversight. We are no longer shipping the backup plugin anyway.

[nixbitcoin]

This should be 1lisx85vzsfzjhdc6zdz0l6bcrdgg6rp5xbc5jmx93mv8qqg2cns. For some reason it still deployed on my end with this but didn't copy over the new version of the scripts (noticed because I was still getting proxy errors with summary). Seems like the src doesn't update properly until you change the sha256.

[GambolingPangolin]

Dammit. I pasted from the wrong buffer! When I did a test deployment, it succeeded like in your case and I even saw new paths with the suffix 6cd4726 both in the deployment output and in the clightning config. This seems like a problem. Builds should always fail if the integrity check fails.

[erikarvstedt]

The store path of a fixed-output derivation is only determined by its name and the sha256 attr. The other attrs are only used when building the drv. To trigger a rebuild (and a new integrity check) you have to change the sha256 (or the name) to yield a store path that doesn't already exist in /nix/store.

[GambolingPangolin]

Ok that makes sense. In light of this behavior, how about we make use the first 7 digits of the sha256 for the version instead of the first digits of the rev?

[erikarvstedt]

If a version number is unavailable, version should consist of a maximally unambiguous and easily identifiable fact about the source. In nixpkgs, it's common practice to use the src's git rev. See:

cd $(nix eval '(<nixpkgs>)'); nix run nixpkgs.ripgrep -c rg "version = .*substring"

The src sha256 fails at the "easily identifiable" part and its value is already implicitly used to calculate the plugin drv hash (/nix/store/{HASH}-...).

[erikarvstedt]

Thanks, this looks great. Here's a last batch of fixups.

My three initial pypkgs-related commits are complex enough to be kept separate. So please don't squash them this time.

There's a nondeterministic error (log) in the zmq plugin, which happens on clightning shutdown. I've witnessed it only once in about 10 local runs of the default test. I'd suggest to ignore this for now.

[GambolingPangolin]

@erikarvstedt Your tweaks look amazing! Where did you learn all of the nix trix? The only thing that I noticed is that some of the subsection headers in usage.md are ## when they should be ### since ---- is the same level ##. install.md also seems to have non-hierarchical sections in the same way. I suppose addressing that belongs in a separate PR.

Do you have any opinion on how to organize the new commits? I'm planning on grouping them into 2 or 3 logical commits.

[erikarvstedt]

Where did you learn all of the nix trix

During the last two years I've made nix my main platform for desktop and server computing. So it's just daily practice and lots of reading in the nixpkgs sources.

I suppose addressing that belongs in a separate PR.

Yes, a separate PR would be great.

Do you have any opinion on how to organize the new commits?

Just

(new to old)
add curated clightning plugins
make-test.nix: use writeText
   Additional commit message:
   Needed for the following commit which adds derivation outputs to `dataFile`.
add txzmq python pkg
add clightning python pkgs
move python packages to pkgs/python-packages

[erikarvstedt]

Here's a squashed version of the branch.

[GambolingPangolin]

The intermittent bug that you observed appears to be this which also has a ticket for when it shows up in twistd. It looks like the fix would be to add explicit SIGINT handling to the zmq plugin. Generally clients should be able to deal with the situation where the zmq publisher crashes, so I think this is pretty low priority.

The way you organized the commits seems pretty much perfect. So I'm going to rebase them and push to this branch.

[erikarvstedt]

ACK 1d44b99

Great work!

[nixbitcoin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 1d44b99340eb4535950f53579ff195de22e8b6cc
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEV3o0Un8+KoXoD+Fk3RH5rVMIs7oFAl+2V9sACgkQ3RH5rVMI
s7rESA/+OTh5+7u3PVVTCDAK7srYotb/Wh7ZHpj8IyJsUmFEYOBI7dfY9H17cOFZ
GO+IayGG/iugeNTjgSx5vd5BVjRwZ+vBPfjzKmuvHPSfvUA4oKBfMHnAR84saXOM
XeHw9zaH/m/bqy2i4OIcCW1fukUyjRk//f6I1HA+PtiZdVzwz6FwfiD21aWPIJH6
UtEzzUYfXSo/9O8Fw3/48INHtSz/XVhr0Cyqbi1rGiw+Iu3z3WJjkQqkWleIxwtU
WjwOrYMwjCbq5zBKE/9NIk3RizcCsngpvy4Qg2LU966rQddBwaBKF7wmi0KDBcqD
vyOjq8hXtDuW1SrWo23L1xtRh/0Q3XixHFUjSjkifNogjwxpqlYXOYF+UTInt/wJ
2SxljvvaIsvt+j1+40MVUTO868USNLwfOhBjFCiX2TJ0EVu4TjeVIkeVHXwdmGCc
Y9ozDPbZZ1wWey7axWjCIx4ADO2+jwnobGaAvyNvdzXwF9y5cHbeh0I7lDLkOiM4
uwkCwjr3s5xjoDEhwsPp2ow+fC59jnSqoMLcIjQBIT5i6IeGYkB8L8tbohXfzJkJ
UWMsjoPzhQ0wnqrOS4XIeaQV3F9/ZpVWPolDp2Fqz8pyxrMlunCBqEz+m5ajWPEm
rrX0eViwXsTLYqPOlzNtLnyTHHzZI4N6Qof6C+E5zSg8vJOpbpg=
=34FD
-----END PGP SIGNATURE-----

[jonasnick]

Thanks everyone! I really like that we're checking the requirements now. Substituting versions is still sketchy (in particular major versions), but I suppose it's the best we can do for now.

During the last two years I've made nix my main platform for desktop and server computing. So it's just daily practice and lots of reading in the nixpkgs sources.

@erikarvstedt that truly must be a lot of reading. Thanks for caring so much about the quality of nix-bitcoin and teaching us plebs in the process :)

[jonasnick]

Just as an aside, we should also add CLBOSS https://lists.ozlabs.org/pipermail/c-lightning/2020-October/000197.html at some point.