💬 fjahr commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2213984176)
tACK 5215c925d1382e71c9e1d642fced8a152c629c7f

I played around with a few different input files, results look reasonable and I think this is a very helpful for analysis of generated asmap files.

For anyone who wants to test but is missing asmap files, you can use the ones here: https://github.com/fjahr/asmap-data

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668598683)
Right, thanks!

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

It's generally best-practice to hold locks for the shortest possible time to minimize contention.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2214078752)
> This is just limiting the max block weight

This had me confused initially, and I tried to combine them, but there's really two different things:

1. The max block weight demanded by the user via `-blockmaxweight`
2. Weight units reserved for the coinbase

With stratum v1 the distinction isn't very important. We just subtract (2) from (1). We could indeed do that at initialization.

But with Stratum v2 (2) is a per-client setting communicated to us (the Template Provider) via the [Coi
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668632974)
I don't seem to be able to get it to work. It may be that's because the iterator doesn't actually encapsulate a pointer iterator, but a pointer to where in the hashtable the entry exists or so. If so, then that just isn't possible.

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