💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2344995517)
done

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2345192358)
👍

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2349207386)
I guess the two sentences are a bit redundant, I'm going to keep the first and lose the second, though.

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2344997699)
You're right for new files we're removing the year(s) entirely.

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2345020780)
Sure taken

💬 l0rinc commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3293705610)
I ran a full IBD with real nodes over Wi-Fi until block 914452 with a brand new Raspberry Pi 5 (16GB RAM, SSD) - it finished successfully in 16h12m using 6GB dbcache.

📝 sekomer opened a pull request: "refactor: Fix typo and correct template parameter inconsistency"
(https://github.com/bitcoin/bitcoin/pull/33394)
- Fixed typo in merkle.cpp where variable was named 'matchh' instead of 'match'
- Fixed Hash160 to use 'T' instead of 'T1' for single template parameter, making it consistent with other single-parameter template functions like Hash<T>

## Impact

These are refactoring changes with no functional impact:
- No behavior changes
- No consensus changes
- All existing tests pass without modification
- Improves code maintainability and reduces cognitive load for developers

📝 mzumsande opened a pull request: "net: do not apply whitelist permissions to onion inbounds"
(https://github.com/bitcoin/bitcoin/pull/33395)
Tor inbound connections do not reveal the peer's actual network address. Do not apply whitelist permissions to them since address-based matching is ineffective.

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3293797977)
> using std::uncaught_exception() to prevent throwing

https://en.cppreference.com/w/cpp/error/uncaught_exception.html indicates `std::uncaught_exception()` is deprecated in C++17 and removed in C++20 - but we could use `std::uncaught_exceptions()`.

Added a few comments, thanks for tackling this.

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2349822297)
This parameter change is a separate concern. Please split it into its own commit to keep the risky behavior change easy to review.
Though personally, I'd pass both by value and move into the members to handle rvalues efficiently.

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2349820667)
Please add `#include <cstdlib>` for [std::abort()](https://en.cppreference.com/w/cpp/utility/program/abort)

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2349889201)
I'm very much against these long and scary comments - seems like a strong code smell. The comment will quickly reflect an old version of the code since it's not high-level enough and we simply won't update it when the code changes. And honestly reading the whole thing didn't help me with understanding the gist of it.

If we feel the code isn't self-explanatory we should adjust the code instead of writing long documentation for it. If you still feel docs are needed, let's do it more high-level
...

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2349921508)
I admit that I don't understand this well enough, why reconnect before deleting the callback? Shouldn't we delete the callback first, then call `noui_reconnect()`?

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2349932872)
nit: we can simplify it a bit (+ pass by value mentioned earlier):
```suggestion
explicit DebugLogHelper(std::string message, MatchFn match = [](auto){ return true; });
```

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2349918985)
`LogError` would flush after this line, so we could safely abort after (since `abort` doesn't call any destructors). Can you confirm that `cerr` will also flush safely?

🤔 enirox001 reviewed a pull request: "test: Prevent disk space warning during node_init_tests"
(https://github.com/bitcoin/bitcoin/pull/33391#pullrequestreview-3226283052)
utACK bdf01c6

💬 hodlinator commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2349949588)
nit: Comment might fit better above the `CalculateDbCacheBytes` function definition?

🤔 hodlinator reviewed a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3226045467)
Reviewed 9f69c646d7eb8790ec9fd6d1d5607f554cc2f4ff

💬 hodlinator commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2350089881)
Might be overly harsh with `Assert()`? Could use `Assume()`, and/or maybe add a comment why this is worth guarding against. Noticed the original suggestion didn't even check the return value (https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2344204033).

💬 hodlinator commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2349989732)
(nit: as stated previously I prefer making simple functions like `ShouldWarnOversizedDbCache` `constexpr` and using `static_assert` instead of runtime checks, https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2282516167).

💬 hodlinator commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2349984030)
Think we should rely on the `NOMINMAX` define you found in *bitcoin/CMakeLists.txt*, also prefer `numeric_limits`. `std::clamp<uint64_t>(` with the lower bound of `0` wasn't doing much so agree with getting rid of that.

~0 on switching from separate `if`-blocks to mutating `mem`-variable.