💬 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

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2002951351)
I forgot to mention one more option that is useful here: `-Xdemangler=llvm-cxxfilt`. Without that option I get:
```
Unexecuted instantiation: addrman_tests.cpp:_ZL17LogAcceptCategoryN5BCLog8LogFlagsENS_5LevelE
```
and with that option:
```
Unexecuted instantiation: addrman_tests.cpp:LogAcceptCategory(BCLog::LogFlags, BCLog::Level)
```

💬 willcl-ark commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#issuecomment-2736162894)
@maflcko thinking more on this after the merge of #31948 my opinion is that we should close this PR, the associated issue (#30028), and, if any action should be take it would be to document that linting in a worktree is possible by running the linter outside of a container? e.g.:

```bash
# install required python dependencies
bash -c '( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )'
```

This change feels like it adds a lot of complexity, for li
...

🤔 l0rinc requested changes to a pull request: "improve MallocUsage() accuracy"
(https://github.com/bitcoin/bitcoin/pull/28531#pullrequestreview-2697495377)
I think this is an important change, I'm running a full reindex to check fi the behavior changes or not.

I left a few recommendations, this patch contains all my recommendations:
```patch
diff --git a/src/memusage.h b/src/memusage.h
index 501f7068ca..5855e9d18b 100644
--- a/src/memusage.h
+++ b/src/memusage.h
@@ -24,9 +24,6 @@
namespace memusage
{

-/** Compute the total memory used by allocating alloc bytes. */
-static size_t MallocUsage(size_t alloc);
-
/** Dynamic memory u
...

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002916382)
Since `step` is guaranteed to be a power of 2, the expression `~(step - 1)` simplifies exactly to `-step`.

<details>
<summary>Reproducer</summary>

```C++
BOOST_AUTO_TEST_CASE(BitwiseStepSimplificationTest)
{
for (uint64_t step{1}; step != 0; step <<= 1) {
std::cout << step << std::endl;
BOOST_CHECK(std::has_single_bit(step));
BOOST_CHECK_EQUAL((~(step - 1)), -step);
}
}
```

</details>

so we should be able to write:
```suggestion
return
...

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002883412)
we could use the C++20 https://en.cppreference.com/w/cpp/numeric/has_single_bit here:
```suggestion
static_assert(std::has_single_bit(step));
```

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002988544)
`step` is basically the `alignment`, right?
We could probably do it in a platform-independent way (and get rid of `static_assert(step > 0);` while we're here):
```C++
constexpr size_t alignment{alignof(std::max_align_t)};
```

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002868062)
'inline' may be redundant on a global static function, but we should be able to make it `constexpr` regardless:
```suggestion
static constexpr size_t MallocUsage(size_t alloc)
...
constexpr size_t overhead{sizeof(void*)};
```

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002984240)
what's the purpose of checking positivity for a `size_t` value? Is this some weird compiler trick or a refactoring leftover?