[init] Check non-emptiness of -blocknotify command prior to executing #10939

practicalswift · 2017-07-26T23:14:40Z

Check that -blocknotify command is non-empty before executing.

To make the BlockNotifyCallback(...) (-blocknotify) behaviour consistent with that of:

AlertNotify(...) (-alertnotify)
AddToWallet(...) (-walletnotify)

promag

Partial ACK. IMO the check should be in init.cpp:

if (!GetArg("-blocknotify", "").empty()) {
    uiInterface.NotifyBlockTip.connect(BlockNotifyCallback);
}

That way it's checked one time only.

promag · 2017-07-27T00:49:32Z

src/wallet/wallet.cpp

@@ -940,7 +940,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
    // notify an external script when a wallet transaction comes in or is updated
    std::string strCmd = GetArg("-walletnotify", "");

-    if ( !strCmd.empty())
+    if (!strCmd.empty())


That change was intentional to show how the logic is handled in AddToWallet (-walletnotify).

promag · 2017-07-27T00:50:46Z

src/init.cpp

-
-    boost::replace_all(strCmd, "%s", pBlockIndex->GetBlockHash().GetHex());
-    boost::thread t(runCommand, strCmd); // thread runs free
+    if (!strCmd.empty()) {


No need to test because in init.cpp there's:

if (IsArgSet("-blocknotify")) uiInterface.NotifyBlockTip.connect(BlockNotifyCallback);

Does IsArgSet(…) test for emptiness?

Ignore if you want to keep the test here.

practicalswift · 2017-07-27T07:35:50Z

@promag I prefer doing a check inside of BlockNotifyCallback(…) (as close as possible to the RunCommand(…)) since:

BlockNotifyCallback(…) might be invoked from other places in the future.
That is consistent with how it is done for AlertNotify(...) (-alertnotify) and AddToWallet(...) (-walletnotify).

promag · 2017-07-27T08:27:28Z

Side note, should we treat empty args as invalid?

practicalswift · 2017-07-27T08:46:03Z

@promag In the specific case of -{alert,block,wallet}notify: yes - no need to execute "" :-)

laanwj · 2017-07-27T09:07:47Z

Side note, should we treat empty args as invalid?

Depends on the specific argument. For some things, an empty string might be a valid value, for others it's not.

For executing a command it makes sense to skip it when the string is empty.
What does system("") do? spawn a shell?

BTW: Why not handle this in runCommand? that seems the shortest path to making this consistent.

laanwj · 2017-07-27T09:09:17Z

What does system("") do? spawn a shell?

It already does nothing! Same for system(" "); etc.
Why do we need this change?

promag · 2017-07-27T11:09:33Z

@laanwj the only reason to test "" in each is to add some custom logging. Otherwise agree with you (despite the fact that we can avoid connecting to the signal in init).

laanwj · 2017-07-27T11:38:17Z

Not convinced that "custom logging" makes sense here. The user provided an empty command, which non-ambiguously means they don't want it to do anything. It doesn't do anything.

(maybe skipping could be considered a small optimization, but could leave it up to the user to just not provide the argument at all so that the signal doesn't get registered? I have a hard time imagining any circumstance where this would matter at all)

practicalswift · 2017-07-27T12:12:50Z

@laanwj My original idea was to make -blocknotify, -alertnotify and -walletnotify behave identically with regards to thread creation and execve(…)-calls in case of an "" argument. Not a big issue, just a small step towards increased user interface consistency :-)

promag · 2017-07-31T15:30:11Z

@laanwj IMO empty commands should be invalid. The same goes for when a command does not exists. Not providing the argument/command should be the correct way to run nothing.

For instance, with find it fails:

$ find . -exec \;
find: -exec: no command specified

To make BlockNotifyCallback(...) (-blocknotify) consistent with: * AlertNotify(...) (-alertnotify) * AddToWallet(...) (-walletnotify)

jnewbery · 2017-09-01T21:10:41Z

(slightly offtopic)

@promag - I'm with you on this one. I'd like there to be better systematic validation of command line-arguments.

I think you've spoken about argument validation on a few issues. Perhaps we should raise this in a future meeting to get concept feedback?

promag · 2017-09-01T21:36:05Z

Yes @jnewbery, IMO it can be improved. We should validate command line arguments in the same way we validate RPC arguments. Again, for me "" is an invalid command.

luke-jr

utACK

luke-jr · 2017-09-02T06:42:15Z

We shouldn't make the empty command invalid so quickly: if one has blocknotify=foobar in the bitcoin.conf file, they may wish to use -blocknotify= on the commandline to temporarily disable it.

(The alternative, -noblocknotify, is probably broken...)

promag · 2017-09-02T08:37:17Z

they may wish to use -blocknotify=

They can remove the line or comment it out?

luke-jr · 2017-09-02T08:42:59Z

Better if no change to the config file is needed for such a temporary purpose.

jnewbery · 2017-09-04T16:00:32Z

