💬 theuni commented on pull request "depends: qt 5.15.14 and fix macOS build with Clang 18":
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2139978109)
Is the bump to .14 also for clang 18, or is that just incidental?

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620982315)
Actually, same comment for `operator=` as well. Maybe it'd be worth a special case in `Reallocate()` itself instead.

💬 fanquake commented on pull request "depends: qt 5.15.14 and fix macOS build with Clang 18":
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2139993780)
> or is that just incidental?

Mostly incidental. We've got to rebuild all Qts after dropping the android patches changes from #30049, so seemed like a convenient time. I also try to bump to the newest source (if possible) before introducing new patches.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620986696)
Are you imagining something like `realloc` on the buffer? AFAIK that just doesn't exist in C++; if you want to shrink an object, you need to construct a new object of smaller size and copy over.

📝 vasild opened a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205)
Put the generic parts from `StaticContentsSock` into a separate class `ZeroSock` so that they can be reused in other mocked `Sock` implementations.

Add a new `DynSock` whose `Recv()` and `Send()` methods can be controlled after the object is created. To achieve that, the caller/creator of `DynSock` provides to its constructor two pipes (FIFOs) - recv-pipe and send-pipe. Whatever data is written to recv-pipe is later received by `DynSock::Recv()` method and whatever data is written to the sock
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620997477)
> Making a separate PR could help

Done, the first two commits from https://github.com/bitcoin/bitcoin/pull/26812 extracted into a separate PR: https://github.com/bitcoin/bitcoin/pull/30205

💬 theuni commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#issuecomment-2140037452)
> Removes bsdmainutils for macOS, as couldn't see where it's needed, and could complete a build with it uninstalled.
Can re-add if someone knows what it's needed for.

IIRC this was needed for hexdump, which is needed to build some tests. Maybe it's already installed by something else?

💬 fanquake commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#issuecomment-2140046553)
> IIRC this was needed for hexdump, which is needed to build some tests.

Right. So not needed to compile depends, but needed by our buildsystem for test-related things. Given that sounds like it's a dependency for all platforms in any case, then it could just be moved into `Common`, if we want to keep it in the depends readme?

⚠️ fanquake opened an issue: "build: make macOS build Clang only"
(https://github.com/bitcoin/bitcoin/issues/30206)
It should be possible to have a macOS (release) build environment that only uses Clang, and has no GCC toolchain installed.

Currently that's not possible, because Qt still reaches out to `g++`. This can be demo'd by Guix building this branch: https://github.com/fanquake/bitcoin/tree/guix_macos_no_gcc, which removes the `gcc-toolchain` package, and associated build infra from the macOS env. It will fail with:
```bash
Configuring qt...
Creating qmake...
make[1]: Entering directory '/bitcoin
...

💬 fanquake commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#issuecomment-2140064712)
> then it could just be moved into Common,

Have done this.

👍 hebasto approved a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2088736908)
ACK ecb278bb19c53b007380e262fa86d809255eeb49, I have reviewed the code and it looks OK.

It seems worth considering to add a couple of edge cases to the `src/test/fuzz/vecdeque.cpp` like self-assignment and self-swap.

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1621023146)
Grr, right.

👍 TheCharlatan approved a pull request: "depends: qt 5.15.14 and fix macOS build with Clang 18"
(https://github.com/bitcoin/bitcoin/pull/30198#pullrequestreview-2088766567)
ACK 0a3631fc352eda849290db940844e5ef723436df

Guix build (aarch64)
```
1efa3e0205032d6cfe5517ce36ab63379afa22268dcbb0b92150719baa030682 guix-build-0a3631fc352e/output/aarch64-linux-gnu/SHA256SUMS.part
e3db302484148c00cab7150e3451d2c95dae14062cb4165c8e5a28d71d77e630 guix-build-0a3631fc352e/output/aarch64-linux-gnu/bitcoin-0a3631fc352e-aarch64-linux-gnu-debug.tar.gz
f9792f8db8635bb19f6a2d7c73f1b0780e5776734c12912c5e7172f357f7fa1f guix-build-0a3631fc352e/output/aarch64-linux-gnu/bitcoin-0a
...

👍 theuni approved a pull request: "depends: qt 5.15.14 and fix macOS build with Clang 18"
(https://github.com/bitcoin/bitcoin/pull/30198#pullrequestreview-2088769385)
utACK 0a3631fc352eda849290db940844e5ef723436df

👍 theuni approved a pull request: "depends: consolidate dependency docs"
(https://github.com/bitcoin/bitcoin/pull/30204#pullrequestreview-2088770597)
ACK 01cd425f083b2d38bced8bda8163d87359ea1f59

🤔 theuni reviewed a pull request: "depends: remove `FORCE_USE_SYSTEM_CLANG`"
(https://github.com/bitcoin/bitcoin/pull/30201#pullrequestreview-2088776148)
Concept ACK

💬 fanquake commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#discussion_r1621045177)
Yea. Depending on when this lands, we could update this to recommend LLVM 18, as that's likely what is going to be used in the CI, if we just `apt install clang` on Ubuntu `24.04`. At a minimum, given we use `-fixup_chains`, we require at least LLVM 16 or later.

💬 maflcko commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#discussion_r1621052228)
Might as well put in `build-essential` when this includes Bitcoin Core dependencies such as `bsdmainutils`?

Ref: https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#dependency-build-instructions

💬 fanquake commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#discussion_r1621057895)
Right, we could, but https://packages.ubuntu.com/mantic/build-essential is basically gcc, g++ and make. Should we then remove make from this list too, and g++ from the macOS list?

I think I'd actually rather remove `bsdmainutils`, as I really wanted the dependencies listed here to just be what is required to actually build depends, rather than everything needed for Bitcoin Core.

💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2140176994)
This seems very useful. I'll try to use it for the (currently very brittle) `Sv2Transport` tests in #29432, and review it along the way.