💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348778419)
Nice! So this looks like a massive speedup on Windows.

I think the two commits can be squashed, because they do the same thing (just to different files), like the previous modifications to the scripts, which were also a single commit.

💬 fanquake commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348779284)
This needs benchmarking on the systems used by devs (i.e Linux, and ideally those that previously reported issues), to ensure that we don't just have the same problem of Linux performance regressions again.

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348782433)
> think the two commits can be squashed

I've put the before/after measurements in the commit messages, I'd prefer them separate

💬 fanquake commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348783243)
> I've put the before/after measurements in the commit messages, I'd prefer them separate

You can put that in the PR description, that will be included in the merge.

💬 theStack commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2348783689)
The re-bucketed test in the commit doesn't match the one in the commit/PR title (p2p_timeouts vs. p2p_node_network_limited), so I suppose one of the two has to be adapted.

🤔 hebasto reviewed a pull request: "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake"
(https://github.com/bitcoin/bitcoin/pull/30888#pullrequestreview-2302930242)
The minimum supported CMake version, 3.22, and the most recent version, 3.30, show similar performance on Linux as in the PR description.

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1758737293)
would it make sense to change the return type to `std::span<const std::byte>` instead?

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348787319)
Thanks a lot for checking @hebasto!

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#issuecomment-2348789890)
> Regarding the abort on CI specifically would be very useful as well in order to catch the issues earlier, not sure about the cons on test-only builds

It should be trivial to extend the check to reject `%n`, but I still don't think it matters much either way, and it would be better in a separate pull request anyway.

💬 hebasto commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#issuecomment-2348791388)
My Guix build:
```
aarch64
86d20fcaf2331084035fba29305dbcd22665a7a0f910ffbe1667541a338129ed guix-build-001b1cf01045/output/aarch64-linux-gnu/SHA256SUMS.part
06ec1b29ed2ac24733fa24083460298e4c9a2cbba8845164f849aebb50c94e91 guix-build-001b1cf01045/output/aarch64-linux-gnu/bitcoin-001b1cf01045-aarch64-linux-gnu-debug.tar.gz
16b21d25dab9222dc69774e0e788c10bac4f15783c4ccb46c11e0608d132021d guix-build-001b1cf01045/output/aarch64-linux-gnu/bitcoin-001b1cf01045-aarch64-linux-gnu.tar.gz
3cdd10a2
...

💬 fanquake commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2348796541)
Guix Build:
```bash
94142f4399e6b57ae5d95364685cf545a20e1974eb3e6061e62b77af57e59a6b guix-build-89bf11b80725/output/aarch64-linux-gnu/SHA256SUMS.part
1bbbbc9c2818eb6c87a7d7164afc61eee299d8417d986832eb0d503484994f4b guix-build-89bf11b80725/output/aarch64-linux-gnu/bitcoin-89bf11b80725-aarch64-linux-gnu-debug.tar.gz
0601f57454694181473f03bc8ff6c6f23f6ff50f38190a9ff42bd351ff739460 guix-build-89bf11b80725/output/aarch64-linux-gnu/bitcoin-89bf11b80725-aarch64-linux-gnu.tar.gz
10ce23a32f6a21e4
...

💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348809433)
review ACK 2a581144f28bad44de40122864f2f7b9fc5000de 👒

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 2a581144f28b
...

💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348832162)
(Also tested on a fresh Ubuntu 24.04 VM and saw a 30x speedup, which is useful, because this part isn't cached by ccache between builds)

💬 maflcko commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2348843336)
> Locally these tests take about 20 seconds and appear in the correct bucket.

I think the sorting isn't really based on the "correct bucket", but rather the inverse overall duration, the buckets are just a guideline/reminder. That is, as long as the longest running test is started first, everything is fine.


Did you measure if this actually makes a meaningful difference?

👍 hebasto approved a pull request: "build: add `standard branch-protection` to hardening flags for aarch64-linux"
(https://github.com/bitcoin/bitcoin/pull/30433#pullrequestreview-2303002464)
ACK 001b1cf010453adbb1316a6fa8911398953afe61.

💬 maflcko commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2348847390)
Also, could rebase for a fresh CI? At least for me the CI timeouts went away earlier this night when trying to reproduce, but I don't know if they are really gone, and whether the issue was inside this repo, or on the CI runners, or on the CI backend software, or on the Cirrus/GHA integration.

💬 maflcko commented on pull request "doc: unit test runner help fixup":
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2348854269)
review ACK 0024d2c6ea0fafe9b9949af4bbcd0c583e580746

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758844489)
thanks for the feedback!

I updated this to
```
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
cmake -B build --toolchain=depends/x86_64-pc-linux-gnu/toolchain.cmake
cmake --build build
build/src/bitcoind -regtest -printtoconsole -debug=ipc
BITCOIND=$(pwd)/build/src/bitcoind build/test/functional/test_runner.py
```

Which does start running the tests please let me know if this looks ok

👍 hebasto approved a pull request: "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake"
(https://github.com/bitcoin/bitcoin/pull/30888#pullrequestreview-2303103922)
ACK 2a581144f28bad44de40122864f2f7b9fc5000de.

The new scripts introduce trailing spaces in the generated header, but these are not essential.

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758847279)
thanks! I removed that in [5a85022](https://github.com/bitcoin/bitcoin/pull/30875/commits/5a85022cc6c315e9c756bbbb63a7ed992e7f23d3)