👍 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
(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
...
(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
(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
...
(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
(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)`
(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
(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
(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.
(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.
(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
...
(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?
(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.
(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.
(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.
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003574299)
Using `ctest` assumes the presence of a build tree, which is not the case in this setup.
I'm unsure about the wording of the comment you're requesting.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003574299)
Using `ctest` assumes the presence of a build tree, which is not the case in this setup.
I'm unsure about the wording of the comment you're requesting.
💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2736992858)
just a no-op rebase
re-review-ACK d190f0facc8379da7610d7161e532d57c6a7eb96 🍓
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted commen
...
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2736992858)
just a no-op rebase
re-review-ACK d190f0facc8379da7610d7161e532d57c6a7eb96 🍓
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted commen
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003585744)
> I see in this job and the `windows-native-test` you have swapped to using powershell rather than cmd.
Not `powershell`, but rather `bash`:https://github.com/bitcoin/bitcoin/blob/04ddddec1ed6cd8af060145ba1e85ada16560ef4/.github/workflows/ci.yml#L24-L28
Additionally, you can check the CI [log](https://github.com/bitcoin/bitcoin/actions/runs/13921287455/job/39030781425?pr=31176#step:12:3):
```
shell: C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}
```
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003585744)
> I see in this job and the `windows-native-test` you have swapped to using powershell rather than cmd.
Not `powershell`, but rather `bash`:https://github.com/bitcoin/bitcoin/blob/04ddddec1ed6cd8af060145ba1e85ada16560ef4/.github/workflows/ci.yml#L24-L28
Additionally, you can check the CI [log](https://github.com/bitcoin/bitcoin/actions/runs/13921287455/job/39030781425?pr=31176#step:12:3):
```
shell: C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}
```
💬 hodlinator commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003595223)
Makes sense, certainly not blocking.
```cmake
# Can't use ctest here like other jobs as we don't have a CMake build tree.
```
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003595223)
Makes sense, certainly not blocking.
```cmake
# Can't use ctest here like other jobs as we don't have a CMake build tree.
```
💬 laanwj commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2003604471)
True, though i suppose the same guidelines for modifying RPC interfaces can be used to review the changes by seeing if the changes follow the guidelines, without mentioning review explicitly.
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2003604471)
True, though i suppose the same guidelines for modifying RPC interfaces can be used to review the changes by seeing if the changes follow the guidelines, without mentioning review explicitly.
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003608968)
Thanks! Taken.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2003608968)
Thanks! Taken.