💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3378291849)
Compared it against master on a Raspberry Pi 5 synchronizing from real peers for realism, ran it twice for good measure until 917000 blocks with dbcache 450:

This isn't the [latest version](https://github.com/bitcoin/bitcoin/compare/a8f9a806751b5755bdec5b096186f70c0bfddcfa..cf209da104d483aa064aa3bec621f1adc9574749) of the PR, but should likely be representative anyway.

<details>
<summary>First run: 19% faster, finished IBD in 13h:11m | IBD | 917000 blocks | dbcache 450 | rpi5-16-2 | aarch
...

🤔 glozow reviewed a pull request: "cluster mempool: control/optimize TxGraph memory usage"
(https://github.com/bitcoin/bitcoin/pull/33157#pullrequestreview-3182210966)
code review ACK 5ac32aa441b

I focused on checking that all the dynamic memory is accounted for, places where `m_cluster_usage` is updated, and correctness of the `SingletonCluster` implementation. Feel free to ignore my nits.

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403086164)
Note to self/reviewers: `Depgraph::Compact()` also calls `shrink_to_fit()`, meaning positions do not change and the mapping is not invalidated.

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2320034473)
Note to self/reviewers: since we've done `ApplyDependencies`, `ClusterSet.m_group_data`, `m_deps_to_add`, and `m_to_remove` are all empty so we don't need to add their dynamic memory usage. We also aren't counting `m_removed` since that'd be at staging level or an `m_unlinked` entry.

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2411544659)
nit: In the `SingletonClusterImpl` case, there is only 1 Entry
```suggestion
```

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2411563912)
nit: perhaps worth a comment that the special "singleton at the end of the cluster" `LinearizationIndex(-1)` is used here

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2411624776)
As far as I can tell, this function should never be called?

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2411594625)
Hm, should we be checking `m_quality` before adding to chunk index? Singletons are always "linearized" optimally of course, but `QualityLevel::OVERSIZED_SINGLETON` is also possible here (probably only theoretically).

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2408429695)
If the cluster is empty, then L718 will call `front()` on an empty vector. Should we `assert` instead of `Assume` that it's not empty?

Alternatively, `if (!Assume(!m_linearization.empty()) return -1;`?

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2410975604)
`m_depgraph` is a `DepGraph<SetType>`

💬 glozow commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2411539360)
Alternatively, both classes could implement an `IsEmpty()` function.

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2411666783)
We're in `Cluser` so `SetType` should be available here, so I think it's redundant

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2411670504)
I'm also fine with other ways of deduplication - or to leave as is.

💬 furszy commented on pull request "doc: how to update a subtree":
(https://github.com/bitcoin/bitcoin/pull/33568#discussion_r2411670406)
No need to add the `--fetch` option. You added a `git fetch` below.

🤔 furszy reviewed a pull request: "doc: how to update a subtree"
(https://github.com/bitcoin/bitcoin/pull/33568#pullrequestreview-3311582322)
Concept ACK

💬 hebasto commented on pull request "Fix windows libc++ `fs::path` `fstream` compile errors":
(https://github.com/bitcoin/bitcoin/pull/33550#issuecomment-3378434466)
> > Added to the nightly builds in [hebasto/bitcoin-core-nightly#74](https://github.com/hebasto/bitcoin-core-nightly/pull/74).
>
> Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):
>
> ```shell
> test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
> test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" r
...

💬 achow101 commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3378543092)
ACK 0fe7d552ab213065b8d5807c3dd9f4e976717529

```
55ee7f209c6cd5592a87472cacaf873e4a108f681465c238b66792a422306a15 guix-build-0fe7d552ab21/output/aarch64-linux-gnu/SHA256SUMS.part
eac376bc61d39740e37283df30c04ed330d1fe2ecc66ae2fb87e484c9f5589e1 guix-build-0fe7d552ab21/output/aarch64-linux-gnu/bitcoin-0fe7d552ab21-aarch64-linux-gnu-debug.tar.gz
7a31fb9bccf15dbb439f5a1600597188d8d97f4f0f9b94ccdf78bd1e90621421 guix-build-0fe7d552ab21/output/aarch64-linux-gnu/bitcoin-0fe7d552ab21-aarch64-lin
...

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2411796107)
> Any reason we can't use `memory_order_relaxed` on loads and stores of `m_interrupt`?

I recall briefly thinking about it, but going down that path would mean guarding all `m_interrupt` accesses with `m_mutex` too. And that seemed like an unnecessary overhead for methods like `Submit` that should be performing a lightweight interruption check before submission.

💬 mzumsande commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2411794397)
I tested this for signet on two computers, and while parallel sync (5 threads) was a great speedup on an SSD, I also observed a slowdown on a HDD compared to master (by a factor 2).
Presumably that's because reading the blocks from disk is the main bottleneck on a HDD, and with parallel indexing there is a lot of jumping back and forth, increasing seek time.
Should it be mentioned in the `-indexworkers` help that it is not advisable to use this option on a HDD?

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2411822083)
> going down that path would mean guarding all m_interrupt accesses with m_mutex too

Hmm I don't see why we would need to do that? If we wanted to make m_interrupt a non-atomic bool we would need to do that. But if it's atomic we can just use relaxed since it's just a flag and not acting as a fence to any other non-atomic memory?