Closes #29760.
Suggested in #29758 (comment).
Closes #29760.
Suggested in #29758 (comment).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
ACK | maflcko, sipsorcery, sipa |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
What is the issue with the bitdeque and miniscript fuzz tests?
Here is a branch with included bitdeque.cpp
and the excerpt from its build log:
0 bitdeque.cpp
1D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20): error C2248: 'bitdeque<128>::BITS_PER_WORD': cannot access private member declared in class 'bitdeque<128>' [D:\a\bitcoin\bitcoin\build_msvc\fuzz\fuzz.vcxproj]
2 D:\a\bitcoin\bitcoin\src\util\bitdeque.h(28,26): note: see declaration of 'bitdeque<128>::BITS_PER_WORD'
3 D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(18,23): note: see declaration of 'bitdeque<128>'
4 D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20): note: the template instantiation context (the oldest one first) is
5 D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17): note: see reference to class template instantiation 'bitdeque<128>::Iterator<true>' being compiled
6 D:\a\bitcoin\bitcoin\src\util\bitdeque.h(89,32): note: while compiling class template member function 'bitdeque<128>::Iterator<true>::difference_type operator -(const bitdeque<128>::Iterator<true> &,const bitdeque<128>::Iterator<true> &)'
7 D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17): note: see the first reference to 'operator -' in 'bitdeque_fuzz_target::<lambda_18>::operator ()'
8D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20): error C2248: 'bitdeque<128>::BITS_PER_WORD': cannot access private member declared in class 'bitdeque<128>' [D:\a\bitcoin\bitcoin\build_msvc\fuzz\fuzz.vcxproj]
9 (compiling source file '../../src/test/fuzz/bitdeque.cpp')
10 D:\a\bitcoin\bitcoin\src\util\bitdeque.h(28,26):
11 see declaration of 'bitdeque<128>::BITS_PER_WORD'
12 D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(18,23):
13 see declaration of 'bitdeque<128>'
14 D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20):
15 the template instantiation context (the oldest one first) is
16 D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17):
17 see reference to class template instantiation 'bitdeque<128>::Iterator<true>' being compiled
18 D:\a\bitcoin\bitcoin\src\util\bitdeque.h(89,32):
19 while compiling class template member function 'bitdeque<128>::Iterator<true>::difference_type operator -(const bitdeque<128>::Iterator<true> &,const bitdeque<128>::Iterator<true> &)'
20 D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17):
21 see the first reference to 'operator -' in 'bitdeque_fuzz_target::<lambda_18>::operator ()'
Here is a branch with included miniscript.cpp
and the excerpt from its build log:
0 miniscript.cpp
1D:\a\bitcoin\bitcoin\src\script\miniscript.h(154,13): error C2248: 'miniscript::Type::Type': cannot access private member declared in class 'miniscript::Type' [D:\a\bitcoin\bitcoin\build_msvc\fuzz\fuzz.vcxproj]
2 D:\a\bitcoin\bitcoin\src\script\miniscript.h(127,5): note: see declaration of 'miniscript::Type::Type'
3 D:\a\bitcoin\bitcoin\src\script\miniscript.h(122,7): note: see declaration of 'miniscript::Type'
4 D:\a\bitcoin\bitcoin\src\test\fuzz\miniscript.cpp(1226,53): note: while evaluating constexpr function 'miniscript::operator ""_mst'
Closes #29760.
Suggested in #29758 (comment).
Once this PR collects concept ACKs, I’m going to open dedicated PRs to fix portability issues in the code.
14@@ -15,7 +15,7 @@ void MockedDescriptorConverter::Init() {
15 // an extended one.
16 if (IdIsCompPubKey(i) || IdIsUnCompPubKey(i) || IdIsXOnlyPubKey(i) || IdIsConstPrivKey(i)) {
17 CKey privkey;
18- privkey.Set(UCharCast(key_data.begin()), UCharCast(key_data.end()), !IdIsUnCompPubKey(i));
19+ privkey.Set(UCharCast(key_data.data()), UCharCast(key_data.data()) + 32, !IdIsUnCompPubKey(i));
key_data.size()
rather than 32
?
UCharCast
is needed? If there is no need for it, it may be better to remove it. If there is need for it, the Set
function signature should be adjusted accordingly.
... + 32
is consistent with other CKey::Set()
calls in our codebase:
https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/key.cpp#L330
https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/key.cpp#L393
https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/key_io.cpp#L218
with an exception in https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/test/orphanage_tests.cpp#L45
utACK 239d1d62ac880dfd2a82c1c4b657e8fa5e5d26d2
Nice, I’m surprised how few changes/disabling of tests were needed to make this work.
134@@ -136,11 +135,13 @@ void initialize()
135 static bool read_stdin(std::vector<uint8_t>& data)
136 {
137 uint8_t buffer[1024];
138- ssize_t length = 0;
139- while ((length = read(STDIN_FILENO, buffer, 1024)) > 0) {
140+ size_t length = 0;
141+ do {
142+ std::cin.read(reinterpret_cast<char*>(buffer), 1024);
std::cin::char_type
for buffer
134@@ -136,11 +135,13 @@ void initialize()
135 static bool read_stdin(std::vector<uint8_t>& data)
136 {
137 uint8_t buffer[1024];
138- ssize_t length = 0;
139- while ((length = read(STDIN_FILENO, buffer, 1024)) > 0) {
140+ size_t length = 0;
143+ length = std::cin.gcount();
144 data.insert(data.end(), buffer, buffer + length);
145- }
146- return length == 0;
147+ } while (length > 0);
148+ return std::cin.eof();
305+
306+ - name: Run fuzz binaries
307+ env:
308+ BITCOINFUZZ: "${{ github.workspace}}\\src\\fuzz.exe"
309+ shell: cmd
310+ run: py -3 test\fuzz\test_runner.py --par %NUMBER_OF_PROCESSORS% --loglevel DEBUG --empty_min_time=60 %RUNNER_TEMP%\qa-assets\fuzz_seed_corpus
0 run: py -3 test\fuzz\test_runner.py --par %NUMBER_OF_PROCESSORS% --loglevel DEBUG %RUNNER_TEMP%\qa-assets\fuzz_seed_corpus
This is for libfuzzer only, so could be removed?
I get 14 compiler errors when building this PR with Visual Studio.
One example on line 282 of transaction_tests.cpp:
UniValue tests = read_json(json_tests::tx_invalid);
Gives compiler error of:
0Error C2664 'UniValue read_json(const std::string &)':
1cannot convert argument 1 from 'const unsigned char [53413]' to 'const std::string &'
2test_bitcoin C:\dev\github\bitcoin\src\test\transaction_tests.cpp 282
It’s easy enough to fix by casting the unsigned char[] to std::string but perhaps there’s a deeper issue?
I get 14 compiler errors when building this PR with Visual Studio.
How does your setup differ from the one in the CI?
I believe this is unrelated to this PR, see #29051.
make clean
and rebuilding should fix it.
I’m on Windows and using msvc but I did do a clean build but it didn’t help.
I suspect it’s down to msvc being less forgiving than gcc, probably due to a recent msvc change although I couldn’t find anything in the release notes.
Apolgies for dumping a screenshot but it does show a succinct summary of the issue. This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file.
I’m on Windows and using msvc but I did do a clean build but it didn’t help.
What VS / MSVC versions do you use?
This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file.
Does it compile on the master branch for you?
I’m on Windows and using msvc but I did do a clean build but it didn’t help.
What VS / MSVC versions do you use?
Visual Studio (including msvc) updates get automatically pushed every couple of weeks. I’m generally always pretty close to the latest stable version.
Currently Visual Studio 2022 17.8.0
0msbuild --version
1MSBuild version 17.8.3+195e7f5a3 for .NET Framework
217.8.3.51904
This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file.
Does it compile on the master branch for you?
No.
I only checked that after posting the compiler error message. The msvc compiler error isn’t related to this PR, it’s on master as well.
The msvc compiler error isn’t related to this PR, it’s on master as well.
Could you please open a separate issue then?
FWIW, I have no compile errors for the master branch @ b5d21182e5a66110ce2796c2c99da39c8ebf0d72 using VS Community 2022 17.9.5. For example, the test_bitcoin.exe
compiles for both Debug
and Release
configurations:
0> msbuild --version
1MSBuild version 17.9.8+b34f75857 for .NET Framework
217.9.8.16306
3> py -3 build_msvc\msvc-autogen.py
4> msbuild build_msvc\bitcoin.sln -p:UseMultiToolTask=true -p:Configuration=Debug -maxCpuCount -v:minimal -noLogo -t:test_bitcoin
5> msbuild build_msvc\bitcoin.sln -p:UseMultiToolTask=true -p:Configuration=Release -maxCpuCount -v:minimal -noLogo -t:test_bitcoin
The branch has been reworked to address @maflcko’s comments.
… but bool return value + exceptions doesn’t make sense, does it?
Whether std::istream::read()
throws or not depends on how exceptions()
is set. The suggested code does not use exceptions.
9@@ -10,19 +10,33 @@
10 #include <string>
11 #include <vector>
12
13+#if defined(__has_builtin)
I don’t really think we want a reversion here (or in addition), and if we were reverting shouldn’t the __GNUC__
case go away?
In any case, I’d rather we just not define this for MSVC.
I’d rather we just not define this for MSVC.
Thanks! Done.
Rebased on top of the merged bitcoin/bitcoin#29820 and bitcoin/bitcoin#29821.
The recent comments have been addressed.
155@@ -156,7 +156,7 @@ def main():
156 fuzz_bin,
157 '-help=1',
158 ],
159- env=get_fuzz_env(target=test_list_selection[0], source_dir=config['environment']['SRCDIR']),
160+ env={**os.environ, **get_fuzz_env(target=test_list_selection[0], source_dir=config['environment']['SRCDIR'])},
The error message is:
0Traceback (most recent call last):
1 File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 406, in <module>
2 main()
3 File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 110, in main
4 test_list_all = parse_test_list(
5 ^^^^^^^^^^^^^^^^
6 File "D:\a\bitcoin\bitcoin\test\fuzz\test_runner.py", line 392, in parse_test_list
7 test_list_all = subprocess.run(
8 ^^^^^^^^^^^^^^^
9 File "C:\hostedtoolcache\windows\Python\3.12.2\x64\Lib\subprocess.py", line 571, in run
10 raise CalledProcessError(retcode, process.args,
11subprocess.CalledProcessError: Command 'D:\a\bitcoin\bitcoin\src\fuzz.exe' returned non-zero exit status 3221226505.
12Error: Process completed with exit code 1.
(see https://github.com/hebasto/bitcoin/actions/runs/8636782427/job/23677472745)
Do you think this message is meaningful enough to put it in to the commit message?
On Windows, when subprocess
sets environment variables, it effectively overrides all of them, which makes PATH
and COMSPEC
environment variable empty for a subprocess on the master branch.
UPD. #29774 (review)
Linux does not need the COMSPEC
environment variable to spawn a new process, right?
UPD. #29774 (review)
COMSPEC
in the commit message?
My previous comments were a bit misleading because only %SystemRoot%
is required to be passed through into a subprocess.
From the Python docs:
If specified,
env
must provide any variables required for the program to execute. On Windows, in order to run a side-by-side assembly the specifiedenv
must include a validSystemRoot
.
Reworked and documented.
143+ std::cin.read(buffer, 1024);
144+ if (std::cin.fail()) return false;
145+ data.insert(data.end(), buffer, buffer + std::cin.gcount());
146 }
147- return length == 0;
148+ return true;
Nit in the first commit:
An alternative, smaller diff, not using the eof
and fail
methods would be:
0diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
1index a8e490b459..f9915187bd 100644
2--- a/src/test/fuzz/fuzz.cpp
3+++ b/src/test/fuzz/fuzz.cpp
4@@ -25,7 +25,6 @@
5 #include <memory>
6 #include <string>
7 #include <tuple>
8-#include <unistd.h>
9 #include <utility>
10 #include <vector>
11
12@@ -135,9 +134,9 @@ void initialize()
13 #if defined(PROVIDE_FUZZ_MAIN_FUNCTION)
14 static bool read_stdin(std::vector<uint8_t>& data)
15 {
16- uint8_t buffer[1024];
17- ssize_t length = 0;
18- while ((length = read(STDIN_FILENO, buffer, 1024)) > 0) {
19+ std::istream::char_type buffer[1024];
20+ std::streamsize length;
21+ while ((std::cin.read(buffer, 1024), length = std::cin.gcount()) > 0) {
22 data.insert(data.end(), buffer, buffer + length);
23 }
24 return length == 0;
86@@ -87,7 +87,7 @@
87 <ClCompile>
88 <WarningLevel>Level3</WarningLevel>
89 <PrecompiledHeader>NotUsing</PrecompiledHeader>
90- <AdditionalOptions>/utf-8 /Zc:__cplusplus /std:c++20 %(AdditionalOptions)</AdditionalOptions>
91+ <AdditionalOptions>/utf-8 /Zc:preprocessor /Zc:__cplusplus /std:c++20 %(AdditionalOptions)</AdditionalOptions>
Haven’t reviewed anything in build_msvc
lgtm ACK 37cda55b3923fa8c18059c8dfd8c61a537d6b8b5 🚗
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: lgtm ACK 37cda55b3923fa8c18059c8dfd8c61a537d6b8b5 🚗
3y0gHEEPCgqu2uip6x8C4jYAbKdEI6Vq/on9xDjK2B/bXYWgzuYYONFj6PQ2s61tsMBlfEN4ZSnCJ3JTpivQlCQ==
I believe this is unrelated to this PR, see #29051.
make clean
and rebuilding should fix it.I’m on Windows and using msvc but I did do a clean build but it didn’t help.
Does it still happen after you clear the git repo carefully and start from scratch? git clean -dffx
, or similar, but this will delete all non-git files, so be careful.
See:
- https://learn.microsoft.com/en-us/cpp/build/reference/zc-preprocessor
- https://learn.microsoft.com/en-us/cpp/preprocessor/preprocessor-experimental-overview
Otherwise, the "traditional" MSVC preprocessor fails to parse the
`FUZZ_TARGET` and `DETAIL_FUZZ` macros because of behavior changes
highlighted in the docs mentioned above.
See https://docs.python.org/3/library/subprocess.html
Haven’t reviewed anything in build_msvc
lgtm ACK 18fd522ca9a74cf8690a6c9b9b729e78c6ed41fd 🔍
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: lgtm ACK 18fd522ca9a74cf8690a6c9b9b729e78c6ed41fd 🔍
3gNifTt5s+7OtYIlaXMdEJ3HLZ9C6g8ytDavFNfVc383QbognEyerTmREnewCCAQL5VnglQrDEpSetYJvM+yaAg==
Mind having another look into this PR?
tACK 09f5a74.
Wrong commit hash?
:)
Every time :( I wish there were timestamps on the Github list. If only my old brain could remember to use git log next time.
tACK 18fd522ca9a74cf8690a6c9b9b729e78c6ed41fd