👍 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

💬 marcofleon commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2194770475)
Let me know if I'm missing something but at what point is the fuzzed wallet funded? I generated a coverage report and the test seems to blocked at `SelectCoins` in `CreateTransactionInternal`. It always results in an insufficient funds error, which means the fuzzer never gets past this line in `CreateTransaction`.
https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/src/wallet/spend.cpp#L1367

💬 ryanofsky commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194772695)
re: https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194662748

Thanks Marco, I forgot about the error() function, but yes looking at #29236 it looks like a lot of incorrect LogError uses did come from that replacement.

I also agree it is good to return errors through `util::Result`. But I think it can be useful in many cases to log the errors as well, or in some cases not try to return very granular error information, but just indicate that a failure happened and refer the use
...

📝 paplorinc opened a pull request: "benchmark: Refactor SipHash_32b to improve accuracy and potential avoid optimization issues"
(https://github.com/bitcoin/bitcoin/pull/30349)
This PR stems from the discussions in https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1649187336

The previous benchmark for `SipHash` was slightly less accurate in representing real-world usage and allowed for potential compiler optimizations that could invalidate the benchmark.
This change aims to ensure the benchmark produces more realistic results.

By modifying the initial values and only incrementing only the first byte of `val`, the benchmark should reflects a more typical
...

💬 paplorinc commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1657205524)
Thank you, I've pushed it in https://github.com/bitcoin/bitcoin/pull/30349

💬 maflcko commented on pull request "refactor: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2194807648)
Changing the behavior of something is not a refactor.

💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194832230)
@ryanofsky all those unnecessary macros add complexity and more ducks to keep in a row -- for no benefit.

One primary benefit I hope to see from the `trace` level is to be able to separate out very verbose, low-level, high-frequency logging. With `net` category logging, for instance, to use `debug` for high-level, lower-frequency events at the peer level, and `trace` for low-level, very high frequency events at the p2p messages level.

👍 pablomartin4btc approved a pull request: "Show maximum mempool size in information window"
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2145552451)
tACK 4a028cf54c0502bc9ba0804bf1ae413b20a007cb


💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657231561)
fixed

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657231916)
Yes, removed