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

💬 Sjors commented on pull request "Add string literal convenience overload to Parse()":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545727555)
An alternative approach could be to not bother with this overload and simply have `Parse` do the removal at the top.

💬 maflcko commented on pull request "Add string literal convenience overload to Parse()":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545733054)
This seems like a lot of bloat to re-implement `std::string_view` behavior?

The correct fix seems to just change the `Parse(` function to accept a string_view instead. I guess this requires restoring the ` std::span<const char> sp{descriptor};` line, but this is just one line, and seems more acceptable than this here?

💬 hebasto commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3557712376)
> > as of v13.1.0, MinGW supports most of the definitions in these headers.
>
> However there is no v13.1.0 of mingw-w64: https://www.mingw-w64.org/changelog/. The latest release is v13.0.0, and at the time of that commit, it was v11.0.1. So v13.1.0 seems to be in reference to the version of GCC, rather than the version of the headers used.

I assume they were referring to the GCC cross-compiler version, not mingw-w64 itself.

💬 Sjors commented on pull request "Add string literal convenience overload to Parse()":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545821411)
I could also just `= delete` the overload, since afaik only test code deals with string literal descriptors.

👍 vasild approved a pull request: "wallet, sqlite: Encapsulate SQLite statements in a RAII class"
(https://github.com/bitcoin/bitcoin/pull/33033#pullrequestreview-3487577017)
ACK 0af7c2a11819a21e11abf34b4229c5c21150d827

💬 Sjors commented on pull request "Add string literal convenience overload to Parse()":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545857356)
Actually string_view seems to just work(tm), I'll switch to that.

💬 maflcko commented on pull request "Add string literal convenience overload to Parse()":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545861257)
Sure, but that doesn't have any benefits, is still bloated, and doesn't even fix the bug:

https://godbolt.org/z/6EeYvvarr

💬 Sjors commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3557810718)
Changed the approach to using `string_view` as suggested by @maflcko.

💬 maflcko commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545909334)
nit: unused/double scope?

💬 maflcko commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545907849)
nit: I wonder how hard it would be to update most of those to use string_view instead of a span?