Merge bitcoin/bitcoin#27632: Raise on invalid -debug and -loglevel config options

daa5a658c0e79172e4dea0758246f11281790d29 refactor: rename BCLog::BLOCKSTORE to BLOCKSTORAGE (Jon Atack)
cf622b214bfe0a97e403f1e9dc54bf5bbfc59fc3 doc: release note re raising on invalid -debug/debugexclude/loglevel (Jon Atack)
6cb1c66041ee14dbedad3aeeb90190ea5dddf917 init: remove config option names from translated -loglevel strings (Jon Atack)
25478292726dd7208b22a8924c8f1fdeac5c33f5 test: -loglevel raises on invalid values (Jon Atack)
a9c295888b82c86ef4629aa2d9061ea152b48f20 init: raise on invalid loglevel config option (Jon Atack)
b0c3995393c592fa96306e077ed64e65d5400882 test: -debug and -debugexclude raise on invalid values (Jon Atack)
4c3c19d943a0a4cf191495f6ebe9b964835607a4 init: raise on invalid debug/debugexclude config options (Jon Atack)

Pull request description:

  and rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum is the same as its value like the other BCLog enums.

  Per discussion in bitcoin-core-dev IRC today from https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-05-11#921458.

ACKs for top commit:
  achow101:
    ACK daa5a658c0e79172e4dea0758246f11281790d29
  ryanofsky:
    Code review ACK daa5a658c0e79172e4dea0758246f11281790d29. Just translated string template cleanup since last review
  pinheadmz:
    re-ACK daa5a658c0e79172e4dea0758246f11281790d29

Tree-SHA512: 4c107a93d8e8ce4e2ee81d44aec672526ca354ec390b241221067f68204beac8b4ba7a65748bcfa124ff2245c4307fa9243ec4fe0b464d0fa69c787fb322c3cc

9 files changed, 67 insertions, 18 deletions

diff --git a/doc/release-notes-27632.md b/doc/release-notes-27632.md
new file mode 100644
index 0000000000..d588247c85
--- /dev/null
+++ b/doc/release-notes-27632.md
@@ -0,0 +1,5 @@
+Updated settings
+----------------
+
+- Passing an invalid `-debug`, `-debugexclude`, or `-loglevel` logging configuration
+  option now raises an error, rather than logging an easily missed warning. (#27632)
diff --git a/src/init.cpp b/src/init.cpp
index 06c8d38036..410c574235 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -77,6 +77,7 @@
 #include <util/fs.h>
 #include <util/fs_helpers.h>
 #include <util/moneystr.h>
+#include <util/result.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/syscall_sandbox.h>
@@ -951,8 +952,10 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
         InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
 
     // ********************************************************* Step 3: parameter-to-internal-flags
-    init::SetLoggingCategories(args);
-    init::SetLoggingLevel(args);
+    auto result = init::SetLoggingCategories(args);
+    if (!result) return InitError(util::ErrorString(result));
+    result = init::SetLoggingLevel(args);
+    if (!result) return InitError(util::ErrorString(result));
 
     nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT);
     if (nConnectTimeout <= 0) {
diff --git a/src/init/common.cpp b/src/init/common.cpp
index 9a52a09cea..f3f7c696c5 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -13,6 +13,7 @@
 #include <tinyformat.h>
 #include <util/fs.h>
 #include <util/fs_helpers.h>
+#include <util/result.h>
 #include <util/string.h>
 #include <util/time.h>
 #include <util/translation.h>
@@ -58,27 +59,28 @@ void SetLoggingOptions(const ArgsManager& args)
     fLogIPs = args.GetBoolArg("-logips", DEFAULT_LOGIPS);
 }
 
