💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054129498)
Sure, done.

💬 fanquake commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821359451)
I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?

💬 hebasto commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821394743)
> I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?

Was related to LTO?

💬 thesamesam commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821405387)
It's the same problem. It just shows up worse with LTO because Qt's sanity check can't fire to tell you and give you a build failure instead.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2821410082)
Changed the serialization of `Obfuscation` from `vector` -> `array` without re-testing my suggestions towards the end. Forgot that one serializes the size and the other does not. Seems to be responsible for some of the test failures on my suggestion-branch.

💬 maflcko commented on pull request "test: Treat executable paths in tests consistently":
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821488921)
Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.

💬 hebasto commented on pull request "test: Treat executable paths in tests consistently":
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821506859)
> Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.

The question is how to choose the configuration for testing.

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