💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574121327)
Agreed, fixed.

💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#issuecomment-3592811541)
@sipa Thanks for the review!

I noticed a few things:
1) `-limitclustercount` and `-limitclustersize` are both debug options at the moment. Is that the right place for them? It seems slightly unusual to me that we'd reference command line options in our general docs that are hidden away like this...?

2) While `-limitancestorsize` and `-limitdescendantsize` have nice warnings now, I don't think I've updated any of the help text for `-limitancestorcount` and `-limitdescendantcount`. I beli
...

💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r2574341508)
<details>
<summary>Yeah, that's wrong because when there's no chain type specified or it's <code>-chain=main</code>, won't be any icon shown on the message box title bar.</summary>

<img width="511" height="188" alt="image" src="https://github.com/user-attachments/assets/67fc8092-710e-404e-b481-aa3572e004d9" />

</details>

At the time of writing it, "maybe" I meant something like this `if (!gArgs.GetChainTypeString().empty()) {`, which is what I'm going to use as a fix.

But I see othe
...

💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#issuecomment-3592957816)
1. Yeah, there is an inconsistency here. Either (1) we treat it as a debug-only option, and don't refer to it in documentation and help texts, or (2) we make it a fully fledged option that has a normal config category. I'd prefer (1).
2. Worth mentioning in release notes at least that these options won't be updated until cluster mempool is well-deployed on the network.
3. Too in the weeds, I think.

💬 ratthakorn2509 commented on pull request "cmake: Set `WITH_ZMQ` to `ON` in Windows presets":
(https://github.com/bitcoin/bitcoin/pull/33971#issuecomment-3593012291)
Ok

💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#issuecomment-3593069665)
> Yeah, there is an inconsistency here. Either (1) we treat it as a debug-only option, and don't refer to it in documentation and help texts, or (2) we make it a fully fledged option that has a normal config category. I'd prefer (1).

In the release notes for 0.12.0, we referenced that there was a way to change the descendant limits without calling out what the command line argument was, and instead referred users to using `--help -help-debug` for more info. I could do that in the release not
...

💬 princesingh956024-design commented on issue "`test_bitcoin-qt`: segfault under LTO (CMAKE_INTERPROCEDURAL_OPTIMIZATION=ON)":
(https://github.com/bitcoin/bitcoin/issues/33548#issuecomment-3593134675)
> podman run -it ubuntu:24.04
> apt install git build-essential cmake pkgconf python3 libevent-dev libboost-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools qt6-tools-dev-tools libgl-dev libqrencode-dev
> git clone https://github.com/bitcoin/bitcoin/
> cd bitcoin
> cmake -B build -DENABLE_IPC=OFF -DBUILD_GUI=ON -DENABLE_WALLET=OFF
> cmake --build build
> ctest --test-dir build # works fine
> ### Enable LTO
> cmake -B build -DENABLE_IPC=OFF -DBUILD_GUI=ON -DENABLE_WALLET=OFF -DCMAKE_INTERPROCEDURAL_
...

💬 billymcbip commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2574646699)
You're also validating flags here. Wouldn't `INVALID_REQUEST` be more appropriate?

💬 billymcbip commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2574659138)
Noob C++ question: When is it better to return a `nullptr` vs. throwing `std::out_of_range("index out of bounds")`?. Wouldn't it be cleaner to throw?

💬 billymcbip commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2574667690)
Noob question: How come you're not throwing directly in this function (`throw std::out_of_range("output_index is out of bounds"`)?

👍 l0rinc approved a pull request: "validation: fetch block inputs on parallel threads >40% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3521928328)
The new version cleverly uses `m_inputs` being empty as the shared shutdown signal (handling both 'no work' and 'shutdown' cases). This finally allowed us to eliminate the `m_request_stop` flag I disliked :).

It also benchmarks real-world I/O latency via a real LevelDB acces, while the fuzz tests use in-memory LevelDB now - sweet!
The new design basically falls back to synchronous fetching gracefully in cases of collisions or delays (we may want to test that specifically).

Regarding 'faster IB
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574407137)
`deterministic` sounds like something we should do in a benchmark, but it just predefines the salts for testability:
https://github.com/bitcoin/bitcoin/blob/9c24cda72edb2085edfa75296d6b42fab34433d9/src/util/hasher.cpp#L22-L25

As far as I can tell we don't need here, so we can likely remove that constructor arg completely:
```C++
explicit CoinsViewCacheAsync(CCoinsViewCache& cache, const CCoinsView& db) noexcept
: CCoinsViewCache{&cache}, m_db{db}, m_barrier{WORKER_THREADS + 1}
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574387786)
I understand that `size() > 0` may be more descriptive than `!empty()`, but there's a dedicated method for this case (there are a few more cases):
```suggestion
if (m_inputs.empty()) return;
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574435070)
Should we maybe document that this assumes that `ConnectBlock` will fetch the inputs in the same order?

nit: could we use the current formatting for new code?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574474663)
> speed up block validation

Should we mention any parallelism here?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574437547)
As https://corecheck.dev/bitcoin/bitcoin/pulls/31132 hints, this could also be passed as const reference.

nit: formatting

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574385347)
nit:
```suggestion
~CoinsViewCacheAsync() override
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574685431)
I'm not sure I understand how we're actually testing this.
`NoAccessCoinsView` is designed to abort on access, so in the shortid collision scenario how does it simulate going to disk?
Shouldn't we assert here that the number of collisions coincides with the number of simulated disk reads?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574457335)
[we should](https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&branch=31132-69310ec0035351e486cfb51cb66492639f6f8b47&id=aureleoules_bitcoin&open=AZrV9y4tqc704xdUcnxD&tab=why) likely init it here instead:
```suggestion
std::barrier<> m_barrier{WORKER_THREADS + 1};
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574535324)
```suggestion
BOOST_CHECK_EQUAL(coin.IsSpent(), spent);
```