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

💬 davidgumberg commented on pull request "Avoid pathological QT text/markdown behavior...":
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3272271535)
CI Failure is unrelated to this PR: https://github.com/bitcoin/bitcoin/issues/33345

🤔 l0rinc requested changes to a pull request: "node: optimize CBlockIndexWorkComparator"
(https://github.com/bitcoin/bitcoin/pull/33334#pullrequestreview-3202982209)
Concept ACK - please consider my suggestion

💬 l0rinc commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#discussion_r2334837666)
do these need to stay pointers (in which case we should hanlde `nullptr`) ir

💬 l0rinc commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#discussion_r2334808680)
All this does is compare by 3 parameters - wouldn't it be simpler to use `std::tie` for lexicographical comparison instead?
```suggestion
bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const
{
// First sort by most total work (descending), then by earliest activatable time (ascending), then by pointer value (ascending)
return std::tie(pa->nChainWork, pb->nSequenceId, pb) < std::tie(pb->nChainWork, pa->nSequenceId, pa);
}
```

*Note that `pa` & `p
...

💬 achow101 commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#issuecomment-3272332928)
ACK d3c5e47391e2f158001e3e199d625852c7f18998

💬 stickies-v commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2334869398)
That's interesting. The https://cmake.org/cmake/help/v3.14/module/FindPython.html docs for `Python::Interpreter` state "Target defined if component Interpreter is found.", so I'm not sure why that's not failing for you.

Since the behaviour of relying on `Python3::Interpreter` is not changed in this PR, I suspect you won't get the "Minimum required Python not found." warning (at the very end of the configure step) that's removed in this PR, since that relies on the same mechanism? Correct?


...

🚀 achow101 merged a pull request: "wallet, refactor: Remove Legacy check and error"
(https://github.com/bitcoin/bitcoin/pull/33082)