💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379299664)
Could you explain a bit more why this is necessary, what is the lock order that would get violated if we did the locking later?

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379312851)
3/30 is integer, so maybe also add an example with a fraction. If we run it often enough, we could probably assert that two values for `total_fanouted` are possible.

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379313360)
nit: remove empty line

💬 fanquake commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789659513)
> Note also, that item 7 in the PR description involves a regression in v26.x.

What is the regression? Which change caused it?

💬 mzumsande commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789702441)
> Concept ACKs on fixing the bugs would be encouraging 😃

I think that some of the commits are straightforward bugfixes (e.g. the missing `MaybeFlipIPv6toCJDNS` calls or the logging improvements), whereas others are fixing some non-ideal behavior in some cases, but have the risk of introducing new non-ideal behavior in other cases because the fix comes with downsides of its own.

For example, if it isn't possible to distinguish situations in which two connections involving the same IP are c
...

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