📝 mzumsande opened a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553)
In case of corruption that leads to a block marked as invalid that is actually valid for the rest of the network, the user currently doesn't receive good error messages, but will get be stuck in an endless headers-sync loop with no explanation (#26931).

This PR improves warnings in two ways:
- When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see https://github.com/bitcoin/bitcoin/issues/26391#issu
...

💬 mzumsande commented on issue "HeadersSync tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33547#issuecomment-3374358704)
I opened #33553 to address the problem from https://github.com/bitcoin/bitcoin/issues/26391 .

💬 mzumsande commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-3374369293)
> I'll work on a PR suggesting to use this check in the pre-sync header loop situation

Forgot about this completely, but opened #33553 now to improve the logging (we'll still search for peers with headers we can use, but will now repeatedly log warning suggesting to reindex if the problem persists)

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408782353)
Let me know how it goes :)

⚠️ plebhash opened an issue: "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage"
(https://github.com/bitcoin/bitcoin/issues/33554)
this is similar to #33463 cc @ryanofsky

however there's no usage of `waitTipChanged`, only `waitNext`

steps to reproduce:
- build `bitcoin-node` from `v30.0rc3` tag
- clone https://github.com/plebhash/sv2-bitcoin-core
- check out `2025-10-06-abort-proxy-io` branch
- launch `bitcoin-node` with `-ipc-bind=unix`
- launch `sv2-bitcoin-core` with `cargo run --example logger "/path/to/node.sock"`

here's a stack trace of `bitcoin-node`:

https://pastebin.com/uugSA6cD

(exceeds 65536 characters so g
...

💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374530494)
also reproducible on `v30-0rc1` tag

💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374565734)
one relevant detail here is that our rust code has evolved from what it looked like when I first reported https://github.com/bitcoin/bitcoin/issues/33463, and there I did not witness this behavior

now, every 10s, we receive a Sv2 [`CoinbaseOutputConstraints`](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputconstraints-client---server) message, which is meant to set the coinbase parameters with regards to blockspace consumption (weight a
...

💬 ryanofsky commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374751529)
(So far I'm not able to reproduce a crash with v30.0rc3, but that might just be because I was testing with regtest.)

The assert you see happening on [line 289](https://github.com/bitcoin-core/libmultiprocess/blob/13424cf2ecc1e5eadc85556cf1f4c65e915f702a/include/mp/proxy-io.h#L289) just happens when the IPC client asks the server to execute multiple IPC calls simultaneously on the same thread. The IPC server does not expect this and currently aborts when it happens.

It is possible for the serve
...

🤔 l0rinc reviewed a pull request: "cluster mempool: control/optimize TxGraph memory usage"
(https://github.com/bitcoin/bitcoin/pull/33157#pullrequestreview-3299619754)
It's my first time diving into cluster mempool, and it seems like a solid implementation.
I like the polymorphic split between `SingletonClusterImpl` and `GenericClusterImpl`.

I've documented my review journey and left many comments. Some are tiny nits, but others appear to be leftovers from previous refactorings that should be corrected - I'm fine with doing them in a follow-up PR as well.
If you decide to apply any suggestions here, I've included a few commit reorganization tips that could ma
...

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