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

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