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

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

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379723181)
This is certainly better. For no good reason, i just chose to follow a pattern we use elsewhere and never reconsidered it. I will take this code.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379749369)
I'm always not sure what to do with these kinds of probabilistic scenarios... Say you run 1000 experiments, and get 1000/0, so the assert fails. Is 1,000,000 sufficient in that case? Or how do you think this should be asserted otherwise.

💬 maflcko commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#discussion_r1379759963)
I agree, but I don't think there is an easy and recommended way to disable a single warning to not error. I could embed it into the compiler (`CXX='clang++ -Wno-error=deprecated-declarations`), but I presume this will be overridden by the configure logic to enable errors. I could set it in `CXXFLAGS`, but I presume this will either be overridden in the configure logic, or it will cause other horrible side-effects, such as disabling O2 (https://github.com/bitcoin/bitcoin/pull/28071/files).
Final
...