💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751802942)
can we still have errors like [this](https://github.com/bitcoin/bitcoin/blob/master/src/logging.h#L253-L265) after this change?

👍 theStack approved a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2292244981)
Tested ACK e9ad9e01409eaa2009f75deb1cf834983c5a6137

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751829061)
Yes, in theory. The goal of this pull request is to make the less likely. According to the pull request description, "This is the first step toward https://github.com/bitcoin/bitcoin/issues/30530 and a follow-up will apply the approach to the other places."

💬 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