💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751830708)
Thanks, adjusted in the last push.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751830795)
Thanks, adjusted in the last push.

💬 naumenkogs commented on pull request "addrman: change internal id counting to int64_t":
(https://github.com/bitcoin/bitcoin/pull/30568#discussion_r1751844750)
I assume this is about the introduced index mismatch at `AddrInfo& info = mapInfo[entry_index];`, specifically the keys of `mapInfo`.

It is technically safe because we always go from a narrower type to the wide one... but that's what is immoral i guess :)

I think it's fine they way this commit works, at most we could add a comment explaining this mismatch.

💬 kevkevinpal commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2340578771)
tested ACK [15cc656](https://github.com/bitcoin/bitcoin/pull/30859/commits/15cc65649b78ffcbd1e1b302c2e4866623cee294)

I'm fine with the extra log added to `test/functional/test_framework/test_framework.py` and `test/functional/test_runner.py`

One nit is that the new error logs are the exact same and defined in two different areas of the code but not any thing that would make me not want this merged

💬 hebasto commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2340581949)
My Guix build:
```
aarch64
02be00bb04d41a22ac8122ecb3fa60979d58e7974d849b91ac48bc535fcf00ce guix-build-820bae4708b0/output/aarch64-linux-gnu/SHA256SUMS.part
d9730cc7efaf928dc0bb75f966c2e0f2cd0ac90e46722d24554da52572b9127b guix-build-820bae4708b0/output/aarch64-linux-gnu/bitcoin-820bae4708b0-aarch64-linux-gnu-debug.tar.gz
7971ccc3128f7fd6f07eb248a4fe5ae8afd98a60a746c69ec3eea62f8c87c3c7 guix-build-820bae4708b0/output/aarch64-linux-gnu/bitcoin-820bae4708b0-aarch64-linux-gnu.tar.gz
5adb847f
...

💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2340588667)
> 100% cache hit rate is expected.

I tested this, and got about 90%:
```bash
ccache -C
ccache --zero-stats

cmake -B some_build_dir -DWITH_CCACHE=ON
cmake --build some_build_dir -j17

ccache --show-stats
Cacheable calls: 445 / 445 (100.0%)
Hits: 0 / 445 ( 0.00%)
Direct: 0
Preprocessed: 0
Misses: 445 / 445 (100.0%)
Local storage:
Cache size (GiB): 0.2 / 30.0 ( 0.75%)
Hits: 0 / 445 ( 0.00%)
Misses:
...

💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1751859171)
In 66b73bbf375ff96bf4a77539c9cf3fe52fbc15d5: can you explain the performance improvement (also how is it related to changing this conditional)?

💬 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