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

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2003144710)
If it's simpler we might as well use the size of `size_t` here: https://github.com/bitcoin/bitcoin/blob/master/src/compat/assumptions.h#L37

👍 vasild approved a pull request: "test, refactor: Add TestNode.binaries to hold binary paths"
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2698161589)
ACK 8f5228d9239464ece06f3a56372aeec29685801c

The two commits in this PR are part of https://github.com/bitcoin/bitcoin/pull/31375 which I already ACKed, thanks @maflcko for pointing this.

Compared with:

`git range-diff 095801b8999851b10e43567389cd293fd7957497~..45d439dab13153a3b570957a9eab63e3e6274611 e60ff180c29dde6015f21132e9bc3846a608edf6~..8f5228d9239464ece06f3a56372aeec29685801c`

💬 vasild commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r2003174116)
I guess this PR needs a rebase because the context of this change reads "src" whereas that code is now "bin" in `master`.

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2736410023)
> * However, it's also the case that if the compiler can prove a statement is side-effect free, it can optimize the call away in non-debug builds.

That makes sense. Might be a good idea to update doc/developer-notes.md pointing out that use case? It currently says:

> * `Assume` should be used to document assumptions when program execution can
> safely continue even if the assumption is violated.

👍 vasild approved a pull request: "net: Block v2->v1 transport downgrade if !fNetworkActive"
(https://github.com/bitcoin/bitcoin/pull/32073#pullrequestreview-2698186277)
ACK 6869fb417096b43ba7f74bf767ca3e41b9894899

💬 vasild commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2736442775)
> SOME kind of sockman is needed to replace libevent ... it would be "nice" to only have to maintain one I/O loop structure in bitcoind.

:100:

💬 m3dwards commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2003213136)
This is only a NIT so feel free to leave it as it is but in my mind an assert is used to signal an expectation of predicted state. In Python asserts are not even executed if run in optimised mode. It seems like we are using it more like an exception here?

```python
if error_num is None:
if isinstance(e, TimeoutError):
error_num = errno.ETIMEDOUT
else:
raise Exception("errno is required
...

💬 ryanofsky commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2736469179)
I had an idea I wanted to suggest here. What if instead of adding C bindings to the bitcoin/bitcoin git repository we took inspiration from @darosior's thoughts about [project scope](https://delvingbitcoin.org/t/antoine-poinsot-on-bitcoin-cores-priorities/1470) and developed the C, rust, and python bindings in a separate bitcoin-core/bindings repository, or even separate bitcoin-core/bindings-{c,rust,python} repositories?

Technically I think there are two ways we could implement this:

1. A
...

🤔 ryanofsky reviewed a pull request: "test, refactor: Add TestNode.binaries to hold binary paths"
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2698253728)
Thanks for the reviews!

Rebased 8f5228d9239464ece06f3a56372aeec29685801c -> d190f0facc8379da7610d7161e532d57c6a7eb96 ([`pr/wrapp.3`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.3) -> [`pr/wrapp.4`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrapp.3-rebase..pr/wrapp.4)) with no changes as suggested https://github.com/bitcoin/bitcoin/pull/31866#discussion_r2003174116

💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r2003224954)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r2003174116

> I guess this PR needs a rebase because the context of this change reads "src" whereas that code is now "bin" in `master`.

Thanks it looks like this should still work and merge without any conflicts but I will rebase to make this more clear.