Support multiple -*notify commands #22372

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:multinotify changing 3 files +43 −32

luke-jr commented at 7:14 PM on June 29, 2021: member

Useful, intuitive, and avoids creating issues with #22354
DrahtBot added the label Validation on Jun 29, 2021
DrahtBot added the label Wallet on Jun 29, 2021
DrahtBot commented at 11:39 AM on June 30, 2021: contributor



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



Conflicts

No conflicts as of last run.

in src/wallet/wallet.cpp:996 in 71adc97c73 outdated

 958 | -        // https://github.com/bitcoin/bitcoin/pull/13339#issuecomment-537384875
 959 | -        // A few ways it could be implemented in the future are described in:
 960 | -        // https://github.com/bitcoin/bitcoin/pull/13339#issuecomment-461288094
 961 | -        boost::replace_all(strCmd, "%w", ShellEscape(GetName()));
 962 | +    if (gArgs.IsArgSet("-walletnotify")) {
 963 | +#ifdef WIN32

kristapsk commented at 12:03 AM on July 1, 2021:

Comment below #ifndef WIN32 from previous code about why it's not supported currently should be copied here IMO.

luke-jr commented at 6:54 PM on December 17, 2021:

Copied

kristapsk commented at 12:27 AM on July 1, 2021: contributor

Concept ACK
benthecarman commented at 5:23 PM on July 3, 2021: contributor

Concept ACK
benthecarman commented at 5:25 PM on July 3, 2021: contributor

Looks like this doesn't touch -startupnotify, does that support multiple already?
luke-jr force-pushed on Jul 3, 2021
luke-jr commented at 6:05 PM on July 3, 2021: member

Whoops, fixed.

in src/init.cpp:686 in 54b25a8c51 outdated

 682 | @@ -683,9 +683,8 @@ static void CleanupBlockRevFiles()
 683 |  #if HAVE_SYSTEM
 684 |  static void StartupNotify(const ArgsManager& args)
 685 |  {
 686 | -    std::string cmd = args.GetArg("-startupnotify", "");
 687 | -    if (!cmd.empty()) {
 688 | -        std::thread t(runCommand, cmd);
 689 | +    for (std::string command : gArgs.GetArgs("-startupnotify")) {

maflcko commented at 6:43 PM on November 10, 2021:

    for (std::string cmd : args.GetArgs("-startupnotify")) {

Seems odd to use gArgs when args is passed in. Also, why change the symbol name to increase the diff?

luke-jr commented at 6:54 PM on December 17, 2021:

gArgs: Fixed.

Symbol name: It doesn't increase the diff to adjust it to our current style.

DrahtBot added the label Needs rebase on Nov 10, 2021
luke-jr referenced this in commit c83d8d7a76 on Dec 14, 2021
luke-jr referenced this in commit 6c7304ff6e on Dec 14, 2021

scripted-diff: test: rename `strCmd` to `command`

-BEGIN VERIFY SCRIPT-
sed -i 's/\bstrCmd\b/command/g' $(git grep -l strCmd)
-END VERIFY SCRIPT-

5498850040

Support multiple -*notify commands 041b1ed8b7
luke-jr force-pushed on Dec 17, 2021
luke-jr commented at 6:55 PM on December 17, 2021: member

Rebased and addressed review comments
DrahtBot removed the label Needs rebase on Dec 17, 2021
DrahtBot added the label Needs rebase on May 6, 2022
DrahtBot commented at 8:51 AM on May 6, 2022: contributor



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
achow101 commented at 6:34 PM on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
achow101 closed this on Oct 12, 2022
kristapsk commented at 2:25 PM on November 23, 2022: contributor

Guess this was closed just because of no rebase after May 6 and lack of ACKs? Seems useful in context of #25975.
luke-jr commented at 11:41 PM on November 26, 2022: member

Feel free to reopen. As with most of my PRs, I continue to maintain it for Knots.
bitcoin locked this on Nov 26, 2023

Contributors

Labels

Wallet Validation Needs rebase

Linked (view graph)

#22354 contrib/init: (OpenRC) use -startupnotify to wait for startup completion [alternative]

Support multiple -*notify commands #22372

Conflicts