Bitcoin Core Github
44 subscribers
120K links
Download Telegram
📝 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
...
💬 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
💬 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.
👍 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_
...
💬 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.
📝 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`.
💬 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
💬 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
...
🚀 fanquake merged a pull request: "depends, doc: Drop package-specific note about CMake"
(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 [
...
⚠️ 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
...
maflcko closed an issue: "ci: aarch64 workers were updated (CCACHE_MAXSIZE=100G)"
(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.
💬 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.
💬 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!
👍 maflcko approved a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2331072265)
lgtm

Thanks for taking the feedback
💬 maflcko 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_r1776965132)
nit in ed57f6ee02d5a6ada23abaea6e2176cd4f173e4f: Would be nice to do it in tinyformat itself, or not at all. ConstevalFormatString is already checked, so extremely unlikely to hit. However, translated strings (which are apparently unreviewed, see https://github.com/bitcoin/bitcoin/issues/30897) are left unchecked and may throw.

It would be good to validate and review the translated strings, or remove translations completely for now, but this is probably a separate issue.

Here, it would be
...
💬 Sjors commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376805725)
I would also prefer it if `bitcoind` (and `bitcoin-qt`) had multiprocess built in. That way the instruction for miners is "Install Bitcoin Core as you always do, just add `ipcbind=linux` to you config". cc @ryanofsky

> 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.

I'll look into that.

> there's a number of other things that need to happen first. We should be switching multipro
...
💬 ryanofsky commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376836413)
It's good to have this PR open as a draft to see what it looks like but it sounds like there are a number of followups that need to happen for it to be ready. For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term. It would also be good to review and merge #30940 which this is currently based on.
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2376861686)
Rebased 7b9aa0b6eb1126cb32da91f3c9d2df12325aa90c -> 2227afac0372358287fb879f3b0bd07fd653f3f8 ([`pr/mine.10`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.10) -> [`pr/mine.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.10-rebase..pr/mine.11)) after base PR's #30509 and #30510 were merged.

Marking ready for review