👍 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
(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
```
(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.
(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
(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
(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
(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
(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
(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
(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
(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
(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
(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.
(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?
(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?
(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
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746765)
done
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746912)
elaborated a bit, let me know if it helps
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746912)
elaborated a bit, let me know if it helps
💬 hebasto commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331991030)
> The alternative [#30787 (comment)](https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298) seems fine as well.
This PR change is minimal.
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331991030)
> The alternative [#30787 (comment)](https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298) seems fine as well.
This PR change is minimal.
👍 hebasto approved a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822#pullrequestreview-2283396791)
re-ACK a7a4e11db8722c85175bcc4d9f73a713e9e61513.
(https://github.com/bitcoin/bitcoin/pull/30822#pullrequestreview-2283396791)
re-ACK a7a4e11db8722c85175bcc4d9f73a713e9e61513.
💬 kevkevinpal commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331995116)
lgtm ACK [a7a4e11](https://github.com/bitcoin/bitcoin/pull/30822/commits/a7a4e11db8722c85175bcc4d9f73a713e9e61513)
did `time cmake -B build` and got the following results
PR30822 ([a7a4e11](https://github.com/bitcoin/bitcoin/pull/30822/commits/a7a4e11db8722c85175bcc4d9f73a713e9e61513))
```
real 0m13.740s
user 0m6.798s
sys 0m4.696s
```
master (d661e2b1b771abafb0b152842d775d3150032230)
```
real 0m25.583s
user 0m17.078s
sys 0m5.127s
```
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331995116)
lgtm ACK [a7a4e11](https://github.com/bitcoin/bitcoin/pull/30822/commits/a7a4e11db8722c85175bcc4d9f73a713e9e61513)
did `time cmake -B build` and got the following results
PR30822 ([a7a4e11](https://github.com/bitcoin/bitcoin/pull/30822/commits/a7a4e11db8722c85175bcc4d9f73a713e9e61513))
```
real 0m13.740s
user 0m6.798s
sys 0m4.696s
```
master (d661e2b1b771abafb0b152842d775d3150032230)
```
real 0m25.583s
user 0m17.078s
sys 0m5.127s
```