💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403014140)
nit: I noticed `m_locator` is a raw C-style array, was `std::array<Locator, MAX_LEVELS>` considered as a slightly safer alternative that may avoid pointer decay - given its tiny size we want to make sure we're not misindexing and it might make the iterations slightly more descriptive:
```C++
for (int level{0}; level < entry.m_locator.size(); ++level) {
```

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2402767616)
I wonder if the new vtable pointer that was introduced by polymorphism is worth it or we could make a fake-inheritance by templates or macros or code generation instead or `std::variant<SingletonCluste, GenericCluster>` parameter types? It would likely be a big refactor and not sure it would be smaller or more readable, just thinking out loud.

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2402810313)
Given that we'd already be failing in the parent call's `Assume(level <= to_level)` (and that `std::unreachable()` is not yet available in C++20) could we maybe use our own `NONFATAL_UNREACHABLE` here?

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403274255)
nit: in other cases the parameters are in a different order, e.g. `void Split(Cluster& cluster, int level) noexcept;`

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2402939151)
nit: what's the difference between an `int level` and the `int in_level` here? It doesn't seem to be shadowing anything.

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403340639)
I find these new unnamed and unbounded primitive arguments quite confusing now

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403287914)
nit: we could minimize the diff slightly with:
```suggestion
auto par_cluster = FindCluster(par, level).first;
auto chl_cluster = FindCluster(chl, level).first;
```

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403454316)
Based on https://en.cppreference.com/w/cpp/container/vector/shrink_to_fit this is just a hint, not guranteed to change anything
> It depends on the implementation whether the request is fulfilled.

But maybe we could still test that the that after deletion the `DynamicMemoryUsage` is the same, but after calling compaction the memory shrinks?

Something like:
```C++
auto last_compaction_pos{real.PositionRange()};
```
...
```C++
const size_t mem_before{real.DynamicMemoryUsage()};
real.
...

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403456079)
for the record, the `SanityCheck` below doesn't seem to fail without this change (i.e. build without fuzz passed if I have reverted this line for commit d9dbbf41b6458b11d9225fb436fc749bd30f9588). Not sure that's expected, it surprised me.

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2409314955)
Thanks, added a fuzz test and allowed existing entries to be attempted in `EmplaceCoinInternalDANGER`

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2409318128)
The modified fuzz already covers that - what would be the advantage of adding a unit test for it as well?

💬 maflcko commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2409727403)
```suggestion
LogWarning("Found invalid chain at least ~6 blocks longer than our best chain. Chain state database corruption likely.");
```

nit: while touching this, could add the correct log severity?

💬 zsaiwj commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3375627230)
V30 倒计时 💛💙💚

💬 Dorex45 commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3375633908)
V30 😃

💬 janb84 commented on pull request "Clear out space on GHA jobs":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3375647372)
> > A run applying to all jobs when using GHA can be seen here: https://github.com/willcl-ark/bitcoin/actions/runs/18274913987
>
> Looking there, it seems like the cleanup step can take up to 10 minutes to complete i.e. https://github.com/willcl-ark/bitcoin/actions/runs/18274913987/job/52024710440. Although I guess that is less of an issue if it's not running in this repo.

Ok I missed this in my review, I also have a slight preference to revert this to only apply to the CentOS job. I woul
...

📝 maflcko opened a pull request: "build: Bump clang minimum supported version to 17"
(https://github.com/bitcoin/bitcoin/pull/33555)

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3375694837)
@rkrux I'm not too worried about reviews just yet, let's focus on getting https://github.com/bitcoin/bitcoin/pull/29675 in.

👋 maflcko's pull request is ready for review: "build: Bump clang minimum supported version to 17"
(https://github.com/bitcoin/bitcoin/pull/33555)

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3375748629)
`dd32dfaaf3...69c015b258`: fix windows compilation

👋 fanquake's pull request is ready for review: "[28.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/33535)