💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3592671116)
Thank you very much for your review and bencharking @l0rinc! The speedup this offers is great. I have taken most of your suggestions.
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3592671116)
Thank you very much for your review and bencharking @l0rinc! The speedup this offers is great. I have taken most of your suggestions.
📝 hebasto opened a pull request: "cmake: Check dependencies after build option interaction"
(https://github.com/bitcoin/bitcoin/pull/33974)
At present, `CMakeLists.txt` interleaves configuration-option handling with dependency discovery. As a result, unnecessary checks may be performed. For example:
```
$ cmake -B build --preset dev-mode -DBUILD_FOR_FUZZING=ON
<snip>
-- Found PkgConfig: /usr/bin/pkg-config (found version "2.3.0")
-- Found ZeroMQ: /usr/lib64 (found suitable version "4.3.5", minimum required is "4.0.0")
-- Performing Test HAVE_USDT_H
-- Performing Test HAVE_USDT_H - Success
-- Found USDT: /usr/include
-- Foun
...
(https://github.com/bitcoin/bitcoin/pull/33974)
At present, `CMakeLists.txt` interleaves configuration-option handling with dependency discovery. As a result, unnecessary checks may be performed. For example:
```
$ cmake -B build --preset dev-mode -DBUILD_FOR_FUZZING=ON
<snip>
-- Found PkgConfig: /usr/bin/pkg-config (found version "2.3.0")
-- Found ZeroMQ: /usr/lib64 (found suitable version "4.3.5", minimum required is "4.0.0")
-- Performing Test HAVE_USDT_H
-- Performing Test HAVE_USDT_H - Success
-- Found USDT: /usr/include
-- Foun
...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574049689)
I now use a leveldb and add mock input data before the benchmark, so it's more of a real world benchmark now :+1: . Thanks!
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574049689)
I now use a leveldb and add mock input data before the benchmark, so it's more of a real world benchmark now :+1: . Thanks!
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574105954)
Removed.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574105954)
Removed.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574107800)
Removed.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574107800)
Removed.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574108138)
Done.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574108138)
Done.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574112696)
Added a reference to `-limitclustersize` since it's analogous.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574112696)
Added a reference to `-limitclustersize` since it's analogous.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574114517)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574114517)
Fixed.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574115280)
Yep, that is what I meant! Fixed.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574115280)
Yep, that is what I meant! Fixed.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574119436)
Agreed, "performance" is misleading. I was thinking "better behavior" myself, but took your language.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574119436)
Agreed, "performance" is misleading. I was thinking "better behavior" myself, but took your language.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574120039)
Done.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574120039)
Done.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574121028)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574121028)
Fixed.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2574121327)
Agreed, fixed.
(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
...
(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
...
(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.
(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
(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
...
(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_
...
(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?
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2574646699)
You're also validating flags here. Wouldn't `INVALID_REQUEST` be more appropriate?