👍 hodlinator approved a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2697929054)
ACK 94967c353ed8041115625b6b3c9acba7961e1f20

- Beginner at CMake/Qt, but couldn't spot anything malicious-looking in CMake changes nor Qt 6 patches.
- Have not tested MacOS builds. Only Linux, Windows Native, Windows Cross, see [Concept review](https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2572359973).

<details><summary>sha256sum-detour</summary>

> hebasto:
> [My Guix build:](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2736277550)

Surprisingly to me
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2738142996)
@hodlinator
> Building on `x86_64`. Maybe just a dirty tree leaking into guix somehow, but surprised that one of the builds matched. :\
> Also built aarch64-linux-gnu, arm-linux-gnueabihf, powerpc64-linux-gnu, riscv64-linux-gnu and x86_64-linux-gnu.

... and hashes do _not_ match https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2736277550, right?

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2738151903)
> @hodlinator
>
> > Building on `x86_64`. Maybe just a dirty tree leaking into guix somehow, but surprised that one of the builds matched. :\
> > Also built aarch64-linux-gnu, arm-linux-gnueabihf, powerpc64-linux-gnu, riscv64-linux-gnu and x86_64-linux-gnu.
>
> ... and hashes do _not_ match [#30997 (comment)](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2736277550), right?

Yes, they do *not* match:

<details><summary>hashes</summary>

```
₿ find guix-build-$(git rev-
...

💬 mabu44 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2738161185)
In reference to my first comment https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2731126667, I note that the imported descriptor and the one returned in the error message are not equal. I imagine that the one in the error message represents the public key for the provided private key. This could reduce the clarity of the error message that no longer enables a direct comparison with the imported descriptors to understand which one generated the error. On the other end, we do not return
...

💬 willcl-ark commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2738172847)
I also cannot reproduce OP:

```bash
will@ubuntu in …/src/core/bitcoin on  master [$!?] via △ v3.31.6 : 🐍 (core)
$ git diff -p | cat
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index a35306b6935..d2904fb3f14 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1480,7 +1480,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
uint8_t ext_flag, key_version;
switch (sigversion) {
case SigVersion::TAPR
...

🤔 sipa reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2700118245)
Big change: `std::vector<ClusterSet>` is gone, replaced by `m_main_clusterset` and `m_staging_clusterset`.
* New helper functions `GetTopLevel()`, `GetSpecifiedLevel()`, `GetClusterSet()` made this fairly easy to do.
* Simplifications resulted in `ClearLocator`, `GetConflicts`, `PullIn`, `MakeTransactionsMissing`, `StartStaging`, `CommitStaging`, `ExtractCluster`.
* `CopyTo` was renamed to `CopyToStaging`.
* `LevelDown` was renamed to `MoveToMain`.
* `MakeTransactionsMissing` was renamed to
...

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004315650)
`NEEDS_SPLIT_OPTIMAL` is gone now after further discussion. Marking resolved.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004306838)
@ajtowns That comment was put in the wrong commit. I've fixed it.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004287728)
Late update (discussed elsewhere): this property actually did not hold. Added comments, and partially rewritten.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004304219)
Done.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004308947)
I added the following comment:

```
// This is a big simulation test for TxGraph, which performs a fuzz-derived sequence of valid
// operations on a TxGraph instance, as well as on a simpler (mostly) reimplementation (see
// SimTxGraph above), comparing the outcome of functions that return a result, and finally
// performing a full comparison between the two.
```

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004286166)
See my comment here: https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2734449766 about how `Assume()` gives you compiler-guaranteed side-effect-equivalence, largely without runtime cost.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004312349)
Together with a lot of changes (getting rid of the `ClusterSet` vector), done.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004316851)
Done.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004313231)
This code is gone now, resolving.

💬 sipa commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2738185787)
@Chand-ra The taproot script unit tests are largely, and don't live in this repository. You need to clone the QA assets repository (https://github.com/bitcoin-core/qa-assets) and then specify the location of its `unit_test_data` directory in the `DIR_UNIT_TEST_DATA` environment variable when running the unit test binary.

🤔 mzumsande reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2699856438)
Concept ACK
only reviewed up to 199a2eb87c3ad9382f2fe4ae97524099224a7b04 so far.

💬 mzumsande commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004115942)
df99f19d81537ecfbf66f6662bc9814b594ef8d2:
I was trying to understand why this fix is correct / the height suddenly changes from 109 to 110, and I find the entire `chainstatemanager_snapshot_init` subtest very confusing:
First, there is a comment saying "Test that simulating a shutdown (...) we end up with a single chainstate that is at tip", followed by a check that there are still 2 chainstates - so the test contradicts itself.

Then the test calls the test-only function `LoadVerifyActiva
...

💬 mzumsande commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004288056)
a1fd5a886252ebd813ee69029dcf17de6883e047:
Why is `m_target_utxohash` a `std::optional<AssumeutxoHash>` if it's only ever used as a bool? The hash, once set , is never read anywhere as far as I can see.

nit: it should be `m_target_utxohash` instead of `m_target_blockhash` in the commit message.

👍 theuni approved a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2700353780)
Thanks for addressing my comment.

I agree, this will be a fun one to get merged. I have several local branches with fun changes on top of this one.

ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.