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

💬 hodlinator commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2350032125)
Stray addition of ` = ` in last commit.

Suggestion: Could remove `total_cache` completely (the name is more fitting for the original copy which is still initialized by the result of this function anyway):
```diff
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -29,14 +29,14 @@ static constexpr size_t MAX_32BIT_DBCACHE{1024_MiB};
namespace node {
size_t CalculateDbCacheBytes(const ArgsManager& args)
{
- size_t total_cache = {DEFAULT_DB_CACHE};
if (std::optional<int64
...

🤔 furszy reviewed a pull request: "test: don't throw from the destructor of DebugLogHelper"
(https://github.com/bitcoin/bitcoin/pull/33388#pullrequestreview-3226297965)
As `DebugLogHelper` is only used within the unit test framework; could use `BOOST_REQUIRE` too.

💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2350121851)
In that case I'd just delete it since the code already states all of this

📝 forkfury opened a pull request: "util/strencodings: guard SAFE_CHARS index and pre-reserve result"
(https://github.com/bitcoin/bitcoin/pull/33396)
- Add defensive bounds check for `rule` parameter in `SanitizeString` to prevent out-of-range access to `SAFE_CHARS` array
- Pre-reserve result capacity and cache reference to allowed charset to avoid repeated lookups
- No behavior change for valid inputs; improves robustness and minor performance

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2350095778)
b526fd4a3ae637ffc0c13353e1f55d690209fff7

avoid the temporary variable by inlining it in the for loop directly?