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

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