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.

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.

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

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

Disallow -noconf since a config file is required

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

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.

Concept ACK

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

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.

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

Further idea:

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

Further idea:

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

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

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