📝 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
...
(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).
(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.
(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( )
...
(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
...
(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."
(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.
```
(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.
(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.
(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)?
(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.
(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
...
(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.
(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
(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
...
(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?
(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 :)
(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
...
(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
...
💬 sipsorcery commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2481584659)
I'm not having any luck getting the fuzzer to run but I was able to replicate the compiler error and fix it by getting chatgpt to rejig the code slightly.
```
/* static const auto setup{
MakeNoLogFileContext<TestingSetup>(ChainType::REGTEST,
TestOpts{
.setup_net = false,
.setup_validation_interface = false,
...
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2481584659)
I'm not having any luck getting the fuzzer to run but I was able to replicate the compiler error and fix it by getting chatgpt to rejig the code slightly.
```
/* static const auto setup{
MakeNoLogFileContext<TestingSetup>(ChainType::REGTEST,
TestOpts{
.setup_net = false,
.setup_validation_interface = false,
...
💬 TheCharlatan commented on pull request "depends: Update minimum required CMake version":
(https://github.com/bitcoin/bitcoin/pull/31300#issuecomment-2481613447)
The selected versions in the patches seem random, why not pick the same value for all of them? Is there also precedent for us making patches just to silence warnings?
(https://github.com/bitcoin/bitcoin/pull/31300#issuecomment-2481613447)
The selected versions in the patches seem random, why not pick the same value for all of them? Is there also precedent for us making patches just to silence warnings?