util: Improve documentation and negation of args #31212

hodlinator commented at 2:15 PM on November 4, 2024: contributor

Document -color as only applying to -getinfo, to be less confusing for bitcoin-cli users.
No longer print version information when getting passed -noversion.
Disallow -nodatadir as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0".
Support -norpccookiefile
Support -nopid
Properly support -noconf (instead of working by accident). Also detect when directories are specified instead of files.

Prompted by investigation in #16545#pullrequestreview-2316714013.

DrahtBot commented at 2:15 PM on November 4, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31212.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	rkrux
Ignored review	l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31349 (ci: detect outbound internet traffic generated while running tests by vasild)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#30529 (Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

DrahtBot added the label Utils/log/libs on Nov 4, 2024

in src/bitcoin-cli.cpp:96 in 1cfbe0ca22 outdated

  92 | @@ -93,7 +93,7 @@ static void SetupCliArgs(ArgsManager& argsman)
  93 |      argsman.AddArg("-netinfo", "Get network peer connection information from the remote server. An optional integer argument from 0 to 4 can be passed for different peers listings (default: 0). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
  94 |  
  95 |      SetupChainParamsBaseOptions(argsman);
  96 | -    argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
  97 | +    argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never. Only applies in combination with -getinfo.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);

rkrux commented at 10:20 AM on November 7, 2024:

Good catch. I was confused initially wondering why doesn't color show up on explicitly setting this arg. in combination gives an impression that somehow both these args need to be passed together.

Only applies to the output of -getinfo.

hodlinator commented at 12:58 PM on November 8, 2024:

Thanks! Applied.

