💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334524197)
I think it is useful to to exercise the change address of `send`.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334524489)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334525262)
Fixing that is orthogonal to this PR

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334526025)
Updated the name and changed this to use a regex that matches only on a 2 index multipath.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334526200)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334526627)
Updated to `expected_key_leaves`

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334526817)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334527364)
It's not necessary to check for the warning.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334528185)
I don't think refactoring like this is helpful as the resulting functions won't be called by anything else anyways.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334528560)
I don't think this needs to be more verbose.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334529325)
The behavior is simple enough that I don't think a separate function will make this better.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334529748)
Changed to figure out how many wallets are needed automatically.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334530009)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2334530225)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3271964809)
> Is is supposed to say "... partial signatures could not be created ..." instead? Based on the tone in the message used.

Yes, done.

💬 BrandonOdiwuor commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2334542180)
Concept ACK

I am however unable to trigger the fatal error on `Ubuntu 24.04.3 LTS` with `Python 3.9.23`

Adding a few debug logs show that `Python3::Interpreter` target is set even if the minimum version is not met
<img width="1512" height="916" alt="Screenshot 2025-09-09 at 22 05 38" src="https://github.com/user-attachments/assets/11db8bc0-bccc-43c1-9717-b079e15ddff5" />

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3271990036)
reACK 794a17186d3019713d29213bedd866baa1c81378 - the only change since last ACK was the span nits being applied

💬 hodlinator commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2334562200)
Hasn't become intuitive to me yet that the first parameter to `subspan()` is an offset and not a size, but appreciate not having to repeat the identifier. Taken in latest push, including the other transform of the same kind.

Thanks for the review!

👍 brunoerg approved a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3202996953)
ACK fe06b92429206aee66d33c843195235f4bb27181 - This new approach of just warning is better than the previous one, nice! I haven't reviewed the code in depth, just did a light code review but tested the behavior in practice and worked fine.

Also, I ran a mutation analysis on these changes and the only uncaught mutant is:
```diff
// Traverse miniscript tree for unsafe use of older()
miniscript::ForEachNode(*m_node, [&](const miniscript::Node<uint32_t>& node) {
-
...

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2334579755)
yeah, that confused me as well - if you need to touch again, we could add a [name hint](https://en.cppreference.com/w/cpp/container/span/subspan) to it:
```C++
std::span{first_chain}.subspan(/*offset=*/1)
```