util: Make default arg values more specific #19471

This PR:

• silents printing of the -daemon option for bitcoin-qt
• prints the correct default value of the -printtoconsole option for bitcoin-qt
• prints the default value of the -server option

On master (abdfd2d0e3ebec7dbead89317ee9192189a35809):

$ ./src/bitcoind -help  # `bitcoin-qt -help' prints the same
...
  -daemon
       Run in the background as a daemon and accept commands
...
  -printtoconsole
       Send trace/debug info to console (default: 1 when no -daemon. To disable
       logging to file, set -nodebuglogfile)
...
  -server
       Accept command line and JSON-RPC commands

With this PR:

$ ./src/bitcoind -help
...
  -daemon
       Run in the background as a daemon and accept commands
...
  -printtoconsole
       Send trace/debug info to console (default: 1). Disabled when -daemon=1.
       To disable logging to file, set -nodebuglogfile
...
  -server
       Accept command line and JSON-RPC commands (default: 1)

$ ./src/qt/bitcoin-qt -help
...
  -printtoconsole
       Send trace/debug info to console (default: 0). To disable logging to
       file, set -nodebuglogfile
...
  -server
       Accept command line and JSON-RPC commands (default: 0)

Updated f0add1453739be415962c1a4223c7244db4171e5 -> 6a5f5d8b3fc2dcad1b64a59fe9beacde739b578b (pr19471.01 -> pr19471.02, diff):

• addressed @MarcoFalke's comment:

Any reason to have the default constructor and specify default values for the default args? Note that the compiler forces initialization when a member is const, so moving the initialization of the defaults to bitcoind.cpp seems cleaner.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #21732 (MOVEONLY: Move common init code to init/common by ryanofsky)
• #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
• #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
• #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

Rebased 6a5f5d8b3fc2dcad1b64a59fe9beacde739b578b -> c1030526ebbe2f36c0943a6ba9a4dac12f8fd68f (pr19471.02 -> pr19471.04) due to the conflicts with #15935 and #18923.

re-run ci

Rebased c1030526ebbe2f36c0943a6ba9a4dac12f8fd68f -> 03bf37b51475e82f9a4df4646138a52c2d8c63f3 (pr19471.04 -> pr19471.05) to include the recent CI changes.

Rebased 03bf37b51475e82f9a4df4646138a52c2d8c63f3 -> 96192efd58df41ae871bed2c9d3032bb221f8630 (pr19471.05 -> pr19471.06) due to the conflict with #19561.

Rebased 96192efd58df41ae871bed2c9d3032bb221f8630 -> c501b996a71db82fce778375fec5482e80e506bf (pr19471.06 -> pr19471.07) due to the conflict with #19098.

@ryanofsky Mind reviewing this PR?

Code review ACK c501b996a71db82fce778375fec5482e80e506bf. Suggested some naming changes below but behavior here is a clear improvement and implementation is good.

Updated c501b996a71db82fce778375fec5482e80e506bf -> 0ae8c14d8d85eda41ecb1731b9788a58e332df09 (pr19471.07 -> pr19471.08, diff):

• addressed @ryanofsky's comments

Rebased 0ae8c14d8d85eda41ecb1731b9788a58e332df09 -> cdd6f157c1780189d2340e06122931a5b8a428c5 (pr19471.08 -> pr19471.09) due to the conflict with #19011.

Code review ACK cdd6f157c1780189d2340e06122931a5b8a428c5. Just rebase and naming updates (thanks!) since last review

Rebased cdd6f157c1780189d2340e06122931a5b8a428c5 -> 88a5061217332e590948b942815083ad5625d8fe (pr19471.09 -> pr19471.10) due to the conflict with #19779.

Rebased 88a5061217332e590948b942815083ad5625d8fe -> 1a02a037a53a10995892c4bd86faa663cad8f656 (pr19471.10 -> pr19471.11) due to the conflict with https://github.com/bitcoin-core/gui/pull/35.

Code review ACK 1a02a037a53a10995892c4bd86faa663cad8f656. No changes, just rebase and a few conflict resolutions

