util: Make default arg values more specific#19471
util: Make default arg values more specific#19471hebasto wants to merge 3 commits intobitcoin:masterfrom
Conversation
|
Updated f0add14 -> 6a5f5d8 (pr19471.01 -> pr19471.02, diff):
|
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
Rebased 6a5f5d8 -> c103052 (pr19471.02 -> pr19471.04) due to the conflicts with #15935 and #18923. |
|
re-run ci |
|
Rebased 1a02a03 -> d796dd0 (pr19471.11 -> pr19471.12) due to the conflict with #21007. |
|
Rebased d796dd0 -> 2ce271c (pr19471.12 -> pr19471.13) on top of the #21447. |
src/qt/bitcoin.cpp
Outdated
| // 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Besides
Line 894 in 2ce271c
bitcoin/src/bitcoin-wallet.cpp
Line 67 in 2ce271c
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Why add a data member to ArgsManager? Just pass it into InitLogging by reference?
There was a problem hiding this comment.
Just pass it into
InitLoggingby reference?
What about interfaces::Node::initLogging()?
There was a problem hiding this comment.
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.
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.
|
Updated 2ce271c -> e719835 (pr19471.13 -> pr19471.14, diff):
|
|
Updated e719835 -> ccc0388 (pr19471.14 -> pr19471.15, diff):
|
|
🐙 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 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(); |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
1 similar comment
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
I won't be able to focus on this stuff in the near future. So closing up for grabs. |
This PR:
-daemonoption forbitcoin-qt-printtoconsoleoption forbitcoin-qt-serveroptionOn master (abdfd2d):
With this PR: