🤔 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

🤔 l0rinc reviewed a pull request: "improve MallocUsage() accuracy"
(https://github.com/bitcoin/bitcoin/pull/28531#pullrequestreview-2698031897)
Some of the usages seem to contain some magical values, tailored to the previous behavior:
* https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.cpp#L1066
* https://github.com/bitcoin/bitcoin/blob/master/src/test/disconnected_transactions.cpp#L39-L43

Do any of them need adjustments now?

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2003105459)
I see you've put the test before the fix after the recent rebase - but it never enters this condition before the fix, so basically dead code is added.
```C++
if node.getmempoolinfo()['usage'] >= 4_990_000:
raise ""
```

Could we either make sure we enter, and update it in the next commit accordingly, or add it after the fix commit?
Also, the commit message claims that "fill mempool leaves room" - I think this needs some explanation.