Raise on invalid -debug and -loglevel config options #27632

jonatack commented at 11:25 PM on May 11, 2023: member

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.

DrahtBot commented at 11:25 PM on May 11, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, pinheadmz, achow101
Concept ACK	jamesob
Stale ACK	vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27231 (Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs by jonatack)
#26697 (logging: use bitset for categories by LarryRuane)
#10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jamesob commented at 1:18 AM on May 12, 2023: member

Concept ACK; trying to -debug= a nonexistent category bit me earlier today.

in src/init/common.h:21 in b9935a4ad5 outdated

  18 |  void AddLoggingArgs(ArgsManager& args);
  19 |  void SetLoggingOptions(const ArgsManager& args);
  20 | -void SetLoggingCategories(const ArgsManager& args);
  21 | -void SetLoggingLevel(const ArgsManager& args);
  22 | +std::optional<bilingual_str> SetLoggingCategories(const ArgsManager& args);
  23 | +std::optional<bilingual_str> SetLoggingLevel(const ArgsManager& args);

vasild commented at 7:26 AM on May 12, 2023:

nit: document those as the return value may not be so obvious to everybody: "if a non-empty optional is returned then an error occurred and it contains the error message".

ryanofsky commented at 12:35 PM on May 12, 2023:

nit: document those as the return value may not be so obvious to everybody: "if a non-empty optional is returned then an error occurred and it contains the error message".

It's good to document, but this convention does seem to be increasingly used. I have a PR to replace std::optional<bilingual_str> with util::Result various places (74655e5870888e0165c62fec75000fe04f062147, #25977)

vasild commented at 12:55 PM on May 12, 2023:

Using util::Result here would be self-documenting.

jonatack commented at 6:53 PM on May 12, 2023:

Thanks @vasild and @ryanofsky. Updated to use util::Result instead.

vasild approved

vasild commented at 7:29 AM on May 12, 2023: contributor

ACK b9935a4ad512cc0d310d1bd363f71f430b787dbb

jonatack force-pushed on May 12, 2023

vasild approved

vasild commented at 8:30 AM on May 15, 2023: contributor

ACK 9c74c94714465ac66d82a7e233312d188c9b2841

in test/functional/feature_logging.py:84 in 9c74c94714 outdated

  79 | +        self.log.info("Test -loglevel raises when invalid values are passed")
  80 | +        exp_stderr = r"Error: Unsupported global logging level -loglevel=abc. Valid values: info, debug, trace."
  81 | +        self.nodes[0].assert_start_raises_init_error(["-loglevel=abc"], exp_stderr, match=ErrorMatch.FULL_REGEX)
  82 | +        exp_stderr = r"Error: Unsupported category-specific logging level -loglevel=net:abc."
  83 | +        self.nodes[0].assert_start_raises_init_error(["-loglevel=net:abc"], exp_stderr, match=ErrorMatch.PARTIAL_REGEX)
  84 | +        exp_stderr = r"Error: Unsupported category-specific logging level -loglevel=net:info:abc."

maflcko commented at 8:58 AM on May 16, 2023:

nit: Any reason to use r-strings when there is no need to and they don't contain any \ chars?

jonatack commented at 2:36 PM on May 31, 2023:

Fixed, thank you.

