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

Code before:
    // ********************************************************* Step 4a: application initialization
    if (!CreatePidFile(args)) {
        // Detailed error printed inside CreatePidFile().
        return false;
    }
    if (!init::StartLogging(args)) {
        // Detailed error printed inside StartLogging().
        return false;
    }

    LogInfo("Using at most %i automatic connections (%i file descriptors available)", nMaxConnections, available_fds);

    // Warn about relative -datadir path.
    if (args.IsArgSet("-datadir") && !args.GetPathArg("-datadir").is_absolute()) {
        LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the "
                  "current working directory '%s'. This is fragile, because if bitcoin is started in the future "
                  "from a different location, it will be unable to locate the current data files. There could "
                  "also be data loss if bitcoin is started while in a temporary directory.\n",
                  args.GetArg("-datadir", ""), fs::PathToString(fs::current_path()));
    }

    assert(!node.scheduler);
    node.scheduler = std::make_unique<CScheduler>();
    auto& scheduler = *node.scheduler;

    // Start the lightweight task scheduler thread
    scheduler.m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { scheduler.serviceQueue(); });

    // Gather some entropy once per minute.
    scheduler.scheduleEvery([]{
        RandAddPeriodic();
    }, std::chrono::minutes{1});

    // Check disk space every 5 minutes to avoid db corruption.
    scheduler.scheduleEvery([&args, &node]{
        constexpr uint64_t min_disk_space = 50 << 20; // 50 MB
        if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) {
            LogError("Shutting down due to lack of disk space!\n");
            if (!(Assert(node.shutdown_request))()) {
                LogError("Failed to send shutdown signal after disk space check\n");
            }
        }
    }, std::chrono::minutes{5});

    LogInstance().SetRateLimiting(std::make_unique<BCLog::LogRateLimiter>(
        [&scheduler](auto func, auto window) { scheduler.scheduleEvery(std::move(func), window); },
        BCLog::RATELIMIT_MAX_BYTES,
        1h));

    assert(!node.validation_signals);
    node.validation_signals = std::make_unique<ValidationSignals>(std::make_unique<SerialTaskRunner>(scheduler));

Review comment (bug):
This is a bit brittle, e.g. if started with `-disableratelimitlogging=0` this would still disable ratelimiting. I'd suggest using the actual boolean value rather than checking if the argument is set. Also, ArgsManager has a built-in negation system: arguments also have a "no{arg}" option to disable it, which currently would be `-nodisableratelimitlogging` which is getting quite verbose.

Would prefer keeping options positive, and switch to `-{no}logratelimit` instead, keeping naming in line with other log options.

Full diff to implement that:

<details>
<summary>git diff on 8dd5c0a044</summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 5f882cefb5..fc6d02246f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1381,11 +1381,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         }
     }, std::chrono::minutes{5});
 
-    if (!args.IsArgSet("-disableratelimitlogging")) {
+    if (args.GetBoolArg("-logratelimit", BCLog::DEFAULT_LOGRATELIMIT)) {
         LogInstance().SetRateLimiting(BCLog::LogRateLimiter::Create(
             [&scheduler](auto func, auto window) { scheduler.scheduleEvery(std::move(func), window); },
             BCLog::RATELIMIT_MAX_BYTES,
             BCLog::RATELIMIT_WINDOW));
+    } else {
+        LogInfo("Log ratelimiting disabled.");
     }
 
     assert(!node.validation_signals);
diff --git a/src/init/common.cpp b/src/init/common.cpp
index b5710be45d..c6236365ea 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -38,9 +38,9 @@ void AddLoggingArgs(ArgsManager& argsman)
     argsman.AddArg("-logsourcelocations", strprintf("Prepend debug output with name of the originating source location (source file, line number and function name) (default: %u)", DEFAULT_LOGSOURCELOCATIONS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logtimemicros", strprintf("Add microsecond precision to debug timestamps (default: %u)", DEFAULT_LOGTIMEMICROS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-loglevelalways", strprintf("Always prepend a category and level (default: %u)", DEFAULT_LOGLEVELALWAYS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-logratelimit", strprintf("Apply ratelimiting to unconditional logging to mitigate disk-filling attacks (default: %u)", BCLog::DEFAULT_LOGRATELIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-shrinkdebugfile", "Shrink debug.log file on client startup (default: 1 when no -debug)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-disableratelimitlogging", "Disable rate limit logging", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 }
 
 void SetLoggingOptions(const ArgsManager& args)
diff --git a/src/logging.h b/src/logging.h
index 623bd97e56..3d97b460cc 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -107,6 +107,7 @@ namespace BCLog {
     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
     constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes per source location that can be logged within the RATELIMIT_WINDOW
     constexpr auto RATELIMIT_WINDOW{1h}; // time window after which log ratelimit stats are reset
+    constexpr bool DEFAULT_LOGRATELIMIT{true};
 
     //! Fixed window rate limiter for logging.
     class LogRateLimiter : public std::enable_shared_from_this<LogRateLimiter>
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index 09d611128c..3b0814635a 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -137,7 +137,7 @@ class TestNode():
         if self.version_is_at_least(239000):
             self.args.append("-loglevel=trace")
         if self.version_is_at_least(299900):
-            self.args.append("-disableratelimitlogging")
+            self.args.append("-nologratelimit")
 
         # Default behavior from global -v2transport flag is added to args to persist it over restarts.
         # May be overwritten in individual tests, using extra_args.

```
</details>


Code after:
    }

    assert(!node.scheduler);
    node.scheduler = std::make_unique<CScheduler>();
    auto& scheduler = *node.scheduler;

    // Start the lightweight task scheduler thread
    scheduler.m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { scheduler.serviceQueue(); });

    // Gather some entropy once per minute.
    scheduler.scheduleEvery([]{
        RandAddPeriodic();
    }, std::chrono::minutes{1});

    // Check disk space every 5 minutes to avoid db corruption.
    scheduler.scheduleEvery([&args, &node]{
        constexpr uint64_t min_disk_space = 50 << 20; // 50 MB
        if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) {
            LogError("Shutting down due to lack of disk space!\n");
            if (!(Assert(node.shutdown_request))()) {
                LogError("Failed to send shutdown signal after disk space check\n");
            }
        }
    }, std::chrono::minutes{5});

    if (args.GetBoolArg("-logratelimit", BCLog::DEFAULT_LOGRATELIMIT)) {
        LogInstance().SetRateLimiting(BCLog::LogRateLimiter::Create(
            [&scheduler](auto func, auto window) { scheduler.scheduleEvery(std::move(func), window); },
            BCLog::RATELIMIT_MAX_BYTES,
            BCLog::RATELIMIT_WINDOW));
    } else {
        LogInfo("Log rate limiting disabled");
    }

    assert(!node.validation_signals);
    node.validation_signals = std::make_unique<ValidationSignals>(std::make_unique<SerialTaskRunner>(scheduler));
    auto& validation_signals = *node.validation_signals;

    // Create client interfaces for wallets that are supposed to be loaded
    // according to -wallet and -disablewallet options. This only constructs
    // the interfaces, it doesn't load wallet data. Wallets actually get loaded
    // when load() and start() interface methods are called below.
    g_wallet_init_interface.Construct(node);
    uiInterface.InitWallet();

    if (interfaces::Ipc* ipc = node.init->ipc()) {
        for (std::string address : gArgs.GetArgs("-ipcbind")) {
            try {
                ipc->listenAddress(address);
            } catch (const std::exception& e) {
                return InitError(Untranslated(strprintf("Unable to bind to IPC address '%s'. %s", address, e.what())));