💬 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?

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2003031317)
Adding the current implementation to the reproducer of @martinus shows this is a behavior change compared to his version - https://godbolt.org/z/cPWz6YnWd
```bash
0: actual=16, estimate=0
```

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002955096)
I'm not exactly sure how we got to the `min_alloc` of `9` on these platforms, but we can probably simplify the platform dependent code to:
```C++
#if defined(__arm__) || SIZE_MAX == UINT64_MAX
constexpr size_t min_alloc{9};
#else
constexpr size_t min_alloc{0};
#endif
```

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2003067985)
can we add some references to avoid having to rely on beliefs?

💬 willcl-ark commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2003085347)
Unrelated to the changes in this PR, but as I was on a fresh `bookworm` docker image (sha256:18023f131f52fc3ea21973cabffe0b216c60b417fd2478e94d9d59981ebba6af), it appears that depends build fails to build bdb on Debian when following current instructions:

```bash
# docker run -it debian:bookworm /bin/bash

apt update
apt install -y git
git clone --depth=1 https://github.com/bitcoin/bitcoin
cd bitcoin/depends
apt install -y cmake curl make patch
make -j10 NO_QT=1

# build logs
Fetch
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2003102265)
I don't see how from "source-based ... operates on AST and preprocessor information" follows that `-O` don't matter.

I did coverage reports locally with `Debug` (`-O0`) and `Release` (`-O2`) and compared them. They are slightly different, which could be due to non-determinism in the tests. `src/sync.cpp` is completely missing from the `-O2` report.

Time to complete the tests:

test | Debug | Release
--- | --- | ---
unit | 60s | 31s
functional | 186s | 76s

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2736277550)
My Guix build:
```
aarch64
7638ac47fcd9ea8a4e2bbb265876aaeba0d8fe774f5bcc884753331d6ffe3429 guix-build-94967c353ed8/output/aarch64-linux-gnu/SHA256SUMS.part
3d7ff7c2c1ad1608946452645d9047ceef0e77f589098c94b5d628476ee5192d guix-build-94967c353ed8/output/aarch64-linux-gnu/bitcoin-94967c353ed8-aarch64-linux-gnu-debug.tar.gz
b4bcb7f99c2a55a741cdbab4947fe43570d9ee481624171e26637deba2e9d464 guix-build-94967c353ed8/output/aarch64-linux-gnu/bitcoin-94967c353ed8-aarch64-linux-gnu.tar.gz
0a20e1b4
...