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

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

This check should be in the loop if it's meant to be a CPU DoS protection

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

> these are all calculated including the tx itself

Don't think this was correct before or after

📝 hazem19952025 opened a pull request: "Hazem19952025/Update SECURITY.md/hazem19952025/ actions"
(https://github.com/bitcoin/bitcoin/pull/33397)
Actions
Done

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new
...