💬 sipa commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1620639863)
Done.
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1620639863)
Done.
💬 sipa commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1620640074)
Done.
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1620640074)
Done.
💬 sipa commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1620640775)
Nice catch, indeed. I've simplified the branches and comments, and added a few `Assume`s to document the expectations.
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1620640775)
Nice catch, indeed. I've simplified the branches and comments, and added a few `Assume`s to document the expectations.
💬 maflcko commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1620667119)
There is a small benefit to `struct.pack` being less code when more than one item is packed or unpacked. Seems fine to use either in this case, up to the author. However, in most other cases where only a single item is (un)packed, the modern alternatives are less code, and clearer.
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1620667119)
There is a small benefit to `struct.pack` being less code when more than one item is packed or unpacked. Seems fine to use either in this case, up to the author. However, in most other cases where only a single item is (un)packed, the modern alternatives are less code, and clearer.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620683143)
Agree adding fuzzing/testing makes sense as a follow-up PR. It's already big enough, and also i don't want to interfere with review by doing more active development here than address review comments.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620683143)
Agree adding fuzzing/testing makes sense as a follow-up PR. It's already big enough, and also i don't want to interfere with review by doing more active development here than address review comments.
💬 TheCharlatan commented on pull request "depends: qt 5.15.14 and fix macOS build with Clang 18":
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2139566088)
0a3631fc352eda849290db940844e5ef723436df Patch and version bump looks good to me, but I'm getting an error when building with your instructions:
```
# clang --version
Ubuntu clang version 18.1.3 (1)
# make -C depends HOST=arm64-apple-darwin FORCE_USE_SYSTEM_CLANG=1 V=1
make: Entering directory '/home/drgrid/bitcoin/depends'
echo Configuring libevent...
Configuring libevent...
rm -rf /home/drgrid/bitcoin/depends/arm64-apple-darwin; mkdir -p /home/drgrid/bitcoin/depends/arm64-apple-darwin/
...
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2139566088)
0a3631fc352eda849290db940844e5ef723436df Patch and version bump looks good to me, but I'm getting an error when building with your instructions:
```
# clang --version
Ubuntu clang version 18.1.3 (1)
# make -C depends HOST=arm64-apple-darwin FORCE_USE_SYSTEM_CLANG=1 V=1
make: Entering directory '/home/drgrid/bitcoin/depends'
echo Configuring libevent...
Configuring libevent...
rm -rf /home/drgrid/bitcoin/depends/arm64-apple-darwin; mkdir -p /home/drgrid/bitcoin/depends/arm64-apple-darwin/
...
💬 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-2139567756)
> So I did apt install lld and it worked. Should that be added to the instructions?
I can add that to the PR instructions here. Adding it the depends install instructions will be part of the next PR, which removes `FORCE_USE_SYSTEM_CLANG`.
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2139567756)
> So I did apt install lld and it worked. Should that be added to the instructions?
I can add that to the PR instructions here. Adding it the depends install instructions will be part of the next PR, which removes `FORCE_USE_SYSTEM_CLANG`.
💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620728482)
Does `std::move(const T&)` make sense here? The `std::construct_at` won't be able to modify `elem` due to its constness, right?
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620728482)
Does `std::move(const T&)` make sense here? The `std::construct_at` won't be able to modify `elem` due to its constness, right?
💬 maflcko commented on pull request "clang-tidy: Add `bugprone-move-forwarding-reference` check":
(https://github.com/bitcoin/bitcoin/pull/30199#issuecomment-2139581156)
utACK 88cdb5967f093cf96e9184a48c0d9e34cea9d341
(https://github.com/bitcoin/bitcoin/pull/30199#issuecomment-2139581156)
utACK 88cdb5967f093cf96e9184a48c0d9e34cea9d341
📝 fanquake opened a pull request: "depends: remove `FORCE_USE_SYSTEM_CLANG`"
(https://github.com/bitcoin/bitcoin/pull/30201)
Remove `FORCE_USE_SYSTEM_CLANG` in favour of always using the system Clang and lld for macOS cross-compilation; rather than downloading precompiled blobs.
For example, anyone using Ubuntu 24.04 should be able to `apt install clang llvm lld .. etc`, and then cross-compile for macOS using:
```bash
# clang --version
Ubuntu clang version 18.1.3 (1)
make -C depends HOST=arm64-apple-darwin FORCE_USE_SYSTEM_CLANG=1
./autogen.sh
CONFIG_SITE=/path/to/depends/arm64-apple-darwin/share/config.site
...
(https://github.com/bitcoin/bitcoin/pull/30201)
Remove `FORCE_USE_SYSTEM_CLANG` in favour of always using the system Clang and lld for macOS cross-compilation; rather than downloading precompiled blobs.
For example, anyone using Ubuntu 24.04 should be able to `apt install clang llvm lld .. etc`, and then cross-compile for macOS using:
```bash
# clang --version
Ubuntu clang version 18.1.3 (1)
make -C depends HOST=arm64-apple-darwin FORCE_USE_SYSTEM_CLANG=1
./autogen.sh
CONFIG_SITE=/path/to/depends/arm64-apple-darwin/share/config.site
...
📝 vasild opened a pull request: "netbase: extend CreateSock() to support creating arbitrary sockets"
(https://github.com/bitcoin/bitcoin/pull/30202)
Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol.
The need for this came up during the discussion in https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1618837102
(https://github.com/bitcoin/bitcoin/pull/30202)
Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol.
The need for this came up during the discussion in https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1618837102
💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620744428)
Opened https://github.com/bitcoin/bitcoin/pull/30202 "netbase: extend CreateSock() to support creating arbitrary sockets"
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620744428)
Opened https://github.com/bitcoin/bitcoin/pull/30202 "netbase: extend CreateSock() to support creating arbitrary sockets"
💬 TheCharlatan commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2139598389)
Post-merge ACK e8c25e8a35e3
```
uname -a && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
Linux starfive 5.15.0-starfive #1 SMP Sun Mar 26 12:29:48 EDT 2023 riscv64 GNU/Linux
00becde2dd12878e3b9f50f27899a6a8b752343dade7c71781632715c3001473 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/SHA256SUMS.part
a685b9cee54014e74639be1e8db2d55b7c008fdb3b31c1c708c364a49b56759a guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/bi
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2139598389)
Post-merge ACK e8c25e8a35e3
```
uname -a && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
Linux starfive 5.15.0-starfive #1 SMP Sun Mar 26 12:29:48 EDT 2023 riscv64 GNU/Linux
00becde2dd12878e3b9f50f27899a6a8b752343dade7c71781632715c3001473 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/SHA256SUMS.part
a685b9cee54014e74639be1e8db2d55b7c008fdb3b31c1c708c364a49b56759a guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/bi
...
💬 laanwj commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2139605484)
Concept and code review (but untested, will use this to make a testing harness for #30043) ACK 77e34ded543e19ba27cab8d0cfc464af27f04bbf
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2139605484)
Concept and code review (but untested, will use this to make a testing harness for #30043) ACK 77e34ded543e19ba27cab8d0cfc464af27f04bbf
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620771700)
That depends on `T`'s move constructor. Typically, this won't do anything, but it's not impossible for a `T::T(const T&&)` constructor to exist (and this may make sense if `T` has `mutable` member variables).
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620771700)
That depends on `T`'s move constructor. Typically, this won't do anything, but it's not impossible for a `T::T(const T&&)` constructor to exist (and this may make sense if `T` has `mutable` member variables).
💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1620771908)
Is it though? If the user has not specified that he wants no automatic connections (`-maxconnections != 0`) I'd argue he'd expect them to happen.
In any case, I think this is such an edge case that it is not worth blocking the PR on it. If you have a strong opinion on this, I'm happy to drop it and squash the rest of the commit to one of the previous ones
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1620771908)
Is it though? If the user has not specified that he wants no automatic connections (`-maxconnections != 0`) I'd argue he'd expect them to happen.
In any case, I think this is such an edge case that it is not worth blocking the PR on it. If you have a strong opinion on this, I'm happy to drop it and squash the rest of the commit to one of the previous ones
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620773911)
Actually, no, you're right. Even if this does anything, it won't be the desired behavior. Will remove.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620773911)
Actually, no, you're right. Even if this does anything, it won't be the desired behavior. Will remove.
💬 maflcko commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#discussion_r1620825787)
nit: Could make sense which version is "recommended" or "tested". I presume the version used in CI? If so, could refer to that.
(https://github.com/bitcoin/bitcoin/pull/30201#discussion_r1620825787)
nit: Could make sense which version is "recommended" or "tested". I presume the version used in CI? If so, could refer to that.
🚀 fanquake merged a pull request: "clang-tidy: Add `bugprone-move-forwarding-reference` check"
(https://github.com/bitcoin/bitcoin/pull/30199)
(https://github.com/bitcoin/bitcoin/pull/30199)
📝 BrandonOdiwuor opened a pull request: "Enhance signet chain configuration in bitcoin.conf"
(https://github.com/bitcoin/bitcoin/pull/30203)
## !! Early draft for feedback on approach !!
Follow up to https://github.com/bitcoin/bitcoin/pull/29838 following https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2063151580 and https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2070170967
This PR builds upon the changes introduced in https://github.com/bitcoin/bitcoin/pull/29838, which enabled the ability to run multiple Signet instances with distinct data directories. Now, it extends this functionality by allowing user
...
(https://github.com/bitcoin/bitcoin/pull/30203)
## !! Early draft for feedback on approach !!
Follow up to https://github.com/bitcoin/bitcoin/pull/29838 following https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2063151580 and https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2070170967
This PR builds upon the changes introduced in https://github.com/bitcoin/bitcoin/pull/29838, which enabled the ability to run multiple Signet instances with distinct data directories. Now, it extends this functionality by allowing user
...