👍 dergoegge approved a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2952838460)
Code review ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

The chosen limits seem reasonable to me.

🤔 maflcko reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2952901140)
lgtm ACK 466722cbe5d6c9ddf35d1590b749d19ec115684e 🎱

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 466722cbe5d6c9dd
...

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163404769)
```suggestion
explicit HTTPRPCTimerInterface(const std::any& context)
: m_context{*Assert(std::any_cast<NodeContext*>(context))}
```

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163412918)
if this turns out problematic (intermittently fail), one could also try `mockscheduler`.

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163408031)
nit: Could probably assert early on construction, rather than when a timer is created, but either is equally fine.

👍 l0rinc approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2952512677)
Can you please update the PR descripton now that there's now `UNCONDITIONAL_ALWAYS category`

I'm mostly ok with the change, I'm testing if it introduces any slowdown and would prefer making the tests a bit more typesafe and compact

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163198581)
Seems to me the first two sentences contradict each other now: "log unconditionally, unless it doesn't fulfill the rate limiting condition"

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163176717)
nit: I find mixing C++20 code with C API a bit clunky, if you touch again, consider either comparing their `std::string_view` wrappers or maybe even adding such a helper to `string.h` for `const char*` values.

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163211212)
Does this "prevent" rate limiting or just allows one more megabyte?

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163209427)
That's a reasonable assumption - if a bug could trigger debug logging, we'd likely have bigger problems I guess

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163285496)
👍 nice and clear test

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163179624)
I understand when the time is split off from the measured code to make sure it's accurate - but I don't see how that's the case here, consider inlining it:
```suggestion
m_limiter.m_last_reset = NodeClock::now();
```

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163230657)
this seems a bit hacky to me - this is basically a parameterized test where we implicitly verify the inputs produce the desired outputs, line-by-line. It could make sense to store it in a way that indicates more clearly which outputs we're expecting for which inputs.

We do need separate `::current()` calls to capture that the lines are assigned properly, we could add those to the parameterized test values as well.

I also find it a bit awkward that we're concatenating the result of a `tfm::
...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163313166)
I find this confusing (either the name or the value), by default debug logs aren't displayed...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163309586)
I'm not a fan of dead comments, could we rather extract it to a named variable, something like `CSipHasher uniform(0, 0)` instead of explaining code with text?

----


nit: Maybe my formatter is off, but do we really need to push the formatted lines so far out?

💬 purpleKarrot commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-2999638262)
Is this setting up the environment for tests to run? Ideally, things like that should be done as [test fixture](https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html) and not executed during build system generation.

🤔 danielabrozzoni reviewed a pull request: "rpc: add optional nodeid param to filter getpeerinfo"
(https://github.com/bitcoin/bitcoin/pull/32741#pullrequestreview-2953107634)
Concept ACK, I think this is a good addition, as introducing a peer_id parameter to filter the getpeerinfo response is useful for debugging.

This needs a release note :)

💬 fanquake commented on pull request "test: disable secp256 tests by default":
(https://github.com/bitcoin/bitcoin/pull/32782#discussion_r2163579556)
```suggestion
sudo apt-get install -y build-essential cmake pkgconf python3 libevent-dev libboost-dev
- name: Build with secp256k1 tests enabled
run: |
cmake -B build \
-DENABLE_WALLET=OFF \
-DSECP256K1_BUILD_TESTS=ON \
-DSECP256K1_BUILD_EXHAUSTIVE_TESTS=ON
cmake --build build -j14 --target noverify_tests tests exhaustive_tests -j $(nproc)
```

💬 fanquake commented on pull request "depends: Build `qt` package for FreeBSD hosts":
(https://github.com/bitcoin/bitcoin/pull/32731#discussion_r2163611300)
> This is not correct. Instead of patching depends/toolchain.cmake.in, you should install X11.

Not sure I understand your suggestiong, or why applying [the patch above](https://github.com/bitcoin/bitcoin/pull/32731#discussion_r2158051127) is not the correct thing to do? When configuring, using the toolchain from depends, we should be using dependencies from depends, not looking for system libraries?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163614334)
I don't think this is a dead comment, the choice for using siphash over e.g. std::hash is not obvious (to me), and just naming the variable `uniform` would not have been clear to me. Would prefer keeping this as is.