👍 theStack approved a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973#pullrequestreview-2330645298)
ACK 286725168ae0309e427b1204a0724a4ba7cbe860
(https://github.com/bitcoin/bitcoin/pull/30973#pullrequestreview-2330645298)
ACK 286725168ae0309e427b1204a0724a4ba7cbe860
📝 maflcko opened a pull request: "ci: Require git to be installed on workers"
(https://github.com/bitcoin/bitcoin/pull/30974)
The fallback `bash -c "$PACKAGE_MANAGER_INSTALL git"` is mostly dead code and trivial to add back, if it is ever needed again.
For now, just require git and remove `PACKAGE_MANAGER_INSTALL`. Also, add some other packages which are needed by podman to docs.
(https://github.com/bitcoin/bitcoin/pull/30974)
The fallback `bash -c "$PACKAGE_MANAGER_INSTALL git"` is mostly dead code and trivial to add back, if it is ever needed again.
For now, just require git and remove `PACKAGE_MANAGER_INSTALL`. Also, add some other packages which are needed by podman to docs.
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1776775938)
We could. I left it as for simplicity, given this case should not be too frequent, so having a higher fanout under these conditions shouldn't be too bad.
I'll consider adding it if it does not make the code much harder to read, or if there is a clear counterargument in favor of it.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1776775938)
We could. I left it as for simplicity, given this case should not be too frequent, so having a higher fanout under these conditions shouldn't be too bad.
I'll consider adding it if it does not make the code much harder to read, or if there is a clear counterargument in favor of it.
👍 BrandonOdiwuor approved a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973#pullrequestreview-2330783910)
ACK 286725168ae0309e427b1204a0724a4ba7cbe860
(https://github.com/bitcoin/bitcoin/pull/30973#pullrequestreview-2330783910)
ACK 286725168ae0309e427b1204a0724a4ba7cbe860
🚀 fanquake merged a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973)
(https://github.com/bitcoin/bitcoin/pull/30973)
📝 Sjors opened a pull request: "guix: add multiprocess binaries"
(https://github.com/bitcoin/bitcoin/pull/30975)
This changes the Guix build process to use `MULTIPROCESS=1` for its `depends` build, and as a result add `bitcoin-node`, `bitcoin-gui` and `bitcoin-wallet` to the release binaries.
Combined with #30955 and https://github.com/Sjors/bitcoin/pull/52 this makes it feasible to implement a Stratum v2 Template Provider (or something similar) as an external tool that communicates with the node via IPC.
A complete implementation can be seen in https://github.com/Sjors/bitcoin/pull/48. That implemen
...
(https://github.com/bitcoin/bitcoin/pull/30975)
This changes the Guix build process to use `MULTIPROCESS=1` for its `depends` build, and as a result add `bitcoin-node`, `bitcoin-gui` and `bitcoin-wallet` to the release binaries.
Combined with #30955 and https://github.com/Sjors/bitcoin/pull/52 this makes it feasible to implement a Stratum v2 Template Provider (or something similar) as an external tool that communicates with the node via IPC.
A complete implementation can be seen in https://github.com/Sjors/bitcoin/pull/48. That implemen
...
💬 Sjors commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1776802441)
8b2546c35de37ea15aa7e22865943747b3006b61: it turns out that doing `make MULTIPROCESS=0` in `depends` causes libmultiprocess to be built anyway. Not sure if that's a known behavior or a cmake regression. cc @hebasto
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1776802441)
8b2546c35de37ea15aa7e22865943747b3006b61: it turns out that doing `make MULTIPROCESS=0` in `depends` causes libmultiprocess to be built anyway. Not sure if that's a known behavior or a cmake regression. cc @hebasto
💬 Sjors commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376573926)
I'll look into the linter once it's clear if the `make MULTIPROCESS=0` behaviour above is intentional.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376573926)
I'll look into the linter once it's clear if the `make MULTIPROCESS=0` behaviour above is intentional.
👍 hebasto approved a pull request: "build: Add missing USDT header dependency to kernel"
(https://github.com/bitcoin/bitcoin/pull/30970#pullrequestreview-2330824333)
ACK ccd10fdb97f9b8268a5cd60d7461967cfe536f16, the diff looks correct.
Ideally, such changes should be enforced by a failure to compile:
```
/home/hebasto/git/bitcoin/src/util/trace.h:12:10: fatal error: sys/sdt.h: No such file or directory
8 | #include <sys/sdt.h>
| ^~~~~~~~~~~
compilation terminated.
```
However, when building with depends, the compiler is still able to include a system-wide header:
```
$ sudo apt install systemtap-sdt-dev
$ make -C depends NO_
...
(https://github.com/bitcoin/bitcoin/pull/30970#pullrequestreview-2330824333)
ACK ccd10fdb97f9b8268a5cd60d7461967cfe536f16, the diff looks correct.
Ideally, such changes should be enforced by a failure to compile:
```
/home/hebasto/git/bitcoin/src/util/trace.h:12:10: fatal error: sys/sdt.h: No such file or directory
8 | #include <sys/sdt.h>
| ^~~~~~~~~~~
compilation terminated.
```
However, when building with depends, the compiler is still able to include a system-wide header:
```
$ sudo apt install systemtap-sdt-dev
$ make -C depends NO_
...
💬 hebasto commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1776829628)
> [8b2546c](https://github.com/bitcoin/bitcoin/commit/8b2546c35de37ea15aa7e22865943747b3006b61): it turns out that doing `make MULTIPROCESS=0` in `depends` causes libmultiprocess to be built anyway.
Yes, any non-empty value is considered "set/enabled":https://github.com/bitcoin/bitcoin/blob/e13da501db9e123ec56adfcb6f01b811445f9ce7/depends/Makefile#L165
> Not sure if that's a known behavior or a cmake regression. cc @hebasto
Definitely, not the latter.
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1776829628)
> [8b2546c](https://github.com/bitcoin/bitcoin/commit/8b2546c35de37ea15aa7e22865943747b3006b61): it turns out that doing `make MULTIPROCESS=0` in `depends` causes libmultiprocess to be built anyway.
Yes, any non-empty value is considered "set/enabled":https://github.com/bitcoin/bitcoin/blob/e13da501db9e123ec56adfcb6f01b811445f9ce7/depends/Makefile#L165
> Not sure if that's a known behavior or a cmake regression. cc @hebasto
Definitely, not the latter.
📝 hebasto opened a pull request: "depends, doc: Drop package-specific note about CMake"
(https://github.com/bitcoin/bitcoin/pull/30976)
CMake is no longer required solely for `libmultiprocess`.
(https://github.com/bitcoin/bitcoin/pull/30976)
CMake is no longer required solely for `libmultiprocess`.
💬 maflcko commented on pull request "depends, doc: Drop package-specific note about CMake":
(https://github.com/bitcoin/bitcoin/pull/30976#issuecomment-2376639742)
review ACK 4cf84b344dea4696de540a0f053aa1ec560ac38c
(https://github.com/bitcoin/bitcoin/pull/30976#issuecomment-2376639742)
review ACK 4cf84b344dea4696de540a0f053aa1ec560ac38c
💬 fanquake commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376667196)
If multiprocess is (currently) unsupported on Windows, then that should be fixed in depends, i.e (for now) don't even try building the packages/functionality for that platform, not leaked into Guix with a workaround.
More generally, if the project decides to start shipping multiprocess as part of it's releases, then there's a number of other things that need to happen first. We should be switching multiprocess from an opt-in in depends, to being built/used by default, as well as testing all o
...
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376667196)
If multiprocess is (currently) unsupported on Windows, then that should be fixed in depends, i.e (for now) don't even try building the packages/functionality for that platform, not leaked into Guix with a workaround.
More generally, if the project decides to start shipping multiprocess as part of it's releases, then there's a number of other things that need to happen first. We should be switching multiprocess from an opt-in in depends, to being built/used by default, as well as testing all o
...
🚀 fanquake merged a pull request: "depends, doc: Drop package-specific note about CMake"
(https://github.com/bitcoin/bitcoin/pull/30976)
(https://github.com/bitcoin/bitcoin/pull/30976)
💬 hebasto commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2376669835)
> > 1. Do you think the current approach of temporarily changing from Release -> RelWithDebInfo for Windows would be acceptable, as a PR that is only run on CI?
>
> Seems fine to do permanent, if there are no downsides or slow-down. cc @hebasto
Here is some historical context:
1. In pre-Cmake times, manually crafted project files had two configurations: "Release" and "Debug".
2. While working on the CMake staging branch, the CI build configuration remained "Release" to allow the use of [
...
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2376669835)
> > 1. Do you think the current approach of temporarily changing from Release -> RelWithDebInfo for Windows would be acceptable, as a PR that is only run on CI?
>
> Seems fine to do permanent, if there are no downsides or slow-down. cc @hebasto
Here is some historical context:
1. In pre-Cmake times, manually crafted project files had two configurations: "Release" and "Debug".
2. While working on the CMake staging branch, the CI build configuration remained "Release" to allow the use of [
...
⚠️ maflcko opened an issue: "ci: aarch64 workers were updated (CCACHE_MAXSIZE=100G)"
(https://github.com/bitcoin/bitcoin/issues/30977)
Just a PSA to share that the `arm64` CI workers received an update. For example, they are now using the `DANGER_CI_ON_HOST_CCACHE_FOLDER` option added in commit fa99e4521b6fc0e7f6636d40bc0d6a7325227374 with `CCACHE_MAXSIZE=100G`.
This should increase the ccache hit rates on "small" changes to 100% (or close to it). A small change is anything that can re-use a previous ccache, for example a doc-only change, a single modified C++ file, or a recent rebase of any pull request with a fixup of such
...
(https://github.com/bitcoin/bitcoin/issues/30977)
Just a PSA to share that the `arm64` CI workers received an update. For example, they are now using the `DANGER_CI_ON_HOST_CCACHE_FOLDER` option added in commit fa99e4521b6fc0e7f6636d40bc0d6a7325227374 with `CCACHE_MAXSIZE=100G`.
This should increase the ccache hit rates on "small" changes to 100% (or close to it). A small change is anything that can re-use a previous ccache, for example a doc-only change, a single modified C++ file, or a recent rebase of any pull request with a fixup of such
...
✅ maflcko closed an issue: "ci: aarch64 workers were updated (CCACHE_MAXSIZE=100G)"
(https://github.com/bitcoin/bitcoin/issues/30977)
(https://github.com/bitcoin/bitcoin/issues/30977)
💬 maflcko commented on issue "ci: aarch64 workers were updated (CCACHE_MAXSIZE=100G)":
(https://github.com/bitcoin/bitcoin/issues/30977#issuecomment-2376719225)
Nothing to discuss here, but please leave a comment if there are any issues after the update, or if the `arm64` ccache hit rate isn't close to 100% when you'd expect it, or if the build takes longer than 10 minutes with a 100% hit rate.
(https://github.com/bitcoin/bitcoin/issues/30977#issuecomment-2376719225)
Nothing to discuss here, but please leave a comment if there are any issues after the update, or if the `arm64` ccache hit rate isn't close to 100% when you'd expect it, or if the build takes longer than 10 minutes with a 100% hit rate.
💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1776938285)
Latest force-push incorporates what we've discussed here, and goes one step further by removing the `const std::string&` overload entirely. I inspected the callsites and none of them seemed necessary to have the overload (see updated PR description), so this felt like the better way forward.
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1776938285)
Latest force-push incorporates what we've discussed here, and goes one step further by removing the `const std::string&` overload entirely. I inspected the callsites and none of them seemed necessary to have the overload (see updated PR description), so this felt like the better way forward.
💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2376782278)
Latest force-push increases the scope of this PR, see updated PR description for an overview. In a nutshell:
- all non-`bilingual_str` `format`/`strprintf` usage is now compile-time checked. Format errors are returned as strings instead of thrown as error, but throwing behaviour can be maintained through `tfm::format_raw`.
- the `const std::string&` `format` overload is removed because it's unnecessary
Thanks a lot @maflcko and @l0rinc for your review!
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2376782278)
Latest force-push increases the scope of this PR, see updated PR description for an overview. In a nutshell:
- all non-`bilingual_str` `format`/`strprintf` usage is now compile-time checked. Format errors are returned as strings instead of thrown as error, but throwing behaviour can be maintained through `tfm::format_raw`.
- the `const std::string&` `format` overload is removed because it's unnecessary
Thanks a lot @maflcko and @l0rinc for your review!