💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098)
Actually, according to `git grep --extended-regexp '%[0-9]\$' -- '*.cpp' '*.h'` positional args are used. Let me fix that up.
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098)
Actually, according to `git grep --extended-regexp '%[0-9]\$' -- '*.cpp' '*.h'` positional args are used. Let me fix that up.
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2333731246)
Guix Build (aarch64):
```bash
f21ba8dde52ecf7fd1e76b5f6efe7d620a7a0baf8099eb6d25d09161a73f2030 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/SHA256SUMS.part
5e89a83e025bfbc37118a1ca0d386b0ffbde22bd3357b06d7e4c3587e10b0ed6 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu-debug.tar.gz
d0b7acb03204f077bc88d0d9008041c1aa854a6e0fd244210099ae2f2b8b4ca3 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu.tar.gz
e46198
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2333731246)
Guix Build (aarch64):
```bash
f21ba8dde52ecf7fd1e76b5f6efe7d620a7a0baf8099eb6d25d09161a73f2030 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/SHA256SUMS.part
5e89a83e025bfbc37118a1ca0d386b0ffbde22bd3357b06d7e4c3587e10b0ed6 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu-debug.tar.gz
d0b7acb03204f077bc88d0d9008041c1aa854a6e0fd244210099ae2f2b8b4ca3 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu.tar.gz
e46198
...
💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746891613)
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746891613)
Thanks, fixed
💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2333742666)
> Could add something like this, but no strong view
Done, thanks!
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2333742666)
> Could add something like this, but no strong view
Done, thanks!
💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746893985)
`WITH_NATPMP` is `OFF` because the `libnatpmp` package is not available in [vcpkg](https://vcpkg.io/en/packages).
`WITH_QRENCODE` is `OFF` because I faced build errors for this [package](https://vcpkg.io/en/package/libqrencode) or its dependencies in the past (I can't recall the exact issue). I've tested it recently and I got another build error, which is weird, because it happens in the main build system:
```
Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
```
It needs to be
...
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746893985)
`WITH_NATPMP` is `OFF` because the `libnatpmp` package is not available in [vcpkg](https://vcpkg.io/en/packages).
`WITH_QRENCODE` is `OFF` because I faced build errors for this [package](https://vcpkg.io/en/package/libqrencode) or its dependencies in the past (I can't recall the exact issue). I've tested it recently and I got another build error, which is weird, because it happens in the main build system:
```
Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
```
It needs to be
...
💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746908659)
> WITH_NATPMP is OFF
> WITH_QRENCODE is OFF
It seems like for at least `libnatpmp`, it'd be better to just set nothing, rather than hardcoding to `OFF`. As a package that doesn't exist can't cause issues, and I assume it could exist at some point, or the user could vendor it themselves, and then they would just need to override this (it also can't cause issues unless we opt into using it, even if it somehow ended up installed).
For libqrencode, if it's going to cause build issues, if the
...
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746908659)
> WITH_NATPMP is OFF
> WITH_QRENCODE is OFF
It seems like for at least `libnatpmp`, it'd be better to just set nothing, rather than hardcoding to `OFF`. As a package that doesn't exist can't cause issues, and I assume it could exist at some point, or the user could vendor it themselves, and then they would just need to override this (it also can't cause issues unless we opt into using it, even if it somehow ended up installed).
For libqrencode, if it's going to cause build issues, if the
...
💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746918973)
> For libqrencode, if it's going to cause build issues, if the user has it and Qt installed, it'd probably be good to document that as the reason it is disabled.
In suggestions about the place for such a note. Asking because the JSON format has no comments, and CMake's parser does not allow extra fields, such as `"_comment:"`.
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746918973)
> For libqrencode, if it's going to cause build issues, if the user has it and Qt installed, it'd probably be good to document that as the reason it is disabled.
In suggestions about the place for such a note. Asking because the JSON format has no comments, and CMake's parser does not allow extra fields, such as `"_comment:"`.
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2333782452)
re-ACK 4ac1981d338f89ed2b814aedaebe81783293e6f5
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2333782452)
re-ACK 4ac1981d338f89ed2b814aedaebe81783293e6f5
👍 TheCharlatan approved a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2285916123)
ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2285916123)
ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef
💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746922365)
> In suggestions about the place for such a note.
I guess somewhere in the Windows build docs? Note, still wondering in regards to:
> Can you explain why some options are passed on the command line, and why some are put into CMakePresets.json; what determines where each option belongs? I'd think it'd be better to have a single source of truth for the CI config.
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746922365)
> In suggestions about the place for such a note.
I guess somewhere in the Windows build docs? Note, still wondering in regards to:
> Can you explain why some options are passed on the command line, and why some are put into CMakePresets.json; what determines where each option belongs? I'd think it'd be better to have a single source of truth for the CI config.
💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746924747)
Presets contain options that seem reasonable in most cases.
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746924747)
Presets contain options that seem reasonable in most cases.
⚠️ vostrnad opened an issue: "28.0rc1 synchronizes much slower on Windows"
(https://github.com/bitcoin/bitcoin/issues/30833)
Bitcoin Core 28.0rc1 (both pre-built and self-built binary) on Windows 10 synchronizes much slower than 27.0 after around block 120,000 when blocks stop being almost empty.
Time to synchronize to block 300,000: (measured around 10 runs each, alternating between the versions)
27.0: **695 seconds**
28.0rc1: **2760 seconds**
Running the Linux pre-built binary in WSL doesn't show the slowdown, so i
...
(https://github.com/bitcoin/bitcoin/issues/30833)
Bitcoin Core 28.0rc1 (both pre-built and self-built binary) on Windows 10 synchronizes much slower than 27.0 after around block 120,000 when blocks stop being almost empty.
Time to synchronize to block 300,000: (measured around 10 runs each, alternating between the versions)
27.0: **695 seconds**
28.0rc1: **2760 seconds**
Running the Linux pre-built binary in WSL doesn't show the slowdown, so i
...
💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746931123)
> Presets contain options that seem reasonable in most cases.
Sure, but that's too vauge to use as reasoning for where CI configuration should live, and doesn't solve this issue:
> For example, I can imagine someone changing CMakePresets.json, without realising that also partially controls the CI, and then we (silently) lose Qt compilation in this build.
I'm not sure the configuration of the CI, should be (unexplicitly) coupled to the CMake Preset. This isn't specific to the GUI option,
...
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746931123)
> Presets contain options that seem reasonable in most cases.
Sure, but that's too vauge to use as reasoning for where CI configuration should live, and doesn't solve this issue:
> For example, I can imagine someone changing CMakePresets.json, without realising that also partially controls the CI, and then we (silently) lose Qt compilation in this build.
I'm not sure the configuration of the CI, should be (unexplicitly) coupled to the CMake Preset. This isn't specific to the GUI option,
...
💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746937151)
> If we want to use Presets for the CI, that's possibly also fine, but they should be dedicated presets, so it's clear what they are for.
You highlighted a problem that is relevant to all CI jobs and is wider than this PR goal. Can we leave it for a follow up dedicated discussion/PR?
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746937151)
> If we want to use Presets for the CI, that's possibly also fine, but they should be dedicated presets, so it's clear what they are for.
You highlighted a problem that is relevant to all CI jobs and is wider than this PR goal. Can we leave it for a follow up dedicated discussion/PR?
💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746942004)
Seems like we could just add the relevant options from the preset to the ci config file for now, which makes the config clear, and removes the possibility for external breakage? If at some point we switch to presets, they would all them be moved to a dedicated CI preset.
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746942004)
Seems like we could just add the relevant options from the preset to the ci config file for now, which makes the config clear, and removes the possibility for external breakage? If at some point we switch to presets, they would all them be moved to a dedicated CI preset.
🤔 marcofleon reviewed a pull request: "fuzz: reduce number of iterations in `crypto_aeadchacha20poly1305` target"
(https://github.com/bitcoin/bitcoin/pull/30826#pullrequestreview-2285983094)
Tested ACK f482d0e366a84008129913b442f0c955de79ac93. Saw the same slight increase in coverage. Executed 100,000 runs several times and total time went from 30-35 sec to 20-25 sec.
(https://github.com/bitcoin/bitcoin/pull/30826#pullrequestreview-2285983094)
Tested ACK f482d0e366a84008129913b442f0c955de79ac93. Saw the same slight increase in coverage. Executed 100,000 runs several times and total time went from 30-35 sec to 20-25 sec.
💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to `std::span` to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1746943429)
> But I do think it would be nice if this PR just added support for std::span and std::byte without necessarily making bigger changes.
I've added support for `const std::span<const unsigned char>` (for vector & array), but `std::byte` seems like a bigger change (as explained now in the PR's description) - unless you'd like me to add it as an extra overload - let me know if that's what you meant.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1746943429)
> But I do think it would be nice if this PR just added support for std::span and std::byte without necessarily making bigger changes.
I've added support for `const std::span<const unsigned char>` (for vector & array), but `std::byte` seems like a bigger change (as explained now in the PR's description) - unless you'd like me to add it as an extra overload - let me know if that's what you meant.
💬 fanquake commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2333821161)
I had hoped we'd be able to land this workaround for `28.x` (to atleast avoid compile failures, without any other changes to other dependency or compiler requirements), and then remove it in master fairly soon, by following up with either a wholesale bump to our minimum required Boost, to 1.81.0+, or I guess a clang-18+ restriction to using Boost 1.81.0+.
I re-tested master (including this change) using Boost 1.74.0:
* clang-18 builds with no output
* clang-19 builds with no output (unrelat
...
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2333821161)
I had hoped we'd be able to land this workaround for `28.x` (to atleast avoid compile failures, without any other changes to other dependency or compiler requirements), and then remove it in master fairly soon, by following up with either a wholesale bump to our minimum required Boost, to 1.81.0+, or I guess a clang-18+ restriction to using Boost 1.81.0+.
I re-tested master (including this change) using Boost 1.74.0:
* clang-18 builds with no output
* clang-19 builds with no output (unrelat
...
💬 sipa commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2333833557)
Try whether #28280 had any impact.
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2333833557)
Try whether #28280 had any impact.
💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746957633)
> > In suggestions about the place for such a note.
>
> I guess somewhere in the Windows build docs?
Thanks! Done.
> Seems like we could just add the relevant options from the preset to the ci config file for now
Implemented.
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746957633)
> > In suggestions about the place for such a note.
>
> I guess somewhere in the Windows build docs?
Thanks! Done.
> Seems like we could just add the relevant options from the preset to the ci config file for now
Implemented.