log: sort logging categories alphabetically #22530

Sorting the logging categories seems more user-friendly with the number of categories we now have, allowing CLI users to more quickly find a particular category.

before

$ bitcoin-cli help logging
...
The valid logging categories are: net, tor, mempool, http, bench, zmq, walletdb, rpc, estimatefee, addrman, selectcoins, reindex, cmpctblock, rand, prune, proxy, mempoolrej, libevent, coindb, qt, leveldb, validation, i2p, ipc

$ bitcoind -h | grep -A8 "debug=<category>"
  -debug=<category>
       ...
       output all debugging information. <category> can be: net, tor,
       mempool, http, bench, zmq, walletdb, rpc, estimatefee, addrman,
       selectcoins, reindex, cmpctblock, rand, prune, proxy, mempoolrej,
       libevent, coindb, qt, leveldb, validation, i2p, ipc.

$ bitcoin-cli logging [] '["addrman"]'
{
  "net": false,
  "tor": true,
  "mempool": false,
  "http": false,
  "bench": false,
  "zmq": false,
  "walletdb": false,
  "rpc": false,
  "estimatefee": false,
  "addrman": false,
  "selectcoins": false,
  "reindex": false,
  "cmpctblock": false,
  "rand": false,
  "prune": false,
  "proxy": true,
  "mempoolrej": false,
  "libevent": false,
  "coindb": false,
  "qt": false,
  "leveldb": false,
  "validation": false,
  "i2p": true,
  "ipc": false
}

after

$ bitcoin-cli help logging
...
The valid logging categories are: addrman, bench, cmpctblock, coindb, estimatefee, http, i2p, ipc, leveldb, libevent, mempool, mempoolrej, net, proxy, prune, qt, rand, reindex, rpc, selectcoins, tor, validation, walletdb, zmq

$ bitcoind -h | grep -A8 "debug=<category>"
  -debug=<category>
       ...
       output all debugging information. <category> can be: addrman,
       bench, cmpctblock, coindb, estimatefee, http, i2p, ipc, leveldb,
       libevent, mempool, mempoolrej, net, proxy, prune, qt, rand,
       reindex, rpc, selectcoins, tor, validation, walletdb, zmq.

$ bitcoin-cli logging [] '["addrman"]'
{
  "addrman": false,
  "bench": false,
  "cmpctblock": false,
  "coindb": false,
  "estimatefee": false,
  "http": false,
  "i2p": false,
  "ipc": false,
  "leveldb": false,
  "libevent": false,
  "mempool": false,
  "mempoolrej": false,
  "net": false,
  "proxy": false,
  "prune": false,
  "qt": false,
  "rand": false,
  "reindex": false,
  "rpc": false,
  "selectcoins": false,
  "tor": false,
  "validation": false,
  "walletdb": false,
  "zmq": false
}

utACK 1131bf993fa88e796bdb950c051add722a29ebfe

Concept ACK

I agree with @MarcoFalke's comment, and I think it's undesirable to hard-code the number of items in any list (24 or 28 in this case, depending on whether the ALL and NONE categories are included) unless really necessary.

Did you consider simply manually sorting the static LogCategories list instead of soting at runtime? A comment can be added just before its declaration that the list should remain sorted when new entries are added.

Also, could you please provide a brief motivation for this PR in the description? Unless it's just me and it's obvious to everyone else. My impression is that it allows readers of this list to more quickly find a particular category (to get the exact naming, knowing only roughly what the name is), but are there other reasons? Perhaps just a few words would help (this suggestion is optional).

Did you consider simply manually sorting the static LogCategories list instead of soting at runtime? A comment can be added just before its declaration that the list should remain sorted when new entries are added.

I did; it seems preferable not to rely on manually maintaining the sort throughout future changes when it can be automated with non-perceptible cost in a non-hotspot. Edit: the test does verify the sort, so it could be manually sorted if people prefer that. Performance-wise, there shouldn't be a distinguishable difference.

My impression is that it allows readers of this list to more quickly find a particular category (to get the exact naming, knowing only roughly what the name is), but are there other reasons? Perhaps just a few words would help (this suggestion is optional).

