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

💬 maflcko commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545896220)
stale comment?

💬 maflcko commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545901557)
string_view is already on `const char` and the const here can be removed. See also a bit related: https://github.com/bitcoin/bitcoin/pull/31650

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2545913179)
Yeah, I was thinking the same. It is just super odd to ignore database errors (in general). I guess the sqlite API is odd here.

💬 Sjors commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545923676)
Interesting, on macOS the compiler refused `Parse("pk(0279...798)"` for me with a clear warning that the method was deleted. But anyway, string_view is better.

💬 Sjors commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545925892)
Dropped.

💬 Sjors commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545932135)
This used to be two tests, I'll drop the extra scope.

💬 Sjors commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3557870296)
Addressed nits.

I'll look into if we can also use `string_view` in one or more of the calls we make, see https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2545907849