💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166469077)
> The internal keys

I assume it's referring to the taproot internal key(s).

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166549418)
I see that I had misunderstood your previous comment, this^ looks fine to me.

> The Sign algorithm must not be executed twice with the same secnonce. Otherwise, it is possible to extract the secret signing key from the two partial signatures output by the two executions of Sign.

This^ is the only precaution I read in BIP 327 related to reusing, and I don't think anything in the above comment goes against this precaution. Specially since using the same partial sig doesn't translate to signi
...

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