👍 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.
👋 mzumsande's pull request is ready for review: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405)
(https://github.com/bitcoin/bitcoin/pull/31405)
💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2737130397)
HI @mabu44 I've reverted the changes kindly confirm that. Can you please ACK on this.
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2737130397)
HI @mabu44 I've reverted the changes kindly confirm that. Can you please ACK on this.
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2737192303)
> HI @mabu44 I've reverted the changes kindly confirm that. Can you please ACK on this.
You are still changing the RPC behavior. `importdescriptors` returns errors within the response object, which is why your current changes are forcing you to modify the functional test code. When I said "bare error string return approach", I basically meant making the underlying wallet methods return a string in case of an error, so it can be wrapped into the response object at the RPC level — similar to th
...
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2737192303)
> HI @mabu44 I've reverted the changes kindly confirm that. Can you please ACK on this.
You are still changing the RPC behavior. `importdescriptors` returns errors within the response object, which is why your current changes are forcing you to modify the functional test code. When I said "bare error string return approach", I basically meant making the underlying wallet methods return a string in case of an error, so it can be wrapped into the response object at the RPC level — similar to th
...
💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2003764326)
I'm just looking at what users experience when they download the release, not self compilation.
I created an Ubuntu 24.10 live CD, booted from it and switched to X.
I extract the v29rc2 archive and try to run bitcoin-qt from finder, but nothing happens. Trying to start it from the command line reveals that it's missing `libxcb-xinerama.so`. Same for older versions e.g. v27
Same happens with Ubuntu 20.04, at least as far back as v29.2
People have been installing dependencies manually
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2003764326)
I'm just looking at what users experience when they download the release, not self compilation.
I created an Ubuntu 24.10 live CD, booted from it and switched to X.
I extract the v29rc2 archive and try to run bitcoin-qt from finder, but nothing happens. Trying to start it from the command line reveals that it's missing `libxcb-xinerama.so`. Same for older versions e.g. v27
Same happens with Ubuntu 20.04, at least as far back as v29.2
People have been installing dependencies manually
...
⚠️ Sjors opened an issue: "Linux download needs installation instructions"
(https://github.com/bitcoin/bitcoin/issues/32097)
Windows has an installer, macOS has the usual drag-me-to-finder logic that most GUI users will be familiar with. But for Linux we just give people are bunch of binaries and a README without any installation instructions.
Worse, it turns out the binaries don't work out of the box on a vanilla Ubuntu system. That's not just because of Wayland, which we don't support yet. Even on X it will miss certain libraries.
https://www.reddit.com/r/BitcoinBeginners/comments/14m8f9q/missing_required_library_
...
(https://github.com/bitcoin/bitcoin/issues/32097)
Windows has an installer, macOS has the usual drag-me-to-finder logic that most GUI users will be familiar with. But for Linux we just give people are bunch of binaries and a README without any installation instructions.
Worse, it turns out the binaries don't work out of the box on a vanilla Ubuntu system. That's not just because of Wayland, which we don't support yet. Even on X it will miss certain libraries.
https://www.reddit.com/r/BitcoinBeginners/comments/14m8f9q/missing_required_library_
...
💬 maflcko commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#issuecomment-2737320204)
For the lint task, the docker wrapper is nice, because it takes care of installing all the exact versions: https://github.com/bitcoin/bitcoin/blob/e568c1dd134e0318c46113cb7dfc23b40e2829e8/ci/lint/04_install.sh#L49-L65
However, given the limited feedback, I wonder if anyone cares about running the linters in a worktree.
For the "real" CI tasks, at least on person complained on IRC IIRC, so at least there seems to be some demand.
(https://github.com/bitcoin/bitcoin/pull/31787#issuecomment-2737320204)
For the lint task, the docker wrapper is nice, because it takes care of installing all the exact versions: https://github.com/bitcoin/bitcoin/blob/e568c1dd134e0318c46113cb7dfc23b40e2829e8/ci/lint/04_install.sh#L49-L65
However, given the limited feedback, I wonder if anyone cares about running the linters in a worktree.
For the "real" CI tasks, at least on person complained on IRC IIRC, so at least there seems to be some demand.