💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2545150228)
There's a related report to libxcb here: https://gitlab.freedesktop.org/xorg/lib/libxcb/-/merge_requests/25.

👍 vasild approved a pull request: "refactor: remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3486800310)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a

Reviewing is learning, I love it! Thanks!

📝 maflcko opened a pull request: "clang-format: Set PackConstructorInitializers: CurrentLine"
(https://github.com/bitcoin/bitcoin/pull/33912)
Now that the minimum supported clang version is 17, the `PackConstructorInitializers` setting can be set to `CurrentLine` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#packconstructorinitializers)

The `CurrentLine` option will either put all constructor initializers on the current line if they fit. Otherwise, it will put each one on its own line.

The `CurrentLine` option is desirable over the current `BinPack` option, because:


...

💬 maflcko commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2545299017)
I don't think the tip can be nullptr in rpc code. This can just be a `CHECK_NONFATAL`, but this seems unrelated to lifetimebound stuff?

💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2545337568)
> this seems unrelated to lifetimebound stuff

Yes, he started with:

> An off-topic nit

💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2545357404)
reminds me that the `bash -c` isn't needed here, now that `env` is used. Feel free to remove, or ignore the unrelated nit.

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3557161480)
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4

Only a [const removal](https://github.com/bitcoin/bitcoin/compare/9a387e9175d34d1eaf83b9008f5e857d1eb04b8e..de4242f47476769d0a7f3e79e8297ed2dd60d9a4?diff=unified&w) since my last ACK

💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2545381694)
maybe unrelated, because I think it was working at some point, but using `podman` on various distros (Debian/Ubuntu/OpenSuse) to try to run the s390x task fails with a segfault in the depends compilation:

```
...
[58/817] Building CXX object qtbase/src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/io/qfsfileengine_iterator.cpp.o
[59/817] Building CXX object qtbase/src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/io/qloggingcategory.cpp.o
FAILED: qtbase/src/tools/bootstrap/C
...

👍 l0rinc approved a pull request: "clang-format: Set PackConstructorInitializers: CurrentLine"
(https://github.com/bitcoin/bitcoin/pull/33912#pullrequestreview-3487068101)
ACK fad0c76d0a109ab7b063f0d405588cf1e6c15e4d

I agree that this is our best option now, mostly because it will minimize diffs

💬 l0rinc commented on pull request "clang-format: Set PackConstructorInitializers: CurrentLine":
(https://github.com/bitcoin/bitcoin/pull/33912#discussion_r2545431725)
> Now that the minimum supported clang version is 17, the PackConstructorInitializers setting can be set to CurrentLine in the clang-format file.

Nit: `CurrentLine` was already available in [16](https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#packconstructorinitializers)
<img width="967" height="703" alt="Image" src="https://github.com/user-attachments/assets/4e1fb719-9f19-496d-b184-9d869cc27a33" />

The new addition in [17](https://releases.llvm.org/17.0.1/tools/
...

💬 fanquake commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3557317662)
> This [branch](https://github.com/hebasto/bitcoin/tree/251119-qt6.9) fails to cross-compile on Debian Bookworm with GCC 12.2.0:

That's the same issue we work around in CMake, which is fixed by just turning off stack-clash-protection: https://github.com/fanquake/bitcoin/tree/qt_6_9_no_stack_clash_win. However it looks like there are other issues, possibly due to https://github.com/qt/qtbase/commit/d25d9f2c2673bc287590d9a83bd7ef1357d7021a#diff-4ae1a340d7e79a6402009cb635708b4950b1633f4bc850aba4
...

💬 maflcko commented on pull request "clang-format: Set PackConstructorInitializers: NextLineOnly":
(https://github.com/bitcoin/bitcoin/pull/33912#discussion_r2545495948)
Thx, nice find. `NextLineOnly` is actually even closer to what this project is using. I've switched to that

⚠️ fanquake opened an issue: "ci: failure to download 0.14.3-win64.zip in Win test cross-built"
(https://github.com/bitcoin/bitcoin/issues/33913)
This is happening semi-frequently:

https://github.com/bitcoin/bitcoin/actions/runs/19520873587/job/55885060054:
```bash
Run ./test/get_previous_releases.py --target-dir $PREVIOUS_RELEASES_DIR

Download failed: Remote end closed connection without response
Releases directory: D:\a\_temp/previous_releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-0.14.3/bitcoin-0.14.3-win64.zip
Error: Process completed with exit code 1.
```

https://github.com/bitcoin/bitcoin/actions/runs/19239483669/job
...

💬 l0rinc commented on pull request "clang-format: Set PackConstructorInitializers: CurrentLine":
(https://github.com/bitcoin/bitcoin/pull/33912#discussion_r2545533671)
I'm fine with both, just want, agree that `BinPack` is not the best one - can you please update the PR description to reflect that the current one wasn't necessarily motivated by the 16 -> 17 upgrade.

📝 Sjors opened a pull request: "Add string literal convenience overload to Parse()"
(https://github.com/bitcoin/bitcoin/pull/33914)
While investigating a silent merge conflict in #33135 I noticed that #32983 changed the descriptor `Parse` function signature from `const std::string& descriptor` to `std::span<const char> descriptor`.

Calling that new version of `Parse` with a string literal will trigger a confusing "Invalid characters in payload" due to the trailing "\0".

It can be worked around by having (the test) wrap string literals in `std::string()`, but it seems better to either disallow literal strings or have a
...

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3557518664)
#33914 fixes this. I based this PR on it.

💬 Sjors commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#issuecomment-3557529213)
See #33914 for a followup.

💬 maflcko commented on issue "ci: failure to download 0.14.3-win64.zip in Win test cross-built":
(https://github.com/bitcoin/bitcoin/issues/33913#issuecomment-3557574741)
yeah, network requests should be retried.

An alternative could be to cache the bins, but I am not sure how large they are and if the diff is worth it. It could look something like this, if the simply retry does not improve things:


```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 7ba353a..3e2e1a1 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -394,6 +394,7 @@ jobs:
env:
PYTHONUTF8: 1
TEST_RUNNER_TIMEOUT_FACTOR: 40
+
...

📝 maflcko opened a pull request: "test: Retry download in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/33915)
Hopefully fixes https://github.com/bitcoin/bitcoin/issues/33913 (intermittent download issues)

If not, the diff there to cache the bins can be considered.

💬 maflcko commented on pull request "clang-format: Set PackConstructorInitializers: CurrentLine":
(https://github.com/bitcoin/bitcoin/pull/33912#discussion_r2545710974)
thx, reworded the pull description, happy to adjust it more, if you have suggestions.