🤔 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

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

💬 willcl-ark commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2736590410)
At commit 20778eb0235df70397fc285f9e3b72270bd4aaf4 I get the following guix output:

```bash
env HOSTS="x86_64-apple-darwin arm64-apple-darwin" ./contrib/guix/guix-build

<snip>

$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
30aa32906f879c5347be27bd9cb1a65b3d839acd72805fe3185276f788971fdb guix-build-20778eb0235d/output/arm64-apple-darwin/SHA256SUMS.part
0763a089bcc5d1d22191414f556bf461cefa8ea8a18971b171cf6fc5570b
...

👍 vasild approved a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2698515349)
ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497

💬 ldionne commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2003386597)
The `-O` flag controls optimization level, which shouldn't have any impact on the AST representation of the code. The compiler builds an AST representation of the code that corresponds to the semantics of the program, and from that generates LLVM IR that implements those semantics. This is then passed on to the optimizer, which performs transformations on the LLVM IR representation (not the AST directly) to produce equivalent but more efficient LLVM IR.

This is much simplified, but if code co
...

👍 dergoegge approved a pull request: "fuzz: Speed up *_package_eval fuzz targets a bit"
(https://github.com/bitcoin/bitcoin/pull/31457#pullrequestreview-2698551104)
Code review ACK fac3d93c2ba84899c2c6516b5449f61ef653d9fa

💬 vasild commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2003397719)
> There are cases where you're comparing a != b and may not be sure which ones getting the wrong value where this could perhaps be helpful, but those **seem pretty rare**

I counted (by eyes, could be +/- a few):
* 16 occurrences where one of the arguments is a constant, e.g. `assert_not_equal(peer.nServices & NODE_BLOOM, 0)`, and
* 59 occurrences where both arguments are variables, e.g. `assert_not_equal(child_one_wtxid, child_two_wtxid)`