The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33764.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
-static-pie bitcoind by fanquake)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
0@@ -0,0 +1,18 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2025-present The Bitcoin Core developers
4+# Distributed under the MIT software license, see the accompanying
5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
6+
7+export LC_ALL=C.UTF-8
8+
9+export CONTAINER_NAME=ci_win64_ucrt
ci_win64 (and the file), as this will be the default eventually, going forward.
A new -Warray-bounds compiler diagnostic should be evaluated and addressed.
If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.
If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.
I think it is just one of the many gcc false-positive warnings. Not sure if it makes sense to document or investigate them individually, given that they are hard to reproduce in light of changing the compiler version or the compile options even slightly.
I think it is just
If it had been documented (in the commit or PR description), we wouldn’t need to guess. Doing that seems like the minimum that should be done, when adding new suppressions; rather than supplying no information, and claiming someone should look in a followup.
should the depends build instructions be updated to mention this?
I’d prefer updating the docs once depends/hosts/mingw32.mk is adjusted so that compilers don’t need to be specified explicitly.
If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.
I think it is just one of the many gcc false-positive warnings.
That sounds plausible, though I’m not entirely convinced at this point.
From https://github.com/bitcoin/bitcoin/actions/runs/19014374041/job/54300038967:
0In file included from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/array:43,
1 from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/span:43,
2 from /home/admin/actions-runner/_work/_temp/src/span.h:10,
3 from /home/admin/actions-runner/_work/_temp/src/uint256.h:10,
4 from /home/admin/actions-runner/_work/_temp/src/consensus/params.h:10,
5 from /home/admin/actions-runner/_work/_temp/src/kernel/chainparams.h:9,
6 from /home/admin/actions-runner/_work/_temp/src/chainparams.h:9,
7 from /home/admin/actions-runner/_work/_temp/src/test/net_tests.cpp:5:
8In static member function ‘static constexpr _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = char; _Up = char; bool _IsMove = false]’,
9 inlined from ‘constexpr _OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = char*; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:521:30,
10 inlined from ‘constexpr _OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = char*; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:548:42,
11 inlined from ‘constexpr _OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = __gnu_cxx::__normal_iterator<char*, __cxx11::basic_string<char> >; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:555:31,
12 inlined from ‘constexpr _OI std::copy(_II, _II, _OI) [with _II = __gnu_cxx::__normal_iterator<char*, __cxx11::basic_string<char> >; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:651:7,
13 inlined from ‘void net_tests::{anonymous}::V2TransportTester::SendMessageA(std::string, std::span<const unsigned char>)’ at /home/admin/actions-runner/_work/_temp/src/test/net_tests.cpp:1304:18:
14/usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:452:30: warning: ‘void* __builtin_memmove(void*, const void*, long long unsigned int)’ offset [0, 1] is out of the bounds [0, 0] [-Warray-bounds=]
15 452 | __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
16 | ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
As the author of this code, would you consider this warning a false positive?
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.
I’m not sure what to do here, as according to https://github.com/mstorsjo/llvm-mingw/issues/523, we are considering switching to using LLVM for Windows builds (has that been discussed at length somewhere)?
If that is that case, then it’s not clear that churning a bunch of code, adding new GCC CIs, and code to Guix, is worthwhile, if it will just need to be re-written again/thrown away soon after?
This looks like a nice and small CI-only/test-only change to flatten the way for #30210. There is already a pull open for the guix changes in #33593, which can then pick up any follow-ups from here and also conclude by removing the msvcrt CI config and msvcrt workarounds in the code.
review ACK 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf 🐂
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf 🐂
3GGNiE1hD13QtTLvcrvA92ZP1OD3ly24HSGZbRQceL4YLLAM21XtAoV9N81XNmrIwRlFV/bq+Wa7ZbWQPllyIAA==
436@@ -445,6 +437,122 @@ jobs:
437 TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
438 run: py -3 test/functional/test_runner.py --jobs $NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix="$RUNNER_TEMP" --combinedlogslen=99999999 --timeout-factor=$TEST_RUNNER_TIMEOUT_FACTOR $EXCLUDE $TEST_RUNNER_EXTRA
439
440+ windows-ucrt-cross:
I guess yaml anchors could be used, but this would increase the diff and review effort again when it is deleted. An alternative would be to use a matrix, but I guess this requires depending both cross-test tasks on both cross-compile tasks? I’d say the current version is fine, but no strong opinion.
You’re right. I considered both of these options while working on this PR and came to the same conclusions.
IMO it should be fine to convert the existing task(s) to a matrix instead of duplicating explicitly. I made a commit in this branch to see ~ what it would look like, and I think it’s pretty simple.
@maflcko is correct though that with a matrix the windows-*-test jobs would both depend on both windows-*-cross builds finishing, which might contribute to slow CI runtimes.
According to the PR description msvcrt is going to be removed soon (tm) when this gets stabilised though, so it’s only temporary I guess, if there’s no desire to adopt the matrix here.
This one simply addresses your comments #33593 (comment) and #33593#pullrequestreview-3398698978.
Somewhat, but if this is the approach, it means that not only the compiler, but also the version of the headers being tested isn’t going to match Guix? (I’m wondering if internalising the headers might be a better approach, as that would be more flexible, and, if there is some plan to migrate to Clang, would likely be needed in any case?).
This one simply addresses your comments #33593 (comment) and #33593 (review).
Somewhat, but if this is the approach, it means that not only the compiler, but also the version of the headers being tested isn’t going to match Guix? (I’m wondering if internalising the headers might be a better approach, as that would be more flexible, and, if there is some plan to migrate to Clang, would likely be needed in any case?).
I’ve gathered the available compiler and header versions here: https://gist.github.com/hebasto/dee3c918da49ee767ccf5eea43276407.
It seems feasible to align Guix and CI.
As mentioned in the PR description, upgrading the compiler in Guix could be considered a step toward matching tool versions.
You #33764 (comment) was parsed by @DrahtBot
Concept ACK. Should probably be based on #33775.
Not sure I understand the dependence on #33775?
CI is using it’s own build, not the guix build right? Also MinGW brings it’s own GCC doesn’t it?
The CI jobs are more useful when using toolchains with versions similar to those in the Guix script.
Not sure I understand the dependence on #33775? CI is using it’s own build, not the guix build right? Also MinGW brings it’s own GCC doesn’t it?
The CI jobs are more useful when using toolchains with versions similar to those in the Guix script.
I think this is just a doc question. I think the trixie/g++-mingw-w64-ucrt64 task can be left as-is, without adding a dependency here. In the future, #33593 could simply update the comment to clarify which one is run in guix (and requires the same toolchain version), and which one is run “stand-alone”.
Not sure I understand the dependence on #33775? CI is using it’s own build, not the guix build right? Also MinGW brings it’s own GCC doesn’t it?
The CI jobs are more useful when using toolchains with versions similar to those in the Guix script.
I think this is just a doc question. I think the
trixie/g++-mingw-w64-ucrt64task can be left as-is, without adding a dependency here. In the future, #33593 could simply update the comment to clarify which one is run in guix (and requires the same toolchain version), and which one is run “stand-alone”.
Thanks @maflcko! The approach you suggested has been taken.
0@@ -0,0 +1,22 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2019-present The Bitcoin Core developers
4+# Distributed under the MIT software license, see the accompanying
5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
6+
7+export LC_ALL=C.UTF-8
8+
9+export CONTAINER_NAME=ci_win64
This is necessary to prepare for introducing the new Windows + UCRT
script and jobs.
17+export PACKAGES="g++-mingw-w64-ucrt64 nsis"
18 export RUN_UNIT_TESTS=false
19 export RUN_FUNCTIONAL_TESTS=false
20 export GOAL="deploy"
21-export BITCOIN_CONFIG="\
22- --preset=dev-mode \
Yeah, I guess there were some silent conflicts.
Indeed. Fixed.
Only changes since my last ack: #33764 (comment)
-Warray-bounds stuffwindows-msvcrt-cross task name after rebasere-ACK 5b06145e4e99f79b1889e5ac8dca89bf0a718c01 🍍
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 5b06145e4e99f79b1889e5ac8dca89bf0a718c01 🍍
37DicHlrrbmxMw461Rynr1JclwvkEbO78Jb5YVbZemnUuGeZ7TArUS3oHVFTiOG3tvrz1QeLtaYOnAZQfRmccAA==
Thanks! Fixed.
Co-authored-by: will <will@256k1.dev>
@willcl-ark’s suggestion has been taken.
Thank you!
review ACK 2e27bd9c3af91eb9fcc626fe65d065df0a80974d 🖊
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 2e27bd9c3af91eb9fcc626fe65d065df0a80974d 🖊
3VdW8j9UHIembvFBZW5872vAJlaVGWUbBOGG1CuEaRmfMCYMPTi086fRbdP5DzDkwdOOSZVikVeX0e86X9RQWCQ==