💬 vostrnad commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2333846869)
I actually benchmarked #28280 right before it got merged: https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2268695679
From the results it definitely seems that PR is not to blame and the regression happened later.
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2333846869)
I actually benchmarked #28280 right before it got merged: https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2268695679
From the results it definitely seems that PR is not to blame and the regression happened later.
👍 fanquake approved a pull request: "ci: Add missed configuration options to "Win64 native" job"
(https://github.com/bitcoin/bitcoin/pull/30755#pullrequestreview-2286047596)
ACK ee22bf55e39e4a40c31465e8ef2663eb7fefc783
(https://github.com/bitcoin/bitcoin/pull/30755#pullrequestreview-2286047596)
ACK ee22bf55e39e4a40c31465e8ef2663eb7fefc783
💬 andrewtoth commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2333868669)
https://github.com/bitcoin/bitcoin/pull/30326?
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2333868669)
https://github.com/bitcoin/bitcoin/pull/30326?
💬 ismaelsadeeq commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1746980280)
adding negatives value to the values makes this test to fail.
should we also prevent passing negative integers?
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1746980280)
adding negatives value to the values makes this test to fail.
should we also prevent passing negative integers?
💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1746980931)
> * I'm probably missing something, but maybe a cleaner workaround instead of setting `Debug` build type and then erasing all the debug flags is just to set a custom build type -DCMAKE_BUILD_TYPE=msan that doesn't have the flags to begin with?
I agree with you, and that was my initial preference when I worked on the CMake staging branch. However, using undefined build configurations is not documented. As a result, the rough consensus among the CMake staging branch contributors was to adher
...
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1746980931)
> * I'm probably missing something, but maybe a cleaner workaround instead of setting `Debug` build type and then erasing all the debug flags is just to set a custom build type -DCMAKE_BUILD_TYPE=msan that doesn't have the flags to begin with?
I agree with you, and that was my initial preference when I worked on the CMake staging branch. However, using undefined build configurations is not documented. As a result, the rough consensus among the CMake staging branch contributors was to adher
...
💬 hebasto commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2333907613)
> Right now looking at `depends/toolchain.cmake.in` it seems like both of these things can happen in some cases, but neither of them happen reliably. Specifically in the current system:
>
> * The toolchain file force-writes into the cache and overwrites user settings.
>
> ...
>
> I think to implement desired behavior, `toolchain.cmake.in` could be changed to:
>
> 1. Stop setting cache variables like `BUILD_GUI` and `WITH_QRENCODE` and instead set custom non-cache variables like `
...
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2333907613)
> Right now looking at `depends/toolchain.cmake.in` it seems like both of these things can happen in some cases, but neither of them happen reliably. Specifically in the current system:
>
> * The toolchain file force-writes into the cache and overwrites user settings.
>
> ...
>
> I think to implement desired behavior, `toolchain.cmake.in` could be changed to:
>
> 1. Stop setting cache variables like `BUILD_GUI` and `WITH_QRENCODE` and instead set custom non-cache variables like `
...
💬 maflcko commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#issuecomment-2333973232)
review-only ACK ee22bf55e39e4a40c31465e8ef2663eb7fefc783
(https://github.com/bitcoin/bitcoin/pull/30755#issuecomment-2333973232)
review-only ACK ee22bf55e39e4a40c31465e8ef2663eb7fefc783
💬 maflcko commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2333991068)
> clang 20 produces warning output for the pragma, and then fails to compile
Are you sure? Compilation did pass for me this morning. Do you still happen to have the compile failure or steps to reproduce?
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2333991068)
> clang 20 produces warning output for the pragma, and then fails to compile
Are you sure? Compilation did pass for me this morning. Do you still happen to have the compile failure or steps to reproduce?
💬 maflcko commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747087717)
An overload for `std::byte` could be one more line of code, but would avoid having to touch all the touched non-header lines in this pull request again. Unless I am missing something obvious, there shouldn't be any downside.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747087717)
An overload for `std::byte` could be one more line of code, but would avoid having to touch all the touched non-header lines in this pull request again. Unless I am missing something obvious, there shouldn't be any downside.
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2334000135)
Force pushed to include all follow-ups from my future commits and to allow positional args. The implementation should now be final and there hopefully won't be any need to touch it in a future PR again. I've updated the PR description to list the two additional bugs in the python code.
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2334000135)
Force pushed to include all follow-ups from my future commits and to allow positional args. The implementation should now be final and there hopefully won't be any need to touch it in a future PR again. I've updated the PR description to list the two additional bugs in the python code.
🚀 fanquake merged a pull request: "ci: Add missed configuration options to "Win64 native" job"
(https://github.com/bitcoin/bitcoin/pull/30755)
(https://github.com/bitcoin/bitcoin/pull/30755)
💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1747106280)
> adding negative value to the values makes this test to fail. should we also prevent passing negative integers?
`-nowallet=-1` should be treated the same as `-nowallet=1` which is the same as `-nowallet` which just clears the lists of wallets to load and is not expected to be an error.
This test is testing what happens when `-nowallet=0` is passed. Previously before https://github.com/bitcoin/bitcoin/pull/22217 `-nowallet=0` was the treated the same as `-wallet=1`, and would cause a walle
...
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1747106280)
> adding negative value to the values makes this test to fail. should we also prevent passing negative integers?
`-nowallet=-1` should be treated the same as `-nowallet=1` which is the same as `-nowallet` which just clears the lists of wallets to load and is not expected to be an error.
This test is testing what happens when `-nowallet=0` is passed. Previously before https://github.com/bitcoin/bitcoin/pull/22217 `-nowallet=0` was the treated the same as `-wallet=1`, and would cause a walle
...
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334033449)
Examples 16-19 seem to get slower?
My results on the last commit look similar to sipa's:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 17,187,614.00 | 58.18 | 4.0% | 0.19 | `LinearizeOptimallyExample00`
| 96,365.70 | 10,377.14 | 0.2% | 0.01 | `LinearizeOptimallyExample01`
| 87,832.50 | 11,385.31 | 3
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334033449)
Examples 16-19 seem to get slower?
My results on the last commit look similar to sipa's:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 17,187,614.00 | 58.18 | 4.0% | 0.19 | `LinearizeOptimallyExample00`
| 96,365.70 | 10,377.14 | 0.2% | 0.01 | `LinearizeOptimallyExample01`
| 87,832.50 | 11,385.31 | 3
...
💬 fanquake commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334037507)
> Do you still happen to have the compile failure or steps to reproduce?
Using this Clang:
```bash
clang++-20 --version
Debian clang version 20.0.0 (++20240903115437+0748f4227cd6-1~exp1~20240903115620.935)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/lib/llvm-20/bin
```
and master (bbf95c0cc57147827b9f4577c641b12dd4170e78). I see:
```bash
cmake -B build -DCMAKE_C_COMPILER=clang-20 -DCMAKE_CXX_COMPILER=clang++-20
cmake --build build --verbose
<snip>
[ 72%] Bu
...
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334037507)
> Do you still happen to have the compile failure or steps to reproduce?
Using this Clang:
```bash
clang++-20 --version
Debian clang version 20.0.0 (++20240903115437+0748f4227cd6-1~exp1~20240903115620.935)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/lib/llvm-20/bin
```
and master (bbf95c0cc57147827b9f4577c641b12dd4170e78). I see:
```bash
cmake -B build -DCMAKE_C_COMPILER=clang-20 -DCMAKE_CXX_COMPILER=clang++-20
cmake --build build --verbose
<snip>
[ 72%] Bu
...
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334042896)
Thanks for the graphs, @glozow. That confirms the impression we got: 16-19 seem to just be really hard, and the later optimizations just don't help with it (and increase per-iteration cost, for no meaningful iteration reduction).
It doesn't surprise me that this is possible (they are after all very far below the conjectured upper bound of $\mathcal{O}(\sqrt{2^N})$, but it does surprise me that such clusters are seen in historical real-world mempool data.
Perhaps it's worth investigating (in a
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334042896)
Thanks for the graphs, @glozow. That confirms the impression we got: 16-19 seem to just be really hard, and the later optimizations just don't help with it (and increase per-iteration cost, for no meaningful iteration reduction).
It doesn't surprise me that this is possible (they are after all very far below the conjectured upper bound of $\mathcal{O}(\sqrt{2^N})$, but it does surprise me that such clusters are seen in historical real-world mempool data.
Perhaps it's worth investigating (in a
...
💬 fanquake commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#issuecomment-2334046532)
cc @stickies-v @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/30828#issuecomment-2334046532)
cc @stickies-v @ryanofsky
💬 maflcko commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334046805)
Ok, I may have only compiled `bitcoind`. If the error is only in the tests, I wonder if the tests can simply be rewritten. I'll take another look later.
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334046805)
Ok, I may have only compiled `bitcoind`. If the error is only in the tests, I wonder if the tests can simply be rewritten. I'll take another look later.
💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747126073)
> An overload for `std::byte` could be one more line of code, but would avoid having to touch all the touched non-header lines in this pull request again.
Yeah sorry for any confusion. My suggestion in https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741079145 here was to use `std::byte` if it was easy or `BasicByte` as a fallback if would be too much work to convert other code to use std::byte. Having two overloads is also fine.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747126073)
> An overload for `std::byte` could be one more line of code, but would avoid having to touch all the touched non-header lines in this pull request again.
Yeah sorry for any confusion. My suggestion in https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741079145 here was to use `std::byte` if it was easy or `BasicByte` as a fallback if would be too much work to convert other code to use std::byte. Having two overloads is also fine.
💬 maflcko commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747142070)
> `BasicByte`
IIRC the reason that serialize code uses a template on the inner type of the span is that call sites have to explicitly specify that they want the "different" span-serialization.
I think the concern doesn't apply to CScript, so a template may be better to avoid, but I haven't tried this myself.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747142070)
> `BasicByte`
IIRC the reason that serialize code uses a template on the inner type of the span is that call sites have to explicitly specify that they want the "different" span-serialization.
I think the concern doesn't apply to CScript, so a template may be better to avoid, but I haven't tried this myself.
💬 andrewtoth commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2334078040)
Reason I suspect #30326 is because `try_emplace` must be doing more work than just `find` and is only called after blocks stop being full because previous coins are now being looked up to be spent.
Relevant stackoverflow https://stackoverflow.com/questions/24209592/performance-of-emplace-is-worse-than-check-followed-by-emplace
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2334078040)
Reason I suspect #30326 is because `try_emplace` must be doing more work than just `find` and is only called after blocks stop being full because previous coins are now being looked up to be spent.
Relevant stackoverflow https://stackoverflow.com/questions/24209592/performance-of-emplace-is-worse-than-check-followed-by-emplace