Repository: bitcoin/bitcoin
File: src/common/config.cpp (C++)

Code before:
        } else {
            if (ignore_invalid_keys) {
                LogPrintf("Ignoring unknown configuration value %s\n", option.first);
            } else {
                error = strprintf("Invalid configuration value %s", option.first);
                return false;
            }
        }
    }
    return true;
}

bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
{
    {
        LOCK(cs_args);
        m_settings.ro_config.clear();
        m_config_sections.clear();
        m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false);
    }

    const auto conf_path{GetConfigFilePath()};
    std::ifstream stream{conf_path};

    // 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;
    }
    // ok to not have a config file
    if (stream.good()) {
        if (!ReadConfigStream(stream, fs::PathToString(conf_path), error, ignore_invalid_keys)) {
            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};
        {
            LOCK(cs_args);
            if (auto* includes = common::FindKey(m_settings.command_line_options, "includeconf")) {
                // ParseParameters() fails if a non-negated -includeconf is passed on the command-line
                assert(common::SettingsSpan(*includes).last_negated());
                use_conf_file = false;
            }
        }
        if (use_conf_file) {
            std::string chain_id = GetChainTypeString();
            std::vector<std::string> conf_file_names;

            auto add_includes = [&](const std::string& network, size_t skip = 0) {
                size_t num_values = 0;

Review comment (bug):
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 https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2506175947, 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 https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2506175947 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:

<details><summary>diff</summary>
<p>

```diff
--- 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()
```
</p>
</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.

Code after:
            m_settings.ro_config[key.section][key.name].push_back(*value);
        } else {
            if (ignore_invalid_keys) {
                LogPrintf("Ignoring unknown configuration value %s\n", option.first);
            } else {
                error = strprintf("Invalid configuration value %s", option.first);
                return false;
            }
        }
    }
    return true;
}

bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
{
    {
        LOCK(cs_args);
        m_settings.ro_config.clear();
        m_config_sections.clear();
        m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false);
    }

    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()) {
        if (!ReadConfigStream(stream, fs::PathToString(conf_path), error, ignore_invalid_keys)) {
            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};
        {
            LOCK(cs_args);
            if (auto* includes = common::FindKey(m_settings.command_line_options, "includeconf")) {
                // ParseParameters() fails if a non-negated -includeconf is passed on the command-line
                assert(common::SettingsSpan(*includes).last_negated());
                use_conf_file = false;
            }