Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 maflcko commented on pull request "doc: fix `loadtxoutset` example":
(https://github.com/bitcoin/bitcoin/pull/30973#issuecomment-2376362674)
review ACK 286725168ae0309e427b1204a0724a4ba7cbe860
🚀 fanquake merged a pull request: "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job"
(https://github.com/bitcoin/bitcoin/pull/30961)
💬 maflcko commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2376384339)
Are all random peers limited peers, which wouldn't serve blocks?
💬 fanquake commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2376402542)
> Are all random peers limited peers,

I don't think so, given some peers would have to be serving (historic) blocks for the background chain to continue syncing? I've now restarted this node, and the behaviour hasn't persisted, both chains are currently syncing. If this gets to the point that the snapshot load completes, I'll close this.
💬 maflcko commented on issue "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated":
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2376413368)
Should be trivial to rebase it, no?


```diff
diff --git a/src/compat/compat.h b/src/compat/compat.h
index 366c648ae7..1f3bf2b018 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -92,8 +92,12 @@ typedef char* sockopt_arg_type;
// building with a binutils < 2.36 is subject to this ld bug.
#define MAIN_FUNCTION __declspec(dllexport) int main(int argc, char* argv[])
#else
+#ifdef HFND_FUZZING_ENTRY_FUNCTION_CXX
+#define MAIN_FUNCTION HFND_FUZZING_ENTRY_FUNCTION_CXX(int
...
👍 theStack approved a pull request: "doc: fix `loadtxoutset` example"
(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.
💬 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.
👍 BrandonOdiwuor approved a pull request: "doc: fix `loadtxoutset` example"
(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)
📝 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
...