💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620553530)
Done in the topmost commit in https://github.com/vasild/bitcoin/commits/extend_CreateSock/.
Separate PR or include in this PR?

🤔 rkrux reviewed a pull request: "test: improve BDB parser (handle internal/overflow pages, support all page sizes)"
(https://github.com/bitcoin/bitcoin/pull/30125#pullrequestreview-2087749494)
Incomplete review and untested yet, left couple comments off the top of my head atm.
Will do another round thoroughly in the next few days.

💬 rkrux commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1620352306)
WDYT about creating a constant tuple for these values? They seem pretty generic enough.

💬 rkrux commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1620344538)
There's a separate effort from @maflcko regarding removing the usage of `struct.pack` from all places in test. Though here it's `struct.unpack` but I guess the reasoning for the removal of `struct.pack` is still applicable.
https://github.com/bitcoin/bitcoin/pull/29401

📝 hebasto opened a pull request: "clang-tidy: Add `bugprone-move-forwarding-reference` check"
(https://github.com/bitcoin/bitcoin/pull/30199)
This PR adds [`bugprone-move-forwarding-reference`](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html) to the clang-tidy checks.

💬 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-2139456069)
Guix Build (aarch64):
```bash
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-0a3631fc352e-aarch64-linux-gnu.tar.gz
708374
...

📝 Sjors opened a pull request: "Introduce Miner interface"
(https://github.com/bitcoin/bitcoin/pull/30200)
Introduce a `Miner` interface for the `getblocktemplate` RPC to use now, and for Stratum v2 to use later.

Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652

💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2139459750)
The commit is all the boilerplate plus a single method `isTestChain()` that's used by the `getblocktemplate` RPC.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2139460402)
@ryanofsky let's continue the interface design in #30200.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620636660)
This PR is already quite large, so let's make a fresh one that this can rebase on if needed.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2139467569)
With a bridged network it indeed works.

Also, as suggested by @vasild, doing `kldload /boot/kernel/netlink.ko` makes things work on FreeBSD 13.2.

💬 sipa commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(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.

💬 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.

💬 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.

💬 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.

💬 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/
...

💬 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`.

💬 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?

💬 maflcko commented on pull request "clang-tidy: Add `bugprone-move-forwarding-reference` check":
(https://github.com/bitcoin/bitcoin/pull/30199#issuecomment-2139581156)
utACK 88cdb5967f093cf96e9184a48c0d9e34cea9d341