🤔 l0rinc requested changes to a pull request: "refactor: covert ContainsNoNUL to ContainsNUL"
(https://github.com/bitcoin/bitcoin/pull/31301#pullrequestreview-2440971933)
While [refactor-only PRs are often frowned upon](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L61) (since the benefit is small, but the risk is big), I think this is small enoughfor us to consider.

You could simplify further by changing the occurrences via https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs (making sure all commit compile)

Note that the `utxo_snapshot.cpp(45,1): error C1001: Internal compiler error` seems un
...

💬 l0rinc commented on pull request "refactor: covert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1845411957)
We should be able to simplify further:
```suggestion
return str.find('\0') != std::string_view::npos;
```

💬 l0rinc commented on pull request "refactor: covert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1845412066)
`[[nodiscard]]` doesn't make a lot of sense here, since the method doesn't have any side-effect

💬 l0rinc commented on pull request "refactor: spanify DecodeBits, use constexpr std::array instead of vector":
(https://github.com/bitcoin/bitcoin/pull/31302#issuecomment-2481195565)
approach NACK, we should only modify this when the affected area is already changed and tested well enough (+ whole build is red, also indicating this wasn't the case). The risk of breaking something is a lot greater while the benefit is likely very small. These should usually be bundled with other related changes, having a concrete purpose (e.g. tests are covering something that cannot happen in reality, new feature being introduced, result is undeniably safer code, etc).

⚠️ mraksoll4 opened an issue: "Bitcoin 28.0 ver. Compiler bug at fuzz when compiling using vs 2022."
(https://github.com/bitcoin/bitcoin/issues/31304)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

compile bug at fuzz , and warning about wildcards.

### Expected behaviour

i exepect success build , only fuzz problem.

### Steps to reproduce

i use this docs

https://github.com/bitcoin/bitcoin/tree/v28.0/build_msvc

and use

Microsoft Visual Studio Community 2022 (64-bit) - Current
Version 17.12.0


### Relevant log output

```
H:\bitcoin-28.0\src\test\fuzz\utxo_snapshot.cpp
...

💬 l0rinc commented on issue "Bitcoin 28.0 ver. Compiler bug at fuzz when compiling using vs 2022.":
(https://github.com/bitcoin/bitcoin/issues/31304#issuecomment-2481232758)
Seems like a duplicate of https://github.com/bitcoin/bitcoin/issues/31303

💬 MORTEZAMIRSALI commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1845443185)
> Done, thanks!

✅ fanquake closed an issue: "Bitcoin 28.0 ver. Compiler bug at fuzz when compiling using vs 2022."
(https://github.com/bitcoin/bitcoin/issues/31304)

📝 l0rinc opened a pull request: "refactor: Fix remaining clang-tidy performance-inefficient-vector errors"
(https://github.com/bitcoin/bitcoin/pull/31305)
PR inspired by https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437847307 (and https://github.com/bitcoin/bitcoin/pull/29458, https://github.com/bitcoin/bitcoin/pull/29606, https://github.com/bitcoin/bitcoin/pull/29607, https://github.com/bitcoin/bitcoin/pull/30093).

I ran the mentioned `clang-tidy` command via
```bash
run-clang-tidy -quiet -p ../build -j $(nproc) -checks='-*,performance-inefficient-vector-operation' | grep -v 'clang-tidy'
```
which revealed 3 tests and 1 prod
...

📝 hebasto opened a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/31306)
This PR switches to the latest [IWYU 0.23](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.23), which is compatible with Clang 19.

The "bugprone-use-after-move" and "modernize-use-starts-ends-with" warnings that emerged have been resolved.

Enabling the "performance-inefficient-vector-operation" and "performance-unnecessary-copy-initialization" checks is deferred to follow-up PRs (e.g., https://github.com/bitcoin/bitcoin/pull/31305).

💬 hebasto commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2481352375)
Also see https://github.com/bitcoin/bitcoin/pull/31306.

💬 hebasto commented on pull request "refactor: add clang-tidy `modernize-use-starts-ends-with` check":
(https://github.com/bitcoin/bitcoin/pull/30868#issuecomment-2481352691)
> Seems like newer Clang 19 has improved this check, and throws out more (historical) instances to change. i.e:
>
> ```shell
> bitcoin/src/torcontrol.cpp:359:27: error: use starts_with instead of compare() == 0 [modernize-use-starts-ends-with,-warnings-as-errors]
> 359 | if (0 == line.compare(0, 20, "net/listeners/socks=")) {
> | ~~~~ ^~~~~~~~~~~~~~ ~
> | starts_with( )

...

👍 theStack approved a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2436530611)
Code-review ACK f50470e33c1bb51531f3fa08f6d29c54df304d6b

To my understanding, the second and fourth commits (db270fa2a33d7c88e58682ab8a219a2e70503c9e and f50470e33c1bb51531f3fa08f6d29c54df304d6b) are pure refactors, while the third one actually changes behaviour for `Descriptor::Expand` call-sites w.r.t. by returning more public key data than before (stored in the `FlatSigningProvider& out` parameter), so the first commit (d1be90704559da45e4d76171230ecb9f4a6d9cf8) is needed to avoid breaking
...

💬 theStack commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1842493705)
in commit f50470e33c1bb51531f3fa08f6d29c54df304d6b: nit: could also update the comment here, e.g. add "... and put it into the passed signing provider."

💬 theStack commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1843938258)
comment nit
```suggestion
// Construct temporary data in `pubkeys`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
```

💬 theStack commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1845517262)
nit: I wonder if this method will still be needed once the legacy wallet (and with that, the `importmulti` RPC call in particular) is gone? If not, could maybe add a TODO here to get rid of it in the future.

📝 hebasto opened a pull request: "build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC"
(https://github.com/bitcoin/bitcoin/pull/31307)
This PR suggest a temporary workaround for a compiler bug [introduced](https://github.com/bitcoin/bitcoin/issues/31303) in Visual Studio 2022 version 17.12.

This workaround is required to fix the CI until the upstream compiler bug is resolved.

💬 sipsorcery commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2481375310)
@hebasto I have never buil the fuzzer before. Is the error from the build [here](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#quickstart-guide)?

💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2481384222)
@sipsorcery
> I have never buil the fuzzer before. Is the error from the build [here](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#quickstart-guide)?

Just configure with `-DBUILD_FUZZ_BINARY=ON` and build the `fuzz` target.

🤔 pablomartin4btc reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2441120863)
tACK a16bd17d34bb7a8e704cbd7d4f741b9d2e309246

Built depends on macOS 14.4 (still got the warning regarding dupe library `secp256k1` while building `test-bitcoin` - I think that was already happening with the cmake integration).

Built depends on Ubuntu 22.04:
- Some files (`depends/source/CMakeLists.txt`, ECMOptional* and QtTopLevel*) were not found during the `make -C depends` due to they missed the suffix `-6.7.3` so I had to manually copy them in order to continue with the build (this w
...