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

💬 brunoerg commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2736531572)
Hi, @jirijakes. I've tried to reproduce it but the unit test fails when running it with this mutation.

Without the change:
```sh
➜ bitcoin-core-dev git:(master) ✗ DIR_UNIT_TEST_DATA=qa-assets/unit_test_data ./build/bin/test_bitcoin --run_test=script_tests
Running 19 test cases...

*** No errors detected
```

With the change:
```sh
➜ bitcoin-core-dev git:(master) ✗ DIR_UNIT_TEST_DATA=qa-assets/unit_test_data ./build/bin/test_bitcoin --run_test=script_tests
Running 19 test cases...
test/script_
...

👍 brunoerg approved a pull request: "test: replace assert with assert_equal and assert_greater_than"
(https://github.com/bitcoin/bitcoin/pull/32091#pullrequestreview-2698323986)
code review ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0