@luke-jr raises a good point. Being able to override and temporarily disable .conf file options from the command line is very useful. This PR allows that (supplying blocknotify= won't cause an error).

Has anyone tested what happens with noblocknotify?

laanwj · 2017-10-04T12:53:47Z

We shouldn't make the empty command invalid so quickly: if one has blocknotify=foobar in the bitcoin.conf file, they may wish to use -blocknotify= on the commandline to temporarily disable it.

Good point! And the first good argument for doing this at all. Being able to overriding the blocknotify command with "" (say, from the command line) to disable it is useful. In that case you don't want an empty command to be executed as that just wastes resources.

utACK cffe85f

…r to executing cffe85f Skip sys::system(...) call in case of empty command (practicalswift) 6fb8f5f Check that -blocknotify command is non-empty before executing (practicalswift) Pull request description: Check that `-blocknotify` command is non-empty before executing. To make the `BlockNotifyCallback(...)` (`-blocknotify`) behaviour consistent with that of: * `AlertNotify(...)` (`-alertnotify`) * `AddToWallet(...)` (`-walletnotify`) Tree-SHA512: 18272166793a5a8b9cc2a727bfbcea53d38c329a55bc975c02db601329d608a61c20e026ce4b616193ecd3810dca4d3e2cb3bf773898a51872008a8dba96763e

…nd prior to executing cffe85f Skip sys::system(...) call in case of empty command (practicalswift) 6fb8f5f Check that -blocknotify command is non-empty before executing (practicalswift) Pull request description: Check that `-blocknotify` command is non-empty before executing. To make the `BlockNotifyCallback(...)` (`-blocknotify`) behaviour consistent with that of: * `AlertNotify(...)` (`-alertnotify`) * `AddToWallet(...)` (`-walletnotify`) Tree-SHA512: 18272166793a5a8b9cc2a727bfbcea53d38c329a55bc975c02db601329d608a61c20e026ce4b616193ecd3810dca4d3e2cb3bf773898a51872008a8dba96763e

practicalswift changed the title ~~Check that -blocknotify command is non-empty before executing~~ [init] Check that -blocknotify command is non-empty before executing Jul 26, 2017

practicalswift changed the title ~~[init] Check that -blocknotify command is non-empty before executing~~ [init] Check non-emptiness of -blocknotify command before executing Jul 26, 2017

practicalswift changed the title ~~[init] Check non-emptiness of -blocknotify command before executing~~ [init] Check non-emptiness of -blocknotify command prior to executing Jul 26, 2017

promag reviewed Jul 27, 2017

View reviewed changes

practicalswift added 2 commits August 14, 2017 17:50

Check that -blocknotify command is non-empty before executing

6fb8f5f

To make BlockNotifyCallback(...) (-blocknotify) consistent with: * AlertNotify(...) (-alertnotify) * AddToWallet(...) (-walletnotify)

Skip sys::system(...) call in case of empty command

cffe85f

practicalswift force-pushed the blocknotify-consistentcy branch from 7f1ba9a to cffe85f Compare August 14, 2017 15:50

luke-jr approved these changes Sep 2, 2017

View reviewed changes

laanwj merged commit cffe85f into bitcoin:master Oct 4, 2017

practicalswift deleted the blocknotify-consistentcy branch April 10, 2021 19:32

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[init] Check non-emptiness of -blocknotify command prior to executing #10939

[init] Check non-emptiness of -blocknotify command prior to executing #10939

practicalswift commented Jul 26, 2017 •

edited

promag left a comment •

edited

promag Jul 27, 2017

practicalswift Jul 27, 2017 •

edited

promag Jul 27, 2017

practicalswift Jul 27, 2017

promag Jul 27, 2017

practicalswift commented Jul 27, 2017 •

edited

promag commented Jul 27, 2017

practicalswift commented Jul 27, 2017 •

edited

laanwj commented Jul 27, 2017

laanwj commented Jul 27, 2017

promag commented Jul 27, 2017

laanwj commented Jul 27, 2017 •

edited

practicalswift commented Jul 27, 2017 •

edited

promag commented Jul 31, 2017 •

edited

jnewbery commented Sep 1, 2017 •

edited

promag commented Sep 1, 2017

luke-jr left a comment

luke-jr commented Sep 2, 2017

promag commented Sep 2, 2017

luke-jr commented Sep 2, 2017

jnewbery commented Sep 4, 2017

laanwj commented Oct 4, 2017 •

edited

[init] Check non-emptiness of -blocknotify command prior to executing #10939

[init] Check non-emptiness of -blocknotify command prior to executing #10939

Conversation

practicalswift commented Jul 26, 2017 • edited

promag left a comment • edited

Choose a reason for hiding this comment

promag Jul 27, 2017

Choose a reason for hiding this comment

practicalswift Jul 27, 2017 • edited

Choose a reason for hiding this comment

promag Jul 27, 2017

Choose a reason for hiding this comment

practicalswift Jul 27, 2017

Choose a reason for hiding this comment

promag Jul 27, 2017

Choose a reason for hiding this comment

practicalswift commented Jul 27, 2017 • edited

promag commented Jul 27, 2017

practicalswift commented Jul 27, 2017 • edited

laanwj commented Jul 27, 2017

laanwj commented Jul 27, 2017

promag commented Jul 27, 2017

laanwj commented Jul 27, 2017 • edited

practicalswift commented Jul 27, 2017 • edited

promag commented Jul 31, 2017 • edited

jnewbery commented Sep 1, 2017 • edited

promag commented Sep 1, 2017

luke-jr left a comment

Choose a reason for hiding this comment

luke-jr commented Sep 2, 2017

promag commented Sep 2, 2017

luke-jr commented Sep 2, 2017

jnewbery commented Sep 4, 2017

laanwj commented Oct 4, 2017 • edited

practicalswift commented Jul 26, 2017 •

edited

promag left a comment •

edited

practicalswift Jul 27, 2017 •

edited

practicalswift commented Jul 27, 2017 •

edited

practicalswift commented Jul 27, 2017 •

edited

laanwj commented Jul 27, 2017 •

edited

practicalswift commented Jul 27, 2017 •

edited

promag commented Jul 31, 2017 •

edited

jnewbery commented Sep 1, 2017 •

edited

laanwj commented Oct 4, 2017 •

edited