💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002650307)
Would be good to remove this line from d79dab0fa999002a0c5b70c1688240e2a5032ce1 in this PR.

💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2735712124)
@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

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