in src/bitcoin-cli.cpp:148 in a8a41f1c96 outdated

 143 | @@ -144,10 +144,10 @@ static int AppInitRPC(int argc, char* argv[])
 144 |          tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
 145 |          return EXIT_FAILURE;
 146 |      }
 147 | -    if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
 148 | +    if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {

rkrux commented at 10:24 AM on November 7, 2024:

Again great catch. I noticed the difference in the output in master and this branch.

➜  bitcoin git:(master) ✗ bitcoinclireg -noversion
Bitcoin Core RPC client version v28.99.0-65b194193661
Copyright (C) 2009-2024 The Bitcoin Core developers

Please contribute if you find Bitcoin Core useful. Visit
<https://bitcoincore.org/> for further information about the software.
The source code is available from <https://github.com/bitcoin/bitcoin>.

This is experimental software.
Distributed under the MIT software license, see the accompanying file COPYING
or <https://opensource.org/licenses/MIT>

➜  bitcoin git:(master) ✗ bitcoinclireg -noversion
error: too few parameters (need at least command)

Wondering now maybe we should disallow negation for version altogether? The output of noversion now is too few params and I fail to see any benefit in allowing this (maybe I'm missing something here). Might as well throw this error?

Negating of -version is meaningless and therefore forbidden

hodlinator commented at 1:05 PM on November 8, 2024:

I'm relying on this comment from unmerged #16545: https://github.com/bitcoin/bitcoin/blob/b5ef85497436c3e9e60c760465d8991592efef07/src/common/args.h#L112-L114

One could imagine a user having a config file containing:

version=1
<...many lines...>
noversion=1

The last occurrence of a bool option takes precedence. Agree it feels very contrived for this specific setting, but I'm trying to stick to the general principle of trying to allow negation if possible.

rkrux commented at 10:59 AM on November 7, 2024: contributor

Concept ACK 9e130811a3f68aea77ddeecf667aa8389a2940e8

Thanks for catching issues here, gave me an opportunity to read the app init code a bit more!

Successful make and functional tests. Left couple suggestions in the first 2 commits. Disallowing noconf and nodatadir conceptually makes sense to me. I tested with these 2 args and the output I received seems okay to me.

➜  bitcoin git:(2024/11/invalid_args) ✗ bitcoinclireg -noconf
Error parsing command line arguments: Negating of -conf is meaningless and therefore forbidden
➜  bitcoin git:(2024/11/invalid_args) ✗ bitcoinclireg -nodatadir
Error parsing command line arguments: Negating of -datadir is meaningless and therefore forbidden

Though I don't completely understand why this comment "ok to not have a config file" (and subsequent flow) is allowed because when I ran the CLI without any arg I received the version info, help options, & too few args message. https://github.com/hodlinator/bitcoin/blob/2024/11/invalid_args/src/common/config.cpp#L138

in src/bitcoin-cli.cpp:151 in a8a41f1c96 outdated

 143 | @@ -144,10 +144,10 @@ static int AppInitRPC(int argc, char* argv[])
 144 |          tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
 145 |          return EXIT_FAILURE;
 146 |      }
 147 | -    if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
 148 | +    if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {
 149 |          std::string strUsage = CLIENT_NAME " RPC client version " + FormatFullVersion() + "\n";
 150 |  
 151 | -        if (gArgs.IsArgSet("-version")) {
 152 | +        if (gArgs.GetBoolArg("-version", false)) {

maflcko commented at 11:15 AM on November 7, 2024:

Could this commit be a scripted-diff?

hodlinator commented at 1:05 PM on November 8, 2024:

Done!

rkrux commented at 12:25 PM on November 7, 2024: contributor

Though I don't completely understand why this comment "ok to not have a config file" (and subsequent flow) is allowed

My guess it is applicable when the daemon is run without a conf because this portion lies in common/.

hodlinator commented at 12:55 PM on November 8, 2024: contributor

Though I don't completely understand why this comment "ok to not have a config file" (and subsequent flow) is allowed

My guess it is applicable when the daemon is run without a conf because this portion lies in common/.

My interpretation is that we shouldn't trigger an error on not finding a configuration file in the ~specified or~ default location. If it's there we read it, otherwise just skip. (The code does error just above if one is unable to open a configuration file at a non-default location).

hodlinator force-pushed on Nov 8, 2024

DrahtBot added the label Needs rebase on Nov 8, 2024

luke-jr commented at 6:40 PM on November 9, 2024: member

Disallow -noconf since a config file is required

But it's not. Seems like -noconf should work to ignore the config file.

hodlinator commented at 3:18 PM on November 11, 2024: contributor

Disallow -noconf since a config file is required

But it's not. Seems like -noconf should work to ignore the config file.

As mentioned in the commit message for 2d65e6ae0756b87e82dbafd032308e8687ec7941:

ArgsManager::GetConfigFilePath() asserts that a path is always set. Explicitly negating it is therefore invalid."

Previously, -noconf would result in an ifstream being opened to the ".bitcoin"-directory (not a file) in ArgsManager::ReadConfigFiles(), also circumventing the assert in aforementioned GetConfigFilePath().

(-conf="" is still allowed to reset to the default bitcoin.conf-file location)."

It's sheer luck when it comes to how ifstream treats opening of directories that has allowed this before the PR.

Here is GetConfigFilePath:

https://github.com/bitcoin/bitcoin/blob/2d65e6ae0756b87e82dbafd032308e8687ec7941/src/common/args.cpp#L761-L765

The part of ArgsManager::ReadConfigFiles which with -noconf would open a directory in the ifstream, and treat it as .good().

https://github.com/bitcoin/bitcoin/blob/2d65e6ae0756b87e82dbafd032308e8687ec7941/src/common/config.cpp#L121-L140

ReadConfigStream calls into GetConfigOptions which gets as far as trying to getline from the directory ifstream, only to return true (success).

https://github.com/bitcoin/bitcoin/blob/2d65e6ae0756b87e82dbafd032308e8687ec7941/src/common/config.cpp#L33-L38

It would be good to add detection of "directory as config path" and trigger an error, regardless of which path is given. But I'm still inclined to disallow -noconf because of the assert in GetConfigFilePath.

l0rinc commented at 3:29 PM on November 11, 2024: contributor

Concept ACK

hodlinator commented at 3:52 PM on November 11, 2024: contributor

Added commit which now guards against passing directory paths as -conf values as per #31212 (comment).

ryanofsky commented at 1:18 PM on November 12, 2024: contributor

Code review d9cfb2a314caa1cdcae8e47a03dad557ad96e069.

This generally looks good except commit 2d65e6ae0756b87e82dbafd032308e8687ec7941 disallowing -noconf. Specifying -noconf to disable reading from the default config file path is useful and something that should be supported, even if now it's only working in a very quirky way by treating the datadir as an empty configuration file. I think a better implementation would not look too different though, maybe like:

--- a/src/common/config.cpp
+++ b/src/common/config.cpp
@@ -131,7 +131,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
     std::ifstream stream{conf_path};
 
     // not ok to have a config file specified that cannot be opened
-    if (IsArgSet("-conf") && !stream.good()) {
+    if ((IsArgSet("-conf") && !IsArgNegated("-conf")) && !stream.good()) {
         error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
         return false;
     }
@@ -213,7 +213,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
 
 fs::path AbsPathForConfigVal(const ArgsManager& args, const fs::path& path, bool net_specific)
 {
-    if (path.is_absolute()) {
+    if (path.is_absolute() || path.empty()) {
         return path;
     }
     return fsbridge::AbsPathJoin(net_specific ? args.GetDataDirNet() : args.GetDataDirBase(), path);

I don't think it's right to infer from Assert(m_config_path) that a configuration file is always required by because that assert is really meant to catch initialization order bugs. The type of m_config_path is std::optional<fs::path> not fs::path so an empty path is ok there.

hodlinator force-pushed on Nov 14, 2024

hodlinator commented at 11:19 PM on November 14, 2024: contributor

@ryanofsky thanks for the review! Updated to cover negation for all users of AbsPathForConfigVal.

hodlinator renamed this:
~~util: Narrow scope of args (-color, -version, -conf, -datadir)~~
util: Improve documentation and negation of args (-color, -version, -datadir, -rpccookiefile, -pid, -conf)
on Nov 14, 2024

hodlinator renamed this:
~~util: Improve documentation and negation of args (-color, -version, -datadir, -rpccookiefile, -pid, -conf)~~
util: Improve documentation and negation of args
on Nov 14, 2024

in src/rpc/request.cpp:99 in cf3e4edc80 outdated

  95 | @@ -96,6 +96,11 @@ static bool g_generated_cookie = false;
  96 |  
  97 |  bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms)
  98 |  {
  99 | +    if (gArgs.IsArgNegated("-rpccookiefile")) {

ryanofsky commented at 3:24 AM on November 15, 2024:

In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)

I think having two separate accesses to the -rpccookiefile in different functions is fragile since GetAuthCookieFile will not do the right thing if called in a different context. It is also not taking advantage of GetPathArg's built in handling of negation. I think a change like:

--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -86,6 +86,7 @@ static const char* const COOKIEAUTH_FILE = ".cookie";
 static fs::path GetAuthCookieFile(bool temp=false)
 {
     fs::path arg = gArgs.GetPathArg("-rpccookiefile", COOKIEAUTH_FILE);
+    if (arg.empty()) return {}; // -norpccookiefile was specified
     if (temp) {
         arg += ".tmp";
     }
@@ -96,11 +97,6 @@ static bool g_generated_cookie = false;
 
 bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms)
 {
-    if (gArgs.IsArgNegated("-rpccookiefile")) {
-        LogInfo("RPC authentication cookie file generation is disabled.");
-        return true;
-    }
-
     const size_t COOKIE_SIZE = 32;
     unsigned char rand_pwd[COOKIE_SIZE];
     GetRandBytes(rand_pwd);
@@ -111,6 +107,11 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie
      */
     std::ofstream file;
     fs::path filepath_tmp = GetAuthCookieFile(true);
+    if (filepath_tmp.empty()) {
+        LogInfo("RPC authentication cookie file generation is disabled.");
+        return true;
+    }
+
     file.open(filepath_tmp);
     if (!file.is_open()) {
         LogInfo("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp));

would make this safer and clearer.

hodlinator commented at 9:53 PM on November 18, 2024:

Thanks! Implemented in latest push with some variation.

in src/httprpc.cpp:73 in cf3e4edc80 outdated

  69 | @@ -70,7 +70,7 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
  70 |  
  71 |  
  72 |  /* Pre-base64-encoded authentication token */
  73 | -static std::string strRPCUserColonPass;
  74 | +static std::optional<std::string> strRPCUserColonPass;

ryanofsky commented at 4:37 AM on November 15, 2024:

In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)

The changes to httprpc.cpp seem too complicated and I don't think they are safe because now that an empty strRPCUserColonPass string is allowed by RPCAuthorized, the TimingResistantEqual function can return true when the client provides an empty auth string.

I also think changing strRPCUserColonPass to std::optional makes state more complicated and code more confusing to think about, even as it weakens the "belt and suspenders" check it is trying to preserve by intializing the variable at the top of the InitRPCAuthentication function and leaving it set even when the function fails.

To fix these issue, I would leave strRPCUserColonPass as std::string instead of making it into a std::optional, leave the InitRPCAuthentication function unchanged, and just change the RPCAuthorized function as follows:

--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -134,8 +134,6 @@ static bool multiUserAuthorized(std::string strUserPass)
 
 static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)
 {
-    if (strRPCUserColonPass.empty()) // Belt-and-suspenders measure if InitRPCAuthentication was not called
-        return false;
     if (strAuth.substr(0, 6) != "Basic ")
         return false;
     std::string_view strUserPass64 = TrimStringView(std::string_view{strAuth}.substr(6));
@@ -148,7 +146,8 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
         strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':'));
 
     //Check if authorized under single-user field
-    if (TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
+    // strRPCUserColonPass is empty when -norpccookiefile is specified
+    if (!strRPCUserColonPass.empty() && TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
         return true;
     }
     return multiUserAuthorized(strUserPass);

I don't think it is worth complicating the code so much just to support keeping a belt and suspender check that is no longer valid now that strRPCUserColonPass being empty is a valid state.

hodlinator commented at 9:55 PM on November 18, 2024:

Thanks for catching this. Was not my intention to leave the door wide open when users pass -norpccookiefile. :man_facepalming: Kept a belt & suspenders bool in latest push.

in src/common/config.cpp:136 in 77c8a538b7 outdated

 128 | @@ -129,9 +129,11 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
 129 |  
 130 |      const auto conf_path{GetConfigFilePath()};
 131 |      std::ifstream stream{conf_path};

ryanofsky commented at 4:55 AM on November 15, 2024:

In commit "args: Properly support -noconf and catch directories" (77c8a538b7164f8b65576600eb4668ac160f6b14)

It doesn't seem good to try to open a file with an empty path. This behavior doesn't seem well defined and the call will unnecessarily be passed down to the os when it shouldn't be. I think it would also clarify the code and avoid the need to add a second IsArgNegated call if this checked for an empty path. Would suggest:

    const auto conf_path{GetConfigFilePath()};
    std::ifstream stream;
    if (!conf_path.empty()) { // path is empty when -noconf is specified
        stream = std::ifstream{conf_path};
        // Calling peek() on ifstreams opened on directories flags them as !good().
        (void)stream.peek();

        // not ok to have a config file specified that cannot be opened
        if (IsArgSet("-conf") && !stream.good()) {
            error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
            return false;
        }
    }

hodlinator commented at 9:57 PM on November 18, 2024:

Fixed in latest push. Also switched from ifstream::peek() to fs::is_directory() after verifying the latter returns true for directory-symlinks as well.

ryanofsky commented at 5:10 AM on November 15, 2024: contributor

Code review 77c8a538b7164f8b65576600eb4668ac160f6b14. Thanks for the updates! This all looks great, my only substantial comments are about the RPC commit which I think is more complicated than it needs to be and may have a bug where it may allow an empty auth string to be accepted if -norpccookiefile is specified, or if there is an initialization failure.

It may be nice to add python functional tests to exercise some of the new behaviors. It would be nontrivial work to add test coverage for all of these behaviors, but it doesn't have to be all-or-nothing. Just having some new end-to-end tests of argument parsing edge cases would be a nice improvement to our existing test coverage.

hodlinator force-pushed on Nov 15, 2024

hodlinator marked this as a draft on Nov 15, 2024

hodlinator force-pushed on Nov 18, 2024

hodlinator commented at 10:01 PM on November 18, 2024: contributor

Thanks for continued review and reminding me to raise the bar for the PR @ryanofsky.

Added tests which helped uncover that bitcoin-cli might as well also support -norpccookiefile too, and part of src/common/init.cpp which was not handling -noconf correctly.

doc args: Document narrow scope of -color e8a2054edc

hodlinator force-pushed on Nov 18, 2024

DrahtBot added the label CI failed on Nov 18, 2024

DrahtBot commented at 10:36 PM on November 18, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33166175203

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label Needs rebase on Nov 18, 2024

DrahtBot removed the label CI failed on Nov 18, 2024

hodlinator force-pushed on Nov 19, 2024

l0rinc commented at 11:33 AM on November 19, 2024: contributor

I'd like to review this, but first, I'd like to understand why it is still in draft form.

hodlinator marked this as ready for review on Nov 19, 2024

hodlinator commented at 11:45 AM on November 19, 2024: contributor

I'd like to review this, but first, I'd like to understand why it is still in draft form.

Had a linefeed issue on Windows in the previous push (and was in a WIP state over the weekend, before that).. but pretty sure it's ready to go.

in src/bitcoin-cli.cpp:97 in e8a2054edc outdated

  93 | @@ -94,7 +94,7 @@ static void SetupCliArgs(ArgsManager& argsman)
  94 |      argsman.AddArg("-netinfo", strprintf("Get network peer connection information from the remote server. An optional argument from 0 to %d can be passed for different peers listings (default: 0). If a non-zero value is passed, an additional \"outonly\" (or \"o\") argument can be passed to see outbound peers only. Pass \"help\" (or \"h\") for detailed help documentation.", NETINFO_MAX_LEVEL), ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
  95 |  
  96 |      SetupChainParamsBaseOptions(argsman);
  97 | -    argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
  98 | +    argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never. Only applies to the output of -getinfo.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);

l0rinc commented at 5:24 PM on November 20, 2024:

slightly unrelated: Checking the different options, it's not always trivial to assess what negation would mean, but

    if (gArgs.IsArgSet("-color")) {
        const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
        if (color == "always") {
            should_colorize = true;
        } else if (color == "never") {
            should_colorize = false;
        } else if (color != "auto") {
            throw std::runtime_error("Invalid value for -color option. Valid values: always, auto, never.");
        }
    }

Indicated that a -nocolor == -color=never could make sense. Or is the ArgsManager::DISALLOW_NEGATION meant to discourage people from requesting that :D?

hodlinator commented at 9:51 PM on November 20, 2024:

Or is the ArgsManager::DISALLOW_NEGATION meant to discourage people from requesting that :D?

Indeed, although I agree with -nocolor potentially equaling -color=never, it is flagged as forbidden and therefore the process is stopped before reaching the quoted code. I'm a bit hesitant to overrule the prior intent of disallowing it.

ryanofsky commented at 11:52 AM on November 21, 2024:

re: #31212 (review)

I could be missing something but it seems pretty obvious to me that -nocolor should be the same as -color=never. I don't think this PR should change that, but it might make sense to have a PR in the future that removes unjustified uses of disallow_negation so options so parsing is more uniform and less surprising. It could be the same PR that adds disallow_negation to prevent -noblocksdir as suggested #31212 (review)

in src/bitcoin-cli.cpp:1 in f02c4c020c outdated

l0rinc commented at 5:37 PM on November 20, 2024:

in scripted-diff: Avoid printing version information for -noversion:

Nit: the replacement doesn't need any escapes, if you edit again, consider simplifying:

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/\b(gArgs|args)\.IsArgSet\("-version"\)/\1.GetBoolArg("-version", false)/g' $(git grep -l '-version')
-END VERIFY SCRIPT-

l0rinc commented at 6:11 PM on November 20, 2024:

in test: -norpccookiefile:

What's the reason for separating the test from the impl? I would understand if this were before the fix (we could check that it reproduces the desired behavior and fails - but we can't do that here since all tests should fail)

l0rinc commented at 6:12 PM on November 20, 2024:

nit in args: Properly support -noconf and catch directories - typo in commit message

l0rinc commented at 6:19 PM on November 20, 2024:

in test: Add tests for directory in place of config files, -noconf:

Same as with the other test, I think it belongs with the related modification

hodlinator commented at 10:19 PM on November 20, 2024:

I fail to see the typo, mind pointing it out?

hodlinator commented at 11:01 PM on November 20, 2024:

I want it to be easy for reviewers to drop the impl commits or re-order and run what is in the test commits to verify that I'm actually fixing issues.

hodlinator commented at 11:01 PM on November 20, 2024:

#31212 (review)

l0rinc commented at 9:36 AM on November 21, 2024:

I usually soft reset and revert the non-test changes to make sure the test fails for the proper reasons - even simpler usually than an interactive rebase. I still think the change and test should be in the same commit to give proper context.

hodlinator commented at 10:14 PM on November 21, 2024:

What would you use as category at the beginning of commit messages when implementations are merged with tests? - Feels logical to have for example net: ... commit + test net: ... commit, you would just have the first message and merge them together?

If the pre-existing tests were to be broken by changing the implementations, I can understand changing them in the same commit, but AFAIK that is not the case in this PR.

My impression is that this project encourages separate commits for tests, see for example: https://github.com/bitcoin/bitcoin/pull/30239/commits

ryanofsky commented at 2:03 AM on November 22, 2024:

re: #31212 (review)

As long as you are only adding the test coverage after fixing the bug (like this PR) I think the question of whether the test changes are in the same commit or a separate one are basically a question of personal style, and different developers will have different preferences.

But a much better practice IMO is to add the new test coverage in a separate commit before fixing the bug. Then the bugfix commit will include test changes, but the test changes will be small, and will usually make it very clear what the bug is, how behavior is changing, and how good the new coverage is.

l0rinc commented at 7:07 AM on November 22, 2024:

Given that currently the CI checks every commit, do we have a way to specifying to the CI that the test commit is meant to fail, i.e. demonstrates the need for the upcoming fix? Or would we add an ignored test before the fix and just unignore it with the fix? I still think that in the current setup it makes more sense to commit them as a singe work-unit - fixes and testing and benching and documenting them are not extras, they're part of the same work unit.

l0rinc commented at 9:01 AM on November 22, 2024:

being opened to the ".bitcoin"-directory (not a file)) - double parenthesis close.

And if we're here already, the other one is missing an of in to track initialization *of* HTTP RPC authentication

ryanofsky commented at 11:18 AM on November 22, 2024:

re: #31212 (review)

The idea is not to add an ignored test, but to add real test coverage. For example if there is a buggy add() function that thinks 2 + 2 = 5, the ideal way to fix it is to first add a test commit that asserts add(2, 2) == 5 with a comment noting the add function is buggy. After this, a separate commit should fix the bug and update the test to assert add(2, 2) == 4. Advantages of this approach:

Clearly shows what the bug is, and makes the behavior change obvious no matter how complicated the changes inside the add() function are.
Minimizes the size of the bugfix commit. Instead of containing large amounts of test setup code, all the test setup is added previously and test changes are minimal.
Makes it obvious how good the coverage is. If bugfix commit includes 50 lines of new test code, this doesn't tell you whether new tests actually cover the bug or how they are connected to it. But if the test code is added in a previous commit and passes, then is changed by the bugfix and still passes, you can see exactly how the tests are connected to the bug, how good the coverage is, and what coverage may be missing.

For examples of this you can see https://github.com/bitcoin/bitcoin/pull/30679/commits/d38e3aed89eea665399eef07f5c3b64b14d4f586, https://github.com/bitcoin/bitcoin/pull/27101/commits/bf1a1f1662427fbf1a43bb951364eface469bdb7, https://github.com/bitcoin/bitcoin/pull/27101/commits/2ca1460ae3a7217eaa8c5972515bf622bedadfce which are all fixing bugs and updating test code added in prior commits. If the test code had been added in the same commits or in subsequent commits, you would have a much less clear idea of what the bugfixes were actually doing or whether they had meaningful test coverage.

l0rinc commented at 12:24 PM on November 22, 2024:

I'm not against that, my concern is just that reproducing old behavior before the fix isn't very useful activity - it's a lot more useful to see that a test fails before the fix with the correct error (i.e TDD), but that's not really possible currently - unless we add something like https://github.com/spockframework/spock/blob/master/spock-core/src/main/java/spock/lang/PendingFeature.java#L13-L17 (where we invert the test, making sure it fails (that's when the test is green) and remove the annotation only in the fix commit).

ryanofsky commented at 12:50 PM on November 22, 2024:

I'm not against that, my concern is just that reproducing old behavior before the fix isn't very useful activity

Strongly disagree :). In 2ca1460ae3a7217eaa8c5972515bf622bedadfce if tests had not been added before that commit checking for incorrect HTTP error codes, you would not know from looking at the change what the behavior was before the bugfix, or how new tests were connected to it. It is very useful to have test coverage for bugs before they are fixed.

it's a lot more useful to see that a test fails before the fix with the correct error

That's another benefit this approach provides. If the bugfix commit show a line of test code changing from assert_equal(status, 200) to assert_equal(status, 400) you know the check would have failed without the bugfix because status variable cannot be equal to two values at the same time. By contrast if the bugfix commit just included an entirely new assert_equal(status, 400) check, it could tell you you nothing about whether the check would fail without the bugfix. It would only be possible determine that from manual testing or from a theoretical CI extension which our CI system does not implement.

Testing for bugs before fixing them is strictly better approach for making changes more understandable and providing more information about test coverage without requiring manual testing or theoretical CI extensions.

l0rinc commented at 1:08 PM on November 22, 2024:

if tests had not been added before that commit checking for incorrect HTTP error codes, you would not know from looking at the change what the behavior was before the bugfix

The test would fail before the fix with the exact reason, not sure we're in a disagreement here.

If the bugfix commit show a line of test code changing from [...] if the bugfix commit just included an entirely new [...]

But my point was exactly that if there's a failing test in the commit before the fix, which reproduces the error, you're not allowed to change the test in the commit that does the fix - to prove that you've solved the problem and not just castrated the test. By modifying both the test and fix in the same commit, we don't know how the test would have behaved without the fix (or whether the previous test would pass with the fix). So the only reliable solution I see is adding failing tests before the fixing commit and not modifying them in the fix commit to make sure the exact same checks were done on both versions. Same with benchmarks, I don't trust the performance comparison if the benchmarks have also changed when the production code was changed.

maflcko commented at 1:14 PM on November 22, 2024:

if tests had not been added before that commit checking for incorrect HTTP error codes, you would not know from looking at the change what the behavior was before the bugfix

The test would fail before the fix with the exact reason, not sure we're in a disagreement here.

The test would be reproducing the bug, thus pass. The next commit would fix the pre-existing bug in the code and the test, and would thus pass as well.

l0rinc commented at 1:30 PM on November 22, 2024:

That's one way to do it, but in that case it's possible that the test passes before the fix as well (had many such reviews), which would indicate that we haven't actually reproduced the problem, therefore the fix might not do what it claims. Only by having the exact same test fail before the fix and pass after (without any change to the test) can we be sure the fix had an effect on the test.

ryanofsky commented at 3:24 PM on November 22, 2024:

Apologies l0rinc. I think we are probably just misunderstanding each other. All the checks you are suggesting are possible with the approach I'm suggesting, and it should only make them easier to implement and understand. And I might not understand what approach you prefer since your first comment #31212 (review) seems to want new tests to be in the same commit as the bugfix, but the last one #31212 (review) seems to want test and nontest changes to be separated (unclear if that means separate commits).

In any case, we did stray off topic for this PR. For this PR I think current test commits are fine, and adding test coverage before making bugfixes would be slightly better but is not very important because the changes in behavior here are not very complicated and not hard to see from the code. I think having squashing test and code commits would also be fine, but not a too meaningful change either way.

l0rinc commented at 9:07 PM on November 22, 2024:

Long story short: I would find it suspicious if the test of a specific fix passes without the fix. The cleanest way I found to make sure this isn't the case is to add the test commit before the fix commit and make sure CI fails if the test passes without the fix (see mentioned Spock PendingFeature). But since we don't have that here, I would prefer having the test in the same work unit as the fix, to provide the necessary context for the current fix. Is this still controversial?

ryanofsky commented at 11:57 PM on November 22, 2024:

re: #31212 (review)

But since we don't have that here, I would prefer having the test in the same work unit as the fix, to provide the necessary context for the current fix. Is this still controversial?

I think it's a perfectly reasonable suggestion. To paraphrase, you are saying that when code changes are made in a commit without test changes (which is how the PR is currently structured, though not what I was suggesting) then it is harder for reviewers and git blame users to determine what the associated tests are. Also including test changes in code commits makes it easier to write an automated checker that could determine whether the test changes would have passed without the code changes, and would indicate how meaningful the test coverage is.

These are reasonable things to want, just like having smaller more focused commits is a reasonable thing to want. So your approach and hodlinators's approach are both fine choices AFAICT. The only thing I'd add is that my suggestion reduces the tradeoff between these approach and gives the best of both worlds: including tests in the same commits as code changes, but minimizing the test diffs there, and additionally making it more obvious what differences in old and new behavior are.

hodlinator commented at 4:40 PM on November 28, 2024:

To me this is a good pattern:

Document former correct behavior by adding tests.
Change implementation and tests to conform to new behavior.

However, if 1) is about documenting buggy behavior, committing a test for that in a way that makes them succeed feels off to me. In that case I would prefer adding tests that prove failure with some special annotation in the commit message for CI to expect failure like what @l0rinc linked here (Spock).

I'm probably biased from having worked in other workflows which encouraged less granular commits.

l0rinc commented at 11:24 AM on November 29, 2024:

in commit message args: Properly support -noconf ...:

have been updated earlier in this PR (https://github.com/bitcoin/bitcoin/commit/bffd92f00f5bfbb5a622d0bd20bfeed9f8b10928 and https://github.com/bitcoin/bitcoin/commit/f5a14f40e5e2ad3504d91ef03929898b3cab85b4).

The commits have changed since, but instead of updating, I would rather recommend removing hashes from the commit message

hodlinator commented at 10:32 AM on December 3, 2024:

Cheers, replaced hashes with commit message titles.

in src/bitcoin-tx.cpp:108 in f02c4c020c outdated

 104 | @@ -105,11 +105,11 @@ static int AppInitRawTx(int argc, char* argv[])
 105 |  
 106 |      fCreateBlank = gArgs.GetBoolArg("-create", false);
 107 |  
 108 | -    if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
 109 | +    if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {

l0rinc commented at 5:40 PM on November 20, 2024:

nit: since we're using all values later (here and in a few other examples below), we might as well leave this as IsArgSet at the top - it would simplify the PR diff, but make the scripted diff more complicated. Will leave it for you to decide, don't have strong feelings either way.

hodlinator commented at 9:58 PM on November 20, 2024:

IsArgSet would be true when the user passes -noversion, so the if-block would be entered, which is intended to be avoided in this PR.

l0rinc commented at 9:32 AM on November 21, 2024:

Hmmm, I though -noversion was meant to be exactly the same in each scenario as -version=false?

hodlinator commented at 9:57 PM on November 21, 2024:

-version=true and -version=false are both interpreted as -version=0 (there is no special parsing for booleans expressed as strings). -version=0 is equivalent to -noversion or -noversion=1.

IsArgSet() returns true if the user has set a given arg to any value (including negation), through command line or settings-file.

l0rinc commented at 9:04 AM on November 22, 2024:

The part I'm not sure about is checking gArgs.GetBoolArg("-version", false) twice - since the inner check has an else, which is a bit confusing, since that can only happen if argc < 2 || HelpRequested(gArgs) are true - seems like the condition can be simplified, especially since if (argc < 2) { is rechecked later. (please verify for other such conditions, not necessarily this one)

ryanofsky commented at 11:39 AM on November 22, 2024:

re: #31212 (review)

I think the idea here is just for -version to take precedence over -help if both are specified. I don't know if this is the best behavior, but I don't see a reason to change in a commit that is just trying to handle -version=0 and -noversion more sensibly.

in src/init.cpp:488 in 4ccc53aff7 outdated

 482 | @@ -483,7 +483,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
 483 |      argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Disables automatic broadcast and rebroadcast of transactions, unless the source peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 484 |      argsman.AddArg("-coinstatsindex", strprintf("Maintain coinstats index used by the gettxoutsetinfo RPC (default: %u)", DEFAULT_COINSTATSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 485 |      argsman.AddArg("-conf=<file>", strprintf("Specify path to read-only configuration file. Relative paths will be prefixed by datadir location (only useable from command line, not configuration file) (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 486 | -    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 487 | +    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);

l0rinc commented at 5:49 PM on November 20, 2024:

Do these same arguments apply to blocksdir & walletdir?

hodlinator commented at 10:31 PM on November 20, 2024:

Potentially yes, but I'm trying to keep the scope of the PR to the args I discovered here: #16545#pullrequestreview-2316714013

Open to exploring it in a follow-up!

ryanofsky commented at 12:16 PM on November 21, 2024:

re: #31212 (review)

I think this makes sense for -blocksdir and could make sense for -walletdir. But if the datadir was initiially created by an older version of Bitcoin and since upgraded it won't actually have a wallets/ subdirectory and will just store wallets in the datadir directly. So it could be reasonable to support -nowalletdir or -walletdir="" as a ways to request the same behavior when opening an old datadir. However, current wallet -walletdir code looks like a mess and I don't think this specifying these actually does anything useful currently.

in src/httprpc.cpp:304 in 80fcd0473a outdated

 302 |          auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
 303 |          if (cookie_perms_arg) {
 304 |              auto perm_opt = InterpretPermString(*cookie_perms_arg);
 305 |              if (!perm_opt) {
 306 | -                LogInfo("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.\n", *cookie_perms_arg);
 307 | +                LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.\n", *cookie_perms_arg);

l0rinc commented at 5:57 PM on November 20, 2024:

Since you edit these already, could you please check if the trailing newline is still needed?

in src/httprpc.cpp:152 in 80fcd0473a outdated

 148 | @@ -148,7 +149,8 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
 149 |          strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':'));
 150 |  
 151 |      //Check if authorized under single-user field
 152 | -    if (TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
 153 | +    // strRPCUserColonPass is empty when -norpccookiefile is specified

l0rinc commented at 5:59 PM on November 20, 2024:

super-nit: formatting seems a bit off (whitespace and missing trailing full stop in first line)

hodlinator commented at 10:58 PM on November 20, 2024:

Yeah, was itching to do that too, thanks for a second vote. :)

in src/httprpc.cpp:310 in 80fcd0473a outdated

 310 |              cookie_perms = *perm_opt;
 311 |          }
 312 |  
 313 | -        if (!GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)) {
 314 | -            return false;
 315 | +        if (std::optional<bool> result{GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)}) {

l0rinc commented at 6:03 PM on November 20, 2024:

You gotta' love these 3-state-booleans :)

nit: To reduce the noise, consider simplified to either

        if (std::optional result{GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)}) {

or

        if (auto result{GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)}) {

hodlinator commented at 10:34 PM on November 20, 2024:

I prefer documenting the type.

l0rinc commented at 9:05 AM on November 22, 2024:

it's quite noisy in this case, but don't have string feelings about it :)

in src/httprpc.cpp:317 in 80fcd0473a outdated

 321 | +            }
 322 | +        } else {
 323 | +            LogInfo("RPC authentication cookie file generation is disabled.");
 324 |          }
 325 |      } else {
 326 |          LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");

l0rinc commented at 6:05 PM on November 20, 2024:

this seems like a slight behavior change: previously when GenerateAuthCookie returned true, this was logged, now it seems to me it may not be (haven't tried the code)

hodlinator commented at 10:45 PM on November 20, 2024:

You mean

LogInfo("Using random cookie authentication.\n");

was run independently of whether GenerateAuthCookie returned true or false, but now it may not be? It seemed wrong to me so I made it only happen when the cookie was actually generated.

If you mean that

LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");

was emitted when GenerateAuthCookie was true, I think you may be mixing up some braces/indentation.

l0rinc commented at 10:33 AM on November 22, 2024:

Yes, I was confused by indentation, resolve the comment por favor

in test/functional/interface_bitcoin_cli.py:168 in d412946cc2 outdated

 163 | @@ -164,6 +164,9 @@ def run_test(self):
 164 |          self.log.info("Test connecting with non-existing RPC cookie file")
 165 |          assert_raises_process_error(1, "Could not locate RPC credentials", self.nodes[0].cli('-rpccookiefile=does-not-exist', '-rpcpassword=').echo)
 166 |  
 167 | +        self.log.info("Test connecting without RPC cookie file and with password arg")
 168 | +        assert_equal(BLOCKS, self.nodes[0].cli('-norpccookiefile', '-rpcuser=' + user, '-rpcpassword=' + password).getblockcount())

l0rinc commented at 6:08 PM on November 20, 2024:

nit: we might as well use f-strings here:

        assert_equal(BLOCKS, self.nodes[0].cli('-norpccookiefile', f'-rpcuser={user}', f'-rpcpassword={password}').getblockcount())

in test/functional/rpc_users.py:135 in d412946cc2 outdated

 130 | +
 131 | +        self.log.info('Starting with -norpccookiefile')
 132 | +        # Start, but don't wait for RPC connection as TestNode.wait_for_rpc_connection() requires the cookie.
 133 | +        with self.nodes[0].busy_wait_for_debug_log([b'init message: Done loading']):
 134 | +            # Pass -rpccookieperms for bonus code coverage of warning
 135 | +            self.nodes[0].start(extra_args=[f"-norpccookiefile", "-rpccookieperms=owner"])

l0rinc commented at 6:09 PM on November 20, 2024:

nit: we don't actually need f-string here:

            self.nodes[0].start(extra_args=["-norpccookiefile", "-rpccookieperms=owner"])

in src/common/config.cpp:140 in 1607d8d1a9 outdated

 140 |  
 141 | -    // not ok to have a config file specified that cannot be opened
 142 | -    if (IsArgSet("-conf") && !stream.good()) {
 143 | -        error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
 144 | -        return false;
 145 | +        // not ok to have a config file specified that cannot be opened

l0rinc commented at 6:15 PM on November 20, 2024:

nit: double negation makes it more complicated than need be. If you insist on having a comment:

        // Config file must be accessible

hodlinator commented at 11:08 PM on November 20, 2024:

Good point about double negation. Changed to:

// If the file is explicitly specified, it must be readable

since the distinction is important.

in src/init/common.cpp:129 in 1607d8d1a9 outdated

 124 | @@ -125,8 +125,10 @@ bool StartLogging(const ArgsManager& args)
 125 |      if (fs::exists(config_file_path)) {
 126 |          LogPrintf("Config file: %s\n", fs::PathToString(config_file_path));
 127 |      } else if (args.IsArgSet("-conf")) {
 128 | -        // Warn if no conf file exists at path provided by user
 129 | -        InitWarning(strprintf(_("The specified config file %s does not exist"), fs::PathToString(config_file_path)));
 130 | +        if (!args.IsArgNegated("-conf")) {
 131 | +            // Warn if no conf file exists at path provided by user

l0rinc commented at 6:18 PM on November 20, 2024:

not sure what this comments adds that the code doesn't already explain

l0rinc commented at 6:25 PM on November 20, 2024: contributor

I love that you search for these inconsistencies and attack them head on. +1 for adding representative tests (which I didn't review in detail yet, will do in the next round of reviews).

scripted-diff: Avoid printing version information for -noversion

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/\b(gArgs|args)\.IsArgSet\("-version"\)/\1.GetBoolArg("-version", false)/g' $(git grep -l '-version')
-END VERIFY SCRIPT-

6ff9662760

args: Disallow -nodatadir

Does not make sense to run without a datadir.

Prior to this change it would be interpreted as a mix of unset and as a relative path of "0".

12f8d848fd

args: Support -nopid bffd92f00f

hodlinator force-pushed on Nov 20, 2024

ryanofsky approved

ryanofsky commented at 3:11 AM on November 22, 2024: contributor

Code review ACK 2bf7e59776fcfb2c8a477cf2abb59606f7b4aacc. This is a nice improvement over status quo, but I'm pretty sure two of the commits 21bc3a3657fea5ed44172963639ee8ca5cd8e46c and 879a8d3a6820a86e6faa8d91f92d0cf374700fb8 are more complicated than they need to be and less robust than they could be.

I posted a simplified branch (diff) and would recommend replacing:

21bc3a3657fea5ed44172963639ee8ca5cd8e46c with 43ec65a6bd5786a47fba91eec96cb68232104904
879a8d3a6820a86e6faa8d91f92d0cf374700fb8 with 3a856c86685a5ab6f3cb78b17cb52cbb1b9ad95b
2bf7e59776fcfb2c8a477cf2abb59606f7b4aacc with ebadf80f2fed1fc3513e76a48696e4f561ce577d (to fix tests)

Issues with current commit "args: Support -norpccookiefile for bitcoind and bitcoin-cli" (21bc3a3657fea5ed44172963639ee8ca5cd8e46c) are:

Tristate optional<bool> is unnecessary when the function can just return true in successful states and false in failed ones.
New "Unable to set specified permissions on cookie file as we aren't generating one" warning doesn't seem like a good idea. Warning about valid but unused options is not consistent with how most other options are handled, and having a warning for this niche case makes the code harder to follow. Also the warning is not shown reliably, such as when when -rpcpassword is specified and -rpccookieperms is also ignored.
New g_http_rpc_auth_initialized variable is unnecessary. Its only purpose to preserve an old "Belt-and-suspenders" check but there is nothing particularly useful about that check. The check made sense previously to detect the formerly invalid state of the "user:pass" string being empty. But now that -norpccookiefile works, it makes sense for that string to be empty, and doesn't make sense to add a new invalid state just for the sake of having some invalid state. At least I can think of more ways adding an unnecessary invalid state could cause bugs than it could prevent them.
New "RPC authentication cookie file reading is disabled" debug log in bitcoin-cli code doesn't seem useful. AFAICT bitcoin-cli doesn't show debug messages and this message doesn't add much information beyond what the more actionable bitcoin-cli error message already conveys. Having this doesn't seem worth hit to complexity & readability of the GetAuthCookie function, and if the error message is unclear it would be better to just improve the error message.
Very subjective, but I think the unrelated code cleanups in this commit make it hard to follow and are very distracting. Would be much happier to see them dropped or moved to a refactoring commit.

Issues with current commit "args: Properly support -noconf and catch directories" (879a8d3a6820a86e6faa8d91f92d0cf374700fb8) are:

The fs::is_directory checks seem to be covering up a problem rather than fixing it. AFAICT the problem with ReadConfigFiles is that is reading all the config files without bothering to check that the reads are successful. If the code changed to check ifstream::eof and verify that the files were read successfully it would be simpler and more reliable.
The "included config file "%s" is a directory." error formatting is buggy. It references the path of the main config file not the included file.
The "data directory contains a config file which is ignored because -noconf is specified error" seems to introduce an inconsistency with how -noconf is handled. I would expect -noconf to always ignore the configuration file, not sometimes ignore it and other times show an error an error about it, and for which of these happens to depend on how the data directory was located.
The new IsArgNegated call in StartLogging seems to be modifying dead code. Or I don't see how the "The specified config file %s does not exist" warning would be triggered when there is already an error about that. Seems better to just drop the redundant code here.

DrahtBot requested review from rkrux on Nov 22, 2024

DrahtBot requested review from l0rinc on Nov 22, 2024

in test/functional/rpc_users.py:196 in 2bf7e59776 outdated

 188 | @@ -166,11 +189,18 @@ def run_test(self):
 189 |          self.stop_node(0)
 190 |  
 191 |          self.log.info('Check that failure to write cookie file will abort the node gracefully')
 192 | -        (self.nodes[0].chain_path / ".cookie.tmp").mkdir()
 193 | +        node0_cookie_file_path =     self.nodes[0].chain_path / ".cookie"
 194 | +        node0_cookie_file_path_tmp = self.nodes[0].chain_path / ".cookie.tmp"
 195 | +        node0_cookie_file_path_tmp.mkdir()
 196 |          self.nodes[0].assert_start_raises_init_error(expected_msg=init_error)
 197 | +        node0_cookie_file_path_tmp.rmdir()

l0rinc commented at 10:11 AM on November 22, 2024:

will the node0_cookie_file_path_tmp file be kept when assertion fails on purpose? If not, you could wrap it in a try/finally (deleting regardless of the assertion's outcome)

hodlinator commented at 12:00 AM on November 24, 2024:

Tests are running in a scratch directory - if part of the test fails, we don't worry about leaving some files behind.

l0rinc commented at 8:08 AM on November 24, 2024:

Can we remove the cleanup in that case?

hodlinator commented at 9:27 AM on November 25, 2024:

The cleanup is still there so that other tests following it in the Python script are not impacted.

l0rinc commented at 10:22 AM on November 29, 2024:

so do I understand it correctly that it's fine for other tests to be affected when this one already fails, leaving the directory?

in src/rpc/request.h:27 in 2bf7e59776 outdated

  23 | @@ -24,7 +24,7 @@ UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue
  24 |  UniValue JSONRPCError(int code, const std::string& message);
  25 |  
  26 |  /** Generate a new RPC authentication cookie and write it to disk */
  27 | -bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms=std::nullopt);
  28 | +std::optional<bool> GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms=std::nullopt);

l0rinc commented at 10:14 AM on November 22, 2024:

as mentioned before, I find this tri-state-boolean quite confusing - if 3 states are indeed needed, maybe it's hiding a different structure such as a variant or enum or optional<Error>

hodlinator commented at 4:19 PM on November 28, 2024:

Reverted in later pushes.

in src/rpc/request.cpp:155 in 2bf7e59776 outdated

 149 | @@ -142,6 +150,10 @@ bool GetAuthCookie(std::string *cookie_out)
 150 |      std::ifstream file;
 151 |      std::string cookie;
 152 |      fs::path filepath = GetAuthCookieFile();
 153 | +    if (filepath.empty()) {
 154 | +        LogDebug(BCLog::RPC, "RPC authentication cookie file reading is disabled.");
 155 | +        return false;

l0rinc commented at 10:16 AM on November 22, 2024:

why is this false and not an empty optional? As mentioned before, I don't really see the line

hodlinator commented at 8:10 PM on November 28, 2024:

GetAuthCookieFile() internally uses GetPathArg() and follows it's pattern (predating this PR): https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3bbac641f05d490fd5c984156b33/src/common/args.cpp#L272-L274

But might change to optional if you insist. Tried out changing it in 01fa9d41d12a68f88f6251ae43234dd250a53bd5. Works fairly well, even uncovered a minor pre-existing bug in that if fs::permissions fails, we currently print out the wrong filename. With that bug it feels like it could arguably be its own micro-PR.

l0rinc commented at 11:36 AM on November 29, 2024:

I'm fine with doing it in a different PR - will leave it up to you

in src/qt/bitcoin.cpp:586 in 2bf7e59776 outdated

 581 | @@ -582,8 +582,8 @@ int GuiMain(int argc, char* argv[])
 582 |  
 583 |      // Show help message immediately after parsing command-line options (for "-lang") and setting locale,
 584 |      // but before showing splash screen.
 585 | -    if (HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
 586 | -        HelpMessageDialog help(nullptr, gArgs.IsArgSet("-version"));
 587 | +    if (HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {
 588 | +        HelpMessageDialog help(nullptr, gArgs.GetBoolArg("-version", false));

l0rinc commented at 10:19 AM on November 22, 2024:

What's the reason for not wanting an about window regardless of the -version flag's value? Doesn't gArgs.IsArgSet("-version") make more sense here?

ryanofsky commented at 12:02 PM on November 22, 2024:

re: #31212 (review)

What's the reason for not wanting an about window regardless of the -version flag's value? Doesn't gArgs.IsArgSet("-version") make more sense here?

The idea is for -noversion and -version=0 to be the same as not specifying a -version argument. -version is just a binary option, it should choose between 2 behaviors, not 3

l0rinc commented at 12:26 PM on November 22, 2024:

what about -noversion=0 :p

There has to be a better way, this is so confusing :/

in src/init/common.cpp:132 in 2bf7e59776 outdated

 124 | @@ -125,8 +125,9 @@ bool StartLogging(const ArgsManager& args)
 125 |      if (fs::exists(config_file_path)) {
 126 |          LogPrintf("Config file: %s\n", fs::PathToString(config_file_path));
 127 |      } else if (args.IsArgSet("-conf")) {

l0rinc commented at 10:32 AM on November 22, 2024:

given that checking the args is not for free (triggers locks and merges from multiple sources), maybe we could introduce a IsArgEnabled which gets rid of this confusing combination (e.g. where -noconf=0 would be set and not negated, I think)

ryanofsky commented at 11:59 AM on November 22, 2024:

re: #31212 (review)

given that checking the args is not for free (triggers locks and merges from multiple sources), maybe we could introduce a IsArgEnabled which gets rid of this confusing combination (e.g. where -noconf=0 would be set and not negated, I think)

#31260 gets away from this error-prone repeated retrieval of settings by forcing the type of every setting to be declared up front, and only providing a single Get accessor for it, not separate GetArg/GetArgs/GetIntArg/GetBoolArg/IsArgSet/IsArgNegated accessors. It also allows default setting values to be declared, so in most cases there is never a need to explicitly check whether a setting has been "set" or not. But in cases where settings do not have simple default values, they can be declared with std::optional type and handle nullopt cases.

DrahtBot requested review from l0rinc on Nov 22, 2024

hodlinator force-pushed on Nov 25, 2024

hodlinator commented at 10:42 AM on November 25, 2024: contributor

Re: #31212#pullrequestreview-2451143073

21bc3a3657fea5ed44172963639ee8ca5cd8e46c

Very subjective, but I think the unrelated code cleanups in this commit make it hard to follow and are very distracting. Would be much happier to see them dropped or moved to a refactoring commit.

Broke out 215be3dfba90a6d5b2b37f0f94bcb621dd3d4a9a from commit. Also broke out eb23f55ae2470bd2376307ca293feb0be972de50 from test-commit.

879a8d3a6820a86e6faa8d91f92d0cf374700fb8

The new IsArgNegated call in StartLogging seems to be modifying dead code. Or I don't see how the "The specified config file %s does not exist" warning would be triggered when there is already an error about that. Seems better to just drop the redundant code here.

It is true that bitcoind.cpp calls into ArgsManager::ReadConfigFiles() before init::StartLogging(), but I prefer keeping it for belt & suspenders reasons, maybe the implicit initializing test inside the call to args.GetConfigFilePath() is changed and a future client of kernel reverses the order compared to bitcoind. Also realized we should add special output for the -noconf case to make things clear to readers of the log.

hodlinator force-pushed on Nov 25, 2024

DrahtBot added the label CI failed on Nov 25, 2024

DrahtBot commented at 10:44 AM on November 25, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33470556641

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

hodlinator force-pushed on Nov 25, 2024

hodlinator commented at 1:34 PM on November 25, 2024: contributor

Can understand the checks failing on MacOS or Windows, but surprised that what I assume to be Linux on Cirrus CI is also failing (https://cirrus-ci.com/task/5004562262654976?logs=ci#L172). Will move to Windows and see how it behaves there. Might have to revert to explicitly checking is_directory().

Edit: Could not reproduce the failure on my Windows install.

hodlinator force-pushed on Nov 25, 2024

hodlinator force-pushed on Nov 26, 2024

hodlinator force-pushed on Nov 28, 2024

hodlinator commented at 1:51 PM on November 28, 2024: contributor

Pushed ~51e34ca022d6704dcf1fa955e3764a97d91bbba8~ c972e691d937f49d7d3f44d2b19b4035c6e67aaf

Unfortunately I wasn't able to use the simplification suggested by @ryanofsky: https://github.com/bitcoin/bitcoin/blob/3a856c86685a5ab6f3cb78b17cb52cbb1b9ad95b/src/common/config.cpp#L138-L141

When opening a directory with an ifstream, Windows CI would return false for is_open(): https://github.com/hodlinator/bitcoin/actions/runs/12052081230/job/33604594555#step:12:3324

When opening a directory with an ifstream on MacOS CI, is_open() would return true, but the EOF-bit would also be set: https://github.com/hodlinator/bitcoin/actions/runs/12052081230/job/33604593738#step:7:6158

So reverted to using is_directory() as it is more robust and clear.

+ Commit 86c6b1e0744d8d767fe75ddaa9fc9dd3643e15aa - Improves combine_logs.py which helped me track down that I had forgotten to pass -regtest in a couple of calls to assert_start_raises_init_error() (creating mainnet debug.logs).

+ Commit eb2d96bb66f109e610ee44cf3fed5c6701e3b8e3 - Refactors feature_config_args.py towards starting the node at beginning of test functions and stopping it at the end to reduce implicit dependencies.

Broke up -noconf and directory checking changes and tests into separate commits.

Simplified warning about ignored config file in common/init.cpp.

Added special output for config file being a directory in init/common.cpp.

hodlinator force-pushed on Nov 28, 2024

DrahtBot removed the label CI failed on Nov 28, 2024

in src/init/common.cpp:126 in c972e691d9 outdated

 122 | @@ -122,10 +123,13 @@ bool StartLogging(const ArgsManager& args)
 123 |  
 124 |      // Only log conf file usage message if conf file actually exists.
 125 |      fs::path config_file_path = args.GetConfigFilePath();
 126 | -    if (fs::exists(config_file_path)) {
 127 | +    if (args.IsArgSet("-conf") && args.IsArgNegated("-conf")) {

l0rinc commented at 10:36 AM on November 29, 2024:

Given that IsArgNegated returns GetSetting(strArg).isFalse() where isFalse returns (typ == VBOOL) && (val != "1") which contradicts: bool isNull() const { return (typ == VNULL); } in IsArgSet returning !GetSetting(strArg).isNull(), it seems to me we can simplify this to:

    if (args.IsArgNegated("-conf"))

hodlinator commented at 10:06 AM on December 3, 2024:

Thanks! Fixed in latest push.

in src/init/common.cpp:133 in c972e691d9 outdated

 130 | +        LogWarning("Config file: %s (is directory, not file)\n", fs::PathToString(config_file_path));
 131 | +    } else if (fs::exists(config_file_path)) {
 132 |          LogPrintf("Config file: %s\n", fs::PathToString(config_file_path));
 133 |      } else if (args.IsArgSet("-conf")) {
 134 | -        // Warn if no conf file exists at path provided by user
 135 |          InitWarning(strprintf(_("The specified config file %s does not exist"), fs::PathToString(config_file_path)));

l0rinc commented at 10:37 AM on November 29, 2024:

I find these kind of messages to be more readable than Config file: %s (is directory, not file) or "Config file: <disabled>

hodlinator commented at 10:09 AM on December 3, 2024:

I prefer having a "Config file: "-prefix to grep for in logs, so following that pattern for added messages here in this PR (but not going to touch the InitWarning as it might be shown in the GUI).

in src/init/common.cpp:129 in c972e691d9 outdated

 122 | @@ -122,10 +123,13 @@ bool StartLogging(const ArgsManager& args)
 123 |  
 124 |      // Only log conf file usage message if conf file actually exists.
 125 |      fs::path config_file_path = args.GetConfigFilePath();
 126 | -    if (fs::exists(config_file_path)) {
 127 | +    if (args.IsArgSet("-conf") && args.IsArgNegated("-conf")) {
 128 | +        LogInfo("Config file: <disabled>");
 129 | +    } else if (fs::is_directory(config_file_path)) {
 130 | +        LogWarning("Config file: %s (is directory, not file)\n", fs::PathToString(config_file_path));

l0rinc commented at 10:38 AM on November 29, 2024:

super-nit: redundant \n (and as mentioned, if we don't need to parse the output, I'd prefer a more user-friendly output)

in src/rpc/request.cpp:126 in c972e691d9 outdated

 123 |      file.close();
 124 |  
 125 |      fs::path filepath = GetAuthCookieFile(false);
 126 |      if (!RenameOver(filepath_tmp, filepath)) {
 127 | -        LogInfo("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
 128 | +        LogWarning("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));

l0rinc commented at 10:40 AM on November 29, 2024:

👍 for changing return false logging to warn! nit: in other cases we've quoted the file paths

hodlinator commented at 10:11 AM on December 3, 2024:

Would rather not change to quote paths in all log messages of this function as it would touch even more lines.

in src/rpc/request.cpp:118 in c972e691d9 outdated

 114 | +        return true;
 115 | +    }
 116 |      file.open(filepath_tmp);
 117 |      if (!file.is_open()) {
 118 | -        LogInfo("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp));
 119 | +        LogWarning("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp));

l0rinc commented at 10:40 AM on November 29, 2024:

same nit: \n is likely redundant in these cases as well

in test/functional/combine_logs.py:86 in c972e691d9 outdated

  86 | -        assert next(glob, None) is None #  more than one debug.log, should never happen
  87 | +    # Find out what the folder is called that holds node 0's debug.log file
  88 | +    debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
  89 | +    if len(debug_logs) > 0:
  90 | +        path = debug_logs[0]
  91 | +        assert len(debug_logs) < 2, 'More than one debug.log, should never happen:\n\t' + '\n\t'.join([str(f) for f in debug_logs])

l0rinc commented at 10:43 AM on November 29, 2024:

nit: "should never happen" -> but it just did, so can we either provide more context or remove the statement that was just invalidated?

        assert len(debug_logs) == 1, (
            'Found multiple debug.log files, expected only one:\n\t' + 
            '\n\t'.join(str(f) for f in debug_logs)
        )

hodlinator commented at 10:13 AM on December 3, 2024:

Thanks, agree that it's a bit redundant in an assert, leftover from previous comment. Updated.

in test/functional/combine_logs.py:84 in c972e691d9 outdated

  84 | -    path = next(glob, None)
  85 | -    if path:
  86 | -        assert next(glob, None) is None #  more than one debug.log, should never happen
  87 | +    # Find out what the folder is called that holds node 0's debug.log file
  88 | +    debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
  89 | +    if len(debug_logs) > 0:

l0rinc commented at 10:47 AM on November 29, 2024:

nit:

    if debug_logs:

hodlinator commented at 10:14 AM on December 3, 2024:

Current way is clearer & safer to me.

in test/functional/feature_config_args.py:31 in c972e691d9 outdated

  26 | @@ -27,9 +27,71 @@ def set_test_params(self):
  27 |          self.wallet_names = []
  28 |          self.disable_autoconnect = False
  29 |  
  30 | +    def setup_network(self):

l0rinc commented at 10:51 AM on November 29, 2024:

My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/

ryanofsky commented at 6:29 PM on December 2, 2024:

In commit "test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning" (eb2d96bb66f109e610ee44cf3fed5c6701e3b8e3)

re: #31212 (review)

My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/

Don't disagree this commit could be a separate PR. A separate PR might attract feedback from reviewers more familiar with how the test framework is designed to be used, and if there is a bigger improvement that could be made here.

But it also seems fine to keep this, and it's fine to note in your overall review comment if you didn't review some part of the PR closely and you don't feel comfortable acking that part.

hodlinator commented at 10:17 AM on December 3, 2024:

I've updated that commit to better document that I'm overriding the 2 methods (unfortunately our .python-version is 3.10.14, python 3.12 includes @override).

in test/functional/feature_config_args.py:260 in c972e691d9 outdated

 256 | @@ -196,10 +257,10 @@ def test_args_log(self):
 257 |                  '-rpcuser=secret-rpcuser',
 258 |                  '-torpassword=secret-torpassword',
 259 |              ])
 260 | +        # Leave node running for test_seed_peers()

l0rinc commented at 10:52 AM on November 29, 2024:

Does it signal the lack of self.stop_node(0)? Does this mean the test run order matters now?

hodlinator commented at 10:21 AM on December 3, 2024:

Updated commit to make this function also call self.stop_node(0), hammering down the last nail that stood out. :)

Hopefully that makes it easier to review. The commit was already making test run order matter less, but the new version takes it one step further. There may still be other state than node stopped/running that prevents re-ordering, but that was the main thing I ran into when adding test functions in the later commits.

in src/httprpc.cpp:301 in c972e691d9 outdated

 299 |          auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
 300 |          if (cookie_perms_arg) {
 301 |              auto perm_opt = InterpretPermString(*cookie_perms_arg);
 302 |              if (!perm_opt) {
 303 | -                LogInfo("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.\n", *cookie_perms_arg);
 304 | +                LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.", *cookie_perms_arg);

l0rinc commented at 10:55 AM on November 29, 2024:

nit Q: I'm not sure I understand the logging quoting standards here, when do we use \" and when ' and when just =%s?

ryanofsky commented at 5:36 PM on December 2, 2024:

re: #31212 (review)

nit Q: I'm not sure I understand the logging quoting standards here, when do we use \" and when ' and when just =%s?

I think these are just quoted to convey that 'owner' 'group' and 'all' are literal strings, not placeholders for permission values.

It would be nice if there was a standard way paths and arguments were quoted in log messages, but I don't think there is a standard right now.

in src/rpc/request.cpp:153 in c972e691d9 outdated

 148 | @@ -142,6 +149,9 @@ bool GetAuthCookie(std::string *cookie_out)
 149 |      std::ifstream file;
 150 |      std::string cookie;
 151 |      fs::path filepath = GetAuthCookieFile();
 152 | +    if (filepath.empty()) {
 153 | +        return true; // -norpccookiefile was specified

l0rinc commented at 11:08 AM on November 29, 2024:

Do I understand correctly that we're basically treating an empty path as an std::nullopt here (and commenting it everywhere since we admit it's hard to understand)? I understand that we can't fix everything in the same PR, but would like us to consider if it still makes sense to build on previous hacks.

ryanofsky commented at 6:20 PM on December 2, 2024:

re: #31212 (review)

Do I understand correctly that we're basically treating an empty path as an std::nullopt here (and commenting it everywhere since we admit it's hard to understand)? I understand that we can't fix everything in the same PR, but would like us to consider if it still makes sense to build on previous hacks.

Just using std::optional here would not be an improvement IMO. An nullopt setting is conventionally an unspecified setting, not a negated/disabled setting. So if we switched to std::optional we would still want to keep the comments clarifying which cases are being handled, and we would need to make sure there was error handling for the path->empty() cases. The convention that a -no prefixed path setting that should not be read from or written to is represented by path{} comes from ArgsManager::GetPathArg and is generally convenient and safe.

I agree there are advantages to using separate types for separate cases, but don't think it's an improvement if adding std::optional increases the number of states that need to be handled or makes new code represent state differently than existing code.

hodlinator commented at 1:19 PM on December 3, 2024:

I agree that empty string is not the best way to signal negation, and nullopt may not be either. Looking forward to Disabled-functionality mentioned in #31212 (review), but keeping as-is for now.

in src/init.cpp:178 in bffd92f00f outdated

 174 | @@ -175,6 +175,8 @@ static fs::path GetPidFile(const ArgsManager& args)
 175 |  
 176 |  [[nodiscard]] static bool CreatePidFile(const ArgsManager& args)
 177 |  {
 178 | +    if (args.IsArgNegated("-pid")) return true;

l0rinc commented at 11:12 AM on November 29, 2024:

it's hard to review 14 commits, as hinted before, could the fix be in the same commit as the test - I personally am lacking a lot of context and wouldn't want this commit to be merged without making sure a test is added as well

hodlinator commented at 10:31 AM on December 3, 2024:

PR does not contain a test for this as I thought it was straightforward enough to skip and I didn't want to add more things to review/maintain. If we pass -nopid we simply do not:

Create the file
Write the PID
Set the bool to remember deleting the file
(Delete the file)

https://github.com/bitcoin/bitcoin/blob/95a0104f2e9869799db84add108ae8c57b56d360/src/init.cpp#L165-L192

in src/httprpc.cpp:311 in d7ecacf1dc outdated

 303 | @@ -307,9 +304,11 @@ static bool InitRPCAuthentication()
 304 |              cookie_perms = *perm_opt;
 305 |          }
 306 |  
 307 | +        assert(strRPCUserColonPass.empty()); // Only support initializing once
 308 |          if (!GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)) {
 309 |              return false;
 310 |          }
 311 | +        if (!strRPCUserColonPass.empty()) LogInfo("Using random cookie authentication.");

l0rinc commented at 11:19 AM on November 29, 2024:

what if it's empty?

ryanofsky commented at 5:49 PM on December 2, 2024:

re: #31212 (review)

what if it's empty?

If it's empty it means the user specified -norpccookiefile, and some other authentication mechanism must be used. There's another log statement LogInfo("RPC authentication cookie file generation is disabled."); in GenerateAuthCookie that could probably be moved here to make this clearer.

hodlinator commented at 10:32 AM on December 3, 2024:

Moved back the log message in latest push to clarify.

in src/common/config.cpp:133 in 2ac863bdf9 outdated

 133 | -    // not ok to have a config file specified that cannot be opened
 134 | -    if (IsArgSet("-conf") && !stream.good()) {
 135 | -        error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
 136 | -        return false;
 137 | +    std::ifstream stream;
 138 | +    if (!conf_path.empty()) { // path is empty when -noconf is specified

l0rinc commented at 11:25 AM on November 29, 2024:

This bothers me more than it should: we're adding comments instead of solving these problems with code - conf_path should be an optional in that case instead of coming up with special value

hodlinator commented at 3:49 PM on December 2, 2024:

We are currently using the nullness of m_config_path to signal something different, initialized/not, although I think it should probably be more explicit. See #31212 (comment), and this code:

https://github.com/bitcoin/bitcoin/blob/2ac863bdf974d7b4b4e8314876d02270400b6240/src/common/args.cpp#L761-L772

We could possibly do optional<optional<path>> m_config_path to flag both initialization and negation... but I'm not sure I want to go down that road. Do you have any concrete suggestion?

l0rinc commented at 4:38 PM on December 2, 2024:

optional of optional sounds like a std::variant - but I'm fine with doing these in a separate PR, of course

ryanofsky commented at 6:45 PM on December 2, 2024:

re: #31212 (review)

optional of optional sounds like a std::variant - but I'm fine with doing these in a separate PR, of course

Yes could use something like std::variant<Disabled, not_empty<fs::path>> if we want a separate type to represent the separate condition. (Disabled could be the type from #31260)). not_empty could be a wrapper for fs::path,std::string, etc that does not allow empty values to be assigned, like not_null from #24423)

But IMO using separate types here is not necessary. I think it's reasonable to use empty fs::path objects to indicate paths that should not be written to or read from. I think the advantage of using separate types is it can force developers to handle disabled cases instead of forgetting about them, but there are other tradeoffs.

I also think whether you are using fs::path or a different type you would want to keep the comment about -noconf because it explains why this code is not reading the config file.

l0rinc changes_requested

l0rinc commented at 11:31 AM on November 29, 2024: contributor

Thanks for enduring through the struggles the Windows gods bestow upon you. While there are parts I can't meaningfully review yet (no idea what to do with the cookies yet), I left a few comments about how some restructuring could help me understand the context better: if a commit should not be merged without another one, I would prefer combining them into a single commit - to avoid the bloat, it makes review harder.

in src/common/init.cpp:70 in 2ac863bdf9 outdated

  85 | -                LogPrintf("Warning: %s\n", error);
  86 | -            } else {
  87 | -                return ConfigError{ConfigStatus::FAILED, Untranslated(error)};
  88 | +        if (fs::exists(base_config_path)) {
  89 | +            if (orig_config_path.empty()) {
  90 | +                LogWarning(

ryanofsky commented at 9:33 PM on December 2, 2024:

In commit "args: Properly support -noconf" (2ac863bdf974d7b4b4e8314876d02270400b6240)

I'm still not sure why this is a useful warning. There would seem to be little reason to specify -noconf unless you are trying to ignore a config file, so why should you be warned if a file you were trying to ignore exists? If anything, you might expect a warning in the opposite case, when a file you specified an option to ignore does not exist. Both warnings seem excessive to me and and I think the approach from 3a856c86685a5ab6f3cb78b17cb52cbb1b9ad95b just adding orig_config_path.empty() to this code so it functions with -noconf would make the most sense.

If you do see value in logging this information though, I think it should use LogInfo or LogDebug, instead of LogWarning.

hodlinator commented at 10:36 AM on December 3, 2024:

Having a latent bitcoin.conf is not ideal and something the user should correct if they want to run with -noconf long-term to reduce potential for confusion ("Why is my change to bitcoin.conf not getting picked up?!!" until realizing -noconf was used). But changed to LogInfo for now.

in src/common/config.cpp:134 in 01b174493e outdated

 130 | @@ -130,6 +131,10 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
 131 |      const auto conf_path{GetConfigFilePath()};
 132 |      std::ifstream stream;
 133 |      if (!conf_path.empty()) { // path is empty when -noconf is specified
 134 | +        if (fs::is_directory(conf_path)) {

ryanofsky commented at 10:08 PM on December 2, 2024:

In commit "args: Catch directories in place of config files" (01b174493ec210c1c627083dfa9e9b86035c7eef)

If macos is setting the eof bit when using ifstream to read from a directory paths as described #31212 (comment), treating directories like empty files, maybe we really do need these is_directory calls, since there might be no other way on macos to distinguish directories from empty files, though I suspect it probably is indicating some error with stream.bad() and we don't really need to do this.

The windows behavior described #31212 (comment) of refusing to open directories is less surprising than the mac behavior, and could be worked around by using fs::exists instead of is_open as a more direct way of determine whether the path exists.

Overall, the current approach in this commit seems ok, but fragile to only check if the file paths are directories and not actually check that files were read successfully. I think a more ideal version of this taking windows and mac behavior into account would look like the following:

--- a/src/common/config.cpp
+++ b/src/common/config.cpp
@@ -129,24 +129,19 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
     }
 
     const auto conf_path{GetConfigFilePath()};
-    std::ifstream stream;
-    if (!conf_path.empty()) { // path is empty when -noconf is specified
-        if (fs::is_directory(conf_path)) {
-            error = strprintf("Config file \"%s\" is a directory.", fs::PathToString(conf_path));
-            return false;
-        }
-        stream = std::ifstream{conf_path};
-        // If the file is explicitly specified, it must be readable
-        if (IsArgSet("-conf") && !stream.good()) {
-            error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
-            return false;
-        }
-    }
-    // ok to not have a config file
-    if (stream.good()) {
+    // Don't try to read the config file if -noconf is specified (so
+    // conf_path.empty() is true), or if the -conf option is unset and the
+    // default config file path doesn't exist.
+    if (!conf_path.empty() && (IsArgSet("-conf") || fs::exists(conf_path))) {
+        std::ifstream stream{conf_path};
         if (!ReadConfigStream(stream, fs::PathToString(conf_path), error, ignore_invalid_keys)) {
             return false;
         }
+        // Error if config stream could not be read in full.
+        if (!stream.eof() || stream.bad()) {
+            error = strprintf("config file \"%s\" could not be read.", fs::PathToString(conf_path));
+            return false;
+        }
         // `-includeconf` cannot be included in the command line arguments except
         // as `-noincludeconf` (which indicates that no included conf file should be used).
         bool use_conf_file{true};
@@ -182,16 +177,11 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
             const size_t default_includes = add_includes({});
 
             for (const std::string& conf_file_name : conf_file_names) {
-                const auto include_conf_path{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)};
-                if (fs::is_directory(include_conf_path)) {
-                    error = strprintf("Included config file \"%s\" is a directory.", fs::PathToString(include_conf_path));
+                std::ifstream conf_file_stream{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)};
+                if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) {
                     return false;
                 }
-                std::ifstream conf_file_stream{include_conf_path};
-                if (conf_file_stream.good()) {
-                    if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) {
-                        return false;
-                    }
+                if (conf_file_stream.eof() && !conf_file_stream.bad()) {
                     LogPrintf("Included configuration file %s\n", conf_file_name);
                 } else {
                     error = "Failed to include configuration file " + conf_file_name;
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -45,7 +45,7 @@ class ConfArgsTest(BitcoinTestFramework):
         self.stop_node(0)
         self.nodes[0].assert_start_raises_init_error(
             extra_args=['-regtest'],
-            expected_msg=f'Error: Error reading configuration file: Config file "{conf_path}" is a directory.',
+            expected_msg=f'Error: Error reading configuration file: config file "{conf_path}" could not be read.',
         )
         conf_path.rmdir()
         os.rename(conf_path.with_suffix('.confbkp'), conf_path)
@@ -55,7 +55,7 @@ class ConfArgsTest(BitcoinTestFramework):
             conf.write(f'includeconf={self.nodes[0].datadir_path}\n')
         self.nodes[0].assert_start_raises_init_error(
             extra_args=['-regtest'],
-            expected_msg=f'Error: Error reading configuration file: Included config file "{self.nodes[0].datadir_path}" is a directory.',
+            expected_msg=f'Error: Error reading configuration file: Failed to include configuration file {self.nodes[0].datadir_path}',
         )
 
         self.nodes[0].replace_in_config([(f'includeconf={self.nodes[0].datadir_path}', '')])
@@ -521,7 +521,7 @@ class ConfArgsTest(BitcoinTestFramework):
 
         # Check that an explicitly specified config file that cannot be opened fails
         none_existent_conf_file = default_data_dir / "none_existent_bitcoin.conf"
-        self.nodes[0].assert_start_raises_init_error(['-conf=' + f'{none_existent_conf_file}'], 'Error: Error reading configuration file: specified config file "' + f'{none_existent_conf_file}' + '" could not be opened.')
+        self.nodes[0].assert_start_raises_init_error(['-conf=' + f'{none_existent_conf_file}'], 'Error: Error reading configuration file: config file "' + f'{none_existent_conf_file}' + '" could not be read.')
 
         # Create the directory and ensure the config file now works
         new_data_dir.mkdir()

</details>

But current code seems fine and this could be done in a followup. Thanks for looking into this and making the current fix easy to understand.

hodlinator commented at 10:50 AM on December 3, 2024:

Would rather not spend more cycles on this when C++ appears to not specify how ifstream interacts with directories. Even if we were to get it working with your approach on the 3 platforms on current CI, others might get it compiling on other setups where is_directory() would again have been more robust.

I do see value in catching whether reading a file fails, but prefer keeping it out of scope of this PR.

ryanofsky approved

ryanofsky commented at 10:26 PM on December 2, 2024: contributor

Code review ACK c972e691d937f49d7d3f44d2b19b4035c6e67aaf. Thanks for breaking the commits up make making things easy to understand.

Changes since last review were simplifying the -norpccookiefile and -noconf fixes by breaking them up and changing some implementation details. There are also new combine_logs and feature_config args test improvements.

DrahtBot requested review from l0rinc on Dec 2, 2024

test: combine_logs.py - Output debug.log paths on error 75bacabb55

test: Harden testing of cookie file existence 6e28c76907

logs: Use correct path and more appropriate macros in cookie-related code

filepath_tmp -> filepath in last message.

More material changes to nearby code in next commit.

e82ad88452

args: Support -norpccookiefile for bitcoind and bitcoin-cli

Replaces belt & suspenders check for initialization in RPCAuthorized() with not allowing empty passwords further down.

39cbd4f37c

test: -norpccookiefile

Both bitcoind and bitcoin-cli.

7402658bc2

test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning

This ensures we don't needlessly start the node, and reduces implicit dependencies between test functions.

test_seed_peers() - Move assert calling RPC to verify correct chain after our own function actually started the node.

312ec64cc0

args: Properly support -noconf

-noconf would previously lead to an ifstream "successfully" being opened to the ".bitcoin"-directory (not a file). (Guards against the general case of directories as configs are added in grandchild commit to this one).

Other users of AbsPathForConfigVal() in combination with negated args have been updated earlier in this PR ("args: Support -nopid" and "args: Support -norpccookiefile...").

483f0dacc4

test: Add tests for -noconf e4b6b1822c

args: Catch directories in place of config files

Previously passing a directory path as -conf would lead to an ifstream being opened for it, and would not trigger any errors.

e85abe92c7

test: Add tests for directories in place of config files 95a0104f2e

hodlinator force-pushed on Dec 3, 2024

ryanofsky approved

ryanofsky commented at 3:49 PM on December 3, 2024: contributor

Code review ACK 95a0104f2e9869799db84add108ae8c57b56d360. Looks good! Thanks for all your work on this breaking the changes down and making them simple.

Since last review, more log newlines are removed, -norpccookiefile code is reorganized a little without changing behavior, combine_logs.py assert is improved, feature_config_args.py more consistently stops nodes after tests, and using -noconf now triggers an info log instead of a warning.

in src/bitcoin-util.cpp:53 in 95a0104f2e

  49 | @@ -50,11 +50,11 @@ static int AppInitUtil(ArgsManager& args, int argc, char* argv[])
  50 |          return EXIT_FAILURE;
  51 |      }
  52 |  
  53 | -    if (HelpRequested(args) || args.IsArgSet("-version")) {
  54 | +    if (HelpRequested(args) || args.GetBoolArg("-version", false)) {

l0rinc commented at 11:15 AM on December 4, 2024:

I'm not sure I understand how -noversion is supposed to work.

I tried changing test/functional/feature_help.py to:

    def run_test(self):
        self.log.info("Start bitcoin with -h for help text")
        self.nodes[0].start(extra_args=['-h', '-noversion']) # added -noversion
        # Node should exit immediately and output help to stdout.
        output, _ = self.get_node_output(ret_code_expected=0)
        assert b'Options' in output
        assert b'version' not in output # added check
        self.log.info(f"Help text received: {output[0:60]} (...)")

which fails since the version is still in the output.

Tried:

        self.log.info("Start bitcoin with -noversion for version information")
        self.nodes[0].start(extra_args=['-version=0']) # -version=1 works
        # Node should exit immediately and output version to stdout.
        output, _ = self.get_node_output(ret_code_expected=0)
        assert b'version' in output
        self.log.info(f"Version text received: {output[0:60]} (...)")

which just hangs with no RPC connection available.

Same for

./build/src/bitcoind -noversion | head -6
2024-12-04T11:20:31Z Bitcoin Core version v28.99.0-d517798ca59a-dirty (debug build)
2024-12-04T11:20:31Z Using the 'arm_shani(1way,2way)' SHA256 implementation
2024-12-04T11:20:31Z Default data directory /Users/lorinc/Library/Application Support/Bitcoin
2024-12-04T11:20:31Z Using data directory /Users/lorinc/Library/Application Support/Bitcoin
2024-12-04T11:20:31Z Config file: /Users/lorinc/Library/Application Support/Bitcoin/bitcoin.conf (not found, skipping)
2024-12-04T11:20:31Z Command-line arg: version=false

the version is still in the header - is this not how one is supposed to use this param?

Note that -version does display a different header, but both of them display the version itself, so the new -noversion param is confusing now:

./build/src/bitcoind -version | head -6
Bitcoin Core daemon version v28.99.0-d517798ca59a-dirty
Copyright (C) 2009-2024 The Bitcoin Core developers

Please contribute if you find Bitcoin Core useful. Visit
<https://bitcoincore.org/> for further information about the software.
The source code is available from <https://github.com/bitcoin/bitcoin>.

ryanofsky commented at 2:18 PM on December 4, 2024:

The version number is always going to be displayed whether you run bitcoind with -version -help or no arguments at all, because it is logged at startup. If you don't want to see the version number you need to run bitcoind with -noprinttoconsole or -daemon to not log to the console.

The point of -version is to show the version number and copyright information and exit, and the point of -noversion is to do the opposite and start up normally. You can think of -version as being short for -print-version-then-exit=1 and -noversion as being short for -print-version-then-exit=0. I doubt there are any realistic use-cases for -noversion, and new behavior seems less confusing than previous behavior. The point of the commit is just to make this option behave like other boolean options, where -nooption does the opposite of -option instead of doing the same thing.

l0rinc commented at 2:21 PM on December 4, 2024:

I'm not yet sure this makes the behavior clearer for the version parameter, but the rest seems fine to me, thanks for clarifying.

No longer print version information when getting passed -noversion.

maybe this needs updating in that case

hodlinator commented at 3:45 PM on December 4, 2024:

Thanks @ryanofsky for arguing my case. One could possibly stretch -noversion to actually remove the version from the default log, but not sure if that would be more useful. The fact that one of -version/-noversion exits quickly is mildly confusing.

Tried:

        self.log.info("Start bitcoin with -noversion for version information")
        self.nodes[0].start(extra_args=['-version=0']) # -version=1 works
        # Node should exit immediately and output version to stdout.
        output, _ = self.get_node_output(ret_code_expected=0)
        assert b'version' in output
        self.log.info(f"Version text received: {output[0:60]} (...)")

which just hangs with no RPC connection available.

That is slightly incorrect. Here is the full error I got when trying to reproduce your change:

2024-12-04T15:35:30.230000Z TestFramework (INFO): PRNG seed is: 6906262290736093521
2024-12-04T15:35:30.230000Z TestFramework (DEBUG): Setting up network thread
2024-12-04T15:35:30.231000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_n5c4b731
2024-12-04T15:35:30.231000Z TestFramework (INFO): Start bitcoin with -h for help text
2024-12-04T15:35:30.232000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
2024-12-04T15:35:30.239000Z TestFramework (INFO): Help text received: b'Bitcoin Core daemon version v28.99.0-95a0104f2e98-dirty\n\nThe' (...)
2024-12-04T15:35:30.239000Z TestFramework (INFO): Start bitcoin with -version for version information
2024-12-04T15:35:30.240000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
2024-12-04T15:36:30.240000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/home/hodlinator/bitcoin/build/test/functional/feature_help.py", line 48, in run_test
    output, _ = self.get_node_output(ret_code_expected=0)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hodlinator/bitcoin/build/test/functional/feature_help.py", line 20, in get_node_output
    ret_code = self.nodes[0].process.wait(timeout=60)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/subprocess.py", line 1264, in wait
    return self._wait(timeout=timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/subprocess.py", line 2045, in _wait
    raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command '['/home/hodlinator/bitcoin/build/src/bitcoind', '-datadir=/tmp/bitcoin_func_test_n5c4b731/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-debugexclude=rand', '-uacomment=testnode0', '-disablewallet', '-logthreadnames', '-logsourcelocations', '-loglevel=trace', '-v2transport=0', '-version=0']' timed out after 60 seconds
2024-12-04T15:36:30.245000Z TestFramework (DEBUG): Closing down network thread
2024-12-04T15:36:30.296000Z TestFramework (INFO): Stopping nodes
2024-12-04T15:36:30.296000Z TestFramework.node0 (DEBUG): Stopping node
Traceback (most recent call last):
  File "/home/hodlinator/bitcoin/build/test/functional/feature_help.py", line 62, in <module>
    HelpTest(__file__).main()
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 155, in main
    exit_code = self.shutdown()
                ^^^^^^^^^^^^^^^
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 318, in shutdown
    self.stop_nodes()
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 587, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_node.py", line 397, in stop_node
    self.stop(wait=wait)
    ^^^^^^^^^
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: [node 0] Error: no RPC connection
[node 0] Cleaning up leftover process

It's the call to process.wait() in the local get_node_output() in feature_help.py that times out. The RPC assertion failure is just a knock-on issue that is fixed by #30660.

<details> <summary> Log with [#30660](/bitcoin-bitcoin/30660/) cherry-picked on top. </summary>

2024-12-04T15:41:34.993000Z TestFramework (INFO): PRNG seed is: 7034279536394037572
2024-12-04T15:41:34.994000Z TestFramework (DEBUG): Setting up network thread
2024-12-04T15:41:34.994000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_3wqbq2m9
2024-12-04T15:41:34.994000Z TestFramework (INFO): Start bitcoin with -h for help text
2024-12-04T15:41:34.995000Z TestFramework.node0 (DEBUG): bitcoind started with PID 881245, waiting for RPC to come up
2024-12-04T15:41:35.002000Z TestFramework (INFO): Help text received: b'Bitcoin Core daemon version v28.99.0-95a0104f2e98-dirty\n\nThe' (...)
2024-12-04T15:41:35.002000Z TestFramework (INFO): Start bitcoin with -version for version information
2024-12-04T15:41:35.003000Z TestFramework.node0 (DEBUG): bitcoind started with PID 881246, waiting for RPC to come up
2024-12-04T15:42:35.004000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/home/hodlinator/bitcoin/build/test/functional/feature_help.py", line 48, in run_test
    output, _ = self.get_node_output(ret_code_expected=0)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hodlinator/bitcoin/build/test/functional/feature_help.py", line 20, in get_node_output
    ret_code = self.nodes[0].process.wait(timeout=60)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/subprocess.py", line 1264, in wait
    return self._wait(timeout=timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/subprocess.py", line 2045, in _wait
    raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command '['/home/hodlinator/bitcoin/build/src/bitcoind', '-datadir=/tmp/bitcoin_func_test_3wqbq2m9/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-debugexclude=rand', '-uacomment=testnode0', '-disablewallet', '-logthreadnames', '-logsourcelocations', '-loglevel=trace', '-v2transport=0', '-version=0']' timed out after 60 seconds
2024-12-04T15:42:35.009000Z TestFramework (DEBUG): Closing down network thread
2024-12-04T15:42:35.060000Z TestFramework (INFO): Stopping nodes
2024-12-04T15:42:35.060000Z TestFramework.node0 (DEBUG): Stopping node
2024-12-04T15:42:35.060000Z TestFramework.node0 (WARNING): Cannot call stop-RPC as we are not connected. Killing process 881246 so that wait_until_stopped will not time out.
2024-12-04T15:42:35.111000Z TestFramework.node0 (DEBUG): Node stopped
2024-12-04T15:42:35.111000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_3wqbq2m9
2024-12-04T15:42:35.111000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_3wqbq2m9/test_framework.log
2024-12-04T15:42:35.111000Z TestFramework (ERROR): 
2024-12-04T15:42:35.112000Z TestFramework (ERROR): Hint: Call /home/hodlinator/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_3wqbq2m9' to consolidate all logs
2024-12-04T15:42:35.112000Z TestFramework (ERROR): 
2024-12-04T15:42:35.112000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-12-04T15:42:35.112000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-12-04T15:42:35.112000Z TestFramework (ERROR):

</details>

in test/functional/combine_logs.py:85 in 75bacabb55 outdated

  85 | -    if path:
  86 | -        assert next(glob, None) is None #  more than one debug.log, should never happen
  87 | +    # Find out what the folder is called that holds node 0's debug.log file
  88 | +    debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
  89 | +    if len(debug_logs) > 0:
  90 | +        assert len(debug_logs) < 2, 'Max one debug.log is supported, ' \

l0rinc commented at 11:34 AM on December 4, 2024:

nit: len(debug_logs) > 0 && len(debug_logs) < 2 is basically len(debug_logs) == 1. Which could likely be consolidated to a switch, something like:

    debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
    match len(debug_logs):
        case 0: chain = 'regtest'
        case 1: chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1)
        case _: raise AssertionError(f"Max one debug.log is supported, found several:\n\t" + "\n\t".join(map(str, debug_logs)))

hodlinator commented at 3:18 PM on December 4, 2024:

Sweet! Will do something like this if I need to retouch for other reasons. (Might use RuntimeError).

hodlinator commented at 9:25 AM on February 3, 2025:

(Resolved by now merged #31433).

l0rinc commented at 12:25 PM on December 4, 2024: contributor

I find a lot of negations very confusing, but I guess it's existing functionality, so we might as well embrace it

l0rinc commented at 2:25 PM on December 4, 2024: contributor

utACK 95a0104f2e9869799db84add108ae8c57b56d360 The code seems better than before, though most of these negations seem confusing to me - but it wasn't introduced here. I have tested the negation of the parameters and reviewed most of the code - but couldn't meaningfully opine on the cookie related new tests, I will leave that to other reviewers.

achow101 commented at 5:53 PM on December 4, 2024: member

ACK 95a0104f2e9869799db84add108ae8c57b56d360

achow101 merged this on Dec 4, 2024

achow101 closed this on Dec 4, 2024

hodlinator deleted the branch on Dec 4, 2024

vasild commented at 8:10 AM on December 5, 2024: contributor

Further idea:

-noconnect=0 is interpreted as -connect=1 which is interpreted as -connect=0.0.0.1

hodlinator commented at 8:24 AM on December 5, 2024: contributor

Further idea:

-noconnect=0 is interpreted as -connect=1 which is interpreted as -connect=0.0.0.1

:sweat_smile: Good one! I'll create a "Good first issue" for it.

hodlinator commented at 8:52 AM on December 5, 2024: contributor

Done! #31426. @vasild let me know what you think of the issue description and proposed solution.

fanquake referenced this in commit 35000e34cf on Dec 8, 2024

sedited referenced this in commit 230a439a4a on Jan 17, 2025

luke-jr referenced this in commit bb42993a30 on Feb 22, 2025

luke-jr referenced this in commit 397cc860ed on Feb 22, 2025

luke-jr referenced this in commit c850b5be46 on Feb 22, 2025

luke-jr referenced this in commit a613155808 on Feb 22, 2025

luke-jr referenced this in commit 9b90eea2a1 on Feb 22, 2025

luke-jr referenced this in commit 0b3f7453b6 on Feb 22, 2025

luke-jr referenced this in commit 88ae15bb58 on Feb 22, 2025

luke-jr referenced this in commit 56c374dfad on Feb 22, 2025

luke-jr referenced this in commit 41970350d4 on Feb 22, 2025

luke-jr referenced this in commit 088e9845f0 on Feb 22, 2025

luke-jr referenced this in commit 6b837312c6 on Feb 22, 2025

luke-jr referenced this in commit 866ba19192 on Feb 22, 2025

luke-jr referenced this in commit e1b776466d on Feb 22, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

bug-castercv502 referenced this in commit 44b075fe71 on Sep 28, 2025

bitcoin locked this on Feb 5, 2026