Rebased 1a02a037a53a10995892c4bd86faa663cad8f656 -> d796dd09dd0866f391cf5663bc8e1cc8ca1b3402 (pr19471.11 -> pr19471.12) due to the conflict with #21007.

Rebased d796dd09dd0866f391cf5663bc8e1cc8ca1b3402 -> 2ce271c84d7ac5cee0cb79f5f04662b8b7c42f5b (pr19471.12 -> pr19471.13) on top of the #21447.

Code review ACK 2ce271c84d7ac5cee0cb79f5f04662b8b7c42f5b. Just rebased after conflict

Updated 2ce271c84d7ac5cee0cb79f5f04662b8b7c42f5b -> e7198353c72e7f6dcfacb4bf1cc4a284afcee0a8 (pr19471.13 -> pr19471.14, diff):

• drop unneeded forward declaration

Updated e7198353c72e7f6dcfacb4bf1cc4a284afcee0a8 -> ccc0388e18e1340387baa0bcdb8a812e48793721 (pr19471.14 -> pr19471.15, diff):

• addressed @MarcoFalke's comment

<!--cf906140f33d8803c4a75a2196329ecb-->

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Sorry, I think I created a lot of conflicts with here with #19160 and #21732. But I think this can be rebased as a change like the diff below. In the diff I consolidated the gui printtoconsole_default server_default options into a single is_gui option in src/bitcoind.cpp and src/qt/bitcoin.cpp because I thought it might be clearer if selection of default option values didn't need to involve these files. But it didn't really clean things up very much and it would be reasonable to split the options up again if this is not a good change.

I think this all could be cleaner after the change in #10102 adding separate BitcoinNodeInit / BitcoinWalletInit / BitcoinGuiInit / BitcoinQtInit / BitcoindInit classes, since it will be easier to add custom hooks for each executable. I will try to split that change into a smaller PR, but there is no need to delay better behavior implemented in this PR waiting for IPC stuff.

diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index cf9e4fad44d..7e44ca7517d 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -170,10 +170,8 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
             return false;
         }
 
-        // -server defaults to true for bitcoind but not for the GUI so do this here
-        args.SoftSetBoolArg("-server", true);
         // Set this early so that parameter interactions go to console
