💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778162781)
I think that since `waitTipChanged` takes a `current_tip` argument the `m_tip_block != uint256::ZERO` check is no longer needed. Though it seems fine to keep it.

IIRC the reason I added it was in case `waitTipChanged` was called during early node initialization, before `m_tip_block` is set to the genesis block.

That scenario is now handled by `m_tip_block != current_tip`.

In term of how the Template Provider works, it used to rely on this check, but now it just waits for `getTip()` to r
...

💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2378614029)
re-utACK cccc5b19fba6d04e586f59bff5f5b2cdef3dffba

The main change since my last review is in 444424b96deef72780f617d1dbab844a2c9d72ab, which is a nice simplification.

💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2378624163)
> The error behaviour can be tested by either manually making `LoadChainstate` return a failure, or deleting some of the block index database files.

Upgrading my review: I tested using the instructions.

📝 vasild opened a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988)
Currently `CConnman` is a mixture of:
* low level socket handling, e.g. send, recv, poll, bind, listen, connect, and
* higher level logic that is specific to the Bitcoin P2P protocol, e.g. V1/V2 transport, choosing which address to connect to, if we manage to connect mark the address good in `AddrMan`, maintaining the number of inbound and outbound connections, banning of peers, interacting with `PeerManager`.

This PR splits the socket handling into a new class which makes the code more mod
...

💬 vasild commented on issue "Split socket handling out of CConnman":
(https://github.com/bitcoin/bitcoin/issues/30694#issuecomment-2378650248)
Done in https://github.com/bitcoin/bitcoin/pull/30988.

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778217444)
The `m_tip_block.IsNull()` is used in fa05721cbc622b9b044af920f63312a5e407a022, but that doesn't matter here, so I am happy to drop it. (In theory it should be `std::optional`, instead of encoding nullopt as zero, but that may be overkill)

> IIRC the reason I added it was in case `waitTipChanged` was called during early node initialization, before `m_tip_block` is set to the genesis block.

I don't think this is true. `m_tip_block` may never be set to the genesis block, and stay zero foreve
...

💬 maflcko commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1778228488)
unrelated bug: But the GUI never sets this, unless `-server` is passed. So no code will run and what was passed in will be returned, incorrectly.

💬 maflcko commented on pull request "fuzz: fix bug in p2p_headers_presync harness":
(https://github.com/bitcoin/bitcoin/pull/30980#issuecomment-2378778821)
Side-note: I wonder why oss-fuzz hasn't found this yet. According to https://oss-fuzz.com/fuzzer-stats?group_by=by-day&fuzzer=afl_bitcoin-core_p2p_headers_presync&project=bitcoin-core the exec/s is `0` for afl. (The libfuzzer one has up to 100 exec/s). I checked the build logs (https://oss-fuzz-build-logs.storage.googleapis.com/index.html#bitcoin-core) for `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` and it seems to be set.

🚀 fanquake merged a pull request: "contrib: Update asmap link in seeds readme"
(https://github.com/bitcoin/bitcoin/pull/30979)

💬 Sjors commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2378828326)
I like the idea of a unified `bitcoin` command.

We could start with seperate binaries for multiprocess, e.g.:

* `bitcoin daemon` (for `bitcoind`)
* `bitcoin daemon multiprocess` for `bitcoin-node`
* `bitcoin gui` (for `bitcoin-qt`)
* `bitcoin gui multiprocess` (for `bitcoin-gui`)

And perhaps not worry about just including multiprocess in the utilities:
* `bitcoin wallet` (for `bitcoin-wallet`)

Then later on the `multiprocess` could be made default and we drop the non-multiprocess
...

💬 fanquake commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2378848997)
```bash
ci_container_base/src/ipc/capnp/mining.cpp -DARENA_DEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES
In file included from /ci_container_base/src/ipc/capnp/mining.cpp:5:
In file included from /ci_container_base/src/ipc/capnp/mining-types.h:9:
In file included from /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/common.capnp.proxy-types.h:6:
In file included from /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/c
...

👍 fanquake approved a pull request: "ci: Inline PACKAGE_MANAGER_INSTALL"
(https://github.com/bitcoin/bitcoin/pull/30974#pullrequestreview-2333269186)
ACK fafd1a0f648fcd602166d01d47da417b0eb380df

💬 fanquake commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2378879411)
Guix Build (x86_64):
```bash
8868809b50ea77b47c45c1cdf802e85540d9428f182773b1cd7e5d73750dce87 guix-build-06b4c339e89e/output/aarch64-linux-gnu/SHA256SUMS.part
fccf8cf14ca3d3ee7812f254d10eb17e62c507f5b2606c6b099c5d00ed5d3446 guix-build-06b4c339e89e/output/aarch64-linux-gnu/bitcoin-06b4c339e89e-aarch64-linux-gnu-debug.tar.gz
2d7cf4959e238fae666476db1cd11112c77708a58bb9d5cd1e0c7986c8c477a6 guix-build-06b4c339e89e/output/aarch64-linux-gnu/bitcoin-06b4c339e89e-aarch64-linux-gnu.tar.gz
22d7016
...

👍 fanquake approved a pull request: "depends: Fix build with `MULTIPROCESS=1` in Guix environment"
(https://github.com/bitcoin/bitcoin/pull/30940#pullrequestreview-2333270577)
ACK 06b4c339e89e593d951a90cd2d1bce944acf3bf7

✅ fanquake closed an issue: "cmake: multiprocess guix build broken"
(https://github.com/bitcoin/bitcoin/issues/30931)

🚀 fanquake merged a pull request: "depends: Fix build with `MULTIPROCESS=1` in Guix environment"
(https://github.com/bitcoin/bitcoin/pull/30940)

🚀 fanquake merged a pull request: "ci: Inline PACKAGE_MANAGER_INSTALL"
(https://github.com/bitcoin/bitcoin/pull/30974)

🤔 hebasto reviewed a pull request: "docs: Add instructions on how to self-sign bitcoin-core binaries for macOS"
(https://github.com/bitcoin/bitcoin/pull/30982#pullrequestreview-2333289289)
Concept ACK.

I've tested the content of the [`bitcoin-28.0rc2-x86_64-apple-darwin.tar.gz`](https://bitcoincore.org/bin/bitcoin-core-28.0/test.rc2/bitcoin-28.0rc2-x86_64-apple-darwin.tar.gz) on macOS (Apple M1) Sequoia 15.0 (24A335).

The `code sign -s - ...` command is not required to run downloaded binaries. However, I didn't test other maOS versions.

💬 maflcko commented on pull request "test: re-bucket long-running tests":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2378900314)
`feature_config_args.py` could be moved as well, but at some point it doesn't matter too much. I wonder if this even has an effect at all.

💬 fanquake commented on pull request "docs: Add instructions on how to self-sign bitcoin-core binaries for macOS":
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2378901637)
> The code sign -s - ... command is not required to run downloaded binaries.

How are the binaries running if they aren't codesigned at all?
If this isn't an issue why are we adding these instructions?