💬 fanquake commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160332666)
> not maintained by Bitcoin Core contributors,
It is. @willcl-ark is a regular contributor to the project.
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160332666)
> not maintained by Bitcoin Core contributors,
It is. @willcl-ark is a regular contributor to the project.
💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160342710)
> > not maintained by Bitcoin Core contributors,
>
> It is. @willcl-ark is a regular contributor to the project.
I’m not saying he’s not a trustworthy person — just that his work isn’t in the Bitcoin Core repository. I think everyone naturally prefers what’s in the official repo, since any code inside it carries more trust from the community.
But as I said, it’s nothing strictly necessary.
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160342710)
> > not maintained by Bitcoin Core contributors,
>
> It is. @willcl-ark is a regular contributor to the project.
I’m not saying he’s not a trustworthy person — just that his work isn’t in the Bitcoin Core repository. I think everyone naturally prefers what’s in the official repo, since any code inside it carries more trust from the community.
But as I said, it’s nothing strictly necessary.
💬 sipa commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160353283)
FWIW, I think there is value in having some form of docker integration *for users*, as part of the repository/release itself and not just maintained by a third party. I'm not familiar enough with docker myself to judge what approaches are appropriate, but it's a sufficiently common request that I think it would be nice if Bitcoin Core offered something like this. But, obviously, only if someone wants to step to maintain it, and make sure infrastructure is available to test it doesn't bitrot.
Fo
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160353283)
FWIW, I think there is value in having some form of docker integration *for users*, as part of the repository/release itself and not just maintained by a third party. I'm not familiar enough with docker myself to judge what approaches are appropriate, but it's a sufficiently common request that I think it would be nice if Bitcoin Core offered something like this. But, obviously, only if someone wants to step to maintain it, and make sure infrastructure is available to test it doesn't bitrot.
Fo
...
💬 willcl-ark commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3160374012)
I get an error compiling this without `bench_bitcoin`:
```
❯ cmake -B build; and cmake --build build; and ./build/test/functional/test_runner.py -j14
-- The CXX compiler identification is Clang 19.1.7
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /etc/profiles/per-user/will/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was s
...
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3160374012)
I get an error compiling this without `bench_bitcoin`:
```
❯ cmake -B build; and cmake --build build; and ./build/test/functional/test_runner.py -j14
-- The CXX compiler identification is Clang 19.1.7
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /etc/profiles/per-user/will/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was s
...
📝 maflcko opened a pull request: "build: Set AUTHOR_WARNING on warnings"
(https://github.com/bitcoin/bitcoin/pull/33144)
Now that the cmake setting `-Werror=dev` is set since commit 6a13a6106e3c1ebe95ba6430184d6260a7b942bd for the CI, guix and the dev cmake preset, it could make sense to notify developers about any warnings.
So do that with a single `AUTHOR_WARNING`.
This can be tested by introducing a bug, like:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6017775fa7..5610e03c66 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -589,7 +589,7 @@ set(Python3_FIND_FRAMEWORK LAST CACHE S
...
(https://github.com/bitcoin/bitcoin/pull/33144)
Now that the cmake setting `-Werror=dev` is set since commit 6a13a6106e3c1ebe95ba6430184d6260a7b942bd for the CI, guix and the dev cmake preset, it could make sense to notify developers about any warnings.
So do that with a single `AUTHOR_WARNING`.
This can be tested by introducing a bug, like:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6017775fa7..5610e03c66 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -589,7 +589,7 @@ set(Python3_FIND_FRAMEWORK LAST CACHE S
...
💬 maflcko commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2257379206)
Done in https://github.com/bitcoin/bitcoin/pull/33144. It should be trivial to revert that one-line patch and rebase this pull on top of it, if there is need.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2257379206)
Done in https://github.com/bitcoin/bitcoin/pull/33144. It should be trivial to revert that one-line patch and rebase this pull on top of it, if there is need.
💬 maflcko commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2257425905)
> > (Actually, while testing, a missing python is now a hard error already when `-DBUILD_GUI=ON`. So I don't think the wording "refactor" accurately represents the changes here.)
>
> To clarify, either the changes here in this pull need to be reverted/fixed up, or the docs should be updated to clarify that python is now required to build the gui.
This bug still exists. `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DBUILD_GUI=ON` now fails with:
```
CMake Error at src/qt/CMakeLists.txt:3
...
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2257425905)
> > (Actually, while testing, a missing python is now a hard error already when `-DBUILD_GUI=ON`. So I don't think the wording "refactor" accurately represents the changes here.)
>
> To clarify, either the changes here in this pull need to be reverted/fixed up, or the docs should be updated to clarify that python is now required to build the gui.
This bug still exists. `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DBUILD_GUI=ON` now fails with:
```
CMake Error at src/qt/CMakeLists.txt:3
...
💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257455023)
It should just auto-detect, if it is only used internally? Seems odd to ask the user to lookup their arch and pass a magic value?
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257455023)
It should just auto-detect, if it is only used internally? Seems odd to ask the user to lookup their arch and pass a magic value?
📝 m3dwards opened a pull request: "CI: silent merge check"
(https://github.com/bitcoin/bitcoin/pull/33145)
With the upcoming potential migration to cirrus runners (#32989) we need a way to periodically check for a silent merge conflict. Cirrus CI currently does this every week.
This is part of a proposal that will look for a label on a PR as a trigger to run the lint and get_previous_releases test. This label could be added manually or better, Drahtbot could periodically add (and remove) this label.
(https://github.com/bitcoin/bitcoin/pull/33145)
With the upcoming potential migration to cirrus runners (#32989) we need a way to periodically check for a silent merge conflict. Cirrus CI currently does this every week.
This is part of a proposal that will look for a label on a PR as a trigger to run the lint and get_previous_releases test. This label could be added manually or better, Drahtbot could periodically add (and remove) this label.
💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257480534)
> It should just auto-detect, if it is only used internally? Seems odd to ask the user to lookup their arch and pass a magic value?
Oh right, it already passes TARGETARCH automatically. I'll remove it from the docker-compose.
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257480534)
> It should just auto-detect, if it is only used internally? Seems odd to ask the user to lookup their arch and pass a magic value?
Oh right, it already passes TARGETARCH automatically. I'll remove it from the docker-compose.
💬 maflcko commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3160548342)
> bench_bitcoin ....................... OFF
thx, fixed
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3160548342)
> bench_bitcoin ....................... OFF
thx, fixed
💬 willcl-ark commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160560375)
I can sympapthezie somewhat with wanting an un-polluted system as a developer, but for that I personally think Nix is a strictly superior solution to docker for this use-case (and indeed I maintain a nix devShell flake [here](https://github.com/bitcoin-dev-tools/bix) for exactly this purpose).
We already have our CI setup containerised for exactly this type of reason too, and the use-case of a developer wanting to build-and-test inside a container is already provided by running a CI job local
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160560375)
I can sympapthezie somewhat with wanting an un-polluted system as a developer, but for that I personally think Nix is a strictly superior solution to docker for this use-case (and indeed I maintain a nix devShell flake [here](https://github.com/bitcoin-dev-tools/bix) for exactly this purpose).
We already have our CI setup containerised for exactly this type of reason too, and the use-case of a developer wanting to build-and-test inside a container is already provided by running a CI job local
...
💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2257511652)
Seems fine, but currently a vector of task names is accepted, see `--task` in https://github.com/maflcko/DrahtBot/blob/2038d4d541b89bf2601614b5656d7d30d73e17be/rerun_ci/src/main.rs#L7-L23
It would be good to allow the same somehow?
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2257511652)
Seems fine, but currently a vector of task names is accepted, see `--task` in https://github.com/maflcko/DrahtBot/blob/2038d4d541b89bf2601614b5656d7d30d73e17be/rerun_ci/src/main.rs#L7-L23
It would be good to allow the same somehow?
🤔 janb84 reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094#pullrequestreview-3093134785)
re ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb
changes since last ACK:
- clang tidy related changes
<details>
New result of clang-tidy :
``` shell
$ ( cd ./src/ && run-clang-tidy -p ../build -j $(nproc) ) | grep walletframe.cpp
<< no warnings>>
```
</details>
(https://github.com/bitcoin/bitcoin/pull/33094#pullrequestreview-3093134785)
re ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb
changes since last ACK:
- clang tidy related changes
<details>
New result of clang-tidy :
``` shell
$ ( cd ./src/ && run-clang-tidy -p ../build -j $(nproc) ) | grep walletframe.cpp
<< no warnings>>
```
</details>
💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#discussion_r2257551159)
Thanks for taking a look!
RE:
> The reason I suggested adding a Snapshot class above was that I thought GUI wanted to display SnapshotMetadata information before activating the snapshot, and a Snapshot class could provide separate getMetadata()
I took some time to test various scenarios downstream in the `gui-qml` project. While we aren't directly utilizing the snapshot data at the moment, I believe your initial instincts about introducing a `Snapshot` class were well-founded. This abstra
...
(https://github.com/bitcoin/bitcoin/pull/33117#discussion_r2257551159)
Thanks for taking a look!
RE:
> The reason I suggested adding a Snapshot class above was that I thought GUI wanted to display SnapshotMetadata information before activating the snapshot, and a Snapshot class could provide separate getMetadata()
I took some time to test various scenarios downstream in the `gui-qml` project. While we aren't directly utilizing the snapshot data at the moment, I believe your initial instincts about introducing a `Snapshot` class were well-founded. This abstra
...
💬 Sjors commented on issue "Awesome multisig PR labyrinth guide":
(https://github.com/bitcoin/bitcoin/issues/24861#issuecomment-3160626450)
I recently made quite a lot of progress in getting MuSig2 setups to work, mainly tested against Ledger which has [BIP388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) support.
You can try for yourself with those branches for Bitcoin Core and HWI:
- https://github.com/Sjors/bitcoin/pull/91
- https://github.com/bitcoin-core/HWI/pull/794
(https://github.com/bitcoin/bitcoin/issues/24861#issuecomment-3160626450)
I recently made quite a lot of progress in getting MuSig2 setups to work, mainly tested against Ledger which has [BIP388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) support.
You can try for yourself with those branches for Bitcoin Core and HWI:
- https://github.com/Sjors/bitcoin/pull/91
- https://github.com/bitcoin-core/HWI/pull/794
⚠️ fanquake opened an issue: "doc: GUI Python dependency is not documented"
(https://github.com/bitcoin/bitcoin/issues/33146)
See this thread: https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2182425599
> (Actually, while testing, a missing python is now a hard error already when -DBUILD_GUI=ON. So I don't think the wording "refactor" accurately represents the changes here.)
> This bug still exists. rm -rf ./bld-cmake && cmake -B ./bld-cmake -DBUILD_GUI=ON now fails with:
> So it would be good to revert this pull, or adjust the docs:
We need to decide whether to document the GUI dep, or re-adjusting the co
...
(https://github.com/bitcoin/bitcoin/issues/33146)
See this thread: https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2182425599
> (Actually, while testing, a missing python is now a hard error already when -DBUILD_GUI=ON. So I don't think the wording "refactor" accurately represents the changes here.)
> This bug still exists. rm -rf ./bld-cmake && cmake -B ./bld-cmake -DBUILD_GUI=ON now fails with:
> So it would be good to revert this pull, or adjust the docs:
We need to decide whether to document the GUI dep, or re-adjusting the co
...
💬 fanquake commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2257570928)
Opened #33146
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2257570928)
Opened #33146
⚠️ fanquake opened an issue: "build: `test_bitcoin` link failure with `-flto=thin` & address sanitizer"
(https://github.com/bitcoin/bitcoin/issues/33147)
```bash
clang --version
clang version 20.1.8 (Fedora 20.1.8-3.fc43)
Target: aarch64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Configuration file: /etc/clang/aarch64-redhat-linux-gnu-clang.cfg
```
```bash
export CC=clang
export CXX=clang++
# CMAKE_INTERPROCEDURAL_OPTIMIZATION is -flto=thin for clang
cmake -B build -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON -DSANITIZERS=address
<snip>
[ 54%] Built target bitcoin-util
[ 55%] Built target bitcoin-wallet
[ 72%] Built target bitcoin_nod
...
(https://github.com/bitcoin/bitcoin/issues/33147)
```bash
clang --version
clang version 20.1.8 (Fedora 20.1.8-3.fc43)
Target: aarch64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Configuration file: /etc/clang/aarch64-redhat-linux-gnu-clang.cfg
```
```bash
export CC=clang
export CXX=clang++
# CMAKE_INTERPROCEDURAL_OPTIMIZATION is -flto=thin for clang
cmake -B build -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON -DSANITIZERS=address
<snip>
[ 54%] Built target bitcoin-util
[ 55%] Built target bitcoin-wallet
[ 72%] Built target bitcoin_nod
...
💬 fanquake commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3160730521)
TSAN failure, same as the one above (https://github.com/bitcoin/bitcoin/actions/runs/16781531680/job/47521224560?pr=32989#step:8:5924):
```bash
2025-08-06T15:41:21.507392Z [test] [net.cpp:3192] [void==================
WARNING: ThreadSanitizer: data race (pid=10235)
Write of size 8 at 0x7234000016f0 by main thread:
#0 blockmanager_tests::blockmanager_readblock_hash_mismatch::test_method() /home/admin/actions-runner/_work/_temp/build/src/test/./test/blockmanager_tests.cpp:143:28 (test_
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3160730521)
TSAN failure, same as the one above (https://github.com/bitcoin/bitcoin/actions/runs/16781531680/job/47521224560?pr=32989#step:8:5924):
```bash
2025-08-06T15:41:21.507392Z [test] [net.cpp:3192] [void==================
WARNING: ThreadSanitizer: data race (pid=10235)
Write of size 8 at 0x7234000016f0 by main thread:
#0 blockmanager_tests::blockmanager_readblock_hash_mismatch::test_method() /home/admin/actions-runner/_work/_temp/build/src/test/./test/blockmanager_tests.cpp:143:28 (test_
...