💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3004555912)
@glozow

> Alternatively, we could add a `set` of reconsiderable wtxids

I think this is the best option. An extra index does not seem worth it, as it's 3 pointers extra per announcement, while a set of reconsiderable wtxids is just 3-4 pointers per reconsiderable wtxid. Another alternative is making the `ByWtxid` index be `(Wtxid, bool, NodeId)`, but that means that some queries (those which want to check for a specific Wtxid/NodeId announcement) need two lookups. The separate set seems mo
...

💬 fanquake commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-3004606908)
Should get a release note, and likely also going to be backported to `29.x`.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166635000)
`100'000` isn't sufficient for my machine with an empty destructor. But `200'000` was for both, decreased.

💬 rkrux commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#issuecomment-3004688762)
Thanks for highlighting.
I'll take a look at #31423 soon, it had fallen off my radar for some reason.

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

💬 fanquake commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2166689603)
> If enough participants have joined the effort (5 or more is recommended)

Just noting that of the last three additions to the repo (https://github.com/asmap/asmap-data/pull/23, https://github.com/asmap/asmap-data/pull/21, https://github.com/asmap/asmap-data/pull/19), it seems like none of them reached 5: I can see 3, 3, and 3 participants.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166642150)
* Reduced indentation in commit message.
* Added validation of the `ScriptSize()` to the test which is a value that depends on child nodes.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166653825)
Regarding naming of accessors: There is some precedent in `Node::IsValid()`. But I also prefer it without prefixes - updated.

Regarding `noexcept`: The compiler should be able to infer `noexcept` by itself for simple accessors, so would rather avoid that noise.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166650264)
I prefer leaving up to the client of the interface to choose which type they want to switch to.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166647841)
Done.

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