📝 maflcko opened a pull request: "ci: Add missing -Wno-error=array-bounds to valgrind fuzz"
(https://github.com/bitcoin/bitcoin/pull/32325)
Due to an upstream GCC issue, any debug/fuzz build which aborts on failed assumes will print a false positive array-bounds warning in `src/test/fuzz/txgraph.cpp`.

This also affects one CI task.

Fix the CI task by ignoring the error for now.

Fixes https://github.com/bitcoin/bitcoin/issues/32276

🤔 furszy reviewed a pull request: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2784336700)
utACK cad39f86fb5a81

> Sounds like this should be an actual test then instead of a benchmark, no?

I'd keep it as is for now and drive all our focus towards #31250. It would be nice to cut down the tree first - we can pick up any leftover later.

💬 maflcko commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2821541818)
I've written a tool to check translations: https://github.com/maflcko/DrahtBot/tree/main/check_translations

This finds the above issues, as well as one more:

```
$ cargo run -- --llm-api-key hi --translation-file /path/to/src/qt/locale/bitcoin_pl.ts --cache-dir cache


Erroneous translation:

<source>Create Wallet</source>
<translation type="unfinished">Stwórz potrfel</translation>

Erroneous translation:

<source>%1 (%2 blocks)</source>
<translation type="
...

📝 vasild opened a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326)
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.

Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the
...

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2054291997)
Thanks for the input, I respect and agree with most of it. The only bit I do not agree with is a general rule to avoid `shared_ptr`. I agree using `unique_ptr` is better than `shared_ptr`, especially if `unique_ptr` is to be used as intented - to manage the lifetime of the owned object. If we start extracting the raw pointers from `unique_ptr` and managing them separately and manually ref-counting then maybe it is better to just use `shared_ptr`.

> FindNode() returns shared_ptr, this can be c
...

💬 maflcko commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2054294507)
style nit in 1da11dbc441790773502ffd5f60dc05191514a83: It would be good to use clang-format. This would reduce the whitespace churn of this commit.

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