💬 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)
```

💬 mzumsande commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3272066604)
> Concept ACK, but I think unconditional logging in the ctor is not the right approach, suggested alternative:

I like the move of the Sqlite log - it seems suficient to log the sqlite version just once instead for each wallet load.

In the latest push I just removed the second log:

I doubt that logging the full path in the success case adds much, while creating anonymization efforts in case of sharing logs publicly. The wallet name is already being logged multiple times:

```
2025-09
...

📝 sipa opened a pull request: "txgraph: use enum Level instead of bool main_only"
(https://github.com/bitcoin/bitcoin/pull/33354)
Part of #30289. Inspired by https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331387778.

Since there has been more than one case in the development of #28676 of calling a `TxGraph` function without correctly setting the `bool main_only` argument that many of its interface functions have, make these mandatory and explicit, using an `enum class Level`.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2334636695)
See https://github.com/bitcoin/bitcoin/pull/33354.

💬 achow101 commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2334647787)
Yes, the effect it has is on the named parameter. `rollback` cannot actually be passed as a positional parameter since it is actually part of the options object which would have to be passed in position 2. However, if `rollback` is passed by name, then we need to apply the conversion.

Order does matter here in that the `options` parameter needs to come first so that the positional is not interpreted as a string.