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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

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.

or none are passed, including not recognizing none:

Any idea when/how this was broken?

Probably in f1379ae from #25614:

replace the unused BCLog::Level:None string "none" with an empty string
as the case will never be hit

So only broken for 24.x.

The behavior was intended to be added in #11191.

I downloaded and tested v23.1 and the behavior wasn't functional: passing 0 or none had no effect.

In v24, f1379ae made none not be recognized, but the logic for the intended behavior remained absent anyway.

I've added this information to the PR description.

The behavior was intended to be added in #11191.

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?

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!

Updated for @ajtowns' feedback to make IsNoneCategory() a static function in 6f4127b (#27231).

diff --git a/src/logging.h b/src/logging.h
index 4dfb7106327..83c285c905d 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -180,7 +180,7 @@ namespace BCLog {
 
         bool WillLogCategory(LogFlags category) const;
         bool WillLogCategoryLevel(LogFlags category, Level level) const;
-        [[nodiscard]] bool IsNoneCategory(const std::string& s) const { return s == "0" || s == "none"; }
+        [[nodiscard]] static bool IsNoneCategory(const std::string& s) { return s == "0" || s == "none"; }

reACK 94b059f

The behavior was intended to be added in #11191.

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.

Has this comment being replied to anywhere already?

Yes, #27231 (comment) and the pull description contains that discussion as well.

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.

Why would you keep the 0 option? Just drop the concept entirely: master...ajtowns:bitcoin:202309-debug-no-none That special case was added well prior to -debugexclude (namely #3067 vs #10123)

I think that question is addressed in the pull description.

Why would you keep the 0 option? Just drop the concept entirely: master...ajtowns:bitcoin:202309-debug-no-none That special case was added well prior to -debugexclude (namely #3067 vs #10123)

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.

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).

Here's the relevant section:

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.

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!

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.

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." 🙄) -- 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

I'm going to open an alternate pull that does what you suggest and see what reviewers (if any) prefer.

🐙 This pull request conflicts with the target branch and needs rebase.

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.

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.