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

💬 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_r2163620350)
Can we name the variable such that the choice is obvious instead of adding extra comments?

💬 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_r2163626313)
They could be the same value, but they're unrelated things, and I don't think it's relevant in this PR. `DEFAULT_MAX_LOG_BUFFER` represents the max size of the buffer that holds log messages (across all locations) that are produced before we start writing them to file.

💬 hebasto commented on pull request "test: disable secp256 tests by default":
(https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-2999816008)
> Paritally addresses: #32770
>
> This PR disables the slow `secp256` test suites by default, both in CI runs and normal developer builds/tests. These tests are static in every run, except for the case in which we update the secp256 subtree.
>
> This is done by decoupling the `SECP256K1_BUILD_TESTS` and `SECP256K1_BUILD_EXHAUSTIVE_TESTS` variables from the `${BUILD_TESTS}` configuration, allowing them to be selected individually.
>
> We would likely still like to run these tests when th
...

💬 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_r2163635733)
Misunderstood that part, thanks. The different base part still applies (though I know this isn't uncommon in the code base)

💬 hebasto commented on pull request "build: add root dir to CMAKE_SYSTEM_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-2999832695)
cc @theuni