💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950441666)
```suggestion
tar xf bitcoin-${VERSION}-win64-codesigning.tar.gz
```
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950441666)
```suggestion
tar xf bitcoin-${VERSION}-win64-codesigning.tar.gz
```
💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2650167452)
The combination did the trick! It's not easy being green.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2650167452)
The combination did the trick! It's not easy being green.
✅ fanquake closed an issue: "ci: intermittent p2p_tx_download.py timeout (test_preferred_tiebreaker_inv)"
(https://github.com/bitcoin/bitcoin/issues/31833)
(https://github.com/bitcoin/bitcoin/issues/31833)
🚀 fanquake merged a pull request: "test: add missing sync to p2p_tx_download.py"
(https://github.com/bitcoin/bitcoin/pull/31837)
(https://github.com/bitcoin/bitcoin/pull/31837)
💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950471063)
```suggestion
tar xf bitcoin-${VERSION}-${ARCH}-apple-darwin-codesigning.tar.gz
```
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950471063)
```suggestion
tar xf bitcoin-${VERSION}-${ARCH}-apple-darwin-codesigning.tar.gz
```
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2650217902)
It would be nice to "Build bitcoin-mine executable." to the summary and add a similar guard as in #31834.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2650217902)
It would be nice to "Build bitcoin-mine executable." to the summary and add a similar guard as in #31834.
💬 maflcko commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650253088)
> I want to check if this helps [#30852 (comment)](https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430) when used with #31545 by not having to keep the base images around anymore.
@0xB10C Are you waiting for me to rebase this once more? I've held back, because there is already one review, but I am happy to rebase.
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650253088)
> I want to check if this helps [#30852 (comment)](https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430) when used with #31545 by not having to keep the base images around anymore.
@0xB10C Are you waiting for me to rebase this once more? I've held back, because there is already one review, but I am happy to rebase.
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2650258443)
As for the raw performance, the runners were upgraded, and the massive scheduling delays of last year (up to ~5hours (?) in peak times, when 10 pull requests or commits were pushed at the same time, resulting in about 100 CI tasks at the same time), it should be a bit smoother this year. I don't have full metrics, but https://cirrus-ci.com/github/bitcoin/bitcoin/master shows "Duration Chart" as a proxy, with the end-to-end maximum duration. Currently, it shows a range of 44 minutes up to 2h:44mi
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2650258443)
As for the raw performance, the runners were upgraded, and the massive scheduling delays of last year (up to ~5hours (?) in peak times, when 10 pull requests or commits were pushed at the same time, resulting in about 100 CI tasks at the same time), it should be a bit smoother this year. I don't have full metrics, but https://cirrus-ci.com/github/bitcoin/bitcoin/master shows "Duration Chart" as a proxy, with the end-to-end maximum duration. Currently, it shows a range of 44 minutes up to 2h:44mi
...
💬 fanquake commented on pull request "guix: remove test-security/symbol-check scripts":
(https://github.com/bitcoin/bitcoin/pull/31818#issuecomment-2650289845)
Guix Build:
```bash
4756d1c55efefe2dd3429d93060555617bb00709772a3c13638aab8c8dd9af2c guix-build-76c090145e9b/output/aarch64-linux-gnu/SHA256SUMS.part
f9fe3c2cb751060366eae0070a1c91897ac49c74bfc53baafca65cb17413ce7e guix-build-76c090145e9b/output/aarch64-linux-gnu/bitcoin-76c090145e9b-aarch64-linux-gnu-debug.tar.gz
6532576a94838e11f8aa98b037f116af32b71eb6f446f67ffc451207b442fd36 guix-build-76c090145e9b/output/aarch64-linux-gnu/bitcoin-76c090145e9b-aarch64-linux-gnu.tar.gz
7ed8370d29955a04
...
(https://github.com/bitcoin/bitcoin/pull/31818#issuecomment-2650289845)
Guix Build:
```bash
4756d1c55efefe2dd3429d93060555617bb00709772a3c13638aab8c8dd9af2c guix-build-76c090145e9b/output/aarch64-linux-gnu/SHA256SUMS.part
f9fe3c2cb751060366eae0070a1c91897ac49c74bfc53baafca65cb17413ce7e guix-build-76c090145e9b/output/aarch64-linux-gnu/bitcoin-76c090145e9b-aarch64-linux-gnu-debug.tar.gz
6532576a94838e11f8aa98b037f116af32b71eb6f446f67ffc451207b442fd36 guix-build-76c090145e9b/output/aarch64-linux-gnu/bitcoin-76c090145e9b-aarch64-linux-gnu.tar.gz
7ed8370d29955a04
...
💬 laanwj commented on pull request "depends: Make default `host` and `build` comparable":
(https://github.com/bitcoin/bitcoin/pull/30584#issuecomment-2650297413)
Tested ACK b28917be363fb5a82effffeadbe4ba27bb1c70ce
- Successfully tested depends build with `gmake NO_QT=1` on OpenBSD 7.6
- Differences in cmakelists.txt show it's no longer doing a cross-build by default (eg it does `try_run()`):
<details>
<summary>libevent</summary>
```diff
--- cmake-libevent-base.txt Tue Feb 11 10:23:06 2025
+++ cmake-libevent-patched.txt Tue Feb 11 10:33:37 2025
@@ -1,5 +1,5 @@
# This is the CMakeCache file.
-# For build in directory: /home/user/bitcoin/depe
...
(https://github.com/bitcoin/bitcoin/pull/30584#issuecomment-2650297413)
Tested ACK b28917be363fb5a82effffeadbe4ba27bb1c70ce
- Successfully tested depends build with `gmake NO_QT=1` on OpenBSD 7.6
- Differences in cmakelists.txt show it's no longer doing a cross-build by default (eg it does `try_run()`):
<details>
<summary>libevent</summary>
```diff
--- cmake-libevent-base.txt Tue Feb 11 10:23:06 2025
+++ cmake-libevent-patched.txt Tue Feb 11 10:33:37 2025
@@ -1,5 +1,5 @@
# This is the CMakeCache file.
-# For build in directory: /home/user/bitcoin/depe
...
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2650310815)
`752fbab26a...53d2c8e0e2`: rebase due to conflicts
This PR is now smaller because part of it was merged via https://github.com/bitcoin/bitcoin/pull/30205, thanks!
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2650310815)
`752fbab26a...53d2c8e0e2`: rebase due to conflicts
This PR is now smaller because part of it was merged via https://github.com/bitcoin/bitcoin/pull/30205, thanks!
💬 laanwj commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950557551)
Yes that seems something to consider when and if we cross that bridge. Currently an `.ots` is already generated for the final SHA256SUMS at release time, not sure to move that to another phase in the process.
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950557551)
Yes that seems something to consider when and if we cross that bridge. Currently an `.ots` is already generated for the final SHA256SUMS at release time, not sure to move that to another phase in the process.
🤔 hebasto reviewed a pull request: "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries"
(https://github.com/bitcoin/bitcoin/pull/31407#pullrequestreview-2608297032)
Concept ACK 46e44a35b85830a60cf622e039db19ccf1989008.
I have reviewed the code and it looks OK. However, I am not entirely confident in 8400ada306063f1412ef3ace57e255783db879ef due to my lack of familiarity with the signapple tool.
Additionally, I did not review the changes to the signapple tool itself.
(https://github.com/bitcoin/bitcoin/pull/31407#pullrequestreview-2608297032)
Concept ACK 46e44a35b85830a60cf622e039db19ccf1989008.
I have reviewed the code and it looks OK. However, I am not entirely confident in 8400ada306063f1412ef3ace57e255783db879ef due to my lack of familiarity with the signapple tool.
Additionally, I did not review the changes to the signapple tool itself.
🤔 Prabhat1308 reviewed a pull request: "fuzz: coinselection: cover `SetBumpFeeDiscount`"
(https://github.com/bitcoin/bitcoin/pull/31806#pullrequestreview-2608300048)
ACK [0289f03](https://github.com/bitcoin/bitcoin/pull/31806/commits/0289f03790151135afbd5281a45a9f6256f0a235)
`result_{selection-algo-type}->SetBumpFeeDiscount()` sets discount amount before Recalculating the Waste . This will catch any `underflow` scenarios if occuring while subtracting the discount from waste.
Also adding it increased fuzz coverage for earlier untouched part.
(https://github.com/bitcoin/bitcoin/pull/31806#pullrequestreview-2608300048)
ACK [0289f03](https://github.com/bitcoin/bitcoin/pull/31806/commits/0289f03790151135afbd5281a45a9f6256f0a235)
`result_{selection-algo-type}->SetBumpFeeDiscount()` sets discount amount before Recalculating the Waste . This will catch any `underflow` scenarios if occuring while subtracting the discount from waste.
Also adding it increased fuzz coverage for earlier untouched part.
💬 laanwj commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950608126)
Not sure if it's a difference in the tool or the magic files used by default, but `file` output seems noticibly different between linux and mac:
mac (`file-5.41`):
```
/bin/ls: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64
- Mach-O 64-bit executable x86_64] [arm64e:Mach-O 64-bit executable arm64e
- Mach-O 64-bit executable arm64e]
/bin/ls (for architecture x86_64): Mach-O 64-bit executable x86_64
/bin/ls (for architecture arm64e): Mach-O 64-bit ex
...
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950608126)
Not sure if it's a difference in the tool or the magic files used by default, but `file` output seems noticibly different between linux and mac:
mac (`file-5.41`):
```
/bin/ls: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64
- Mach-O 64-bit executable x86_64] [arm64e:Mach-O 64-bit executable arm64e
- Mach-O 64-bit executable arm64e]
/bin/ls (for architecture x86_64): Mach-O 64-bit executable x86_64
/bin/ls (for architecture arm64e): Mach-O 64-bit ex
...
👍 hebasto approved a pull request: "build: set build type and per-build-type flags as early as possible"
(https://github.com/bitcoin/bitcoin/pull/31711#pullrequestreview-2608325691)
ACK 56a9b847bba2b8deb6a9c3f3a7eb95b4c71c2d14.
(https://github.com/bitcoin/bitcoin/pull/31711#pullrequestreview-2608325691)
ACK 56a9b847bba2b8deb6a9c3f3a7eb95b4c71c2d14.
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2650448405)
> > IMO, there is isn't a good reason to break compatibility here at all, if a shim that just adds one new line and one new file to the build [#31161 (comment)](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2623350369) can provide it.
>
> Thanks! Applied.
Reverted back due to the GenerateSymlinks module was not robust enough.
> In its current form, this change seems likely to cause confusion and wasted time because when upgrading or downgrading around the change, **builds
...
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2650448405)
> > IMO, there is isn't a good reason to break compatibility here at all, if a shim that just adds one new line and one new file to the build [#31161 (comment)](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2623350369) can provide it.
>
> Thanks! Applied.
Reverted back due to the GenerateSymlinks module was not robust enough.
> In its current form, this change seems likely to cause confusion and wasted time because when upgrading or downgrading around the change, **builds
...
👍 laanwj approved a pull request: "build: disable bitcoin-node if daemon is not built"
(https://github.com/bitcoin/bitcoin/pull/31834#pullrequestreview-2608355209)
Code review ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213
(https://github.com/bitcoin/bitcoin/pull/31834#pullrequestreview-2608355209)
Code review ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213
💬 laanwj commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950628441)
Would it be possible to use `bitcoin_daemon_status` here somehow, instead of repeating the expression ? (just a small maintainability thing to keep the reporting and usage consistent if it changes in the future)
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950628441)
Would it be possible to use `bitcoin_daemon_status` here somehow, instead of repeating the expression ? (just a small maintainability thing to keep the reporting and usage consistent if it changes in the future)
📝 fanquake opened a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840)
Our CMake toolchain for a Darwin cross build currently contains:
```bash
set(CMAKE_AR "/usr/bin/llvm-ar")
set(CMAKE_RANLIB "/usr/bin/llvm-ranlib")
set(CMAKE_STRIP "/usr/bin/llvm-strip")
set(CMAKE_OBJCOPY "arm64-apple-darwin-objcopy")
set(CMAKE_OBJDUMP "/usr/bin/llvm-objdump")
```
`objcopy` isn't currently used for the Darwin build (only for Linux and splitting the debug symbols), but we shouldn't be producing a toolchain file that refers to nonexistent tools.
(https://github.com/bitcoin/bitcoin/pull/31840)
Our CMake toolchain for a Darwin cross build currently contains:
```bash
set(CMAKE_AR "/usr/bin/llvm-ar")
set(CMAKE_RANLIB "/usr/bin/llvm-ranlib")
set(CMAKE_STRIP "/usr/bin/llvm-strip")
set(CMAKE_OBJCOPY "arm64-apple-darwin-objcopy")
set(CMAKE_OBJDUMP "/usr/bin/llvm-objdump")
```
`objcopy` isn't currently used for the Darwin build (only for Linux and splitting the debug symbols), but we shouldn't be producing a toolchain file that refers to nonexistent tools.