💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1668634381)
> I wrote a simple patch to make the existing `Result` more flexible (allow inner error types other than `bilingual_str`), for places that want to use something like `std::expected<A, B>` in code today by using `Result<A, B>`.

I think it could be a good idea to introduce a `util::Expected<A, B>` class if you see places in our code that need to return structured failure information but do not need to return error messages along with the failure data. If we have a `util::Expected` class it shou
...

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