💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002746377)
After https://github.com/bitcoin/bitcoin/pull/30718 this should likely be:
```suggestion
utxo_to_spend=confirmed_utxos.pop(0), target_vsize=32500)
```

💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2735823728)
Rebased after #32088 and #32057, which both simplify this PR.

📝 Sjors opened a pull request: "Move some tests and documentation from testnet3 to testnet4"
(https://github.com/bitcoin/bitcoin/pull/32096)
In preparation for dropping testnet3 entirely in #31974 this PR migrates a few things to testnet4:

* the ZMQ examples
* developer docs
* various unit tests

It drops `testnet3` from `MAGIC_BYTES` in the test framework, since no test uses it.

💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2735840269)
I'm unsure if I got the `argsman_tests` changes right.

🤔 hodlinator reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2697452534)
Code review 6bc347dfd55dd88ed216aae437a716e4cc4d4dde

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002785421)
nit: Seems like this list used to be alphabetically ordered.

Similar in ubsan:
`implicit-integer-sign-change:*/qarraydata.h` should probably be after `implicit-integer-sign-change:*/new_allocator.h`.

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002758330)
Should minimum libgcc version be documented in dependencies.md?

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002835161)
Thanks for expanding the description!

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002766420)
nit: More correct?
```suggestion
The current precise version for Qt is specified in [qt_details.mk](/depends/packages/qt_details.mk).
```

💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2002846230)
a0c134c4525e6ab6ca937b744ced6d963c389c96: I don't see any harm in just using mainnet for ZMQ, so maybe that's better?

👍 vasild approved a pull request: "ci: build multiprocess on most jobs"
(https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2697597345)
ACK f50f6e33b9ab84409bbe838744ecf346bdda8909

💬 vasild commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2002847784)
nit: the other files use 1-space indentation. This should work without the `\` as well.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002855130)
Historically, it has been documented here:https://github.com/bitcoin/bitcoin/blob/e568c1dd134e0318c46113cb7dfc23b40e2829e8/contrib/devtools/symbol-check.py#L35

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2002865850)
I did it accidentally in `00_setup_env_native_valgrind.sh`, which I've now adjusted locally to be consistent.

Which other files use 1-space indentation? Dropping `\` does seem nice.

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2002875046)
I think `StartStaging()` gets a bit simpler if you know that it can only be called with `m_clustersets.size()==1`:

```c++
void TxGraphImpl::StartStaging() noexcept
{
+ Assume(m_clustersets.size() == 1); // vs < MAX_LEVELS
+ SplitAll(0); // vs m_clustersets.size() - 1
+ ApplyDependencies(0); // ditto
m_clustersets.emplace_back();
+ auto& stage = m_clustersets[1]; // vs m_clustersets.back()
+ auto& main = m_clustersets[0]; // vs *(m_clustersets.rbegin() + 1);

...

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002879568)
That's the right place imo. It's deliberately a low version to make sure the distributed binaries run on older distributions. But it's not something normal users building the software need worry about.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2735911498)
@hodlinator

Thank you for your review!

Your feedback has been addressed.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002881464)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2735911498).

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002882550)
Thanks! [Taken](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2735911498).

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002888510)
The system has both X and Wayland, but only tested on Wayland.

If we don't support Wayland, then I suppose it's sufficient if a stock desktop Ubuntu running X can start Bitcoin Core without requiring the user to install something.

But if Bitcoin Core worked with Wayland out of the box before and after this PR doesn't, it will confuse users. Especially since Wayland has been the default for a few years now on Ubuntu, though they still make switching back to X easy.

I made a note to test
...