💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166695337)
Think it's better as is, documenting the properties of the Node inside the optional.

💬 vasild commented on pull request "depends: Build `qt` package for FreeBSD hosts":
(https://github.com/bitcoin/bitcoin/pull/32731#discussion_r2166737725)
I tested the latest version of this PR (173394d951). It compiles successfully with `gmake` and `cmake --toolchain`. And this is the result:

```
$ ldd ./build/bin/bitcoin-qt
./build/bin/bitcoin-qt:
libfontconfig.so.1 => not found (0)
libfreetype.so.6 => not found (0)
libxkbcommon.so.0 => not found (0)
libxkbcommon-x11.so.0 => not found (0)
libxcb-xkb.so.1 => not found (0)
libxcb.so.1 => not found (0)
libxcb-cursor.so.0 => not found (0)
libxcb-icccm.so.4 => not found (0)
lib
...

💬 sipa commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3004818271)
ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf

🤔 janb84 reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2958218335)
ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675

PR makes a step to remove the dependency on libevent, this pr removes the dependency in the wallet re-locking functionality.

(I do agree with NIT comments above and hope to see https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2165986820 included)

- code review ✅
- ran test ✅

💬 purpleKarrot commented on pull request "cmake: Use `HINTS` instead of `PATHS` in `find_*` commands":
(https://github.com/bitcoin/bitcoin/pull/32805#issuecomment-3004844018)
utACK

Nit: Is there a problem on master? It looks like the result from `libqrencode.pc` is ignored, but if that is not causing actual problems, maybe `pkg_check_modules` could be dropped from `FindQRencode.cmake`?
Also, what platforms actually provide the `qrencoded` library in the Debug configuration?

👍 vasild approved a pull request: "depends: Build `qt` package for FreeBSD hosts"
(https://github.com/bitcoin/bitcoin/pull/32731#pullrequestreview-2958316212)
ACK 173394d9511ed091e73eb12cb28f819026c09576

Links against some dynamic libraries from `./depends/`, but that is also on Linux: https://github.com/bitcoin/bitcoin/pull/32731#discussion_r2166737725

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2166835523)
Done. Please stop bullying my poor English

👍 theStack approved a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2958410039)
re-ACK 5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2 🎹

(as per `$ git range-diff cf6e417...5fe7915`, changes were mostly tackling rkrux' suggestions, which I agree are all improvements 👍 )
micro-nit: not worth it now to further invalidate acks, but fwiw, I'm still not a huge fan of cluttering the `ParseKeyPath{,num}` functions with extra booleans to restrict/detect properties that can be trivially detected after and think the previously suggested approach (`IsKeyPathsHardened` helper) was fine
...

📝 vasild converted_to_draft a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988)
Currently `CConnman` is a mixture of:
* low level socket handling, e.g. send, recv, poll, bind, listen, connect, and
* higher level logic that is specific to the Bitcoin P2P protocol, e.g. V1/V2 transport, choosing which address to connect to, if we manage to connect mark the address good in `AddrMan`, maintaining the number of inbound and outbound connections, banning of peers, interacting with `PeerManager`.

---

This PR splits the socket handling into a new class which makes the code m
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3005059779)
Marked as draft as [suggested](https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2995857066). Will focus on https://github.com/bitcoin/bitcoin/pull/32747, thanks!

👍 willcl-ark approved a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2958504721)
tACK 6c2538d5bfe

Based on `git range-diff 3a350c8a1b5...6c2538d5bfe` (which just picks up recently-merged commits).

Tested that this builds from depends successfully on NixOS.

📝 luke-jr opened a pull request: "Bugfix: Wallet: Skip inaccessible directories rather than abort the wallet list entirely"
(https://github.com/bitcoin/bitcoin/pull/32812)
Followup to #32736 to restore the intended behaviour (absent the detailed logging which appears to not be practical with std::filesystem).

💬 maflcko commented on pull request "Bugfix: Wallet: Skip inaccessible directories rather than abort the wallet list entirely":
(https://github.com/bitcoin/bitcoin/pull/32812#issuecomment-3005196233)
pretty sure the tests are going to fail now? maybe the ci should go back to run in a user account, if possible?

📝 l0rinc opened a pull request: "clang-format: modernize and realign clang-format configuration"
(https://github.com/bitcoin/bitcoin/pull/32813)
Updates `.clang-format` file to reflect modern Clang-Format standards while preserving most of the existing formatting behavior, except for the very last commit which [aligns `AfterStruct` brace placement](https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072678126) with `AfterClass` for code formatting.

* the first few commits modernize the deprecated config keys to help with filtering later;
* followed by the removed redundant/default values via scripted diffs verified via `clang-
...

💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r2167065003)
Because `FinishTransaction` now takes a `CMutableTransaction&` instead of a `const CMutableTransaction&`, we can't give it an rvalue.

🤔 maflcko reviewed a pull request: "clang-format: modernize and realign clang-format configuration"
(https://github.com/bitcoin/bitcoin/pull/32813#pullrequestreview-2958744097)
last commit seems fine, but the others i am not sure. at a minimum you'll have to drop the scripted diffs from executing, because they can't be reproduced anyway with different clang-format versions

💬 maflcko commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167057157)
a666d8ab6f3c019a9f7e184beb9b90affbb8075c: not sure about removing the default values. I don't think there is any harm in having them. In fact, we don't want this to silently change from down under with the next clang update.

💬 maflcko commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167063237)
7dda3d2c0e65ac1fa08ac3b24a13f2005c6bef1a: not sure about changing the values. seems fine to just keep the old value, which will likely work forever in a backward compatible way

👍 vasild approved a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-2958772275)
ACK 3e7abceecfd790bc0887f647d3f731328e19810f

💬 l0rinc commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167075660)
What about the other default values, what if those change?