Oh sorry. User-friendliness for CLI users, exactly as you describe. Updated the pull description.

Did you consider simply manually sorting the static LogCategories list instead of soting at runtime? A comment can be added just before its declaration that the list should remain sorted when new entries are added.

I did; it seems preferable not to rely on manually maintaining the sort throughout future changes when it can be automated with relatively non-perceptible cost in a non-hotspot.

Sounds like something that can be checked by some linter script.

Concept ACK. Tried and it works as expected (returns debug categories in alphabetical order)

<details><summary>bitcoind -h | grep -A8 "debug=<category>"</summary>

</details>

<details><summary>bitcoin-cli help logging</summary>

</details>

<details><summary>bitcoin-cli logging [] '["addrman"]'</summary>

</details>

I think it's undesirable to hard-code the number of items in any list (24 or 28 in this case, depending on whether the ALL and NONE categories are included) unless really necessary.

Not sure about this. Maybe there is a better approach for test in https://github.com/bitcoin/bitcoin/pull/22530/commits/f720cfa824f1be863349e7016080f8fb1c3c76c2 or this test will fail if we add new category in future and it can be incremented accordingly.

ACK https://github.com/bitcoin/bitcoin/pull/22530/commits/a744e79e4d07358d331d8e2f732511e2eca0e3f0

cr ACK a744e79e4d07358d331d8e2f732511e2eca0e3f0

Nice small usability improvement!

the test does verify the sort, so it could be manually sorted if people prefer that.

I'd prefer that. I'm -0 on the PR as it stands. Even though the runtime cost is small (in CPU and memory for the additional code), on principle, I prefer not to do things at runtime that can easily be done at compile time (or, I guess in this case, pre-compile time!). I like the idea of keeping the test, or a linter as @kristapsk suggested. The test is probably better since it verifies actual behavior.

If we do go with the manual sort idea, let's add a comment before the declaration of LogCategories[], something like "This list should be sorted" or "Please keep this list sorted."

For me, both discussed approaches of either manually maintining the sort order of the categories at (pre-)compile-time or doing it at runtime (the current PR approach) are fine. Though like LarryRuane, I'd have a slight preference to the static approach; OTOH, since this part is really not performance-critical (as argued by (jonatack)) the dynamic approach seems acceptable. Most importantly, the user-friendliness is improved and we also have a test that ensures that.

Tested ACK a744e79e4d07358d331d8e2f732511e2eca0e3f0 (also tested the test by checking that it fails on the master branch :))

Thanks for the feedback. As to the approach of automated versus manual sorting, I don't have a strong opinion. In this context of an inexpensive operation not in a hotspot, on a project with many contributors, the avoidance of future maintainance + "keep this manually sorted"-style documentation that automating the sort enables seems slightly more compelling to me, but either way seems fine.

tACK a744e79e4d07358d331d8e2f732511e2eca0e3f0

This is an improvement. If someone wants to coordinate and make a policy of manual sorting, it can be done in a followup.

ACK a744e79e4d07358d331d8e2f732511e2eca0e3f0

Thanks for the reviewing and feedback! Updated per @vasild's #22530 (review)

<details><summary><code>git diff a744e79 d596dba</code></summary><p>

diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py
index b6117b9e7b..563f2ea43e 100755
--- a/test/functional/rpc_misc.py
+++ b/test/functional/rpc_misc.py
@@ -71,9 +71,9 @@ class RpcMiscTest(BitcoinTestFramework):
         assert_equal(list(node.logging()), sorted_logging_categories)
 
         # Test logging help returns the logging categories string in alphabetical order.
-        first_3_categories = ', '.join(sorted_logging_categories[0:3])
+        categories = ', '.join(sorted_logging_categories)
         logging_help = self.nodes[0].help('logging')
-        assert f"valid logging categories are: {first_3_categories}" in logging_help
+        assert f"valid logging categories are: {categories}" in logging_help

</p></details>

re-ACK d596dba9877e7ead3fb5426cbe7e608fbcbfe3eb (CI error is obviously unrelated)

ACK d596dba9877e7ead3fb5426cbe7e608fbcbfe3eb