💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557226776)
Yeah, that looks very good to me, I took this almost exactly as is, squashed the `NUM_WALLETS` in there because these lines are now touched as well and made you co-author of that commit. Thanks a lot!

💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557228453)
It's gone, probably left-over from an earlier version of the code where this was still the case

💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#issuecomment-3572052555)
Addressed the feedback from @rkrux , thanks for the review!

💬 Ataraxia009 commented on pull request "init: Changing the rpcbind argument being ignored to a pop up warning":
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2557263164)
right but is there an actual case where people do that? The only reason you would need to `-rpcallowip` is if you are exposing the rpc server with `-rpcbind`?

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3572110958)
No strong opinion on whether this should be backported. In `sv2-tp` I found it easy to work around by calling `waitNext()` in a loop with a reasonable timeout: https://github.com/stratum-mining/sv2-tp/blob/fbe0984ca21e2b6ae0f554bbc5b2819332e59afc/src/sv2/template_provider.cpp#L131-L144

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3572122250)
reACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38

💬 monlovesmango commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#discussion_r2557293653)
As far as I can tell `descendants` from `getTransactionAncestry` actually finds the parent(/ancestor) with the most descendants and returns that count, not the count of children for `txid` in question.

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2557315230)
Thanks, fixed in https://github.com/bitcoin/bitcoin/commit/38f2f53963e833d25bc71df0713fb28cff17cc43

💬 ryanofsky commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#discussion_r2557363728)
Thanks for the pointers. Just looking at the current code I'm confused about why it isn't doing the right thing. It looks like `native_capnp_type` should be getting set to "build" here:

https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/depends/funcs.mk#L318

And then `native_capnp_cxx` should be getting set to `build_CXX` here:

https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/depends/funcs.mk#L4

And then the `CXX` environm
...

💬 plebhash commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3572260427)
we also have workarounds on https://github.com/stratum-mining/sv2-apps/pull/59 but they're substantially increasing code complexity, which will likely have a negative impact on maintainability on the mid-long term

we have https://github.com/stratum-mining/sv2-apps/issues/81 to keep track of this but we'd really appreciate if this got backported instead of landing on v31+

💬 furszy commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2557392174)
Cool. It would be good to add a test case for it.

💬 Christewart commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3572313467)
> > Hi AJ, are these the test vectors you had in mind?
>
> I was thinking functional tests, to also make sure there isn't some obscure policy rule that the unit tests miss that nevertheless ends up blocking successful spends of things that should be spendable. Unit tests are good too though.

:+1:

> The things that (per this PR's arguments) should be spendable are `1 <pubkey> <invalid> 2 CMS` and `1 <invalid> <pubkey> 2 CMS`.

I took a closer look through `script_tests.json` and it see
...

🤔 mzumsande reviewed a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3501853880)
re-ACK 8343a9ffcc752f77eb2248315d10b6dff4a5c98b

💬 frankomosh commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#discussion_r2557421280)
Thanks for suggestion. Can we keep it this way for now?

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3572366307)
@willcl-ark's [suggestion](https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2556213666) has been taken.

Thank you!

💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2557502178)
@frankomosh should be addressed

💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2557503481)
@optout21 for vision

🤔 l0rinc requested changes to a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3497639202)
Thank you for considering the previous changes.

Something kept bothering me in the usage of sorted sets, how awkward it was to use them as changing priority queues, so I stepped back and checked why we even need such an optimization in the first place, given don't expect to have millions of entries here.

I have included a benchmark in my suggestions to back my opinion with actual data, please consider it, and feel free to add any part to the current PR if you agree that it simplifies the logic
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2557559349)
As mentioned above, this hasher is quite heavy, making this a pessimization:

<img width="823" height="411" alt="Image" src="https://github.com/user-attachments/assets/7b8ca62e-6e99-40fd-ad39-5b81e1b02f1c" />

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556376564)
As the [developer notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes) hint at, this probably isn't fixed in [*16.2*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_2-release-notes) yet, only in [*16.3*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes).

And it seems https://github.com/bitcoin/bitcoin/pull/33932 doesn't change the situation since we're [deliberately downloading](https://git
...