💬 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! :)

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919214193)
Done.

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1919215050)
Do we care about keeping the checksum calculation compatible? For almost all users, this PR being merged will mean a new asmap database anyway, so they'll incur the price of addrman invalidation that brings anyway.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825)
> Code review ACK [e9edf28](https://github.com/bitcoin/bitcoin/commit/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.

@ryanofsky

Thank you for your review! The branch has been updated per your feedback.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919269581)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825).

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919271417)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825).

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919272736)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825).

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919285177)
I like that idea too. But what if there is a large amount of orphans left? There shouldn't be a ton, but I don't know how to feel about a peer being granted more than the normal amount of compute because they are going to be disconnected.

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1919306217)
> The last push wasn't correct. This will fail

Thanks for catching, I did this because without quotes the linter complains about SC2086: https://www.shellcheck.net/wiki/SC2086 and I didn't test any images built with the default `CI_IMAGE_PLATFORM` value.

I am trying to use the native platform by default now in the latest push

Don't fully grasp the context of #28138 but `native_nowallet_libbitcoinkernel` <details>
<summary>
worked on my fedora 40 machine even when this line fails.
</
...

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1919306517)
Thanks, fixed with a different approach.