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

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657242662)
unrelated nit: (one line above) `insecure_GetRandHash` was replaced years ago by `InsecureRand256`. Can ideally be fixed up in a follow-up.

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

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


Thanks for the reviews @furszy @rkrux

Force-pushed from b3ac1179ff90fe261af4e6dc9c7af64e1518b435 to d8febf6d9ea018cf8e980ee036fb5ccd8ea8887a
Compare diff [b3ac1179ff..734076c6d](https://github.com/bitcoin/bitcoin/compare/b3ac1179ff90fe261af4e6dc9c7af64e1518b435..734076c6de1781f957c8bc3bf7ed6951920cfcf6)

---

Changes:
- Rebased after [PR 30309](https://github.com/bitcoin/bitcoin/pull/30309) and updated the maximum transaction weight check to use the user passed value when given or
...