💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1919110665)
This is no longer relevant for the recent [push](https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610).

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2596757762)
re-ACK 0f716f28896c6edfcd4e2a2b25c88f478a029c7b

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1919132992)
Okay, I've checked CMake docs again.

`CMAKE_REQUIRED_LINK_OPTIONS` is forwarded to `target_link_options()`, which is [designed](https://cmake.org/cmake/help/latest/command/target_link_options.html) "to add any link options".

I'll update the PR shortly.

📝 darosior opened a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676)
Based on https://github.com/bitcoin/bitcoin/pull/31022, this introduces a fuzz target for `PCPRequestPortMap` and `NATPMPRequestPortMap`.

Like in #31022 we set `CreateSock` to return a `Sock` which mocks the responses from the server and uses a mocked steady clock for the `Wait`s. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.

I ran both targets overnight
...

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1919145857)
> I meant as `LINK_OPTIONS`. I'd rather have LDFLAGS forward to that unless there's a reason not to.

In that case a static library (e.g., `/usr/lib/libFuzzingEngine.a`) would be placed _before_ object files being linked, and the linker might fail.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2596810683)
> Can you update the PR description to have a list of pre-requisite PRs?

There's a tracking issue with everything listed #31246

👍 ryanofsky approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2556956641)
Code review ACK e9edf28ba4ff4ebf02ae90874794c51c55ef789e. Nice changes! Again I think using consistent build locations that match install locations should make it is easier to see what binaries are available, simplify scripts and wrappers, and avoid bugs that only happen in developer builds but not final installs or vice-versa.

Since last review the main change seems to be that subproject binaries are not affected anymore by this PR, which is great.

I don't understand the details of the di
...

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919027627)
In commit "cmake: Set top-level target output locations" (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

It wasn't really clear to me what how this comment "The top-level target output locations for subprojects remain unmodified" is related to code it is commenting on.

Would maybe suggest changing to something more explanatory like "These need to be included before CMAKE_*_OUTPUT_DIRECTORY variables are set, so output locations of subproject tests and libraries are not overridden."

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919075538)
In commit "cmake: Set top-level target output locations" (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

I was confused why univalue seemed to be handled differently than the other subprojects above.

Might be helpful to add a comment like "# The univalue subproject is different from the other subprojects because we actually use the CMakeLists.txt in the project subdirectory instead of defining separate cmake targets, so use add_subdirectory() instead of include() to include it."

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919045923)
In commit "cmake: Set top-level target output locations" (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

This seems ok, but I don't understand why these include lines are moving from `CMakeLists.txt` to `src/CMakeLists.txt`. Unclear whether it's necessary for this PR, or maybe it's just nice to consolidate these together with univalue?

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919080284)
In commit "cmake: Set top-level target output locations" (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

These two lines seem to be duplicating lines at the top this file. I assume this is a copy and paste error and would suggest deleting one of the copies.

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919050514)
In commit "cmake: Set top-level target output locations" (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

It seems like it might make things more obvious if included files were still referenced by path instead of by name: as `../cmake/crc32.cmake` instead of `crc32`.

This would also avoid the need to change CMAKE_MODULE_PATH. And it might make sense conceptually because these files don't act like typical cmake [modules](https://cmake.org/cmake/help/book/mastering-cmake/chapter/Modules.html) bec
...

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919164306)
Ideally the xor key would be written at the time of blocksdir creation. But I had a go at that and it means that non-bitcoind (kernel, tests, etc) binaries wouldn't get a key written.

So yeah, filtering it out here is probably the most straightforward. Blah.

💬 hebasto commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#discussion_r1919177312)
`message(CONFIGURE_LOG ...)` was [added](https://cmake.org/cmake/help/latest/command/message.html#configure-log) in CMake version 3.26. Our minimum required one is 3.22.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919184203)
> Unclear whether it's necessary for this PR, or maybe it's just nice to consolidate these together with univalue?

The latter.

💬 instagibbs commented on pull request "test: add coverage for immediate orphanage eviction case":
(https://github.com/bitcoin/bitcoin/pull/31628#issuecomment-2596870892)
https://github.com/bitcoin/bitcoin/actions/runs/12815476373/job/35734215113

having a real test failure that I can't seem to replicate, investigating

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919195245)
The `include`d subprojects are subtrees managed with our custom CMake scripts. While there was an initial plan to switch to their upstream CMake scripts, this approach is currently [considered questionable](https://github.com/bitcoin/bitcoin/pull/30905).

Conversely, `univalue` was promoted from a subtree to our own codebase, adopting the same per-directory approach.

✅ dergoegge closed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153)

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2596901446)
Added an additional inspector function:
* `CountDistinctClusters`, which efficiently counts how many distinct clusters a list of specified Refs belong to, for helping with enforcing RBF cluster policy limits.

👍 danielabrozzoni approved a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2557317115)
tACK eb325bc0efd3f999189e155ba836be92e6cc9af7 - I reviewed the code and manually verified the behavior of the affected RPC arguments, both old and new.

Thank you for splitting the changes into several separate commits; it made the review process much easier! :)