⚠️ ellenkampguus opened an issue: "Old wallet support (Berkeley 4.8)"
(https://github.com/bitcoin/bitcoin/issues/33273)
### Please describe the feature you'd like to see added.

I can imagine the developer's side of removing support for the old wallets using Berkeley 4.8. However, isn't one of the things people holding Bitcoin want is hold it for a long time? I think it is important to make sure older wallets can just be opened in the default Bitcoin node and wallet software.

This issue relates to "The Bitcoin Devs" not supporting "Bitcoin as it is supposed to be" according to the more purist Bitcoiners, as it s
...

🤔 maflcko reviewed a pull request: "test: Remove polling loop from test_runner (take 2)"
(https://github.com/bitcoin/bitcoin/pull/33141#pullrequestreview-3172171987)
force pushed with small formatting changes. Should be trivial to re-review via `--ignore-all-space`.

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2312988889)
> I can imagine this will result in new test failures we haven't seen before.

I'd doubt that test will fail due to scheduling them more tightly without sleeps in-between. Though, if they do, that is a good thing, because it hints at a bug that we should be aware of and ideally should fix.

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2312989089)
thx, formatted

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2312976795)
thx, kept the type

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2312976260)
Yeah, maybe. Though,

* we can't really use the return code anyway (there is none), so the only difference would be that we have to explicitly shutdown the executor somewhere.
* There is no natural place to put it, because doing it in `get_next` seems confusing. Similarly, doing it before sys.exit seems like a layer violation.
* putting an explicit shutdown somewhere is inconsistent, because the code path isn't hit on exceptions (base exceptions)

In practise it doesn't matter, because if
...

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2312945307)
thx, formatted

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2312989301)
thx, restored original code

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2312977171)
thx, ran formatter

✅ achow101 closed an issue: "Old wallet support (Berkeley 4.8)"
(https://github.com/bitcoin/bitcoin/issues/33273)

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