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

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2313212435)
The two comments are saying the same thing, I'd suggest to just keep one

💬 maflcko commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241343804)
ctest doesn't have a default timeout, so it would be a bit odd to expose users to a unit test run that never finishes, albeit rare?

> this issue was more artificial, happening as a result of the way the test was written.


This feature is experimental anyway, so maybe the unit test could be rewritten or removed temporarily for the 30.x release branch, if fixing it is too invasive for now?

💬 Sjors commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241353079)
Or we could add a timeout for this specific test. I don't think we should remove it, because we want to catch unknown issues on platforms / circumstances that our CI doesn't cover.

💬 maflcko commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241422055)
> add a timeout

Sure, but I'd say the timeout should be added in the C++ code, not in ctest, possibly with an error message explaining the known issue.