💬 naumenkogs commented on pull request "addrman: change internal id counting to int64_t":
(https://github.com/bitcoin/bitcoin/pull/30568#issuecomment-2340598157)
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f

The reasoning in the original post makes sense, this is an improvement. I reviewed the code visually.

💬 kevkevinpal commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2340604581)
nit
it might be useful for reviewers to know what PR/PR's the `After the transition from Autotools to CMake` refers to, if you can add to the description

I'm assuming it is these ones
https://github.com/bitcoin/bitcoin/pull/30454
https://github.com/bitcoin/bitcoin/pull/30664

👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2292315783)
Code review ACK 966027bdda53d150321af7db48b57f9756c54e68, no changes since last rebase

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2340615910)
> nit it might be useful for reviewers to know what PR/PR's the `After the transition from Autotools to CMake` refers to, if you can add to the description

Good idea.

> I'm assuming it is these ones #30454 #30664

Yes it't these two, linked them in the PR description.

💬 fanquake commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2340618396)
https://cirrus-ci.com/task/4580438562308096?logs=ci#L1723
```bash
Run clusterlin_ancestor_finder with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_corpora/clusterlin_ancestor_finder')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2205311796
INFO: Loaded 1 modules (615399 inline 8-bit counters): 615399 [0x55b1778f3be8, 0x55b177989fcf),
INFO: Loaded 1 PC tables (6
...

💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1751878338)
When building across different build directories, using the `base_dir` option helps work around absolute paths in `-I` compiler options, increasing the `ccache` cache hit rate.

Maybe, the word "effectiveness" is more appropriate, rather than "performance".

The change in the condition is not directly related to improving the hit rate. It enhances code correctness, as the method using of the `CMAKE_{C,CXX}_COMPILER_LAUNCHER` variables is applicable only to the mentioned generators.

💬 hebasto commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2340651422)
@maflcko @fanquake

I believe all comments have been addressed, unless I missed something.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2340661923)
@fanquake Thanks! Fixed, I think.

💬 naumenkogs commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2340676138)
ACK d4c7c4009da14c84576d43ab4a1241967cfa5ffc

The change makes the code much easier to understand, and I couldn't find any bugs or flaws.

💬 naumenkogs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2340682733)
Concept ACK

💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751908222)
It might be less obvious (and maybe not even a problem) for this simple harness but the decode/encode and encode/decode round-trips are different tests. Their input spaces are not the same, which means that reusing the same fuzz input for both leads to less efficient fuzzing (e.g. the corpus will include inputs that are only relevant to one of the tests but both will be executed).

👍 hebasto approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2292424496)
ACK 820bae4708b013a1634bc62e0c6b56c9688ac645.

I wonder if we can get rid of the `HAVE_BUILD_INFO` macro simultaneously or in a follow-up.

💬 hebasto commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#issuecomment-2340716861)
cc @fanquake @ryanofsky

💬 naumenkogs commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2340717004)
ACK f1704c021e615f740130fff7cd47093d708a3028

👍 naumenkogs approved a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-2292430025)
ACK f1704c021e615f740130fff7cd47093d708a3028

💬 naumenkogs commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1751912464)
8a82ab9840a46f6382fca92b4f6bb60e5fc5ac6d

nit
Deserves commenting what if `preferred_net` is not set (Well I guess there is already a comment "empty = all").

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2340770254)
> I wonder if we can get rid of the `HAVE_BUILD_INFO` macro simultaneously or in a follow-up.

Sounds reasonable and simple enough (if I didn't miss anything), added a commit for that in this PR.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2292507327)
Rebased 61c7ad56818888b2ee07db02134456aaf3e9aedc -> 9c98c42a0166e9e201f6e9d32a0692fad5a185f0 ([`pr/mine-types.8`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.8) -> [`pr/mine-types.9`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.8-rebase..pr/mine-types.9)) adding suggested cmake comment, fixing depends package which pointed at an old libmultiprocess version causing https://cirrus-ci.com/task/465
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751966743)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751359880

> On Intel macOS 14.6.1 this doesn't work for me (CI feels the same).
>
> ```
> git clean -dfx
> cmake -B build -DWITH_MULTIPROCESS=true
> ...
> -- Configuring done (29.3s)
> CMake Error at src/CMakeLists.txt:335 (add_dependencies):
> The dependency target "bitcoin_ipc_headers" of target "bitcoin_ipc_test"
> does not exist.
> ```

Thanks, good catch. The `bitcoin_ipc_headers` target didn't exist because
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751951659)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751353778

> [988dace](https://github.com/bitcoin/bitcoin/commit/988dacead4a9a6850b767a8ced0c08b47fece56d): can you add a comment for why this isn't in `src/test/CMakeFile.txt`? A tl&dr of https://github.com/bitcoin/bitcoin/pull/30510/files#r1747656292

Thanks, added comment