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

lightningd: Added --alt-subdaemon command to allow alternate subdaemons. #3372

Merged

Conversation

ksedgwic
Copy link
Collaborator

@ksedgwic ksedgwic commented Dec 23, 2019

This PR adds --alt-subdaemon to facilitate substituting a standard subdaemon with an alternate.

Examples:

# Use remote_hsmd instead of lightning_hsmd for signing:
lightningd --alt-subdaemon=lightning_hsmd:remote_hsmd ...

# Same as previous but daemon is located w/ absolute path:
lightningd --alt-subdaemon=lightning_hsmd:/some/where/remote_hsmd ...

Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

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

That's a cool idea. Just some (mostly style) nits, I'll leave the conceptual review to the others.

lightningd/lightningd.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/lightningd.h Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/lightningd.c Show resolved Hide resolved
lightningd/lightningd.c Show resolved Hide resolved
lightningd/lightningd.c Outdated Show resolved Hide resolved
lightningd/options.c Show resolved Hide resolved
@devrandom
Copy link
Contributor

seems like there was an intermittent failure in the build for 6c727d6

@arowser
Copy link
Contributor

arowser commented Dec 25, 2019

Nice pr, @rustyrussell @cdecker another idea: if change the message channels between sub daemons from stdio to socket , then the subdaemon could run on different machines, even run remote-hsmd on a real remote HSM.

@ZmnSCPxj
Copy link
Collaborator

@arowser how would we start the processes remotely? I suggest rather that such setups can be done with a bridge daemon that replaces --alt-subdaemon locally, then just splice (on Linux; otherwise just copy-in-userspace) from the stdio to a local pipe and to a remote socket (on Linux, pipes are "just" representations of kernel buffers, so the local pipe is only needed to properly adapt to any combination of endpoint devices), with the bridge daemon knowing how to properly initiate starting the remote process.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Some trivial changes might be nice, but this is fine as is, too!

Thanks!

lightningd/options.c Outdated Show resolved Hide resolved
lightningd/options.c Show resolved Hide resolved
lightningd/options.c Show resolved Hide resolved
doc/lightningd-config.5.md Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

@ksedgwic Thanks for your patience!

Almost there: let's not skip the final d (my typo, sorry!):

diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md
index c72121bff..9d6548505 100644
--- a/doc/lightningd-config.5.md
+++ b/doc/lightningd-config.5.md
@@ -109,12 +109,12 @@ by *--conf*.
 
  **subdaemon**=*SUBDAEMON*:*PATH*
 Specifies an alternate subdaemon binary.
-Current subdaemons are *channel*, *closing*,
-*connect*, *gossip*, *hsm*, *onchain*, and *opening*.
+Current subdaemons are *channeld*, *closingd,
+*connectd*, *gossipd*, *hsmd*, *onchaind*, and *openingd*.
 If the supplied path is relative the subdaemon binary is found in the
 working directory. This option may be specified multiple times.
 
- So, **subdaemon=lightning_hsmd:remote_signer** would use a
+ So, **subdaemon=hsmd:remote_signer** would use a
 hypothetical remote signing proxy instead of the standard *lightning_hsmd*
 binary.
 
diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c
index 2f658affc..a43f3f07a 100644
--- a/lightningd/lightningd.c
+++ b/lightningd/lightningd.c
@@ -316,11 +316,9 @@ static void memleak_help_alt_subdaemons(struct htable *memtable,
 
 const char *subdaemon_path(const tal_t *ctx, const struct lightningd *ld, const char *name)
 {
-	/* Strip the leading "lightning_" and following 'd' before looking
-	 * in alt_subdaemons
-	 */
-	assert(strlen(name) > 10 + 1);	/* len("lightning_") + len("d") */
-	const char *short_name = tal_strndup(ctx, name + 10, strlen(name) - 11);
+	/* Strip the leading "lightning_" before looking in alt_subdaemons */
+	assert(strlen(name) > strlen("lightning_"));
+	const char *short_name = tal_strdup(ctx, name + strlen("lightning_"));
 
 	/* Is there an alternate path for this subdaemon? */
 	const char *dpath;

And if (strncmp(sdname, subdaemons[i] + 10, strlen(sdname)) == 0)
should probably be strcmp, e.g.: if (streq(sdname, subdaemons[i] + strlen("lightning_"))
otherwise you can do --subdaemon=c:mydaemon and replace all subdaemons starting with c, which is not what we want?

Cheers!

lightningd/lightningd.c Outdated Show resolved Hide resolved
@ksedgwic
Copy link
Collaborator Author

I think that was a spurious CI failure (timeout), force pushed to try again.

@cdecker
Copy link
Member

cdecker commented Feb 3, 2020

ACK 95b5bdb

@rustyrussell rustyrussell merged commit 5fd0ed7 into ElementsProject:master Feb 4, 2020
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.

7 participants