💬 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

💬 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_r2163644659)
By "dead" I meant that it doesn't bite back (like code does), it can state absolutely anything and the compiler won't help us. Comments usually seem to me like we've given up on trying to express our design within the boundaries of the language. In a few cases here I think the code can be slightly improved to eliminate the need for extra comments.

👍 vasild approved a pull request: "test: refactor out same-txid-diff-wtxid tx to reuse in other tests"
(https://github.com/bitcoin/bitcoin/pull/32385#pullrequestreview-2953228936)
ACK afaaba69eddd50bc22b94ca7c0f9195773aaa111

The code seems to do what is intended and I confirm that it works. I guess it is useful as a generic tool to create a new transaction and two new children of it that have the same txid and different wtxid.

That is useful for the tests of https://github.com/bitcoin/bitcoin/pull/29415 too. However those tests already have a transaction and it would be easier for them and less code, if `malleate_tx()` could create a malleated/changed version of an
...

💬 vasild commented on pull request "test: refactor out same-txid-diff-wtxid tx to reuse in other tests":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163608973)
The name `build_malleated_children` might be confusing since there is nothing "malleated" in those transactions. They are created as such and never [changed or malleated](https://en.bitcoin.it/wiki/Transaction_malleability) afterwards.

Also in the name of `ValidWitnessMalleatedTx`.

💬 vasild commented on pull request "test: refactor out same-txid-diff-wtxid tx to reuse in other tests":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163643282)
This works! Following the hint from https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2160767733 I could integrate it into the tests of https://github.com/bitcoin/bitcoin/pull/29415 to try to send two transactions that have the same txid and different wtxid.

However using it is a bit involved - one has to create a new dedicated parent transaction, get it in the mempool and then broadcast the two children. Also, one has to handle the fees manually. Like this:

```py
self.log.info("S
...

🤔 ismaelsadeeq reviewed a pull request: "util: detect and warn when using exFAT on MacOS"
(https://github.com/bitcoin/bitcoin/pull/31453#pullrequestreview-2953272636)
Thanks for taking my suggestion @willcl-ark

Your modified string is even better!

Just a couple of comments and then I will re-test again.

💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163635372)
nit: use `fs::quoted` here and other places, will avoid the slashes?

💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163641986)
nit: would be nice to be just generic not specific to macOs

💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163670725)
Having a constructor here will prevent the C.I failure I think, emplace_back will forward the values to constructor which is missing now.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2163681310)
@andrewtoth, right! I will change that next time I retouch, "resolving" this.

@stratospher, thanks! Lets continue the discussion in https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163643282

👍 ismaelsadeeq approved a pull request: "mempool: use `FeeFrac` for ancestor/descendant score comparators"
(https://github.com/bitcoin/bitcoin/pull/32799#pullrequestreview-2953351336)
Code review ACK 922adf66ac7420e21ea171d3586ea84554e5d91b

💬 ismaelsadeeq commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2999938644)
cc @fjahr and @polespinasa, friendly pinged since you take a look initially.