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

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002903318)
`bitcoin-qt`, when dynamically linked to the system's Qt libraries, should support Wayland.

For Wayland support with depends, please see https://github.com/bitcoin/bitcoin/pull/22708.

💬 vasild commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2002908678)
```
ci/test/00_setup_env_native_asan.sh
ci/test/00_setup_env_native_fuzz.sh
ci/test/00_setup_env_native_fuzz_with_valgrind.sh
ci/test/00_setup_env_native_tidy.sh
ci/test/00_setup_env_native_valgrind.sh
```

💬 jsarenik commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2735999894)
> @jsarenik @carnhofdaki the behaviour default signet is not changed. I have updated the PR description to include this in case it was unclear.
>
> This patch only affects custom signets with `signetchallenge` and the goal was to run those on custom directories `signet_XXXXXXXX` so as not to interfere with the default signet which runs on `signet` directory if you run multiple signets

Thank you for this clarification.

utACK abe8639

💬 carnhofdaki commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2736007460)
utACK abe8539