Merge bitcoin#29798: Logging cleanup

a7432dd logging: clarify -debug and -debugexclude descriptions (Anthony Towns)
74dd33c rpc: make logging method reject "0" category and correct the help text (Vasil Dimov)
8c6f3bf logging, refactor: minor encapsulation improvement and use BCLog::NONE instead of 0 (Vasil Dimov)
160706a logging, refactor: make category special cases explicit (Ryan Ofsky)

Pull request description:

  * Move special cases from `LOG_CATEGORIES_BY_STR` to `GetLogCategory()` (suggested [here](bitcoin#29419 (comment))).

  * Remove `"none"` and `"0"` from RPC `logging` help because that help text was wrong. `"none"` resulted in an error and `"0"` was ignored itself (contrary to what the help text suggested).

  * Remove unused `LOG_CATEGORIES_BY_STR[""]` (suggested [here](bitcoin#29419 (comment))).

  This is a followup to bitcoin#29419, addressing leftover suggestions + more.

ACKs for top commit:
  LarryRuane:
    ACK a7432dd
  ryanofsky:
    Code review ACK a7432dd. Only changes since last review are removing dead if statement and adding AJ's suggested -debug and -debugexclude help improvements, which look accurate and much more clear.

Tree-SHA512: 41b997b06fccdb4c1d31f57d4752c83caa744cb3280276a337ef4a9b7012a04eb945071db6b8fad24c6a6cf8761f2f800fe6d8f3d8836f5b39c25e4f11c85bf0

Author: ryanofsky

src/init/common.cpp

@@ -27,9 +27,9 @@ void AddLoggingArgs(ArgsManager& argsman)
 {
     argsman.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file (default: %s). Relative paths will be prefixed by a net-specific datadir location. Pass -nodebuglogfile to disable writing the log to a file.", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-debug=<category>", "Output debug and trace logging (default: -nodebug, supplying <category> is optional). "
-        "If <category> is not supplied or if <category> = 1, output all debug and trace logging. <category> can be: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.",
+        "If <category> is not supplied or if <category> is 1 or \"all\", output all debug logging. If <category> is 0 or \"none\", any other categories are ignored. Other valid values for <category> are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.",
         ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories.", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories. This takes priority over \"-debug\"", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logips", strprintf("Include IP addresses in debug output (default: %u)", DEFAULT_LOGIPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-loglevel=<level>|<category>:<level>", strprintf("Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC. Possible values are %s (default=%s). The following levels are always logged: error, warning, info. If <category>:<level> is supplied, the setting will override the global one and may be specified multiple times to set multiple category-specific levels. <category> can be: %s.", LogInstance().LogLevelsString(), LogInstance().LogLevelToStr(BCLog::DEFAULT_LOG_LEVEL), LogInstance().LogCategoriesString()), ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logtimestamps", strprintf("Prepend debug output with timestamp (default: %u)", DEFAULT_LOGTIMESTAMPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);

src/logging.cpp

@@ -168,8 +168,6 @@ bool BCLog::Logger::DefaultShrinkDebugFile() const
 }
 
 static const std::map<std::string, BCLog::LogFlags, std::less<>> LOG_CATEGORIES_BY_STR{
-    {"0", BCLog::NONE},
-    {"", BCLog::NONE},
     {"net", BCLog::NET},
     {"tor", BCLog::TOR},
     {"mempool", BCLog::MEMPOOL},
@@ -201,28 +199,23 @@ static const std::map<std::string, BCLog::LogFlags, std::less<>> LOG_CATEGORIES_
     {"txreconciliation", BCLog::TXRECONCILIATION},
     {"scan", BCLog::SCAN},
     {"txpackages", BCLog::TXPACKAGES},
-    {"1", BCLog::ALL},
-    {"all", BCLog::ALL},
 };
 
 static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_FLAG{
     // Swap keys and values from LOG_CATEGORIES_BY_STR.
     [](const auto& in) {
         std::unordered_map<BCLog::LogFlags, std::string> out;
         for (const auto& [k, v] : in) {
-            switch (v) {
-            case BCLog::NONE: out.emplace(BCLog::NONE, ""); break;
-            case BCLog::ALL: out.emplace(BCLog::ALL, "all"); break;
-            default: out.emplace(v, k);
-            }
+            const bool inserted{out.emplace(v, k).second};
+            assert(inserted);
         }
         return out;
     }(LOG_CATEGORIES_BY_STR)
 };
 
 bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str)
 {
-    if (str.empty()) {
+    if (str.empty() || str == "1" || str == "all") {
         flag = BCLog::ALL;
         return true;
     }
@@ -251,8 +244,11 @@ std::string BCLog::Logger::LogLevelToStr(BCLog::Level level)
     assert(false);
 }
 
-std::string LogCategoryToStr(BCLog::LogFlags category)
+static std::string LogCategoryToStr(BCLog::LogFlags category)
 {
+    if (category == BCLog::ALL) {
+        return "all";
+    }
     auto it = LOG_CATEGORIES_BY_FLAG.find(category);
     assert(it != LOG_CATEGORIES_BY_FLAG.end());
     return it->second;
@@ -278,10 +274,9 @@ static std::optional<BCLog::Level> GetLogLevel(std::string_view level_str)
 std::vector<LogCategory> BCLog::Logger::LogCategoriesList() const
 {
     std::vector<LogCategory> ret;
+    ret.reserve(LOG_CATEGORIES_BY_STR.size());
     for (const auto& [category, flag] : LOG_CATEGORIES_BY_STR) {
-        if (flag != BCLog::NONE && flag != BCLog::ALL) {
-            ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});
-        }
+        ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});
     }
     return ret;
 }

src/logging.h

@@ -119,7 +119,7 @@ namespace BCLog {
         std::atomic<Level> m_log_level{DEFAULT_LOG_LEVEL};
 
         /** Log categories bitfield. */
-        std::atomic<uint32_t> m_categories{0};
+        std::atomic<uint32_t> m_categories{BCLog::NONE};
 
         void FormatLogStrInPlace(std::string& str, LogFlags category, Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const;
 
@@ -132,6 +132,8 @@ namespace BCLog {
         void LogPrintStr_(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
             EXCLUSIVE_LOCKS_REQUIRED(m_cs);
 
+        std::string GetLogPrefix(LogFlags category, Level level) const;
+
     public:
         bool m_print_to_console = false;
         bool m_print_to_file = false;
@@ -145,8 +147,6 @@ namespace BCLog {
         fs::path m_file_path;
         std::atomic<bool> m_reopen_file{false};
 
-        std::string GetLogPrefix(LogFlags category, Level level) const;
-
         /** Send a string to the log output */
         void LogPrintStr(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
             EXCLUSIVE_LOCKS_REQUIRED(!m_cs);

src/rpc/node.cpp

@@ -221,7 +221,6 @@ static RPCHelpMan logging()
             "The valid logging categories are: " + LogInstance().LogCategoriesString() + "\n"
             "In addition, the following are available as category names with special meanings:\n"
             "  - \"all\",  \"1\" : represent all logging categories.\n"
-            "  - \"none\", \"0\" : even if other logging categories are specified, ignore all of them.\n"
             ,
                 {
                     {"include", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to debug logging",