📝 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_
...
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160782020)
Can we merge this, the builds seem fine
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160782020)
Can we merge this, the builds seem fine
💬 dergoegge commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160793658)
This has 3 nacks with no reasonable rebuttals, ready for close?
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160793658)
This has 3 nacks with no reasonable rebuttals, ready for close?
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257729014)
Thanks, implemented. I was trying to avoid adding a constant to `src/logging.h`.
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257729014)
Thanks, implemented. I was trying to avoid adding a constant to `src/logging.h`.
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257729365)
Added
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257729365)
Added
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160849089)
> Concept NACK
>
> What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo.
This makes conducting a fork easier, and rebasing the fork/absorbing cores changes if required in a fork easier for a maintainer.
The code is also a lot cleaner, given we check for bitcoin-utils in a better way,
and don't have an unnecessary Bitcoin-Core based text in the build.sh
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160849089)
> Concept NACK
>
> What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo.
This makes conducting a fork easier, and rebasing the fork/absorbing cores changes if required in a fork easier for a maintainer.
The code is also a lot cleaner, given we check for bitcoin-utils in a better way,
and don't have an unnecessary Bitcoin-Core based text in the build.sh
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160852920)
> Concept NACK
>
> Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed?
This is addressed, and a fair callout.
Now the downstream patch for a forked client is not a two line downstream patch
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160852920)
> Concept NACK
>
> Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed?
This is addressed, and a fair callout.
Now the downstream patch for a forked client is not a two line downstream patch
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160856511)
> This has 3 nacks with no reasonable rebuttals, ready for close?
Just addressed them.
Feel like this is objectively more reusable code
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160856511)
> This has 3 nacks with no reasonable rebuttals, ready for close?
Just addressed them.
Feel like this is objectively more reusable code
💬 KurtisStirling commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160870648)
> This has 3 nacks with no reasonable rebuttals, ready for close?
Rebuttal: claiming to be open source, but not acting in the spirit of open source, is bad for Core's already tarnished reputation.
Sure, no one has to do anything, but ngaf about people outside of your immediate circle is no way to win kudos.
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160870648)
> This has 3 nacks with no reasonable rebuttals, ready for close?
Rebuttal: claiming to be open source, but not acting in the spirit of open source, is bad for Core's already tarnished reputation.
Sure, no one has to do anything, but ngaf about people outside of your immediate circle is no way to win kudos.