💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2605428948)
Forgot to fix a test, force-pushed doing it. Now CI is happy!

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1924184950)
> Is that redundant after [34c33a7](https://github.com/bitcoin/bitcoin/commit/34c33a747128e1c26b1931ac3fa0c4ed6b6a7bb7)?

No. There are other non-PSBT callers of `ProduceSignature` which may still pass `SIGHASH_DEFAULT` to legacy input types, notably `signrawtranssaction`.

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1924194259)
Done

💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r1924194604)
Added a check for the size.

💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2605456917)
Will add decoding test vectors.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924202539)
Should, because loading a legacy wallet will add the P2WPKH script to `mapScripts` for all keys, regardless of pre or post segwit. This is done in memory only, on loading.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924211960)
I believe the only way is through `importmulti`.

Versions without miniscript cannot sign for miniscript. I feel like that's obvious and we don't need a test for that.

💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605486813)
Approach ACK

IMO the `check_` prefix on the benchmark file name is a bit odd, just `connectblock.cpp` should be fine.

And there is quite a bit of duplication in the file benchmarks file, I played around with it a bit and I would suggest something like this: https://github.com/fjahr/bitcoin/commit/12eab5df067915ed83f91bf0777fcd0189f02b85 feel free to use it.

And I think you can take this out of draft status :)

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924212782)
Miniscript is a subset of script, so this sentence is correct.

🤔 instagibbs reviewed a pull request: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666#pullrequestreview-2565394877)
a couple documentation nits only

💬 instagibbs commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1924193994)
documentation should note that the list is appended to

💬 instagibbs commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1924186528)
they are added as new invs IFF they are deemed good candidates for resolution

👍 darosior approved a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2565484357)
re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924251495)
I don't think any part of this comment is relevant to the current code.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924252155)
Done

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924252349)
Expanded the comment a bit.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924252703)
Changed this to also delete the offending script from `spks`.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924253038)
Reverted

👍 darosior approved a pull request: "doc: Amend notes on benchmarking"
(https://github.com/bitcoin/bitcoin/pull/31690#pullrequestreview-2565507923)
ACK 2db6923332c7daa20d250cc5aa6bde080a7d0caf

I hear others' criticisms but i think it's an improvement over the current doc. We can iterate in follow-ups on how best to phrase these general guidelines.

👍 hodlinator approved a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2565511430)
re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc :rocket: