💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2331961628)
Well. At the least i still need to port it to the new build system (and rebase and squash). There are likely some other smaller comments and nits here and there that i can easily address, but it's kind of become a mess (no one's fault but github and how it handles PRs with lots of comments), so need to take some time to sort it out.

💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2331965165)
Updated b14d87085fca777b1d14ff051d31d41932597f06 -> 3ae35b427fe59bc9ab24d07c1adb46faa702de20 ([`pr/weakcheck.10`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.10) -> [`pr/weakcheck.11`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/weakcheck.10..pr/weakcheck.11)) simplifying by dropping `cd` since cmake --build ignores the current directory.

👍 maflcko approved a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822#pullrequestreview-2283372310)
lgtm

The alternative https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298 seems fine as well.

💬 maflcko commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#discussion_r1745732446)
Comment indent seems off compared to the other cmake code? I guess there is no `.cmake-format.yml`, so feel free to ignore.

👍 TheCharlatan approved a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2283380659)
Re-ACK 3ae35b427fe59bc9ab24d07c1adb46faa702de20

👍 hebasto approved a pull request: "build: work around issue with Boost <= 1.80 and Clang >= 18"
(https://github.com/bitcoin/bitcoin/pull/30821#pullrequestreview-2283387713)
ACK cd062d6684908d526be7423f8f1057b891254a3c, tested on Fedora 40 using the downloaded [Boost 1.74](https://archives.boost.io/release/1.74.0/source/) and commands as follows:
```
$ cmake -B build -DCMAKE_C_COMPILER=clang-18 -DCMAKE_CXX_COMPILER=clang++-18 -DBoost_INCLUDE_DIR=/home/hebasto/boost_1_74_0
$ cmake --build build -t test_bitcoin
```

💬 fanquake commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#discussion_r1745742588)
Thanks, fixed the comments.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745745593)
never actually reachable as this is already caught in `IsStandardTx`, removed this error condition

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745745724)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745745966)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746064)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746181)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746321)
added an optional sync argument, only unused during reorg test

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746418)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746514)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746680)
great idea, added

👍 Sjors approved a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2282985518)
ACK e59097a0a56ecaffdb38aaa94e232f5b45881897

You can see it in action in `sv2_connman_tests` in https://github.com/Sjors/bitcoin/pull/50.

I didn't test the `CNetMessage` functionality and `Eof()` method.

Maybe split 187ba684a9d5e63d09b43511b7128cce9edbedf3 into one commit that moves the implementation and another that splits the class.

💬 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#discussion_r1745516413)
187ba684a9d5e63d09b43511b7128cce9edbedf3: shouldn't `Sock{INVALID_SOCKET}` be preserved in the new `ZeroSock` constructor?

💬 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#discussion_r1745496410)
187ba684a9d5e63d09b43511b7128cce9edbedf3 any particular reason for moving this?

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746765)
done