💬 maflcko commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2821598466)
review ACK cad39f86fb5a81f0e3b5116e8e989bab8af89718 🍥

<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: review ACK cad39f86fb5a
...

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054309204)
This rename makes obvious that maybe something is wrong in the current code in `master`. Is the intention really to ignore inbound connections when, according to the comment we intent to "Look for an existing connection"?

For example, if `1.1.1.1` connects to `2.2.2.2:8333`, then the TCP connection will look like:
`1.1.1.1:53481 <-> 2.2.2.2:8333` where `53481` is a random port picked by the initiator's OS. Now, if `2.2.2.2` intends to connect to `1.1.1.1:8333` then this check will return fal
...

💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054324859)
In 9e657358271e7a045c, to make the CI happy
```suggestion
const CNetAddr& net_addr{addr};
```

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2054329479)
you mean like this, i.e. one less space for the block?
```C++
bench.epochs(/*numEpochs=*/1).epochIterations(/*numIters=*/1) // run the migration exactly once
.run([&] {
```
What do you mean exactly by "would reduce the whitespace churn of this commit", we still need to indent it (and the methods are aligned this way, I like that more). You can convince me otherwise, but if it's a `nit`, I'd prefer the current one.

Or if you meant
```C++
bench.epochs(/*numEpochs=*/1).epochIterations
...

💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2821655105)
Review ACK 1e415d2cb703fcd1b622e5343fa264425601c8d modulo CI fixup

Happy to see these methods see an update.

I tried adding `EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex)` to the declarations, but it looks like that lock is already held by some of their callers.

💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054336652)
I ought to resuscitate some of https://github.com/bitcoin/bitcoin/pull/28248 that was looking at this (or will review if you do).

💬 mzumsande commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054341299)
I don't know if this is intentional, or a bug or something in betweeen.

If I remember correctly , #28248 suggested to change that, or something very similar.
But changing it is a bit scary to review, because sometimes IP addresses are shared - especially local IPs such as 127.0.0.1. So just changing it to ignore the port would have the potential to make local setups that probably exist out there not work any longer.

💬 maflcko commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2054357836)
It is just a nit. I am just saying that for new (or touched) code, it would be better to use clang-format, according to https://github.com/bitcoin/bitcoin/blob/96a5cd8000d2b500585834277cc5f5e23181a6f3/doc/developer-notes.md?plain=1#L66. If there is an issue with clang-format, it would be good to fix it. Using clang-format consistently makes it easier when touching code in the future again.

💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054365166)
Yes, I see review feedback on that in https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789702441 and https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1370747413.

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2821716161)
`1e415d2cb7...cc8f239663`: fix CI tidy

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054370924)
Fixed, thanks!

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054383490)
Love how you deciphered this at an instant! :heart:

I will look into adding a comment to the call sites about this.

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2054400875)
I'm all for consistency - but is this not what you're getting when formatting the latest commit (except for that 1 space difference I mentioned above)?
If I need to touch it again, I'll put the run to the previous line to minimize the diff (though I consider this to be slightly more readable).

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054408043)
Hmm, in `CConnman::OpenNetworkConnection()` the code does:

* if `pszDest` is not set then use `AlreadyConnectedToAddress(addrConnect)` which ignores the port
* if `pszDest` is set then `OutboundConnectedToStr(pszDest)` which compares the port as well.

Changing that logic is, I guess, out of the scope of this non-functional refactor PR. If to be done it would better be assessed in isolation in its own PR.

📝 maflcko opened a pull request: "test: Add missing check for empty stderr in util tester"
(https://github.com/bitcoin/bitcoin/pull/32327)
Now that wine support was removed from the CI in 25b56fd9b469f8e5d36f0132c3b79a5214e3372a, it can probably be removed from the util tester as well.

If someone really needs this, they can comment the new check out, or submit a patch to add an option/env var to silence the new check.

👍 fanquake approved a pull request: "ci: Add missing -Wno-error=array-bounds to valgrind fuzz"
(https://github.com/bitcoin/bitcoin/pull/32325#pullrequestreview-2784608357)
ACK fa653cb4169f03e4f327b18a1ff103d72dcc61f7 - checked native fuzz

✅ fanquake closed an issue: "ci: fuzz_with_valgrind job broken"
(https://github.com/bitcoin/bitcoin/issues/32276)

🚀 fanquake merged a pull request: "ci: Add missing -Wno-error=array-bounds to valgrind fuzz"
(https://github.com/bitcoin/bitcoin/pull/32325)

✅ fanquake closed a pull request: "tests: Improve stderr validation in test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/32183)

💬 fanquake commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/32183#issuecomment-2821799863)
Closing in favour of #32327.