jonatack
commented at 1:40 am on March 9, 2023:
member
The logging RPC does not work as intended and as documented in its help when 0 or none are passed.
Per the RPC logging help:
0$ ./src/bitcoin-cli help logging
12In addition, the following are available as category names with special meanings:
3 - "all", "1" : represent all logging categories.
4 - "none", "0" : even if other logging categories are specified, ignore all of them.
The behavior and documentation were added in #11191, but over time and extended refactoring, the behavior no longer works in master and versions 22/23/24 (the last supported versions currently) – passing 0/none has no effect. As there was no test coverage, the regressions went uncaught. In v24, none became unrecognized:
During the same time period, passing 1 and all has been operational and documented.
Solution: detect none/0 values and add test coverage in any case, and either:
leave the functionality out, raise with an error message if the values are passed, and update the RPC help documentation, or
fix the behavior by returning early.
Both solutions involve essentially the same code size and complexity. Given that all/1 has been operational, and that none/0 has been documented and appears per the code of the original PR to have been operational, it seems preferable for consistency to support none/0 in symmetry with all/1 and as ACKed, intended, and documented in #11191.
Done by this pull:
add missing RPC logging logic and test coverage
update the -debug and -debugexclude config options to use the same code for consistency in behavior between them, and for consistency with the logging RPC that provides the same behavior for both -include and -exclude
improve the user-facing documentation and examples; can be tested with ./src/bitcoind -h | grep -A12 "\-debug=<\|debugexclude" && ./src/bitcoin-cli help logging
If it is decided to backport a fix, commit Fix RPC logging behavior when "0" and "none" values are passed could probably suffice.
DrahtBot
commented at 1:40 am on March 9, 2023:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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:
#28318 (logging: Simplify API for level based logging by ajtowns)
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.
DrahtBot added the label
RPC/REST/ZMQ
on Mar 9, 2023
jonatack marked this as ready for review
on Mar 9, 2023
It’s still not clear if this was broken from the time it was merged, or at some other later point. This also does not work with 22.x, and any earlier versions are EOL.
If it’s been broken forever (or at least is in all currently maintained releases), and no ones even noticed, we could take advantage of that, to remove some of the complexity here; do we definitely need multiple different ways of achieving the same logging toggling?
jonatack renamed this:
rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs
Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs
on Mar 10, 2023
jonatack force-pushed
on Mar 10, 2023
jonatack marked this as a draft
on Mar 10, 2023
jonatack force-pushed
on Mar 15, 2023
jonatack force-pushed
on Mar 16, 2023
DrahtBot added the label
Needs rebase
on Mar 23, 2023
jonatack force-pushed
on Mar 31, 2023
jonatack
commented at 4:04 pm on March 31, 2023:
member
Rebased.
could take advantage of that, to remove some of the complexity here; do we definitely need multiple different ways of achieving the same logging toggling?
Exploring this, it doesn’t look like there would be much code simplification gained by dropping none for 0 only, which would no longer be in symmetry with all/1 that have been operational for a long time. A couple lines could be saved by removing the -debugexclude config option, but that option is practical to have, as seen by its use in our own unit/fuzz and functional test frameworks.
It seems the way forward from here is to detect none/0 values, add test coverage and then either:
leave the functionality out, raise with an error message if none/0 values are passed, and update the RPC help documentation, or
simply fix the original behavior by returning early.
Both involve similar code size/complexity and the second option seems preferable. The updated code here is about the same length as before, modulo missing test coverage and improved documentation.
In the last pushes I’ve updated the PR description and first commit message with this info and improved the code and tests. This should be ready for further review!
jonatack marked this as ready for review
on Mar 31, 2023
DrahtBot removed the label
Needs rebase
on Apr 2, 2023
DrahtBot added the label
Needs rebase
on Apr 3, 2023
jonatack force-pushed
on Apr 4, 2023
DrahtBot removed the label
Needs rebase
on Apr 4, 2023
in
src/init/common.cpp:91
in
f868de5293outdated
99+ Assume(std::any_of(DEBUG_LOG_OPTS.cbegin(), DEBUG_LOG_OPTS.cend(), [&](const auto& d) { return opt == d; }));
100+ if (!args.IsArgSet(opt)) return;
101+ const std::vector<std::string>& categories{args.GetArgs(opt)};
102+ if (std::any_of(categories.cbegin(), categories.cend(), [](const auto& c) { return LogInstance().IsNoneCategory(c); })) return;
103+ for (const auto& c : categories) {
104+ const bool success{opt == DEBUG_LOG_OPTS[0] ? LogInstance().EnableCategory(c) : LogInstance().DisableCategory(c)};
Feels like this is undoing whatever convenience/readability you set up by defining DEBUG_LOG_OPTS in the first place. Do you think more debug args will be added in the future? Or can we just use
This commit is to have the same behavior (and code) for the -debug and -debugexclude config options. I’m not sure what your suggestion is, but I’m happy to look at a working example.
Well I meant something like a switch statement with "-debug" and "-debugexclude" literal strings as cases. I think that would make it more readable and if anything is ever added to DEBUG_LOG_OPTS[] it won’t necessarily break. But that might just be too long or out of style
Done, and also dropped DEBUG_LOG_OPTS and the no-longer useful Assume check (the NONFATAL_UNREACHABLE check does it better), and made the two EnableOrDisableLogCategories functions simpler and more similar. Very helpful feedback (thanks!)
0diff --git a/src/init/common.cpp b/src/init/common.cpp
1index a5b07420d94..c99a512459b 100644
2--- a/src/init/common.cpp
3+++ b/src/init/common.cpp
4@@ -77,26 +77,28 @@ void SetLoggingLevel(const ArgsManager& args)
5 }
6 }
7 8-// We run each of these configuration options through SetLoggingCategories. The order matters;
9-// -debugexclude settings take priority over -debug ones, so -debugexclude is run after -debug.
10-static constexpr std::array DEBUG_LOG_OPTS{"-debug", "-debugexclude"};
11+static bool EnableOrDisable(const std::string& opt, const std::string& category)
12+{
13+ if (opt == "-debug") return LogInstance().EnableCategory(category);
14+ if (opt == "-debugexclude") return LogInstance().DisableCategory(category);
15+ NONFATAL_UNREACHABLE();
16+}
1718+// Maintain the same logic in both of the EnableOrDisableLogCategories functions in the codebase.
19 static void EnableOrDisableLogCategories(const ArgsManager& args, const std::string& opt)
20 {
21- if (!args.IsArgSet(opt)) return;
22- Assume(std::any_of(DEBUG_LOG_OPTS.cbegin(), DEBUG_LOG_OPTS.cend(), [&](const auto& d) { return opt == d; }));
23 const std::vector<std::string>& categories{args.GetArgs(opt)};
24 if (std::any_of(categories.cbegin(), categories.cend(), [](const auto& c) { return LogInstance().IsNoneCategory(c); })) return;
25 for (const auto& c : categories) {
26- const bool success{opt == DEBUG_LOG_OPTS[0] ? LogInstance().EnableCategory(c) : LogInstance().DisableCategory(c)};
27- if (!success) InitWarning(strprintf(_("Unsupported logging category %s=%s."), opt, c));
28+ if (!EnableOrDisable(opt, c)) InitWarning(strprintf(_("Unsupported logging category %s=%s."), opt, c));
29 }
30 }
3132 void SetLoggingCategories(const ArgsManager& args)
33 {
34- for (const std::string& opt : DEBUG_LOG_OPTS) {
35- EnableOrDisableLogCategories(args, opt);
36+ // debugexclude settings take priority over debug ones, so run debugexclude last
37+ for (const std::string& opt : {"-debug", "-debugexclude"}) {
38+ if (args.IsArgSet(opt)) EnableOrDisableLogCategories(args, opt);
39 }
40 }
4142diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp
43index 3bae7bef722..9c32839231f 100644
44--- a/src/rpc/node.cpp
45+++ b/src/rpc/node.cpp
46@@ -209,14 +209,13 @@ static RPCHelpMan getmemoryinfo()
47 };
48 }
4950+// Maintain the same logic in both of the EnableOrDisableLogCategories functions in the codebase.
51 static void EnableOrDisableLogCategories(const UniValue& categories, bool enable)
52 {
53- const std::vector<UniValue>& category_values{categories.get_array().getValues()};
54- for (const auto& category : category_values) {
55- if (LogInstance().IsNoneCategory(category.get_str())) return;
56- }
57- for (const auto& category : category_values) {
58- const std::string& c{category.get_str()};
59+ const std::vector<UniValue>& cats{categories.get_array().getValues()};
60+ if (std::any_of(cats.cbegin(), cats.cend(), [](const auto& c) { return LogInstance().IsNoneCategory(c.get_str()); })) return;
61+ for (const auto& cat : cats) {
62+ const std::string& c{cat.get_str()};
63 const bool success{enable ? LogInstance().EnableCategory(c) : LogInstance().DisableCategory(c)};
64 if (!success) throw JSONRPCError(RPC_INVALID_PARAMETER, "Unsupported logging category: " + c);
65 }
in
src/init/common.cpp:32
in
fba713a172outdated
30- "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.",
31+ argsman.AddArg("-debug=<category>", "Output debug and trace logging (default: 0). "
32+ "If <category> is omitted, or is 1 or all, output all debug and trace logging. If <category> is 0 or none, any other categories passed are ignored. Other valid values for <category> are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories. See also the -debugexclude configuration option that takes priority over -debug.",
33 ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
34- 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);
35+ argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category (default: 1). Takes priority over -debug and can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. If <category> is 1 or all, exclude all debug and trace logging. If <category> is 0 or none, any other categories passed are ignored. Other valid values for <category> are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to exclude multiple categories.", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
I left them without quotes as none of the other category values in the -debug and -debugexclude helps are in quotes. But you may be right that it’s nevertheless clearer to add them here. Done.
in
src/logging.cpp:227
in
a1abce1650outdated
223@@ -225,7 +224,7 @@ std::string LogCategoryToStr(BCLog::LogFlags category)
224 // Each log category string representation should sync with LogCategories
225 switch (category) {
226 case BCLog::LogFlags::NONE:
227- return "";
228+ return "0";
Thanks for making me re-verify. This case will never be hit and is only present to appease the compiler (error: enumeration value 'NONE' not handled in switch), so leaving it as an empty string, added a comment, and moved it to the last case in the switch.
pinheadmz
commented at 7:30 pm on April 5, 2023:
member
concept ACK
github code review, will fine-tooth comb review later this week.
Played with build locally and tried out examples from new help message. A few questions…
achow101 added this to the milestone 25.0
on Apr 11, 2023
Could you combine this into one function? You have another method called EnableOrDisableLogCategories() in node.cpp which might be confusing. And unlike in node.cpp, I don’t think the common.cpp method is called anywhere else besides SetLoggingCategories()
Good question. Both of the EnableOrDisableLogCategories() functions are created in this PR. They are doing pretty much the same thing, but are not trivially combinable into one function. If one is changed, it’s likely that both should be (maybe a comment to that effect should be added to them – Edit: done). I therefore wanted both to be standalone functions with the same name in order that searching/git grepping returns both. (Edit: updated this sentence to clarify.)
That, along with how extracting that function simplifies SetLoggingCategories to highlight that both options are being plugged into the same code, were the motivations.
Cool, just to be clear I meant combining this EnableOrDisableLogCategories() into SetLoggingCategories() and then you only have one named EnableOrDisableLogCategories() in the code over in node.cpp
That, along with how extracting that function simplifies SetLoggingCategories to highlight that both options are being plugged into the same code, were the motivations.
okie doke 👍
pinheadmz approved
pinheadmz
commented at 6:25 pm on April 12, 2023:
member
ACKe9f6fd0e028e6a5669926926792c70c31a26c403
reviewed code, built and ran tests locally. played with feature in regtest, tried to break it, couldn’t break it.
glozow removed this from the milestone 25.0
on Apr 14, 2023
DrahtBot added the label
Needs rebase
on Jun 20, 2023
jonatack force-pushed
on Jun 20, 2023
jonatack
commented at 11:37 pm on June 20, 2023:
member
Rebased following merge of #27632; only change is to commit 4351f26203c6daab321d478a3992e1723cc60814 per git range-diff ee22ca5 5991edd 630b38e.
DrahtBot removed the label
Needs rebase
on Jun 20, 2023
in
src/init/common.cpp:81
in
630b38e92boutdated
97+ if (opt == "-debug") return LogInstance().EnableCategory(category);
98+ if (opt == "-debugexclude") return LogInstance().DisableCategory(category);
99+ NONFATAL_UNREACHABLE();
100+}
101+
102+// Maintain similar logic in both of the EnableOrDisableLogCategories() functions in the codebase.
I still don’t totally understand why you need to split this up into three functions, one of which is a kinda-duplicate The comment makes sense but I’m not familiar enough with the general style of the project to determine if having two functions with the same name doing basically the same thing is acceptable.
makes clear that -debug and -debugexclude use the same logic
avoids duplicate code for -debug and -debugexclude
separates the logic to be kept the same, into a method that has the same name and logic as EnableOrDisableLogCategories() in the RPC code (for the logging RPC “include” and “exclude” options), which was added back in 2017-2018 and updated since then. Combining the two into a single method seems non-trivial (maybe for a later pull) but this comment and the shared function name grep-ability makes more clear what logic needs to be the same.
0src/test/logging_tests.cpp:262:29: warning: loop variable 's' of type 'const std::string&' {aka 'const std::__cxx11::basic_string<char>&'} binds to a temporary constructed from type 'const char* const' [-Wrange-loop-construct]
1262|for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
2|^3src/test/logging_tests.cpp:262:29: note: use non-reference type 'const std::string' {aka 'const std::__cxx11::basic_string<char>'} to make the copy explicit or'const char* const&' to prevent copying
Thank you, Luke, updated. Is that with GCC? I don’t see the warning with Clang 16.0.6.
0 BOOST_FIXTURE_TEST_CASE(logging_IsNoneCategory, LogSetup)
1 {
2- for (const std::string& s : {"none", "0"}) {
3+ for (const char* const& s : {"none", "0"}) {
4 BOOST_CHECK(LogInstance().IsNoneCategory(s));
5 }
6- for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
7+ for (const char* const& s : {"", "NONE", "net", "all", "1"}) {
8 BOOST_CHECK(!LogInstance().IsNoneCategory(s));
9 }
10 }
in
test/functional/feature_logging.py:38
in
08a367c929outdated
37+ "-debugexclude=tor",
38+ ])
39 assert os.path.isfile(self.relative_log_path("foo.log"))
40+ # In addition to net and tor, leveldb/libevent/rand are excluded by the test framework.
41+ for category in ["leveldb", "libevent", "net", "rand", "tor"]:
42+ assert not node.logging()[category]
0 # In addition to net and tor, leveldb/libevent/rand are excluded by the test framework.
1+ result = node.logging()
2 for category in ["leveldb", "libevent", "net", "rand", "tor"]:
3- assert not node.logging()[category]
4+ assert not result[category]
in
test/functional/feature_logging.py:48
in
08a367c929outdated
49+ f"-debuglogfile={tempname}",
50+ "-debugexclude=none",
51+ "-debugexclude=qt",
52+ ])
53 assert os.path.isfile(tempname)
54+ assert all(v == True for v in node.logging().values()) # -debugexclude=[none,qt] does not exclude qt
Because -debugexclude=none" is passed. It looks like the passed extra args are concatenated to self.args before starting the process, i.e. self.process = subprocess.Popen(self.args + extra_args... in test_framework/test_node.py#L219, so the “none” value cancels the -debugexclude={libevent,leveldb,rand} args in the same file.
0- assert all(v == True for v in node.logging().values()) # -debugexclude=[none,qt] does not exclude qt
1+ # Expect the "none" value passed in -debugexclude=[none,qt] to mean that
2+ # none of the categories passed with -debugexclude are excluded: neither
3+ # qt passed here nor leveldb/libevent/rand passed by the test framework.
4+ assert all(v == True for v in node.logging().values())
luke-jr changes_requested
jonatack force-pushed
on Jun 30, 2023
jonatack
commented at 9:02 pm on June 30, 2023:
member
Updated the tests with @luke-jr’s review feedback (thanks!)
0--- a/src/test/logging_tests.cpp
1+++ b/src/test/logging_tests.cpp
2@@ -262,11 +262,11 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
3 4 BOOST_FIXTURE_TEST_CASE(logging_IsNoneCategory, LogSetup)
5 {
6- for (const std::string& s : {"none", "0"}) {
7- BOOST_CHECK(LogInstance().IsNoneCategory(s));
8+ for (const char* const& c : {"none", "0"}) {
9+ BOOST_CHECK(LogInstance().IsNoneCategory(c));
10 }
11- for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
12- BOOST_CHECK(!LogInstance().IsNoneCategory(s));
13+ for (const char* const& c : {"", "NONE", "net", "all", "1"}) {
14+ BOOST_CHECK(!LogInstance().IsNoneCategory(c));
15 }
16 }
1718diff --git a/test/functional/feature_logging.py b/test/functional/feature_logging.py
19index 7958a743853..8448eff4a43 100755
20--- a/test/functional/feature_logging.py
21+++ b/test/functional/feature_logging.py
22@@ -34,8 +34,9 @@ class LoggingTest(BitcoinTestFramework):
23 ])
24 assert os.path.isfile(self.relative_log_path("foo.log"))
25 # In addition to net and tor, leveldb/libevent/rand are excluded by the test framework.
26+ result = node.logging()
27 for category in ["leveldb", "libevent", "net", "rand", "tor"]:
28- assert not node.logging()[category]
29+ assert not result[category]
3031 self.log.info("Test alternative log file name outside datadir and -debugexclude={none,qt}")
32 tempname = os.path.join(self.options.tmpdir, "foo.log")
33@@ -45,7 +46,10 @@ class LoggingTest(BitcoinTestFramework):
34 "-debugexclude=qt",
35 ])
36 assert os.path.isfile(tempname)
37- assert all(v == True for v in node.logging().values()) # -debugexclude=[none,qt] does not exclude qt
38+ # Expect the "none" value passed in -debugexclude=[none,qt] to mean that
39+ # none of the categories passed with -debugexclude are excluded: neither
40+ # qt passed here nor leveldb/libevent/rand passed by the test framework.
41+ assert all(v == True for v in node.logging().values())
4243 self.log.info("Test -debugexclude=1/all excludes all categories")
jonatack requested review from maflcko
on Jul 12, 2023
jonatack requested review from luke-jr
on Jul 12, 2023
jonatack requested review from ajtowns
on Jul 12, 2023
jonatack requested review from pinheadmz
on Jul 12, 2023
pinheadmz approved
pinheadmz
commented at 3:58 pm on July 20, 2023:
member
re-ACK93b387fe70174d174b840016da8531ef65ec313e
Confirmed changes were compiler warning fix, better test comment, and very nice clean-up in common.cpp. Built branch and ran tests locally.
283@@ -288,6 +284,8 @@ std::string LogCategoryToStr(BCLog::LogFlags category)
284 return "scan";
285 case BCLog::LogFlags::ALL:
286 return "all";
287+ case BCLog::LogFlags::NONE: // this case is normally never hit
I think we should avoid adding comments like this, because they don’t really provide any value to the developer. If anything, a comment explaining what the “not normal” case is, and/or why that’s particularly relevant, would be more useful, because that’s the first question I’d have after reading this comment. Also unsure why this code has been re-ordered.
Thank you @fanquake for the feedback and having a look. Updated to commit 88211f2b7e12542ba5e65bba4e3398c9592430ea per the diff in #27231 (comment), replacing the comment with clearer code along with added explanation in the commit message.
jonatack force-pushed
on Aug 1, 2023
jonatack
commented at 10:04 pm on August 1, 2023:
member
Updated per git diff 93b387f ce8faad to address @fanquake’s review feedback (thanks!) and replace the comment with clearer code.
0diff --git a/src/logging.cpp b/src/logging.cpp
1index f197943a0bd..d3f61ca54c2 100644
2--- a/src/logging.cpp
3+++ b/src/logging.cpp
4@@ -4,6 +4,7 @@
5 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
6 7 #include <logging.h>
8+#include <util/check.h>
9 #include <util/fs.h>
10 #include <util/string.h>
11 #include <util/threadnames.h>
12@@ -284,9 +285,9 @@ std::string LogCategoryToStr(BCLog::LogFlags category)
13 return "scan";
14 case BCLog::LogFlags::ALL:
15 return "all";
16- case BCLog::LogFlags::NONE: // this case is normally never hit
17- return "";
18- }
19+ case BCLog::LogFlags::NONE:
20+ Assume(false);
21+ } // no default case, so the compiler can warn about missing cases
22 assert(false);
23 }
jonatack force-pushed
on Aug 1, 2023
DrahtBot added the label
CI failed
on Aug 2, 2023
DrahtBot removed the label
CI failed
on Aug 2, 2023
Unit tests for LogPrintfCategory and LogPrintLevel with BCLog::ALL/NONE
to specify expected current behavior and check future changes for regressions.
b5d0dd7c25
Functional test that RPC logging does not return all/none categoriese87de92856
jonatack
commented at 6:05 pm on August 17, 2023:
member
Updated per git range-diff 6ce5e8f ce8faad 05143b2 to take review feedback by @ajtowns (thanks!) and rebase for CI.
DrahtBot added the label
CI failed
on Aug 18, 2023
DrahtBot removed the label
CI failed
on Aug 18, 2023
jonatack requested review from pinheadmz
on Aug 28, 2023
jonatack
commented at 1:14 pm on August 31, 2023:
member
This seems close, has been reviewed by five reviewers (thanks!) and updated for their feedback. I propose to give this another week for (re-)ACKs and merge. If it hasn’t gone in by then, will revert to https://github.com/bitcoin/bitcoin/commit/630b38e92b4e40d49d69970b6c5bc02aa39ba210 with 2 ACKs and handle the minor updates since that commit in a follow-up.
You’d call Log(std::nullopt, BCLog::Level::Debug) in that case (or have constexpr auto NONE = std::nullopt<LogFlags> and call Log(BCLog::NONE, BCLog::Level::Debug) as normal). If NONE isn’t safe for some functions (eg here where it results in an assert(false)), we should just express that in the type system.
jonatack
commented at 0:09 am on September 1, 2023:
Yes, that’s a good insight. It looks like it will make sense to do that when removing the deprecated macros.
If you specify ["net","sheep"] you get an “Unsupported logging category” error, but if you specify ["none","sheep"] you don’t get an error. Same for the -debug config option. Not sure if that’s a bug or a feature, though.
jonatack
commented at 0:11 am on September 1, 2023:
Thank you for testing! It is a feature; see the -debug and -debugexclude helps after a81900ae7bef1c6e2c5137ffbb0ddc4bf530ad2a and the RPC logging help since #11191. The behavior now also has test coverage after this pull.
ajtowns
commented at 4:23 pm on August 31, 2023:
contributor
ACK05143b2b94cc1d62f27fc73212aa59f9d471f2b8
Fix RPC logging behavior when "0" and "none" values are passed
Per the RPC logging help:
```
In addition, the following are available as category names with special meanings:
- "all", "1" : represent all logging categories.
- "none", "0" : even if other logging categories are specified, ignore all of them.
```
The behavior and documentation were added in #11191, but over time and extended
refactoring, the behavior is not longer working in Bitcoin Core versions
22/23/24 (the last supported versions currently) -- passing `0` or `none` has no
effect. As there was no test coverage, the regressions went uncaught. In v24,
`none` became unrecognized.
During the same time period, passing `1` and `all` has been operational and documented.
We therefore need to detect none/0 values and add test coverage in any case, and either:
- leave the functionality out, raise with an error message if none/0 are passed,
and update the RPC help documentation, or
- fix the behavior by returning early.
Both solutions involve essentially the same code size and complexity. Given
that all/1 has been operational, and that none/0 has been documented and appears
to have been operational previously, it seems preferable for consistency to
support none/0 in symmetry with all/1, and as ACKed, intended, and documented
since #11191.
---
This commit therefore adds:
- the missing logic to make the functionality work
- a missing logging.h include header
- the relevant info in the logging RPC help doc
6f4127be01
Use same code/behavior for -debug and -debugexclude config options
for consistency between them and for consistency with the logging RPC that uses
the same logic for both -include and -exclude.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
70210b1fc8
Update -debug and -debugexclude help documentation
Can be tested with:
./src/bitcoind -h | grep -A12 "\-debug=<\|debugexclude"
d6b55ca5c1
Functional test coverage for RPC logging include/exclude with all/none9987531a48
Functional test coverage for -debug/-debugexclude config options
and convert some comments to logging.
8330c38df6
log: remove unused BCLog::NONE enums in logging.cpp code
These cases should never be hit and are not needed; the 0/none logging category
logic is handled only by the callers of IsNoneCategory() at these times:
1) during bitcoind startup init
2) when the `logging` RPC is invoked
89303c910e
Improve RPC logging documentation and examples
and make the logging example in the developer notes zsh-compatible
(for instance, zsh is currently the default shell in macOS).
The RPC logging help can be tested with:
./src/bitcoin-cli help logging
94b059ffe4
jonatack force-pushed
on Sep 1, 2023
jonatack
commented at 0:42 am on September 1, 2023:
member
It’s still not clear if this was broken from the time it was merged, or at some other later point. This also does not work with 22.x, and any earlier versions are EOL.
If it’s been broken forever (or at least is in all currently maintained releases), and no ones even noticed, we could take advantage of that, to remove some of the complexity here; do we definitely need multiple different ways of achieving the same logging toggling?
Has this comment being replied to anywhere already?
It does seem like it would be pretty easy to remove this behaviour – removing “none” would mean that anyone who specified it would get an error on startup, which is a good clue to fix their config. And you get the same “actually, for this run I want to disable debugging stuff” by adding -nodebug at the end (eg, on the cli) or by specifying “-debugexclude=all”. For the logging rpc, just replace the argument with [] instead of adding none seems reasonable.
jonatack
commented at 7:37 pm on September 12, 2023:
member
Has this comment being replied to anywhere already?
Yes, #27231 (comment) and the pull description contains that discussion as well.
ajtowns
commented at 2:25 am on September 13, 2023:
contributor
Exploring this, it doesn’t look like there would be much code simplification gained by dropping none for 0 only, which would no longer be in symmetry with all/1 that have been operational for a long time.
I think that question is addressed in the pull description.
I would have thought it was obvious that I don’t see an answer to that question in the pull description, so that’s an impressively unconstructive response. In the PR description, you claim:
leave the functionality out, raise with an error message if the values are passed, and update the RPC help documentation, or
fix the behavior by returning early.
Both solutions involve essentially the same code size and complexity.
But that’s trivially not true: removing the code obviously results in a lower code size, and it also reduces the number of special cases that have to be documented and understood by users.
No longer convinced this makes sense.
jonatack
commented at 11:08 pm on September 13, 2023:
member
I would have thought it was obvious that I don’t see an answer to that question in the pull description, so that’s an impressively unconstructive response.
It’s reasonable to think one could gloss over the long-ish discussion in the OP (or what I attempted to communicate with it), as I think it replies to the question (at least, as I understood it).
But that’s trivially not true: removing the code obviously results in a lower code size
No, not if we check for the values and raise with an error message if the values are passed, with relevant test coverage. This ought to be done given that the functionality has been documented in the RPC help for around 7 years now, and perhaps also as it’s not clear at what point it broke.
That is what I meant by "Solution: detect none/0 values and add test coverage in any case", for either way requires code, test coverage, and documentation changes. As I wrote, I prefer the route of fixing what was ACKed, intended, and documented in #11191.
This pull improves a lot of things here. It not only fixes broken functionality while using lesser or equal lines of code (apart from test coverage and added docs), it also adds test coverage that has been missing for a long time and improves the documentation. It would be nice to see this finally move forward, and I appreciate your reviews of it!
ajtowns
commented at 1:47 am on September 14, 2023:
contributor
But that’s trivially not true: removing the code obviously results in a lower code size
No, not if we check for the values and raise with an error message if the values are passed, with relevant test coverage.
We already check for invalid categories and issue an error message for those. Doing the same thing for none/0 doesn’t require extra code or extra test coverage.
This ought to be done given that the functionality has been documented in the RPC help for around 7 years now, and perhaps also as it’s not clear at what point it broke.
FWIW, it broke in #12954 (“This is purely a refactor with no behavior changes.” :roll_eyes:) – previously getCategoryMask evaluated all the strings, returning null if “none” was passed in and erroring out if any were invalid; after that PR EnableOrDisableLogCategories dropped the special handling of “none” and enabled/disabled each category as it went, which means logging ["net", "nosuchcategory"] changed its behaviour from just giving an error, to first enabling net logging, then giving an error.
#11191 was merged 2017-11-30 and #12954 was merged 2018-05-01 so this behaviour only worked for five months if you were running master, and only for the 0.16.x release. 0.16.0 was also the first release that considered logging a public api (#11626).
(I have an alias git-history which runs git log --ignore-cr-at-eol -M -C -L${2},${3}:${1}; so git blame rpc/node.cpp to get the line numbers of the function, then git-history rpc/node.cpp 187 204 makes it easy to trace things through. At least until the refactorings get too intense. AAAA+++ excellent alias, highly recommended)
That is what I meant by "Solution: detect none/0 values and add test coverage in any case", for either way requires code, test coverage, and documentation changes. As I wrote, I prefer the route of fixing what was ACKed, intended, and documented in #11191.
Why? The people who ACKed an old PR aren’t gods, and we don’t have to take their opinions at the time as gospel. As far as I can see, the other ways we (now) have of getting this behaviour are strictly superior.
The reasons to have that behaviour originally was that “-debug” was originally a bool flag that got upgraded to support multiple distinct categories, and the arg parser at the time wasn’t as clever/complicated as it is today. The reason given in 11191 for doing anything was "0","1" are allowed in both array of <include> and <exclude>. "0" is ignored and "1" is treated same as "all". It is confusing, so forbid them. so “forbid them” for none/0 still seems pretty reasonable.
This pull improves a lot of things here. It not only fixes broken functionality while using lesser or equal lines of code (apart from test coverage and added docs), it also adds test coverage that has been missing for a long time and improves the documentation. It would be nice to see this finally move forward, and I appreciate your reviews of it!
These commits have a bunch of extra complexity compared to dropping support for this behaviour:
Fix RPC logging behaviour – bunch of extra code, with a requirement to keep it in sync with code in init, vs dropping the documentation
Update -debug help docs – bunch of extra text to read documenting a special case that most people won’t want
Functional test coverage for RPC – the 0/none loop can be omitted entirely (unknown category just means previously listed categories get actioned)
Other commits seem mostly fine with 0/none special casing dropped:
Unit tests for LogPrintfCategory – fine
Functional test does not return all/none – fine
Improve RPC logging doc and examples – fine
Functional test coverage for -debug – replace -debug=0 with -nodebug, drop -debugexclude=none check
Remove unused NONE enums – fine-ish
jonatack
commented at 7:26 pm on January 8, 2024:
member
I’m going to open an alternate pull that does what you suggest and see what reviewers (if any) prefer.
DrahtBot added the label
Needs rebase
on Jan 10, 2024
DrahtBot
commented at 9:13 pm on January 10, 2024:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
fanquake
commented at 1:59 pm on March 6, 2024:
member
I’m going to open an alternate pull that does what you suggest and see what reviewers (if any) prefer.
Did this end up getting opened? Moving this PR to draft for now in any case.
fanquake marked this as a draft
on Mar 6, 2024
fanquake closed this
on Mar 18, 2024
jonatack
commented at 4:29 pm on April 8, 2024:
member
Will re-open as a new PR, along with the alternative version, as it is a good and mature patch with many updates made for reviews and nits.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-16 15:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
