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

feature: Added ability for users to add a startup command #15367

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@benthecarman
Copy link
Contributor

commented Feb 7, 2019

Thoughts for adding the feature is for users to be able to add things like electrum-personal-server or lnd to run whenever Bitcoin Core is running. Open to feedback about the feature.

@benthecarman benthecarman force-pushed the benthecarman:startup_commands branch from 412ef2a to b58073e Feb 7, 2019

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

I don't think this should be the scope of Bitcoin Core. Service dependencies should be handled by the init / service management system you are using. Or you can just create a simple shell script, to run all the stuff you need. Besides, it is usually recommended to run different services under different users, if possible, to increase security.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@kristapsk Startup services can't trigger when bitcoin is ready to be actually used. That is the big reason for a notify here. Yes, as an alternative software could just poll until its ready but the same could be said for blocknotifys.

Historically speaking-- startup scripts people have created for bitcoin have been uniformly low quality (e.g. making the software unusable providing no way to get access to the rpc cookie, killing the process and corrupting its state on shutdown, etc). I have generally recommended against using these package scripts due to their low quality, so I think it would be asking a lot of them to expect them to start intelligently starting other services when so far not corrupting the Bitcoin install has been a challenge for them.

@promag

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

I understand the need, I'm not fond of the solution. I think it's perfectly sane to poll the interface or locally run something like netstat.

Does anyone knows an example using this strategy?

Show resolved Hide resolved src/init.cpp Outdated
@gmaxwell

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

I understand the need, I'm not fond of the solution. I think it's perfectly sane to poll the interface or locally run something like netstat.

Huh. Netstat can not tell you if bitcoind is done starting. If you connect as soon as the socket is listening you'll get "bitcoin is starting" errors returned. The gain here over simply retrying is that you can start connecting only when it's actually ready.

(And yes, true you could just sit in a retry loop, but you could also poll for all the other notifys and that didn't stop us from adding them. :) )

@practicalswift

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

Concept ACK

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

Startup services can't trigger when bitcoin is ready to be actually used. That is the big reason for a notify here.

Didn't thought about that. Yes, then it makes sense, concept ACK from me too.

@benthecarman benthecarman force-pushed the benthecarman:startup_commands branch from b58073e to b515ccf Feb 11, 2019

@benthecarman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Moved function call to very end of init sequence

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16097 (WIP: Prevent meaningless negating of arguments by hebasto)
  • #15218 (validation: Flush state after initial sync by andrewtoth)

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.

@promag

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Still not convinced about this. I think the general approach is to poll the interface of interest.

Docker, for instance, supports a directive to check if a container is healthy https://docs.docker.com/v17.09/engine/reference/builder/#healthcheck

See also https://docs.docker.com/compose/startup-order/

Anyway, what if you want to trigger some command when both bitcoind and other service are up and available? You would have to poll the second. Take your example, what if you want to run something else after bitcoind and lnd are up?

Also, from the client point of view, I think it's a good practice to have retry mechanisms, no because of the startup, but because in runtime a service can be unavailable for some reason.

@abitfan

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Electrum (and most likely lnd as well) already have a retry mechanism for rpc and the rpc can be unavailable for other reasons as well so this doesn't add much functionality.
You can watch debug.log for "init message: Done loading" or setup a rawtx zmq publisher and consider bitcoind started when the first notification comes in.

Not specific to this pr but running random commands is not a good security practice and can lead to disasters like privilege escalation. Consider the scenario where someone out of hurry and not wanting to deal with file permissions puts his notifywhatever script in a world writable place and forgets about it only to have his wallet flushed some months later.

@benthecarman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Consider the scenario where someone out of hurry and not wanting to deal with file permissions puts his notifywhatever script in a world writable place and forgets about it only to have his wallet flushed some months later.

While that is true the same could be done with the other notify options, which are ran much more often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.