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

💬 ryanofsky commented on pull request "refactor: Make uint256S(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2214432177)
Concept ACK, and would be very nice for this to cover ParseHex. If it did, it seems like it would fix the unexpected consensus library dependency on the util library that hebasto reported in https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843:

https://github.com/bitcoin/bitcoin/blob/a83f050dbe1392fc6b1b6c2a140c7346653b40d3/src/pubkey.cpp#L193

👋 maflcko's pull request is ready for review: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071)

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668526162)
4b1e978b2bafd9da564aa52d2ce64a723cf64036 nit:
```suggestion
// Verify that any graph of transactions has its ancestry correctly computed by DepGraph, and if
```

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1644451932)
maybe mention that caller must ensure inc includes `Ancestors(inc)`