log, refactor: Disallow category args to logging macros

Disallow passing BCLog category constants to LogInfo(), LogWarning(), and
LogError() macros, and disallow omitting BCLog categories when calling
LogDebug() and LogTrace() macros.

Additionally, drop category information from log output at higher levels as
suggested by Hodlinator
bitcoin#29256 (comment).

None of these restrictions are technically necessary, and not everybody
believes they are good. However, they have existed since the
Log{Trace,Debug,Info,Warning,Error} macros were introduced in bitcoin#28318, and
removing these restrictions is orthogonal to the goals of this PR which are:
(1) to allow the Log macros to work without accessing global state and (2) to
support local logging customization with additional metadata.

This change just more clearly documents the current logging restrictions and
provides better error messages to developers when enforcing them.

Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>

Author: ryanofsky

src/logging.h

@@ -424,11 +424,38 @@ namespace BCLog {
 
 namespace detail {
 //! Internal helper to get log context object from the first macro argument.
-template <typename Context>
+template <bool take_category, typename Context>
 requires (Context::log_context)
 static const Context& GetContext(const Context& ctx LIFETIMEBOUND) { return ctx; }
-static inline Context GetContext(LogFlags category) { return Context{LogInstance(), category}; }
-static inline Context GetContext(std::string_view fmt) { return Context{LogInstance()}; }
+
+template <bool take_category>
+static const Context& GetContext(const Context& ctx LIFETIMEBOUND) { return ctx; }
+
+template <bool take_category>
+static Context GetContext(LogFlags category)
+{
+    //! Trigger compile error if caller tries to pass a category constant as a
+    //! first argument to a logging call that specifies take_category == false.
+    //! There is no technical reason why all logging calls could not accept
+    //! category arguments, but for various reasons, such as (1) not wanting to
+    //! allow users filter by category at high priority levels, and (2) wanting
+    //! to incentivize developers to use lower log levels to avoid log spam,
+    //! passing category constants at higher levels is forbidden.
+    static_assert(take_category, "Cannot pass BCLog::LogFlags category argument to Info/Warning/Error logging call. Please switch to Debug/Trace call, or drop the category argument!");
+    return Context{LogInstance(), category};
+}
+
+template <bool take_category>
+static Context GetContext(std::string_view fmt)
+{
+    //! Trigger compile error if caller does not pass a category constant as the
+    //! first argument to a logging call that specifies take_category == true.
+    //! There is no technical reason why category arguments need to be required,
+    //! but categories are useful for finding and filtering relevant messages
+    //! when debugging, so we want to encourage them.
+    static_assert(!take_category, "Missing required BCLog::LogFlags category argument for Debug/Trace logging call. Category can only be omitted for Info/Warning/Error calls.");
+    return Context{LogInstance()};
+}
 
 //! Internal helper to format log arguments and call a logging function.
 template <typename LogFn, typename Context, typename ContextArg, typename... Args>
@@ -526,11 +553,11 @@ inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags
 //! Internal helper to conditionally log. Only evaluates arguments when needed.
 // Allow __func__ to be used in any context without warnings:
 // NOLINTBEGIN(bugprone-lambda-function-name)
-#define LogPrint_(level, ratelimit, ...)                                       \
+#define LogPrint_(level, ratelimit, take_category, ...)                        \
     do {                                                                       \
-        const auto& ctx{BCLog::detail::GetContext(FirstArg_((__VA_ARGS__)))};  \
+        const auto& ctx{BCLog::detail::GetContext<take_category>(FirstArg_((__VA_ARGS__)))}; \
         if (const BCLog::Level lvl{level}; LogEnabled(ctx, lvl)) {             \
-            const BCLog::LogFlags cat{ctx.category};                           \
+            const BCLog::LogFlags cat{take_category ? ctx.category : BCLog::LogFlags::ALL}; \
             const bool rl{ratelimit};                                          \
             SourceLocation loc{SourceLocation{__func__}};                      \
             BCLog::detail::Format([&](auto&& message) {                        \
@@ -580,12 +607,12 @@ inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags
 //! disk filling attacks. Users enabling logging at Debug and lower levels are
 //! assumed to be developers or power users who are aware that -debug may cause
 //! excessive disk usage due to logging.
-#define LogError(...) LogPrint_(BCLog::Level::Error, true, __VA_ARGS__)
-#define LogWarning(...) LogPrint_(BCLog::Level::Warning, true, __VA_ARGS__)
-#define LogInfo(...) LogPrint_(BCLog::Level::Info, true, __VA_ARGS__)
-#define LogDebug(...) LogPrint_(BCLog::Level::Debug, false, __VA_ARGS__)
-#define LogTrace(...) LogPrint_(BCLog::Level::Trace, false, __VA_ARGS__)
-#define LogPrintLevel_(ctx, level, ratelimit, ...) LogPrint_((level), (ratelimit), (ctx), __VA_ARGS__)
+#define LogError(...) LogPrint_(BCLog::Level::Error, true, false, __VA_ARGS__)
+#define LogWarning(...) LogPrint_(BCLog::Level::Warning, true, false, __VA_ARGS__)
+#define LogInfo(...) LogPrint_(BCLog::Level::Info, true, false, __VA_ARGS__)
+#define LogDebug(...) LogPrint_(BCLog::Level::Debug, false, true, __VA_ARGS__)
+#define LogTrace(...) LogPrint_(BCLog::Level::Trace, false, true, __VA_ARGS__)
+#define LogPrintLevel_(ctx, level, ratelimit, ...) LogPrint_((level), (ratelimit), true, (ctx), __VA_ARGS__)
 
 >>>>>>> 05d22deffdb (log, refactor: Allow log macros to accept context arguments)
 #endif // BITCOIN_LOGGING_H

src/test/logging_tests.cpp

@@ -122,9 +122,9 @@ BOOST_AUTO_TEST_CASE(logging_local_logger)
     LogTrace(log, "trace %s", "arg");
 
     constexpr auto expected{std::to_array({
-        "[net:error] error arg\n",
-        "[net:warning] warning arg\n",
-        "[net:info] info arg\n",
+        "[error] error arg\n",
+        "[warning] warning arg\n",
+        "info arg\n",
         "[net] debug arg\n",
         "[net:trace] trace arg\n",
     })};
@@ -141,13 +141,23 @@ BOOST_FIXTURE_TEST_CASE(logging_context_args, LogSetup)
     LogError("error");
     LogWarning("warning");
     LogInfo("info");
+    // LogDebug("debug"); // Not allowed because category is required!
+    // LogTrace("trace"); // Not allowed because category is required!
     LogError("error %s", "arg");
     LogWarning("warning %s", "arg");
     LogInfo("info %s", "arg");
+    // LogDebug("debug %s", "arg"); // Not allowed because category is required!
+    // LogTrace("trace %s", "arg"); // Not allowed because category is required!
 
     // Test logging with category constant arguments.
+    // LogError(BCLog::NET, "error"); // Not allowed because category is forbidden!
+    // LogWarning(BCLog::NET, "warning"); // Not allowed because category is forbidden!
+    // LogInfo(BCLog::NET, "info"); // Not allowed because category is forbidden!
     LogDebug(BCLog::NET, "debug");
     LogTrace(BCLog::NET, "trace");
+    // LogError(BCLog::NET, "error %s", "arg"); // Not allowed because category is forbidden!
+    // LogWarning(BCLog::NET, "warning %s", "arg"); // Not allowed because category is forbidden!
+    // LogInfo(BCLog::NET, "info %s", "arg"); // Not allowed because category is forbidden!
     LogDebug(BCLog::NET, "debug %s", "arg");
     LogTrace(BCLog::NET, "trace %s", "arg");
 
@@ -179,15 +189,15 @@ BOOST_FIXTURE_TEST_CASE(logging_context_args, LogSetup)
         "[net] debug arg",
         "[net:trace] trace arg",
 
-        "[tor:error] error",
-        "[tor:warning] warning",
-        "[tor:info] info",
+        "[error] error",
+        "[warning] warning",
+        "info",
         "[tor] debug",
         "[tor:trace] trace",
 
-        "[tor:error] error arg",
-        "[tor:warning] warning arg",
-        "[tor:info] info arg",
+        "[error] error arg",
+        "[warning] warning arg",
+        "info arg",
         "[tor] debug arg",
         "[tor:trace] trace arg",
     })};
@@ -232,9 +242,9 @@ BOOST_FIXTURE_TEST_CASE(logging_CustomContext, LogSetup)
     }
     constexpr auto expected{std::to_array({
         "[validation] Custom #1 foo1: bar1",
-        "[validation:info] Custom #2 foo2: bar2",
-        "[validation:warning] Custom #3 foo3: bar3",
-        "[validation:error] Custom #4 foo4: bar4",
+        "Custom #2 foo2: bar2",
+        "[warning] Custom #3 foo3: bar3",
+        "[error] Custom #4 foo4: bar4",
     })};
     BOOST_CHECK_EQUAL_COLLECTIONS(log_lines.begin(), log_lines.end(), expected.begin(), expected.end());
 }