💬 maflcko commented on pull request "suppressions: note that `type:ClassName::MethodName` should be used":
(https://github.com/bitcoin/bitcoin/pull/28147#issuecomment-1789703076)
Not sure why the symbolizer doesn't work on Fedora, but I guess this should be reverted? Steps to reproduce on a fresh install of Fedora 39:


```
dnf install gcc-c++ libtool make autoconf automake python3 clang llvm libcxx-devel libcxxabi-devel git ccache libevent-devel boost-devel -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && ./autogen.sh && ./configure CC=clang CXX=clang++ --with-sanitizers=fuzzer,integer,undefined --enable-fuzz
...

💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789704725)
Reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and described in https://github.com/bitcoin/bitcoin/pull/28248/commits/21f8426c823fd83c40fd81f08ff2bf39ec6a4575, there is an interaction between the new network-specific automatic outbound logic and the addnode list: the former doesn't account for the latter.

```markdown
Notable changes
===============

P2P and network changes
-----------------------

- Nodes with multiple reachable networks will actively try
...

💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789710867)
> Maybe some of the straightforward bugfixes could be split out into another PR?

Yes -- looking at doing that, or dropping here for now any that aren't relatively clear.

💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1379352136)
Yes, I've been running this branch with its logging on for several months, and grepping the log turns up dozens, sometimes hundreds, of connection attempts per day from one or two inbound I2P peers on a given day that are already connected. I'm not sure at the moment of writing whether the behavior is malicious or due to buggy outbound logic (which would only be fixed in future releases and not existing releases connecting to us, though) and am drilling down more, but it seems like a good idea n
...

💬 furszy commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1379378823)
I'm still not sure about #28142 but, if we ever do something like that, we would have an encrypted wallet without encrypted private keys. So, might want to change the "without private keys" to "without any encrypted record".

🤔 stickies-v reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1706454012)
Approach ACK. It's a fairly extensive PR to review, and the changes don't always help with verbosity (e.g. `(!(*Assert(Assert(m_context)->shutdown))())` instead of `StartShutdown`), but it seems like the right direction to go regardless.

I've gone through the code multiple times now and it looks good, so I don't think I'll come up with (m)any more comments, but I'm not fully comfortable yet with all the implications of the changes so I'll have to spend some more time on it before signing off.
...

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1377685958)
This seems a bit brittle and probably better handled by having `LoadBlockIndex()` return e.g. a `util::Result` (especially after https://github.com/bitcoin/bitcoin/pull/25665), but orthogonal to this refactor so I think it's best to leave as is here.

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1377678893)
note for other reviewers (and future self): `NodeImpl::startShutDown()` does have [this additional code block](https://github.com/bitcoin/bitcoin/blob/d51fb9caa622add96c6b594e162da5584eb927fc/src/node/interfaces.cpp#L124-L128), but the `InterruptRPC()` and `StopRPC()` calls are executed only once regardless and I think the ordering remains the same too, so I don't see (but am not at all familiar with the GUI code) how this could cause issues

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1377746899)
nit: missing `#include <util/check.h>`

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1377777492)
nit: long line

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1379378726)
I don't think we need to expose those internal error details. Also, I know we exclude `server.cpp` from the CHECK_NONFATAL linter, but given that this is an RPC method I think it would be preferable here?
```suggestion
CHECK_NONFATAL((*CHECK_NONFATAL(EnsureAnyNodeContext(jsonRequest.context).shutdown))());
```

👍 ryanofsky approved a pull request: "depends: Bump to capnproto-c++-1.0.1"
(https://github.com/bitcoin/bitcoin/pull/28735#pullrequestreview-1709159489)
Code review ACK fa7d8377f7541b0785049c4659dc61bf727bd3f3

💬 ryanofsky commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#discussion_r1379382592)
In commit "depends: Bump to capnproto-c++-1.0.1" (fa7d8377f7541b0785049c4659dc61bf727bd3f3)

I think setting `-Wno-error=deprecated-declarations` would be a little better.

Treating warnings as errors can be helpful since warnings often signal potential bugs. But treating deprecated declarations as errors is not the same, since deprecated declarations in capnproto suggest that the code works fine, but there's a just newer API available.

Not too important, though, since is easy to change l
...

📝 achow101 opened a pull request: "tests: Fix LCOV_OPTS to be in the correct position"
(https://github.com/bitcoin/bitcoin/pull/28771)
`lcov`'s `-a` option takes an argument. With `LCOV_OPTS` immediately after `-a`, the first additional argument becomes the argument to `-a` which is incorrect.

Also add `LCOV_OPTS` to more `lcov` calls.

📝 achow101 opened a pull request: "test: Generate coverage report without running tests"
(https://github.com/bitcoin/bitcoin/pull/28772)
When generating a coverage report, separate the testing from the generation of the coverage report. This is useful when checking the coverage of a small set of tests.

👍 pinheadmz approved a pull request: "Do the SOCKS5 handshake reliably"
(https://github.com/bitcoin/bitcoin/pull/28649#pullrequestreview-1709180046)
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78

Built and ran tests locally. Confirmed the changes since my last ACK are using `Span` instead of `data` and `len`

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmVC0dcACgkQ5+KYS2KJ
yToWvw/+JIyhSZGWsQz2aeiSo/NPEFUShQLyI6MB/dNiotx/WvbzwR5PkUWzg067
hJnFgdo3sS89WTLDoqpew
...

💬 TheCharlatan commented on pull request "build: Update `qt` package up to 5.15.11":
(https://github.com/bitcoin/bitcoin/pull/28769#issuecomment-1790205335)
Guix builds (x86_64 and aarch64):
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
f3d6ac10842f48884b36d704cc4303ee001197a5c39f7d65116324ab919f4497 guix-build-8047bb6feaa9/output/aarch64-linux-gnu/SHA256SUMS.part
a651622edbca1c9badf7f4018296639929f9553c257bc2cdb464c240e5ab29d2 guix-build-8047bb6feaa9/output/aarch64-linux-gnu/bitcoin-8047bb6feaa9-aarch64-linux-gnu-debug.tar.gz
6ad8004e3739c51380032a411f43054a776818803
...

👍 TheCharlatan approved a pull request: "build: Update `qt` package up to 5.15.11"
(https://github.com/bitcoin/bitcoin/pull/28769#pullrequestreview-1709603720)
ACK 8047bb6feaa9ee5d6c1edb7640baaf228450bc6b

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379714659)
I tried a whole bunch of combinations. Say, you move the `LOCK(m_peer_mutex);` to L5831, where `m_peer_map` is used.

Then you get something [like this](https://cirrus-ci.com/task/6561881997967360?logs=ci#L3228) in Cirrus (not exactly!).

```
node0 2023-08-17T12:01:53.647510Z [msghand] [sync.cpp:97] [potential_deadlock_detected] POTENTIAL DEADLOCK DETECTED
node0 2023-08-17T12:01:53.647516Z [msghand] [sync.cpp:98] [potential_deadlock_detected] Previous lock order was:
node0 2023-08-17
...

💬 S3RK commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#discussion_r1379714710)
Should we then include #26008 and maybe #28574 in 27.0?