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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25308 (refactor: Reduce number of LoadChainstate parameters and return values by ryanofsky)
• #25203 (logging: update to severity-based logging by jonatack)
• #21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)

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.

Code Review ACK ecff20d

Concept ACK

I do think this is an intermediate state. I imagine at some point we'll want to have a category and log level for every message, so the need to log messages with just a category goes away again.

@laanwj No strong opinion but I thought the same, tried a version of it, and initially concluded that (a) [category:all] for unconditional logging might be unnecessarily verbose, and (b) all doesn't seem useful as a severity level that a user can set in our logging (it would do the same as setting debug currently), though I could be overlooking a use case.

I also was under the assumption that this is permanent. If it wasn't, we probably should not do it, but then I am also wondering if there is a better alternative.

Right, my understanding was that the None category and level would eventually go away, and it'd be more like logging systems in other applications. That it's an intermediate solution until the existing messages are "sorted out". And after threshold configuration (like #25287) was added. But I could be wrong.

In any case, permanent or not, I'm ok with this as intermediate solution.

I could imagine a scenario where LogPrintf and LogPrintfCategory might go away someday (along with None) if they are more or less converted to, say, Info level, and users setting the level to Warning or Error just wouldn't care to see those messages. But that seems a little radical for now (or not, IDK, Info will be the default and I would use that or a more detailed level myself).

I guess the statements under discussion are mostly:

• Single logs (per process lifetime), for example init logs
• Heartbeat logs (when a block came in)

I can see how they are useful unconditionally to make later debugging easier, which would be my reason to keep them unconditional.
However, I can also see how there might be a user only interested in Warning/Error logs, not in Info logs. So I guess one alternative is to change None to Info and make it the default severity. (That is probably also the way python does it)

For an idea, open https://github.com/bitcoin/bitcoin/pull/25203/files, click to load the files with a larger diff, and search for LogPrintfCategory. They probably could all be changed to Error, Warning, or Info. The default debug log would be shockingly quiet, though.

I can see how they are useful unconditionally to make later debugging easier, which would be my reason to keep them unconditional.

Yes, I don't disagree. It does feel somewhat strange to me to have log messages that figure higher than Error in severity, but aren't errors. But if used sparingly for this purpose it makes sense.

So I guess one alternative is to change None to Info and make it the default severity. (That is probably also the way python does it)

The default debug log would be shockingly quiet, though.

I think the default logging should remain the same. So if we use Info as default log level for what is now unconditional, for most categories we'd want to have the standard threshold at Info. For some (leveldb, qt) it could deviate, becuase they are spammy.

Users would currently need to turn on debug logging though, to see Info logging.

Edit: right, change Info to be unconditional. Hmm. That may need some shifting, though in the latest pass I moved most non-essential logging into debug anyway.

Users would currently need to turn on debug logging though, to see Info logging.

Yes. The current hardcoded LogAcceptCategory thresholding implementation is silly I temporarily added it for leveldb category, basically, until there's more fine-grained configuration possible. The default threshold for non-spammy cagtegories should probably be Info and higher.

I think the current LogPrint messages should be of level Debug, and current LogPrintf should be Info or higher. I'm fine with having an unconditional None level for marker messages as @MarcoFalke says.

In the parent PR I propose also a trace level, for the really verbose low-level messages.

Right, makes sense. Though we shouldn't go over the top with number of severity levels either. But trace sounds OK for some cases like low-level networking logging.

Code review ACK ecff20d