💬 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

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

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657233836)
added the line back, thanks

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657236943)
Yes it's not needed.
I removed this commit
This can come as a separate PR, the size output can also be added to the other funding RPC's?

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657237193)
I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot. Renaming Warning to Critical and Error to Fatal for the reasons given seems fine, but the old names shouldn't be kept around in that case.

💬 theuni commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#discussion_r1657238803)
Done.

💬 theuni commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2194854058)
> I've got this on a branch that I'll PR as soon as #30141 is merged: https://github.com/TheCharlatan/bitcoin/commits/kernelRmGlobals/ . I try to keep the open PRs limited to get more focused review.

Aha, I see.

I think this one is simple and self-contained enough to go in on its own unless you're opposed.

I'll ping you on these in the future to make sure you don't already have them teed up.

Also, concept ACK on your next branch. Will give #30141 a final review and ACK to keep things
...