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

👍 willcl-ark approved a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2698056788)
reACK 3dd8223d44928a7e7c2cdc451655a8ef7d1ef5be

I've only tested the revised macOS instructions on a macOS system that already had other packages installed on it (not fresh), as I don't have a fresh machine to test on currently.

💬 ryanofsky commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2736298832)
@hebasto since you asked me to create this PR and separate it from https://github.com/bitcoin/bitcoin/pull/31741, do you have any particular concerns or thoughts here?

I think this change is an improvement by itself, not just a fix for IPC issues because it adds a clear error message if `-DSANITIZERS=fuzzer` is specified without `-DBUILD_FOR_FUZZING=ON` instead of causing confusing link errors. I also think it is a conceptual improvement to only add flags appropriate for building libraries to
...

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2003138838)
I'm seeing trailing `\` everywhere on master:

https://github.com/bitcoin/bitcoin/blob/e568c1dd134e0318c46113cb7dfc23b40e2829e8/ci/test/00_setup_env_native_asan.sh#L26-L35

💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2003145552)
> Unrelated to the changes in this PR

Indeed I'd like to avoid changing existing instructions here.

💬 vasild commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2003163651)
Yes, those are two independent things - one-space indentation and the trailing backslash.

`00_setup_env_native_tsan.sh` seems to be an exception that is does not use indentation.

The trailing backslash seems to be used everywhere.

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2003099949)
`__arm__` only applies to [32 bits](https://stackoverflow.com/a/41666292), no need for the comments (I'd prefer having them in the commit messages). Before merging we have to make sure we're not just testing ARM on 32 bits