Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 Sjors commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2139890557)
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.
📝 fanquake opened a pull request: "depends: consolidate dependency docs"
(https://github.com/bitcoin/bitcoin/pull/30204)
Adds missing `g++` for macOS. This is needed by Qt:
```bash
Configuring qt...
Creating qmake...
gmake[1]: Entering directory '/bitcoin/depends/work/build/arm64-apple-darwin/qt/5.15.14-4bca24c8f89/qtbase/qmake'
gmake[1]: g++: No such file or directory
gmake[1]: *** [Makefile:250: main.o] Error 127
```

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.

`xz-utils` was al
...
💬 maflcko commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#issuecomment-2139941683)
utACK e99ecf01a54dd0d1b3b384b70125caa819724578
💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620955671)
It's a shame that this triggers a reallocation when `m_offset == 0`. Might it be worth optimizing for that case?
💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1620962324)
I tried to track down if/how it could be a race condition, but given that it's in init and all locks seems to be in place, I don't see how that could possibly be the case.
💬 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