💬 achow101 commented on issue "Old wallet support (Berkeley 4.8)":
(https://github.com/bitcoin/bitcoin/issues/33273#issuecomment-3241043087)
A migration tool will exist in Bitcoin Core for probably perpetuity. This tool is accessible on CLI with the `migratewallet` RPC and from the GUI with File > Migrate Wallet. When attempting to load a legacy wallet, users will be given specific error messages that inform them how to perform the migration.

💬 maflcko commented on pull request "test: add logging to mock external signers":
(https://github.com/bitcoin/bitcoin/pull/32928#issuecomment-3241184822)
> This will hopefully aid in debugging #32855.

For reference, the issue was something else, and has been fixed now, in the meantime.

💬 Sjors commented on pull request "wallet: relax external_signer flag constraints":
(https://github.com/bitcoin/bitcoin/pull/33112#discussion_r2313128076)
Brought it back.

💬 Sjors commented on pull request "wallet: relax external_signer flag constraints":
(https://github.com/bitcoin/bitcoin/pull/33112#issuecomment-3241199456)
Rebased and addressed nits.

💬 maflcko commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2313132654)
No strong opinion on the confusion. Happy to review a follow-up, or separate pull, or push a commit here, but I'll hold back on this for now, to not invalidate 2 acks.

💬 maflcko commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2313132857)
I guess the nodes aren't connected, so they don't use the p2p ports and they can be re-used here. No strong opinion, but this should be a separate commit with proper documentation.

💬 hodlinator commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2313160387)
> especially since many of those aren't counting declarations, e.g ...

That is precisely one of the reasons why running clang-format with the two different values on the entire src/ and comparing the relative diff size is helpful. Lines such as the ones you quoted will not contribute to the relative difference in the size of the change.

---

> That's not how we usually decide outcomes, rather by better explanations -

I try to present explanations, but then you respond with:

> That
...

🤔 maflcko reviewed a pull request: "fuzz: enhance wallet_fees by mocking mempool stuff"
(https://github.com/bitcoin/bitcoin/pull/33210#pullrequestreview-3172471199)
re-ACK dd4ff4d1ddd15213dd21413a39cff049a21b0fab 🔟

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK dd4ff4d1ddd15213dd21
...

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2313144770)
nit in 5c195cee21e821ba85f4e0064be6d82037bdf73e: Unrelated change? Either revert, or add a trailing comma for the last field?

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2313159186)
nit in 7d9443e504f65d861c76550ad927796e083d20dd: When passing a mutable reference, you can just use `Type& ref`. `const std::unique_ptr<Type>& ref` is similar, but more verbose and confusing.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2313164738)
nit in dd4ff4d1ddd15213dd21413a39cff049a21b0fab: Could insert `{}` around the if body for multi-line if?

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3241264685)
I switched to emitting a warning.

I'll look into modifying miniscript / descriptor classes to avoid the string parsing.

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2313175225)
I dropped this option, so probably no longer needed to document it? I expect it's quite rare for people to even encounter the warning.

💬 maflcko commented on pull request "coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation":
(https://github.com/bitcoin/bitcoin/pull/31449#discussion_r2313188865)
isn't the serialized format using varint compression anyway, so any hardcoded overhead will be wrong here anyway?

I'd say it is fine to leave this as-is, because both this pull and current master are equally "wrong" and in the presence of doubt, it seems better to not change the code?

💬 Sjors commented on pull request "test: add logging to mock external signers":
(https://github.com/bitcoin/bitcoin/pull/32928#issuecomment-3241304440)
@maflcko updated the PR description. I assume more logging is still useful though.

💬 Sjors commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241320504)
Now that the subtree was updated with #33241 and most cases are fixed, do we still want to fix the more rare https://github.com/bitcoin-core/libmultiprocess/issues/189 for the v30 milestone?

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3241325460)
Only change is rebase.

re-ACK a4fa7b97aa73b177ea416388acfc2aba25c79e19 🚇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: r
...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2313212375)
seems fine to upstream this, but this shouldn't be a blocker, i'd say

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2313210541)
```suggestion
Assume(req.indexes[i] > req.indexes[i-1]);
```

Just so the assertion isn't active in release builds

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2313213195)
There's only one invariant we're checking, perhaps we don't need the function?