💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088714491)
`m_node_mutex` is a recursive mutex, allowing multiple nested locks from a single thread. But see https://github.com/bitcoin/bitcoin/pull/32326 which changes it to a non-recursive mutex which is easier to reason about.

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2879829734)
`8b93e0894f...bd6c57f387`: address suggestions

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088718421)
Because the only caller of `GenerateWaitSockets()` has a vector so I decided to pass a const reference to a vector. However, that was the case as well before this PR and the change from raw pointer to `shared_ptr` (the aim of this PR) is unrelated to that. Changed back to `std::span`.

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088722206)
Yes, I meant that the location of the deletes is unchanged. Before we had `DeleteNode()` which did `FinalizeNode()` + `delete` and after that commit, `DeleteNode()` is expanded into the callers that do `FinalizeNode()` directly instead of calling `DeleteNode()` + `clear()` on the vector which is equivalent to the `delete` that was in `DeleteNode()`.

Elaborated a little bit in the commit message.

💬 maflcko commented on pull request "test: add skip_if_running_under_valgrind()":
(https://github.com/bitcoin/bitcoin/pull/32492#issuecomment-2879855860)
lgtm ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6

I haven't tested or confirmed this, but the diff looks plausible.

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088731770)
I did not want to bloat this PR with changes to the interface of `ProcessMessages()` and `SendMessages()`. They take a raw pointer. Since they do not take ownership and the object is going to be alive for the duration of the call and there will always be an object (will never pass `nullptr`), then those functions can take `CNode&`.

Here is a patch to do that on top of this PR. Could be a followup. I do not want to bloat this PR with this unless reviewers want it.

<details>
<summary>[patch
...

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2879863257)
`bd6c57f387...20657a7c8e`: rebase

⚠️ fanquake opened an issue: "test: consider Clang `type` sanitizer"
(https://github.com/bitcoin/bitcoin/issues/32495)
Clang 20 introduced an experimental type sanitizer that "detects violations of C/C++ type-based aliasing rules." See here for more info: https://clang.llvm.org/docs/TypeSanitizer.html. Someone might want to run this against our code, to see if it works, or detects real issues. This could likely be done with:
```bash
make -C depends/ CC=clang CXX=clang++ CXXFLAGS="-stdlib=libc++" NM=llvm-nm AR=llvm-ar RANLIB=llvm-ranlib STRIP=llvm-strip
cmake -B build --toolchain /bitcoin/depends/x86_64-pc-linux-
...

💬 maflcko commented on pull request "test: fix two intermittent failures in wallet_basic.py":
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2879883748)
review ACK e7ad86e1ca3b0b2f2795e91c2f9959486c67dd90 🎩

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

📝 fanquake opened a pull request: "depends: add Qt `-ltcg` for Darwin, drop it for Windows"
(https://github.com/bitcoin/bitcoin/pull/32496)
The related Windows patches were dropped in 5e794e62024eef612e1fbb71c76ea54d17435c14, and "Cross-compiling does not support LTO." (from #30997).

Using this with native Darwin builds works ok.

💬 luke-jr commented on pull request "qt: drop unused watch-only functionality":
(https://github.com/bitcoin/bitcoin/pull/32459#issuecomment-2879908057)
Shouldn't this be on the GUI repo?

💬 luke-jr commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2879919952)
>From what I can tell, compilers on Linux systems, will be defining _GNU_SOURCE

`_GNU_SOURCE` isn't supposed to ever be defined by default, and I don't see us defining it anywhere...?

💬 maflcko commented on issue "test: wallet_reorgsrestore.py failure under valgrind":
(https://github.com/bitcoin/bitcoin/issues/32493#issuecomment-2879940657)
Does it always happen, or did this start at some point in the git history?

👍 laanwj approved a pull request: "cmake: Add application manifests when cross-compiling for Windows"
(https://github.com/bitcoin/bitcoin/pull/32396#pullrequestreview-2839952072)
re-ACK c0f41523973e33a8cd104a8ebf4906a4d99f3eed

💬 fanquake commented on pull request "test: fix two intermittent failures in wallet_basic.py":
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2880021915)
@achow101 note that you've ACK'd a commit has not in this PR, but for an equivalent change.

✅ fanquake closed an issue: "failure in wallet_basic.py --descriptors"
(https://github.com/bitcoin/bitcoin/issues/27249)

✅ fanquake closed an issue: "qa: Failure in wallet_basic.py spendzeroconfchange test"
(https://github.com/bitcoin/bitcoin/issues/32456)

🚀 fanquake merged a pull request: "test: fix two intermittent failures in wallet_basic.py"
(https://github.com/bitcoin/bitcoin/pull/32483)

💬 l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088818525)
Nice, I like that a lot more - addressed the reformatting and unification in separate commits, let me know if you think this is better.
Pushed a rebase separately to simplify review from GitHub as well (I know you do range-diff), and to make sure the latest CI is run against it.

💬 fanquake commented on pull request "test: fix two intermittent failures in wallet_basic.py":
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2880037334)
Backported to `29.x` in #32292.