-void SetLoggingLevel(const ArgsManager& args)
+util::Result<void> SetLoggingLevel(const ArgsManager& args)
 {
     if (args.IsArgSet("-loglevel")) {
         for (const std::string& level_str : args.GetArgs("-loglevel")) {
             if (level_str.find_first_of(':', 3) == std::string::npos) {
                 // user passed a global log level, i.e. -loglevel=<level>
                 if (!LogInstance().SetLogLevel(level_str)) {
-                    InitWarning(strprintf(_("Unsupported global logging level -loglevel=%s. Valid values: %s."), level_str, LogInstance().LogLevelsString()));
+                    return util::Error{strprintf(_("Unsupported global logging level %s=%s. Valid values: %s."), "-loglevel", level_str, LogInstance().LogLevelsString())};
                 }
             } else {
                 // user passed a category-specific log level, i.e. -loglevel=<category>:<level>
                 const auto& toks = SplitString(level_str, ':');
                 if (!(toks.size() == 2 && LogInstance().SetCategoryLogLevel(toks[0], toks[1]))) {
-                    InitWarning(strprintf(_("Unsupported category-specific logging level -loglevel=%s. Expected -loglevel=<category>:<loglevel>. Valid categories: %s. Valid loglevels: %s."), level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString()));
+                    return util::Error{strprintf(_("Unsupported category-specific logging level %1$s=%2$s. Expected %1$s=<category>:<loglevel>. Valid categories: %3$s. Valid loglevels: %4$s."), "-loglevel", level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())};
                 }
             }
         }
     }
+    return {};
 }
 
-void SetLoggingCategories(const ArgsManager& args)
+util::Result<void> SetLoggingCategories(const ArgsManager& args)
 {
     if (args.IsArgSet("-debug")) {
         // Special-case: if -debug=0/-nodebug is set, turn off debugging messages
@@ -88,7 +90,7 @@ void SetLoggingCategories(const ArgsManager& args)
             [](std::string cat){return cat == "0" || cat == "none";})) {
             for (const auto& cat : categories) {
                 if (!LogInstance().EnableCategory(cat)) {
-                    InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debug", cat));
+                    return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)};
                 }
             }
         }
@@ -97,9 +99,10 @@ void SetLoggingCategories(const ArgsManager& args)
     // Now remove the logging categories which were explicitly excluded
     for (const std::string& cat : args.GetArgs("-debugexclude")) {
         if (!LogInstance().DisableCategory(cat)) {
-            InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat));
+            return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat)};
         }
     }
+    return {};
 }
 
 bool StartLogging(const ArgsManager& args)
diff --git a/src/init/common.h b/src/init/common.h
index 44c3a502ee..b61a77c6d4 100644
--- a/src/init/common.h
+++ b/src/init/common.h
@@ -8,13 +8,15 @@
 #ifndef BITCOIN_INIT_COMMON_H
 #define BITCOIN_INIT_COMMON_H
 
+#include <util/result.h>
+
 class ArgsManager;
 
 namespace init {
 void AddLoggingArgs(ArgsManager& args);
 void SetLoggingOptions(const ArgsManager& args);
-void SetLoggingCategories(const ArgsManager& args);
-void SetLoggingLevel(const ArgsManager& args);
+[[nodiscard]] util::Result<void> SetLoggingCategories(const ArgsManager& args);
+[[nodiscard]] util::Result<void> SetLoggingLevel(const ArgsManager& args);
 bool StartLogging(const ArgsManager& args);
 void LogPackageVersion();
 } // namespace init
diff --git a/src/logging.cpp b/src/logging.cpp
index 7725fefeb7..a5cfb0d28e 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -179,7 +179,7 @@ const CLogCategoryDesc LogCategories[] =
     {BCLog::LOCK, "lock"},
 #endif
     {BCLog::UTIL, "util"},
-    {BCLog::BLOCKSTORE, "blockstorage"},
+    {BCLog::BLOCKSTORAGE, "blockstorage"},
     {BCLog::TXRECONCILIATION, "txreconciliation"},
     {BCLog::SCAN, "scan"},
     {BCLog::ALL, "1"},
@@ -280,7 +280,7 @@ std::string LogCategoryToStr(BCLog::LogFlags category)
 #endif
     case BCLog::LogFlags::UTIL:
         return "util";
-    case BCLog::LogFlags::BLOCKSTORE:
+    case BCLog::LogFlags::BLOCKSTORAGE:
         return "blockstorage";
     case BCLog::LogFlags::TXRECONCILIATION:
         return "txreconciliation";