-        InitLogging(args);
+        InitLogging(args, node.is_gui);
         InitParameterInteraction(args);
         if (!AppInitBasicSetup(args)) {
             // InitError will have been called with detailed error, which ends up on console
diff --git a/src/init.cpp b/src/init.cpp
index 24d67f48dc1..09faa05f28c 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -347,7 +347,7 @@ void SetupServerArgs(NodeContext& node)
     SetupHelpOptions(argsman);
     argsman.AddArg("-help-debug", "Print help message with debugging options and exit", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); // server-only for now
 
-    init::AddLoggingArgs(argsman);
+    init::AddLoggingArgs(argsman, /* can_daemonize= */ !node.is_gui, /* default_printtoconsole= */ !node.is_gui);
 
     const auto defaultBaseParams = CreateBaseChainParams(CBaseChainParams::MAIN);
     const auto testnetBaseParams = CreateBaseChainParams(CBaseChainParams::TESTNET);
@@ -545,11 +545,16 @@ void SetupServerArgs(NodeContext& node)
     argsman.AddArg("-rpcwhitelist=<whitelist>", "Set a whitelist to filter incoming RPC calls for a specific user. The field <whitelist> comes in the format: <USERNAME>:<rpc 1>,<rpc 2>,...,<rpc n>. If multiple whitelists are set for a given user, they are set-intersected. See -rpcwhitelistdefault documentation for information on default whitelist behavior.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
     argsman.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", ArgsManager::ALLOW_BOOL, OptionsCategory::RPC);
     argsman.AddArg("-rpcworkqueue=<n>", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
-    argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
+    argsman.AddArg("-server", strprintf("Accept command line and JSON-RPC commands (default: %u)", !node.is_gui), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 
 #if HAVE_DECL_FORK
+    if (!node.is_gui) {
     argsman.AddArg("-daemon", strprintf("Run in the background as a daemon and accept commands (default: %d)", DEFAULT_DAEMON), ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS);
     argsman.AddArg("-daemonwait", strprintf("Wait for initialization to be finished before exiting. This implies -daemon (default: %d)", DEFAULT_DAEMONWAIT), ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS);
+    } else {
+        hidden_args.emplace_back("-daemon");
+        hidden_args.emplace_back("-daemonwait");
+    }
 #else
     hidden_args.emplace_back("-daemon");
     hidden_args.emplace_back("-daemonwait");
@@ -740,9 +745,10 @@ void InitParameterInteraction(ArgsManager& args)
  * Note that this is called very early in the process lifetime, so you should be
  * careful about what global state you rely on here.
  */
-void InitLogging(const ArgsManager& args)
+void InitLogging(const ArgsManager& args, bool is_gui)
 {
-    init::SetLoggingOptions(args);
+    init::SetLoggingOptions(args, /* is_daemon= */ args.GetBoolArg("-daemon", DEFAULT_DAEMON) || args.GetBoolArg("-daemonwait", DEFAULT_DAEMONWAIT), /* default_printtoconsole= */ !is_gui);
+
     init::LogPackageVersion();
 }
 
@@ -1169,7 +1175,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
      * that the server is there and will be ready later).  Warmup mode will
      * be disabled when initialisation is finished.
      */
-    if (args.GetBoolArg("-server", false)) {
+    if (args.GetBoolArg("-server", !node.is_gui)) {
         uiInterface.InitMessage_connect(SetRPCWarmupStatus);
         if (!AppInitServers(node))
             return InitError(_("Unable to start HTTP server. See debug log for details."));
diff --git a/src/init.h b/src/init.h
index 328eda9c7e8..b0c953ab8a3 100644
--- a/src/init.h
+++ b/src/init.h
@@ -28,7 +28,7 @@ class thread_group;
 void Interrupt(NodeContext& node);
 void Shutdown(NodeContext& node);
 //!Initialize the logging infrastructure
-void InitLogging(const ArgsManager& args);
+void InitLogging(const ArgsManager& args, bool is_gui);
 //!Parameter interaction: change current parameters depending on various rules
 void InitParameterInteraction(ArgsManager& args);
 
diff --git a/src/init/common.cpp b/src/init/common.cpp
index 79e0c9da782..8ba2262b4f3 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -58,7 +58,7 @@ bool SanityChecks()
     return true;
 }
 
-void AddLoggingArgs(ArgsManager& argsman)
+void AddLoggingArgs(ArgsManager& argsman, bool can_daemonize, bool default_printtoconsole)
 {
     argsman.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: %s)", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-debug=<category>", "Output debugging information (default: -nodebug, supplying <category> is optional). "
@@ -74,15 +74,19 @@ void AddLoggingArgs(ArgsManager& argsman)
 #endif
     argsman.AddArg("-logsourcelocations", strprintf("Prepend debug output with name of the originating source location (source file, line number and function name) (default: %u)", DEFAULT_LOGSOURCELOCATIONS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logtimemicros", strprintf("Add microsecond precision to debug timestamps (default: %u)", DEFAULT_LOGTIMEMICROS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-printtoconsole", strprintf("Send trace/debug info to console (default: %u).%s To disable logging to file, set -nodebuglogfile", default_printtoconsole, can_daemonize ? " Disabled when -daemon=1." : ""), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-shrinkdebugfile", "Shrink debug.log file on client startup (default: 1 when no -debug)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
 }
 
-void SetLoggingOptions(const ArgsManager& args)
+void SetLoggingOptions(const ArgsManager& args, bool is_daemon, bool default_printtoconsole)
 {
     LogInstance().m_print_to_file = !args.IsArgNegated("-debuglogfile");
     LogInstance().m_file_path = AbsPathForConfigVal(args.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE));
-    LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", !args.GetBoolArg("-daemon", false));
+    if (is_daemon) {
+        LogInstance().m_print_to_console = false;
+    } else {
+        LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", default_printtoconsole);
+    }
     LogInstance().m_log_timestamps = args.GetBoolArg("-logtimestamps", DEFAULT_LOGTIMESTAMPS);
     LogInstance().m_log_time_micros = args.GetBoolArg("-logtimemicros", DEFAULT_LOGTIMEMICROS);
 #ifdef HAVE_THREAD_LOCAL
diff --git a/src/init/common.h b/src/init/common.h
index fc4bc1b2800..6efa7917093 100644
--- a/src/init/common.h
+++ b/src/init/common.h
@@ -18,8 +18,8 @@ void UnsetGlobals();
  *  necessary library support.
  */
 bool SanityChecks();
-void AddLoggingArgs(ArgsManager& args);
-void SetLoggingOptions(const ArgsManager& args);
+void AddLoggingArgs(ArgsManager& argsman, bool can_daemonize, bool default_printtoconsole);
+void SetLoggingOptions(const ArgsManager& args, bool is_daemon, bool default_printtoconsole);
 void SetLoggingCategories(const ArgsManager& args);
 bool StartLogging(const ArgsManager& args);
 void LogPackageVersion();
diff --git a/src/node/context.h b/src/node/context.h
index 06adb33a80e..1bdaaddb6b7 100644
--- a/src/node/context.h
+++ b/src/node/context.h
@@ -39,6 +39,9 @@ class WalletClient;
 struct NodeContext {
     //! Init interface for initializing current process and connecting to other processes.
     interfaces::Init* init{nullptr};
+    //! Bool indicating if this is a GUI process that does not support
+    //! daemonization, and should also disable the RPC server by default.
+    bool is_gui{false};
     std::unique_ptr<CAddrMan> addrman;
     std::unique_ptr<CConnman> connman;
     std::unique_ptr<CTxMemPool> mempool;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 8befbf5e307..8352936c182 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -71,7 +71,7 @@ private:
     ChainstateManager& chainman() { return *Assert(m_context->chainman); }
 public:
     explicit NodeImpl(NodeContext* context) { setContext(context); }
-    void initLogging() override { InitLogging(*Assert(m_context->args)); }
+    void initLogging() override { InitLogging(*Assert(m_context->args), m_context->is_gui); }
     void initParameterInteraction() override { InitParameterInteraction(*Assert(m_context->args)); }
     bilingual_str getWarnings() override { return GetWarnings(true); }
     uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
@@ -93,7 +93,7 @@ public:
     {
         StartShutdown();
         // Stop RPC for clean shutdown if any of waitfor* commands is executed.
-        if (gArgs.GetBoolArg("-server", false)) {
+        if (gArgs.GetBoolArg("-server", !m_context->is_gui)) {
             InterruptRPC();
             StopRPC();
         }
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index de71b7dea7d..33a9c24d281 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -311,11 +311,7 @@ void BitcoinApplication::startThread()
 
 void BitcoinApplication::parameterSetup()
 {
-    // Default printtoconsole to false for the GUI. GUI programs should not
-    // print to the console unnecessarily.
-    gArgs.SoftSetBoolArg("-printtoconsole", false);
-
-    InitLogging(gArgs);
+    InitLogging(gArgs, /* is_gui= */ true);
     InitParameterInteraction(gArgs);
 }
 
@@ -464,6 +460,7 @@ int GuiMain(int argc, char* argv[])
     util::ThreadSetInternalName("main");
 
     NodeContext node_context;
+    node_context.is_gui = true;
     std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);
 
     // Subscribe to global signals from core
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index ffc51151458..42793c1abf6 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -101,7 +101,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
     SelectParams(chainName);
     SeedInsecureRand();
     if (G_TEST_LOG_FUN) LogInstance().PushBackCallback(G_TEST_LOG_FUN);
-    InitLogging(*m_node.args);
+    InitLogging(*m_node.args, m_node.is_gui);
     AppInitParameterInteraction(*m_node.args);
     LogInstance().StartLogging();
     SHA256AutoDetect();

<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

I won't be able to focus on this stuff in the near future.

So closing up for grabs.