Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 fanquake commented on pull request "doc: update unix build doc with build flags":
(https://github.com/bitcoin/bitcoin/pull/32269#issuecomment-3131921976)
Rebased this is in #33088.
🤔 danielabrozzoni reviewed a pull request: "cli: return local services in -netinfo"
(https://github.com/bitcoin/bitcoin/pull/31886#pullrequestreview-3066765486)
tACK 943ee6768d9f94da70aaf90d346d4cf2ea1fa39d

I left a micro nit, feel free to ignore if you don't need to retouch
💬 danielabrozzoni commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2239320315)
nit: add local services to the comment
👍 ryanofsky approved a pull request: "depends: hard-code necessary c(xx)flags rather than setting them per-host"
(https://github.com/bitcoin/bitcoin/pull/32584#pullrequestreview-3067012972)
Code review ACK 9954d6c833381a44e1ea34d182ffe4d61b65d2ba. No changes since last review other than improving the commit message. Change overall makes sense because it deduplicates host definitions, stops dropping `-std` flags from package builds when custom CFLAGS/CXXFLAGS environment variables are set, and stops passing duplicate flags to cmake that have no effect.
👋 sdaftuar's pull request is ready for review: "[WIP] Cluster mempool implementation"
(https://github.com/bitcoin/bitcoin/pull/28676)
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3132158578)
> I fuzzed this branch ([3bfaedb](https://github.com/bitcoin/bitcoin/commit/3bfaedbd7d9d6e71e9df03fbbb51a33c6184cca6)) with [fuzzamoto](https://github.com/dergoegge/fuzzamoto) and it found an assertion crash in `CTxMemPool::check`.

@dergoegge @instagibbs Thank you both for detecting and pinpointing the bug! Should be fixed now in this branch.
💬 purpleKarrot commented on pull request "kernel: create monolithic kernel static library":
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132165160)
For https://habla.news/u/purplekarrot.net/cmake-import-export, I have set up several example projects for exporting cmake packages and I deliberately mismatched consumers by using gcc, clang, and msvc.

I am making the bold claim that there is no working alternative to shipping a shared library with cmake package information. Consuming a static library requires a consistent toolchain. `.pc` files are not reloatable.

I will happily remove my NACK once I am proven wrong with a counterexample.
...
👍 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