diff --git a/src/logging.h b/src/logging.h
index e7c554e79f..fc03c8eac3 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -65,7 +65,7 @@ namespace BCLog {
         LOCK        = (1 << 24),
 #endif
         UTIL        = (1 << 25),
-        BLOCKSTORE  = (1 << 26),
+        BLOCKSTORAGE = (1 << 26),
         TXRECONCILIATION = (1 << 27),
         SCAN        = (1 << 28),
         ALL         = ~(uint32_t)0,
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 1368ae6f6d..87cd291c7e 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -573,7 +573,7 @@ void BlockManager::UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const
         const bool removed_blockfile{fs::remove(BlockFileSeq().FileName(pos), ec)};
         const bool removed_undofile{fs::remove(UndoFileSeq().FileName(pos), ec)};
         if (removed_blockfile || removed_undofile) {
-            LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
+            LogPrint(BCLog::BLOCKSTORAGE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
         }
     }
 }
@@ -642,7 +642,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
 
     if ((int)nFile != m_last_blockfile) {
         if (!fKnown) {
-            LogPrint(BCLog::BLOCKSTORE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString());
+            LogPrint(BCLog::BLOCKSTORAGE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString());
         }
         FlushBlockFile(!fKnown, finalize_undo);
         m_last_blockfile = nFile;
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index beb9398c74..2699d316da 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -200,7 +200,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
         const char* argv_test[] = {"bitcoind", "-loglevel=debug"};
         std::string err;
         BOOST_REQUIRE(args.ParseParameters(2, argv_test, err));
-        init::SetLoggingLevel(args);
+
+        auto result = init::SetLoggingLevel(args);
+        BOOST_REQUIRE(result);
         BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::Level::Debug);
     }
 
@@ -212,7 +214,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
         const char* argv_test[] = {"bitcoind", "-loglevel=net:trace"};
         std::string err;
         BOOST_REQUIRE(args.ParseParameters(2, argv_test, err));
-        init::SetLoggingLevel(args);
+
+        auto result = init::SetLoggingLevel(args);
+        BOOST_REQUIRE(result);
         BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::DEFAULT_LOG_LEVEL);
 
         const auto& category_levels{LogInstance().CategoryLevels()};
@@ -229,7 +233,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
         const char* argv_test[] = {"bitcoind", "-loglevel=debug", "-loglevel=net:trace", "-loglevel=http:info"};
         std::string err;
         BOOST_REQUIRE(args.ParseParameters(4, argv_test, err));
-        init::SetLoggingLevel(args);
+
+        auto result = init::SetLoggingLevel(args);
+        BOOST_REQUIRE(result);
         BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::Level::Debug);
 
         const auto& category_levels{LogInstance().CategoryLevels()};
diff --git a/test/functional/feature_logging.py b/test/functional/feature_logging.py
index fe4f02dfe6..b0788e2a2d 100755
--- a/test/functional/feature_logging.py
+++ b/test/functional/feature_logging.py
@@ -69,6 +69,36 @@ class LoggingTest(BitcoinTestFramework):
         # just sanity check no crash here
         self.restart_node(0, [f"-debuglogfile={os.devnull}"])
 
+        self.log.info("Test -debug and -debugexclude raise when invalid values are passed")
+        self.stop_node(0)
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=["-debug=abc"],
+            expected_msg="Error: Unsupported logging category -debug=abc.",
+            match=ErrorMatch.FULL_REGEX,
+        )
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=["-debugexclude=abc"],
+            expected_msg="Error: Unsupported logging category -debugexclude=abc.",
+            match=ErrorMatch.FULL_REGEX,
+        )
+
+        self.log.info("Test -loglevel raises when invalid values are passed")
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=["-loglevel=abc"],
+            expected_msg="Error: Unsupported global logging level -loglevel=abc. Valid values: info, debug, trace.",
+            match=ErrorMatch.FULL_REGEX,
+        )
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=["-loglevel=net:abc"],
+            expected_msg="Error: Unsupported category-specific logging level -loglevel=net:abc.",
+            match=ErrorMatch.PARTIAL_REGEX,
+        )
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=["-loglevel=net:info:abc"],
+            expected_msg="Error: Unsupported category-specific logging level -loglevel=net:info:abc.",
+            match=ErrorMatch.PARTIAL_REGEX,
+        )
+
 
 if __name__ == '__main__':
     LoggingTest().main()