🤔 pablomartin4btc reviewed a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2302866708)
ACK fabd78e2e30e557e04739e29c9c221ad47245df1

I find this very practical. 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, perhaps someone can add some issues related with it.

📝 TheCharlatan opened a pull request: "kernel: Move background load thread to node context"
(https://github.com/bitcoin/bitcoin/pull/30896)
The thread handle is never used by the ChainstateManager, so move it out and into the node context. Users of the kernel library now no longer have to manually join the thread when destructing the ChainstateManager.

💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758696378)
Fixed.

🤔 tdb3 reviewed a pull request: "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes"
(https://github.com/bitcoin/bitcoin/pull/30875#pullrequestreview-2302866367)
Approach ACK

💬 tdb3 commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758692937)
nit: IMHO, could get rid of the "As of 0.12" part, since we're using CMake now and and 0.12 didn't use it.

💬 tdb3 commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758698079)
nit: "configure script" is an Autotools thing, right?

💬 tdb3 commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758698884)
Same here ("configure script")

💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2348721889)
I made a significant change here, making `m_position` an `std::optional<int64_t>`, which is `std::nullopt` when the position is unknown (due to `ftell` failing, and no successful `fseek` afterwards).

The position being unknown will permit normal operation, but throws an error on read/write when a xor key is set, or when invoking `tell`.

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1758707876)
> I don't think the binary sizes would be a problem after we merge something like #30882 and the extra folder for the corpus is also not a huge cost (they compress really nicely).

Are you sure? IIUC 30882 creates a separate binary for each fuzz target, so depending on the compile/link options (static, without lto, ...), each fuzz target may or may not be heavy. For example, the OSS-Fuzzing already went down due to storage limitations (https://github.com/google/oss-fuzz/pull/12232). I am not s
...

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1758717219)
Thanks for the directions @dergoegge, @maflcko.

> I don't mean to derail this PR, it's already improving things by splitting up base_encode_decode. Feel free to leave it as is.

I'm all for separation of concerns, that's what I've done here as well.
If there's a simple and non-controvertial way to make it even better, let me know!

🤔 hebasto reviewed a pull request: "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake"
(https://github.com/bitcoin/bitcoin/pull/30888#pullrequestreview-2302909740)
Here are benchmarks on Windows using the following command in PowerShell:
```PowerShell
> (Measure-Command { cmake -DRAW_SOURCE_PATH="src/bench/data/block413567.raw" -DHEADER_PATH="build/bench/data/block413567.raw.h" -DRAW_NAMESPACE="benchmark::data" -P cmake/script/GenerateHeaderFromRaw.cmake }).TotalSeconds
```

- the master branch @ 07c7c96022dd325be1cd3b353d575eb6a5593f55 using `file(APPEND ...)` calls:
```
296.326647
```

- this PR (three consecutive runs)
```
1.6235875
1.54560
...

👍 maflcko approved a pull request: "kernel: Move background load thread to node context"
(https://github.com/bitcoin/bitcoin/pull/30896#pullrequestreview-2302911283)
Make sense to rename and move the thread, now that it is doing more general init things and not only importing blocks.

ACK 3a26c839c63ecda2d3b2491d6d84a03c1d8f508f 🤠

<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}"
RUTRmVTMeKV5npGrKx1nqXCw5ze
...

💬 maflcko commented on pull request "kernel: Move background load thread to node context":
(https://github.com/bitcoin/bitcoin/pull/30896#discussion_r1758724516)
nit: While the thread shouldn't be joinable when chainman is null, it may be cleaner to remove the check, or add a comment why it is needed?

💬 theStack commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758726928)
One idea for improved readability might be to name it `ensure_after`, and reorder the (mandatory) `duration` argument to be before the test function lambda? Then the call-site would look like e.g.
`ensure_after(duration=3, lambda: some_condition == True)`

💬 fanquake commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#issuecomment-2348776617)
Guix Build:
```bash
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
3cdd10a2eb9e0bdb
...

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