💬 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)`

👍 dergoegge approved a pull request: "leveldb: pull upstream C++23 changes"
(https://github.com/bitcoin/bitcoin/pull/31766#pullrequestreview-2698577016)
utACK c8fab356171a0e283d5716647e3243c04810ac51

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2003409216)
Not so concerned about people running in optimised node since 5+ functional tests fail in that way, but changed in latest push to:
https://github.com/bitcoin/bitcoin/blob/cac218dcbdd2b5682a7ac8659aac0306e68a65f5/test/functional/test_framework/test_node.py#L325-L331

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2736778341)
> HI @furszy can you please give some clarity on this comment [#32023 (comment)](https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2731126667)

It is a valid concern. Bubbling up the error seems to be the best option to not couple components.
Could go back to implementing https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1992789597 or a bare error string return approach.

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2003423317)
@ldionne, that is very useful. Thanks for the clarification! With my limited knowledge I was thinking that maybe AST is generated after optimization.

Just to clarify on this particular issue - I am fine with whatever `CMAKE_BUILD_TYPE=` or omitting it and leaving the default. This is only a documentation after all. People can tweak the commands as they wish before executing them.

💬 l0rinc commented on pull request "leveldb: pull upstream C++23 changes":
(https://github.com/bitcoin/bitcoin/pull/31766#issuecomment-2736826828)
I have first compared every modified file against the whole latest versions of https://github.com/google/leveldb/blob/main/util/env_posix.cc, https://github.com/google/leveldb/blob/main/util/env_windows.cc and https://github.com/google/leveldb/blob/main/util/no_destructor.h, but realized we're too far behind.

<details>
<summary>Full diff</summary>

```patch
diff --git a/src/leveldb/util/env_posix.cc b/src/leveldb/util/env_posix.cc
index 86571059ba..25399f9745 100644
--- a/src/leveldb/ut
...

💬 m3dwards commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003523628)
I see in this job and the `windows-native-test` you have swapped to using powershell rather than cmd. We previously had a problem when doing that that `TEST_RUNNER_EXTRA=''` was causing an error of `WARNING! Test '' not found in full test list.` when running the function tests. See: https://github.com/bitcoin/bitcoin/issues/29534

Why does this work now?

💬 hodlinator commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003510077)
Thread: https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2000738410:

Nice! Hopefully that satisfies others in the thread.

Taking one step back, would it make sense to add a comment about why we call the test executables directly here instead of going through ctest like other jobs? I'm guessing we don't want to depend on ctest as we just download executables via artifacts.

💬 maflcko commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2003563894)
> 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.

I can't reproduce this locally. For me, sync.cpp is missing from both reports and -O0 and -O2 have no effect on it missing. So this could be a bug, but I don't think optimization has anything to do with that.