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

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

...

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

💬 hebasto commented on pull request "depends: Update minimum required CMake version":
(https://github.com/bitcoin/bitcoin/pull/31300#issuecomment-2481616301)
> Is there also precedent for us making patches just to silence warnings?

`depends/patches/qrencode/cmake_fixups.patch`?

This PR is not about silencing warnings but ensuring that deprecated CMake versions are not used.

💬 furszy commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2481647298)
> Something else that's useful to note is that even a short rescan, on a weak or otherwise loaded machine, can cause RPC calls to start timing out.

Just a recommendation.
Can make the rescan lighter and faster by enabling `-blockfilterindex`. It will avoid reading and traversing blocks with no matching elements.

💬 gmart7t2 commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2481760335)
I have that, and it does make it faster. But I still get RPC timeouts when the machine is loaded, particularly if the wallet already has a lot of descriptors in it. It doesn't only rescan for the one(s) I am adding, but also for all the pre-existing descriptors in the wallet.

⚠️ achow101 reopened an issue: "importdescriptors always rescans"
(https://github.com/bitcoin/bitcoin/issues/31263)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Running `importdescriptors` with `timestamp: now` rescans the last 2 hours worth of blocks.

The docs say that '"now" can be specified to bypass scanning' but that doesn't appear to be true:

```
"timestamp": timestamp | "now", (integer / string, required) Time from which to start rescanning the blockchain for this descriptor, in UNIX epoch time

...

💬 furszy commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2481775432)
> I have that, and it does make it faster. But I still get RPC timeouts when the machine is loaded, particularly if the wallet already has a lot of descriptors in it. It doesn't only rescan for the one(s) I am adding, but also for all the pre-existing descriptors in the wallet.

Yes. Thats something I already have in my TO DO list. It requires a rescanning restructuring first.

💬 PastaPastaPasta commented on pull request "refactor: covert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1845839618)
Applied in https://github.com/bitcoin/bitcoin/pull/31301/commits/6077262788333759f1ddc27539e86cd04ac8d076