in src/init/common.cpp:92 in effc523d70 outdated

  88 | @@ -88,7 +89,7 @@ void SetLoggingCategories(const ArgsManager& args)
  89 |              [](std::string cat){return cat == "0" || cat == "none";})) {
  90 |              for (const auto& cat : categories) {
  91 |                  if (!LogInstance().EnableCategory(cat)) {
  92 | -                    InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debug", cat));
  93 | +                    return util::Error{Untranslated(strprintf("Unsupported logging category -debug=%s", cat))};

maflcko commented at 9:07 AM on May 16, 2023:

q: Can you explain why it is fine to drop the translation?

jonatack commented at 2:37 PM on May 31, 2023:

Kept the translation -- thank you.

in src/init/common.cpp:104 in effc523d70 outdated

  97 | @@ -97,9 +98,10 @@ void SetLoggingCategories(const ArgsManager& args)
  98 |      // Now remove the logging categories which were explicitly excluded
  99 |      for (const std::string& cat : args.GetArgs("-debugexclude")) {
 100 |          if (!LogInstance().DisableCategory(cat)) {
 101 | -            InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat));
 102 | +            return util::Error{Untranslated(strprintf("Unsupported logging category -debugexclude=%s", cat))};
 103 |          }
 104 |      }
 105 | +    return true;

maflcko commented at 9:08 AM on May 16, 2023:

Not sure if we want to use Result before it supports <void>, but I guess it will be less code to change in the future, compared to std::optional<bilingual_str>, so I guess it is fine for now.

maflcko commented at 7:26 PM on May 16, 2023:

unrelated: A minimal (but hacky) way to add support for void to Result would be:

diff --git a/src/util/result.h b/src/util/result.h
index 972b1aada0..b99995c7e5 100644
--- a/src/util/result.h
+++ b/src/util/result.h
@@ -31,16 +31,19 @@ struct Error {
 //! `std::optional<T>` can be updated to return `util::Result<T>` and return
 //! error strings usually just replacing `return std::nullopt;` with `return
 //! util::Error{error_string};`.
-template <class T>
+template <class M>
 class Result
 {
 private:
+    using T = std::conditional_t<std::is_same_v<M, void>, std::monostate, M>;
+
     std::variant<bilingual_str, T> m_variant;
 
     template <typename FT>
     friend bilingual_str ErrorString(const Result<FT>& result);
 
 public:
+    Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {}  // constructor for void
     Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}
     Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {}

jonatack commented at 2:38 PM on May 31, 2023:

Rebased to current master containing 5f49cb1 (#25977) to be able to use Result with support for <void>, and updated the return types from bool to void.

ryanofsky referenced this in commit 5f49cb1bc8 on May 24, 2023

fanquake referenced this in commit 4d13fe47be on May 26, 2023

sidhujag referenced this in commit 20272f34e0 on May 26, 2023

jonatack force-pushed on May 31, 2023

jonatack commented at 8:18 AM on June 1, 2023: member

Thank you for the review feedback, @MarcoFalke; addressed, and rebased to current master containing 5f49cb1 (#25977) to use Result with support for <void>. Invalidates previous ACK by @vasild (thanks!)

achow101 commented at 8:20 PM on June 1, 2023: member

ACK 1a27bec1e9e40166d6ff7f0492115107289e7609

DrahtBot requested review from vasild on Jun 1, 2023

pinheadmz requested review from pinheadmz on Jun 2, 2023

in src/test/logging_tests.cpp:205 in 24a9d70139 outdated

 199 | @@ -200,7 +200,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
 200 |          const char* argv_test[] = {"bitcoind", "-loglevel=debug"};
 201 |          std::string err;
 202 |          BOOST_REQUIRE(args.ParseParameters(2, argv_test, err));
 203 | -        init::SetLoggingLevel(args);
 204 | +
 205 | +        auto result = init::SetLoggingLevel(args);
 206 | +        BOOST_REQUIRE(result);

pinheadmz commented at 4:18 PM on June 2, 2023:

If you're touching unit tests anyway, why not add a test for invalid args here as well?

jonatack commented at 9:55 PM on June 13, 2023:

The unit tests were touched for the nodiscard attribute. End-to-end functional tests seem to be the usual way we test for raising on an invalid config. I didn't look if they can be replaced by faster unit tests. Might be a follow-up if you're interested?

pinheadmz approved

pinheadmz commented at 4:27 PM on June 2, 2023: member

ACK 1a27bec1e9e40166d6ff7f0492115107289e7609

Reviewed code and ran tests locally. Confirmed expected behavior locally with regtest:

--> bcd -regtest -debug=poop
Error: Unsupported logging category -debug=poop
--> bcd -regtest -loglevel=poop
Error: Unsupported global logging level -loglevel=poop. Valid values: info, debug, trace.

Just had a question about workflow/testing below, not a deal breaker.

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

in src/init/common.cpp:92 in 398aa75f94 outdated

  88 | @@ -88,7 +89,7 @@ void SetLoggingCategories(const ArgsManager& args)
  89 |              [](std::string cat){return cat == "0" || cat == "none";})) {
  90 |              for (const auto& cat : categories) {
  91 |                  if (!LogInstance().EnableCategory(cat)) {
  92 | -                    InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debug", cat));
  93 | +                    return util::Error{strprintf(_("Unsupported logging category -debug=%s"), cat)};

ryanofsky commented at 1:02 PM on June 12, 2023:

In commit "init: raise on invalid debug/debugexclude config options" (398aa75f94114684dfee5ea5a2bc9ebe0860bb77)

This commit is adding command line argument names to a translated string, which is something we want to avoid to avoid translation errors. See #20404. Would be better to leave the previous strprintf in place.

Could also remove argument names from translated strings in later commit "init: raise on invalid loglevel config option" (24a9d701390c7fc0d20be44b68d752c051505df9)

jonatack commented at 11:17 PM on June 12, 2023:

Thanks @ryanofsky, I'll update per your feedback.

ryanofsky approved

ryanofsky commented at 1:23 PM on June 12, 2023: contributor

Code review ACK 1a27bec1e9e40166d6ff7f0492115107289e7609. I left a review comment about this adding some argument names to translated strings, which is bad but already happens other places, so it might not be very important to fix.

I do think it would be good if this PR added release notes, because the changes are not backwards compatible and will probably break some existing configurations or misconfigurations.

It also looks like this has had enough code review to be ready to merge. @jonatack you could respond and say if you'd prefer to merge this as-is or make more updates.

jonatack force-pushed on Jun 13, 2023

jonatack commented at 9:49 PM on June 13, 2023: member

Updated with @ryanofsky's review feedback (thanks!) -- the CI failures look unrelated and perhaps require rebasing to current master (edit: done).

DrahtBot added the label CI failed on Jun 13, 2023

jonatack force-pushed on Jun 13, 2023

in src/init/common.cpp:102 in 02756cbd63 outdated

  98 | @@ -97,9 +99,10 @@ void SetLoggingCategories(const ArgsManager& args)
  99 |      // Now remove the logging categories which were explicitly excluded
 100 |      for (const std::string& cat : args.GetArgs("-debugexclude")) {
 101 |          if (!LogInstance().DisableCategory(cat)) {
 102 | -            InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat));
 103 | +            return util::Error{strprintf(_("Unsupported logging category %s=%s"), "-debugexclude", cat)};

maflcko commented at 7:35 AM on June 14, 2023:

Any reason to remove the trailing dot? This will invalidate the translations and is inconsistent with the other Error messages in this pull?

jonatack commented at 12:28 PM on June 14, 2023:

Fixed, thanks.

jonatack force-pushed on Jun 14, 2023

jonatack commented at 12:35 PM on June 14, 2023: member

Updated with translation string simplifications and improvements and a release note per @ryanofsky and @MarcoFalke feedback and rebased for the CI. Diff since the previous ACKs: git range-diff 681ecac 1a27bec 8bbfeec

init: raise on invalid debug/debugexclude config options 4c3c19d943

test: -debug and -debugexclude raise on invalid values b0c3995393

init: raise on invalid loglevel config option a9c295888b

test: -loglevel raises on invalid values 2547829272

jonatack force-pushed on Jun 14, 2023

DrahtBot removed the label CI failed on Jun 14, 2023

in src/init/common.cpp:75 in 1798c21f8c outdated

  72 |              } else {
  73 |                  // user passed a category-specific log level, i.e. -loglevel=<category>:<level>
  74 |                  const auto& toks = SplitString(level_str, ':');
  75 |                  if (!(toks.size() == 2 && LogInstance().SetCategoryLogLevel(toks[0], toks[1]))) {
  76 | -                    return util::Error{strprintf(_("Unsupported category-specific logging level -loglevel=%s. Expected -loglevel=<category>:<loglevel>. Valid categories: %s. Valid loglevels: %s."), level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())};
  77 | +                    return util::Error{strprintf(_("Unsupported category-specific logging level %s=%s. Expected %s=<category>:<loglevel>. Valid categories: %s. Valid loglevels: %s."), "-loglevel", level_str, "-loglevel", LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())};

ryanofsky commented at 3:54 PM on June 15, 2023:

In commit "init: remove config option names from translated -loglevel strings" (1798c21f8c2557dbcfbcb8d33d6d70b0ea539a98)

Could use %1$s, %2$s format strings to remove the redundant "-loglevel" translation argument and remove ambiguity about which of the two arguments a translator is supposed to use.

jonatack commented at 4:31 PM on June 15, 2023:

Good idea, done (thanks!)

ryanofsky approved

ryanofsky commented at 3:56 PM on June 15, 2023: contributor

Code review ACK 8bbfeecdda5648b497fc0442c8929563ee70f301. Looks great! Changes since last review are keeping option names out of translated strings, and adding a release note

DrahtBot requested review from achow101 on Jun 15, 2023

DrahtBot requested review from pinheadmz on Jun 15, 2023

init: remove config option names from translated -loglevel strings 6cb1c66041

doc: release note re raising on invalid -debug/debugexclude/loglevel cf622b214b

refactor: rename BCLog::BLOCKSTORE to BLOCKSTORAGE

so the enum name is the same as its value, like the other BCLog enums.

daa5a658c0

jonatack force-pushed on Jun 15, 2023

jonatack commented at 4:34 PM on June 15, 2023: member

Updated to incorporate @ryanofsky's #27632 (review) to use format strings in https://github.com/bitcoin/bitcoin/pull/27632/commits/6cb1c66041ee14dbedad3aeeb90190ea5dddf917 to drop a redundant translation argument and remove ambiguity about which of the two arguments a translator is supposed to use (thanks!)

ryanofsky approved

ryanofsky commented at 4:29 PM on June 20, 2023: contributor

Code review ACK daa5a658c0e79172e4dea0758246f11281790d29. Just translated string template cleanup since last review

pinheadmz approved

pinheadmz commented at 5:01 PM on June 20, 2023: member

re-ACK daa5a658c0e79172e4dea0758246f11281790d29

Confirmed trivial change since last review with git range-diff 681ecac5c2 daa5a658c0 1a27bec1e9 rebuilt and reran tests as well

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

achow101 commented at 5:41 PM on June 20, 2023: member

ACK daa5a658c0e79172e4dea0758246f11281790d29

DrahtBot removed review request from achow101 on Jun 20, 2023

achow101 merged this on Jun 20, 2023

achow101 closed this on Jun 20, 2023

jonatack deleted the branch on Jun 20, 2023

sidhujag referenced this in commit ab0ed39dfa on Jun 21, 2023

Sjors commented at 9:42 AM on August 21, 2023: member

If you liked this PR, you may also like #27277 :-)

jonatack commented at 10:35 PM on August 21, 2023: member

If you liked this PR, you may also like #27277 :-)

Will have a look.

janus referenced this in commit 959a79cbab on Sep 11, 2023

Fabcien referenced this in commit e0df40da76 on Dec 15, 2023

bitcoin locked this on Aug 20, 2024