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

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1656931808)
Something like:

```cpp
static const uint256 ctx_seed = ...;
const uint256& seed{seedtype == SeedRand::SEED ? ctx_seed : uint256::ZERO};
LogPrintf(... seed);
MakeRandDeterministicDANGEROUS(seed);

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1656579259)
In fact, I don't understand this whole function. I'd presume the performance bottleneck is always `Impl().rand64()` and whether a few bit operations or shifts use an operator known at compile time or not shouldn't matter, no?

Locally I could only find a difference when using `InsecureRandomContext`, but that doesn't use the `randbits<>()` method outside of test, or at all? So I wonder if the function is useful at all.

Though, maybe I just did the wrong benchmarks.

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1657178875)
Could go even more old-school and do:
```C++
constexpr const char* EXPECTED[]{
...
BOOST_CHECK_EQUAL_COLLECTIONS(log_lines.begin(), log_lines.end(), std::cbegin(EXPECTED), std::cend(EXPECTED));
```

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1657187907)
fixed