💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2340420173)
It also doesn't look cmake related, because the latest 28.x build also times out: https://github.com/bitcoin/bitcoin/runs/29755145359

📝 hebasto opened a pull request: "build: Improve `ccache` performance for different build directories"
(https://github.com/bitcoin/bitcoin/pull/30861)
This PR makes it possible to reuse the `ccache` cache populated during a build in another build directory:
```
$ cmake -B build_1 -DWITH_CCACHE=ON
$ cmake --build build_1 -t bitcoind
$ cmake -B build_2 -DWITH_CCACHE=ON
$ cmake --build build_2 -t bitcoind # 100% cache hit rate is expected.
```

The first commit has been picked from https://github.com/bitcoin/bitcoin/pull/30803.

Please refer to https://github.com/bitcoin/bitcoin/pull/20353 for historical context.

Addresses this [com
...

💬 hebasto commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2340474631)
> However, ccache does _not_ hit across two different build dirs, compiling the same commit.

Fixed in https://github.com/bitcoin/bitcoin/pull/30861.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2340474919)
ACK faa32adbcf4c04f0a426eaba4a43b29a293de72b

<details>
<summary>Details</summary>

* brace initialization in base.cpp
* pos*t*itional typo fix
* added ConstevalFormatString overload

</details>

📝 hebasto converted_to_draft a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811)
Closes https://github.com/bitcoin/bitcoin/issues/30799:

```
$ ./build/src/bitcoind -logsourcelocations -asmap=/tmp/no_file 2>&1 | head -1
2024-09-04T07:01:03Z [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v28.99.0-b94fe85fbe2f (release build)
```

💬 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
...