Bitcoin Core Github
44 subscribers
121K links
Download Telegram
👍 stickies-v approved a pull request: "doc: move `cmake -B build -LH` up in Unix build docs"
(https://github.com/bitcoin/bitcoin/pull/33088#pullrequestreview-3067205611)
ACK 6757052fc439bedd1fa88ee1d23b4f17cf2c4f7a

For all other platforms, this configure help lives close to the `cmake --build build` command, which makes most sense to me. I did note that `build-windows.md` and `build-windows-msvc.md` documentation does not provide the `cmake -B build -LH` help at all. If relevant, could be added in this PR? No blocker.

Example diff (untested):

<details>
<summary>git diff on 6757052fc4</summary>

````diff
diff --git a/doc/build-windows-msvc.md b/doc/b
...
💬 enirox001 commented on issue "build: Build warnings from deprecated std::get_temporary_buffer in std::stable_sort calls":
(https://github.com/bitcoin/bitcoin/issues/33080#issuecomment-3132302364)
Thanks for looking into this. To clarify - I'm actually using Clang 20.1.8, not GCC. The GCC paths in the warnings are because Clang uses the system's GCC standard library headers on Linux.

Exact reproduction steps:
1. `cmake --preset=libfuzzer`
2. `cmake --build build_fuzz`

System: Ubuntu 22.04, Clang 20.1.8

The warnings appear specifically with this libfuzzer preset configuration. If this is a Clang-specific issue or configuration-specific, feel free to close if not worth addressing.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3132351636)
> Follow-up from a while back: let's just explicitly say which layer of txgraph we're querying: [instagibbs@bb48f6c](https://github.com/instagibbs/bitcoin/commit/bb48f6c4736c227bcee6c4dda8e95b0b0287cfef)

@instagibbs Done, should be fixed in my last push.
👍 brunoerg approved a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3067374697)
reACK 5888b4a2a5566c64141b78a0e7660a166ec99775

minimal changes since my last review.
💬 maflcko commented on issue "build: Build warnings from deprecated std::get_temporary_buffer in std::stable_sort calls":
(https://github.com/bitcoin/bitcoin/issues/33080#issuecomment-3132392576)
This warning is harmless and can be ignored. It should go away the next time you upgrade your Ubuntu (and thus the GCC package and std lib). Alternatively, you can use libc++ instead.
💬 maflcko commented on pull request "doc: move `cmake -B build -LH` up in Unix build docs":
(https://github.com/bitcoin/bitcoin/pull/33088#issuecomment-3132408117)
lgtm ACK 6757052fc439bedd1fa88ee1d23b4f17cf2c4f7a
🚀 glozow merged a pull request: "cluster mempool: add TxGraph work controls"
(https://github.com/bitcoin/bitcoin/pull/32263)
🤔 ryanofsky reviewed a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3067368157)
I wonder if @vasild @TheCharlatan @BrandonOdiwuor who added ACKs previously could review the current code? The [current changes](https://github.com/bitcoin/bitcoin/compare/e17fb86382eafb7ccfddb56bca981f6a12201c85..082b416a1bcbfbbdcafb3a3eb1ae800c93385203) are basically the same as the [changes from 6 months ago](https://github.com/bitcoin/bitcoin/compare/99b7b857d3b8b9ce9bd7501e2480583369740c55..4aaed63d4e6a8b472c6c308b921d82332ed18545]) so should not be too difficult to review or reack.

I'm
...
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2239738884)
In commit "build: depends makes libmultiprocess by default" (d4f18bb9de76d2caf41b6db05c494b333008a3f5)

Notes to help explain changes:

| Variable | Description |
|--|--|
| <pre>$(ipc_packages)</pre> | Constant list of packages needed to build with IPC features, defined in `depends/packages/packages.mk`. Always set regardless of `NO_IPC`. |
| <pre>$(ipc_packages_)</pre> | Same as `$(ipc_packages)`, except empty if `NO_IPC` is set. |
| <pre>@ipc_packages@</pre> | Same as `$(ipc_packages_)
...
💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3132539036)
> [txorphan_crash.txt](https://github.com/user-attachments/files/21437145/txorphan_crash.txt)
>
> The `LimitOrphans` in `AddTx` ends up decreasing `m_unique_orphan_usage` in this case.

Thank you, fixed
💬 darosior commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3132553058)
CI failure is random:
> Downloading https://github.com/boostorg/optional/archive/boost-1.87.0.tar.gz -> boostorg-optional-boost-1.87.0.tar.gz
error: https://github.com/boostorg/optional/archive/boost-1.87.0.tar.gz: failed: status code 503
👍 maflcko approved a pull request: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/33079#pullrequestreview-3067550243)
I guess DEBUG or sanitizers may use more stack space, so the "real" limit may be lower for them, albeit unknown.

lgtm
💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/33079#discussion_r2239830856)
is there a reason why this works now, but seemed to have failed earlier this year? Ref: https://github.com/bitcoin/bitcoin/runs/33490765963

```
[17:50:05.473] /ci_container_base/ci/test/03_test_script.sh: line 153: 6815 Segmentation fault (core dumped) DIR_UNIT_TEST_DATA="${DIR_UNIT_TEST_DATA}" LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" "${BASE_OUTDIR}"/bin/test_bitcoin --catch_system_errors=no -l test_suite
💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/33079#discussion_r2239841785)
For reference, this seems to have failed previously (second log of https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2500175006), but is now fixed after https://github.com/bitcoin/bitcoin/pull/32351?
💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3132610634)
> This only affects the binary release and not source built locations, so actually, I don't care either way.

Thanks, yes. Effects of this PR:

* For **source builds** (i.e. developer builds) — No changes.
* For **source installs** (i.e. `cmake --install` output) — `test_bitcoin` and `test_bitcoin-qt` and `bench_bitcoin` are installed in `${CMAKE_PREFIX_PATH}/libexec` instead of `${CMAKE_PREFIX_PATH}/bin`, so they are no longer on `PATH`. But they can still be invoked from the `libexec/` di
...
🤔 janb84 reviewed a pull request: "doc: move `cmake -B build -LH` up in Unix build docs"
(https://github.com/bitcoin/bitcoin/pull/33088#pullrequestreview-3067676411)
ACK 6757052fc439bedd1fa88ee1d23b4f17cf2c4f7a

This PR moves information on getting all the available options of `cmake -B build` to a more prominent place, line 13. The PR seems to me a logical change and makes me wonder why this information was at the end of the documentation in the first place.
💬 darosior commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3132675457)
Turns out we also didn't test the disconnection logic after a second block building on top of an invalid block is submitted to us (nor the disconnection logic for when an invalid compact block header is submitted to us). This can be checked with:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..f8b9adf910a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1822,7 +1822,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid
...
💬 fanquake commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-3132684789)
Guix Build (aarch64):
```bash
d7dcb667b0d801e51490f677fe08be66ba635df93085d0479dcd00443b93bbf2 guix-build-9954d6c83338/output/aarch64-linux-gnu/SHA256SUMS.part
296a26bd69bc458e5a5953876703c664bf760f24ed93c61a43b418522c5c8e3a guix-build-9954d6c83338/output/aarch64-linux-gnu/bitcoin-9954d6c83338-aarch64-linux-gnu-debug.tar.gz
118a81b7e941929f2d95d426e2b2a5d698b90f381819fc68572f5ec06d05b825 guix-build-9954d6c83338/output/aarch64-linux-gnu/bitcoin-9954d6c83338-aarch64-linux-gnu.tar.gz
24d45e
...
💬 darosior commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3132701279)
> In that case you'll already get a network split when the hard fork side mines a block.

Discussed this with AJ (and others). For reference, in this case the disconnection would happen after a second block being submitted on top a first invalid one. We would not disconnect upon a first invalid block being submitted through compact blocks. It turns out that this logic was not tested at all, so i opened https://github.com/bitcoin/bitcoin/pull/33083 which exercises various cases in `MaybePunishN
...
💬 ryanofsky commented on pull request "kernel: create monolithic kernel static library":
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132708611)
> Consuming a static library requires a consistent toolchain. .pc files are not reloatable.

@purpleKarrot I read your post the other day and enjoyed it and learned a number of things. And I understand why cmake files are more flexible than .pc files and why they can't support mismatched compilers / libraries and relocation.

But is there something wrong with providing and using .pc files in the case where there is a consistent toolchain and nothing is relocated? This seems like a common & u
...