util: improve bitcoin-wallet exit codes #24428

1. 
   fanquake commented at 10:22 am on February 23, 2022: member

   Refactors bitcoin-wallet so that it doesn’t return a non-zero exit code by default, and makes the option handling more inline with the other binaries. i.e outputting Error: too few parameters if you don’t pass any options.

   Fixing this means we can check the process output in gen-manpages.py; which addresses the remaining review comment from #24263.

2. fanquake requested review from laanwj on Feb 23, 2022
3. in src/bitcoin-wallet.cpp:27 in 394fdcc9d6 outdated

   23@@ -24,6 +24,7 @@
   24 #include <string>
   25 #include <tuple>
   26 
   27+static constexpr int CONTINUE_EXECUTION=-1;
   

   ---

   ---

   

   maflcko commented at 10:34 am on February 23, 2022:

   0static constexpr int CONTINUE_EXECUTION{-1};
   

   forgot to run clang-format?

   ---

   

   fanquake commented at 11:25 am on February 23, 2022:

   Thanks, done.
4. util: improve bitcoin-wallet exit codes

   Adjust the exit codes / functionality of bitcoin-wallet such that it's
   not returning a non-zero exit code if there isn't a problem.
   
   This is a followup from #24263, and should allow us to add and
   additional check=True to our gen-manpages.py script.

   05a0be9546
5. scripts: check binary exit code in gen-manpages.py

   Now that bitcoin-wallet no-longer returns a non-zero exit code when it
   shouldn't do, we can add check=True to our subprocess call.
   
   Follows up to the comments in
   https://github.com/bitcoin/bitcoin/pull/24263#discussion_r806126705.

   dd532ee9c4
6. fanquake force-pushed on Feb 23, 2022
7. DrahtBot added the label Scripts and tools on Feb 23, 2022
8. DrahtBot added the label Utils/log/libs on Feb 23, 2022
9. in src/bitcoin-wallet.cpp:55 in dd532ee9c4

   51@@ -49,13 +52,13 @@ static void SetupWalletToolArgs(ArgsManager& argsman)
   52     argsman.AddCommand("createfromdump", "Create new wallet file from dumped records");
   53 }
   54 
   55-static bool WalletAppInit(ArgsManager& args, int argc, char* argv[])
   56+static int WalletAppInit(ArgsManager& args, int argc, char* argv[])
   

   ---

   ---

   

   laanwj commented at 12:38 pm on February 23, 2022:

   Could maybe make this option<int>, a categorical difference seems slightly neater to me than reserving a return code for continuing execution.

   ---

   

   luke-jr commented at 2:49 am on March 11, 2022:

   Especially since EXIT_SUCCESS and EXIT_FAILURE are not guaranteed to be non--1 :)
10. 
    laanwj commented at 12:38 pm on February 23, 2022: member

    Concept ACK
11. 
    theStack commented at 0:57 am on February 24, 2022: contributor

    Concept ACK
12. in src/bitcoin-wallet.cpp:118 in dd532ee9c4

    111@@ -105,7 +112,10 @@ int main(int argc, char* argv[])
    112     SetupEnvironment();
    113     RandomInit();
    114     try {
    115-        if (!WalletAppInit(args, argc, argv)) return EXIT_FAILURE;
    116+        const int ret = WalletAppInit(args, argc, argv);
    117+        if (ret != CONTINUE_EXECUTION) {
    118+            return ret;
    119+        }
    

    ---

    ---

    

    luke-jr commented at 2:55 am on March 11, 2022:

    Maybe it’d be better to just have WalletAppInit call exit(2) directly?
13. 
    gruve-p commented at 2:45 pm on April 4, 2022: contributor

    Concept ACK
14. maflcko added the label Waiting for author on Apr 4, 2022
15. maflcko marked this as a draft on Apr 4, 2022
16. fanquake removed the label Waiting for author on Aug 31, 2022
17. fanquake added the label Up for grabs on Aug 31, 2022
18. fanquake closed this on Aug 31, 2022

    ---

19. maflcko removed the label Up for grabs on Sep 12, 2022
20. 
    maflcko commented at 11:13 am on September 12, 2022: member

    Alternative in #26067 using std::optional<int>
21. fanquake deleted the branch on Nov 9, 2022
22. bitcoin locked this on Nov 9, 2023

Contributors
fanquake maflcko laanwj luke-jr theStack gruve-p

Labels
Scripts and tools Utils/log/libs