⚠️ ryanofsky opened an issue: "RFC: Misused LogError and LogWarning macros"
(https://github.com/bitcoin/bitcoin/issues/30348)
As a programmer, when I see `LogError` and `LogWarning` macros, I assume `LogError` should be used to log information about a failure that happened, and `LogWarning` should be used to log information about an unusual condition which happened that might indicate a real problem depending on other factors and intent.

But [developer notes](https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md) suggest very different meanings for these macros. They
...

💬 brunoerg commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2194572754)
Concept ACK

👍 theStack approved a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2145284260)
Code-review ACK c9dacd958d7c1e98b08a7083c299d981e4c11193

Nice approach using a `EncryptedP2PState` subclass for each test scenario, I agree with other reviewers that this looks much cleaner now and is easier to follow.

💬 theStack commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1657086402)
small nit, potential follow-up: could set 'regtest' as default value for the `net` parameter in the `EncryptedP2PState` class constructor (`__init__`), so it doesn't have to be specified repeatedly here.

💬 maflcko commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194662748)
> 61 cases where it appeared to be used incorrectly

No opinion about naming, but of the cases that are used incorrectly, I wonder if their surrounding functions are better suited to use `Result` instead of a `void` or `bool` return value. (I presume many of them were a result of the `bool error()` helper transformation/removal, which was the first step toward moving some of those functions toward `Result`)

💬 willcl-ark commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490)
I'm largely in agreement with the sentiment here and think the introduction of the `LogFatal` is nice to distiguish fatal errors.

Does

```cpp
LogCritical("pruned datadir may not have more than %d blocks; only checking available blocks\n",
```
...actually require user intervention or other corrective action?

nit: `LogWarning` remains in L728 after this change:

```md
The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for
```

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1657145216)
I suggested `constexpr auto EXPECTED = std::to_array({` above but maybe it doesn't work for you?
[Should support `constexpr` that way under C++20 as far as I can tell](https://en.cppreference.com/w/cpp/container/array/to_array). Are you using some fancy interface to GitHub that doesn't yet show suggestion-embeds?

👍 tdb3 approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2145363000)
re ACK 4f6b00c94b59c8206c77647c97f4ed188fa99401

Ran rpc_users locally (passed).
Re-ran the manual check in https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2105557076 (same results).
Also sanity checked on a Windows machine running Python 3.9.13 that `platform.system()` does return "Windows".
Left one minor nit.

💬 tdb3 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1657131463)
nit: spelling

```diff
- InterpretPermSting
+ InterpretPermString
```

💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194707816)
Level::Error now meaning that the node needs to be shut down does seem to be a deviation, and perhaps adding Level::Fatal is a good idea if this distinction is needed. Not sure about Level::Critical.

IMO the severity-level logging veered in an unfortunate direction with the unneeded proliferation of Log{Category} macros having hardwired names and an inconsistent API with varying numbers of args and documentation duplicated/moved from `enum class Level` to the developer notes.

All of that w
...

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657166039)
Suggest moving all this developer-facing documentation back to the Level enum class where the levels are defined.

User-facing documentation would be in the logging RPC help.

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657169459)
It would be good to shed this proliferation of macros with hardwired names in favor of one single, consistent `Log` one.

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194736217)
Tentative Concept ACK on adding Fatal if the distinction is considered essential, unsure about Critical replacing Warning.

👍 AngusP approved a pull request: "Mining interface followups, reduce cs_main locking, test rpc bug fix"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2145441782)
Code Review ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed

👍 maflcko approved a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2141300770)
Left some nits/questions. Happy to re-ACK if you address them.

ACK f0e5681e97450b696de26ff21e61f961370a9946 📲

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4
...

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1654844892)
nit in 0807afa5c431f34f70dcbdb58de65480216ca800: Unused include?

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657159709)
nit in 818fb4aa837c748cf241f20efc81e1b2df0594e1: Missing GetRand and GetRandDur

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1656925084)
nit in 6d7f91db5069ddbbaa48c7e3f842ec68f0f717bb:

I see you want to preserve the previous behavior, but I think it makes sense to log this even when `ZEROS` are passed.

This makes it clear that any env-provided seed is ignored for this sub-test.

Also, the diff would be smaller by one line :sweat_smile:

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1656587979)
nit in https://github.com/bitcoin/bitcoin/commit/7f24615249a53ed4ac000013f86634c2b81569f2: Missing include for

```cpp
#include <concepts>
#include <type_traits>

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1656563853)
nit in e4eb03715d5944e999a399538919089862d8c7c6: I don't understand this branch. To me it looks identical to the fallback branch, just with `Bits==1` manually hardcoded. But it is known at compile time, so the compiler can do it for you, and you can just remove this branch, no? (Same for `Bits==0`)

I tested it locally and didn't see a speedup when including this branch. (Maybe a trivial speedup when *removing* it, but this is likely noise)

If there is a benefit that I am missing, it would
...