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

💬 casey commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1845558046)
`cmake` presets only work for cmake commands, however there are a bunch of commands one should know for working on core.

💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2481546911)
Assuming the remaining CI failure is #31307

📝 hebasto opened a pull request: "ci, iwyu: Treat warnings as errors for specific targets"
(https://github.com/bitcoin/bitcoin/pull/31308)
This PR is the first step towards treating IWYU warnings as errors. It utilises CMake's native support via the [`CMAKE_CXX_INCLUDE_WHAT_YOU_USE`](https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_INCLUDE_WHAT_YOU_USE.html)) variable. While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the `iwyu_tool.py` script or CMake's built-in functionality.

At this stage, only the h
...

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1845646642)
Given that https://github.com/bitcoin/bitcoin/pull/31305 fixes the issue, are we disabling it as a temporary measure?

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1845650560)
Right.

It depends on which PR is merged first :)

💬 theStack commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1845652266)
This call tests for P2WSH inside P2WSH and always returns `false`, hence the whole if condition is not fulfilled and the two lines below will never be executed (can be verified easily by putting an `assert(false)` in the body and running unit and wallet functional tests -- they still pass without crash). I assume this was meant to be
```suggestion
is_valid_script(witness_script, ScriptContext::P2WSH)) {
```
It's surprising though that the tests still passed. Either the tests
...