👍 stickies-v approved a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404#pullrequestreview-2163348609)
ACK 6af51e819847e737449609daa214e16f9453e85d

This seems quite tricky to have test coverage for (since it only manifests in `bitcoin-qt`), and unfortunately the `EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)` annotations didn't catch this either, so I'm not sure if there's much more we can do to prevent regressions?

Review note: easy steps to reproduce are [here](https://github.com/bitcoin/bitcoin/issues/30400#issuecomment-2213692311). Thanks again for finding this @achow101 .

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2214110228)
> Needs rebase (for #29625)

Currently working on some simulations, will rebase soon

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#issuecomment-2214115661)
Note: 9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f introduced a double lock bug in `bitcoin-qt` as described in https://github.com/bitcoin/bitcoin/issues/30400.

👍 brunoerg approved a pull request: "contrib: asmap-tool - Compare ASMaps with respect to specific addresses"
(https://github.com/bitcoin/bitcoin/pull/30246#pullrequestreview-2163378260)
reACK 5215c925d1382e71c9e1d642fced8a152c629c7f

verified the changes since my last review, lgtm.

💬 sr-gi commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2214140849)
> @instagibbs @sr-gi maybe @mzumsande. This will likely affect you. Any feedback?

I'm really not familiar with the build system, but I was able to build this without any changes in my setup on macOS 14.5 with clang 15.0.0

📝 maflcko opened a pull request: "test: [refactor] Pass TestOpts"
(https://github.com/bitcoin/bitcoin/pull/30407)
Currently optional test context setup settings are passed by adding a new optional argument to the constructors. For example `extra_args`. This is problematic, because:

* Adding more optional settings in the future requires touching all affected constructors, increasing their verbosity.
* Setting only a later option requires setting the earlier ones.
* Clang-tidy named args passed to `std::make_unique` are not checked.

Fix all issues by adding a new struct `TestOpts`, which holds all opt
...

💬 maflcko commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#discussion_r1668692252)
some notes:

* make_unique code is not checked via clang-tidy named args.
* Listing all options that are not changed from the default seems verbose and brittle.

See https://github.com/bitcoin/bitcoin/pull/30407 for a fix.

🤔 maflcko reviewed a pull request: "test: Add arguments for creating a slimmer TestingSetup"
(https://github.com/bitcoin/bitcoin/pull/30399#pullrequestreview-2163416539)
Concept ACK. Thanks!

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668712676)
Actually, all this code is gone now.

💬 brunoerg commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#issuecomment-2214197447)
Concept ACK

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1668742748)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1665942681

> [79a970d](https://github.com/bitcoin/bitcoin/commit/79a970d4f12458a175317c453e251db7846c3561): I am not really sure, what the point of `Update` here is. A simple `return Interrupted{};` compiles as well. I fail to see why it would be wrong, because the default constructed monostate should not be returned either way.
>
> (Same for all code below)
>
> My recommendation would be to keep the code as simple as possibl
...

💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668742977)
Sounds like a good idea. The approach has the small drawback that the `CNode` state has to be extended by a boolean variable (something like e.g. `m_initial_version_message_sent`), but that seems to be the lesser evil than changing the `NetEventsInterface`. Will adapt later if there are no objections.

💬 dergoegge commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668746222)
Please put that state on `Peer` if you go this route.

🚀 ryanofsky merged a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355)

👍 ryanofsky approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2163569279)
Code review ACK 606a7ab862470413ced400aa68a94fd37c8ad3d3. Just small formatting, include, and static_assert changes since last review.

I think it would be great to follow up on dergoegge's comment about fuzzing
https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1668437385. It seems like it could make fuzzing output much more useful. I don't think it is critical to do it as part of this PR though, since the fuzz test currently relies on global state and this PR isn't changing that.

✅ glozow closed an issue: "Double lock detected in `Warnings::GetMessages()`"
(https://github.com/bitcoin/bitcoin/issues/30400)

🚀 glozow merged a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404)

💬 mzumsande commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2214416290)
> @instagibbs @sr-gi maybe @mzumsande. This will likely affect you. Any feedback?

Thanks - no objections, I've updated everywhere, so no longer affected by this.

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2214421760)
> It seems that `libbitcoin_consensus` still depends on `libbitcoin_util` via the `ParseHex()` call in `pubkey.cpp`.
>
> However, the `"consensus util"` dependency is not allowed in `contrib/devtools/check-deps.sh`.

Nice find! This seems to point to a bug in the `check-deps.sh` script for exported template functions. In this case, the fact that pubkey.cpp depends on TryParseHex function is detected in generated `consensus_imports.txt` file:

```
_Z11TryParseHexIhESt8optionalISt6vectorIT
...

🚀 fanquake merged a pull request: "build: Bump clang minimum supported version to 16"
(https://github.com/bitcoin/bitcoin/pull/30263)