Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jul 8, 2020

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 (abdfd2d):

$ ./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)

@hebasto
Copy link
Member Author

hebasto commented Jul 8, 2020

Updated f0add14 -> 6a5f5d8 (pr19471.01 -> pr19471.02, diff):

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 8, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@hebasto
Copy link
Member Author

hebasto commented Jul 24, 2020

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

@maflcko
Copy link
Member

maflcko commented Jul 25, 2020

re-run ci

@maflcko maflcko closed this Jul 25, 2020
@hebasto hebasto reopened this Aug 26, 2020
@maflcko maflcko removed the GUI label Sep 7, 2020
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hebasto
Copy link
Member Author

hebasto commented Mar 16, 2021

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

@hebasto
Copy link
Member Author

hebasto commented Mar 16, 2021

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

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 2ce271c. Just rebased after conflict

// Default printtoconsole to false for the GUI. GUI programs should not
// print to the console unnecessarily.
gArgs.SoftSetBoolArg("-printtoconsole", false);
gArgs.SoftSetBoolArg("-printtoconsole", SERVER_ARGS_OPTIONS.printtoconsole_default);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the point of overwriting the arg here, but keeping the default value as !args.GetBoolArg("-daemon", false) where it is parsed? It would be cleaner to not do any overwriting here and simply use printtoconsole_default as default value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides

LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", !args.GetBoolArg("-daemon", false));
we also have
LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", args.GetBoolArg("-debug", false));
with a different default value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bitcoin-wallet is a completely separate binary that isn't even modified in this PR. I fail to see how this is relevant to the discussion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcoFalke

It would be cleaner to not do any overwriting here and simply use printtoconsole_default as default value.

Yes, you right. But at the parsing place the SERVER_ARGS_OPTIONS is out of the scope. It is possible to bring it into the scope by adding a data member to the ArgsManager but it will make a diff bigger. Is it worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add a data member to ArgsManager? Just pass it into InitLogging by reference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pass it into InitLogging by reference?

What about interfaces::Node::initLogging()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the point of overwriting the arg here, but keeping the default value as !args.GetBoolArg("-daemon", false) where it is parsed? It would be cleaner to not do any overwriting here and simply use printtoconsole_default as default value.

It will change the current behavior, as for bitcoind the printtoconsole_default must be disabled when the -daemon option is set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcoFalke

Thanks! Updated.

hebasto added 2 commits April 18, 2021 17:17
A struct is used instead of bit flags as args could have non-boolean
types.
This change makes -help command print correct default values for some
options.
@hebasto
Copy link
Member Author

hebasto commented Apr 18, 2021

Updated 2ce271c -> e719835 (pr19471.13 -> pr19471.14, diff):

  • drop unneeded forward declaration

@hebasto
Copy link
Member Author

hebasto commented Apr 18, 2021

Updated e719835 -> ccc0388 (pr19471.14 -> pr19471.15, diff):

@DrahtBot
Copy link
Contributor

🐙 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".

@ryanofsky
Copy link
Contributor

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

@DrahtBot
Copy link
Contributor

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.

1 similar comment
@DrahtBot
Copy link
Contributor

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.

@hebasto
Copy link
Member Author

hebasto commented Apr 22, 2022

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

So closing up for grabs.

@hebasto hebasto closed this Apr 22, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants