test: `minisketch_tests.cpp` can fail when built with MSVC

hebasto commented at 6:43 pm on October 5, 2022: member

To trigger the failure, consider the diff as follows:

 0--- a/src/test/minisketch_tests.cpp
 1+++ b/src/test/minisketch_tests.cpp
 2@@ -27,6 +27,11 @@ BOOST_AUTO_TEST_CASE(minisketch_test)
 3         uint32_t start_b = start_a + a_not_b;
 4         uint32_t end_b = start_b + both + b_not_a;
 5 
 6+#ifdef _MSC_VER
 7+        // FIXME
 8+        BOOST_TEST_MESSAGE("This message *breaks* this test when running with '-l test_suite'");
 9+#endif
10+
11         Minisketch sketch_a = MakeMinisketch32(10);
12         for (uint32_t a = start_a; a < end_a; ++a) sketch_a.Add(a);
13         Minisketch sketch_b = MakeMinisketch32(10);

The observing behavior:

0>build_msvc\x64\Release\test_bitcoin.exe -t minisketch_tests
1Running 1 test case...
2unknown location(0): fatal error: in "minisketch_tests/minisketch_test": memory access violation occurred at address 0xffffffff, while attempting to  read inaccessible data
3C:\Users\hebasto\bitcoin\src\test\minisketch_tests.cpp(18): last checkpoint: "minisketch_test" test entry
4
5*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

Assuming either MSVC bug or non-portable code?

hebasto added the label Bug on Oct 5, 2022

hebasto added the label Windows on Oct 5, 2022

hebasto added the label Tests on Oct 5, 2022

hebasto commented at 7:15 pm on October 5, 2022: member

cc @sipsorcery @sipa

Hoping on your insights as this issue is the current problem in #25797.

bitcoin deleted a comment on Oct 5, 2022

maflcko commented at 7:21 pm on October 5, 2022: member

BOOST_TEST_MESSAGE isn’t thread safe, but this shouldn’t be the issue. Does it also happen in valgrind/sanitizers?

Unrelated: The BOOST_CHECK needs to be replaced by BOOST_REQUIRE or Assert to avoid UB in case of failure.

hebasto commented at 10:48 am on October 6, 2022: member

With an additional diff as follows:

 0--- a/src/node/minisketchwrapper.cpp
 1+++ b/src/node/minisketchwrapper.cpp
 2@@ -40,7 +40,7 @@ uint32_t FindBestImplementation()
 3             for (uint64_t e = 0; e < 84; ++e) {
 4                 sketch.Add(e*1337 + b*13337 + offset);
 5             }
 6-            offset += (*sketch.Decode(32))[0];
 7+            offset += sketch.Decode(32).value()[0];
 8             auto stop = GetTimeMicros();
 9             benches.push_back(stop - start);
10         }

the error message is more descriptive:

0>build_msvc\x64\Release\test_bitcoin.exe -t minisketch_tests
1Running 1 test case...
2unknown location(0): fatal error: in "minisketch_tests/minisketch_test": class std::bad_optional_access: Bad optional access
3C:\Users\hebasto\bitcoin\src\test\minisketch_tests.cpp(18): last checkpoint: "minisketch_test" test entry
4
5*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

A quick debugging led to SketchImpl::Decode() which returns here:https://github.com/bitcoin/bitcoin/blob/5e82b9ba96b6c5614a1187382a086e5694dff544/src/minisketch/src/sketch_impl.h#L403

hebasto commented at 10:50 am on October 6, 2022: member

@MarcoFalke

Does it also happen in valgrind/sanitizers?

For binaries compiled with MSVC?

maflcko commented at 1:40 pm on October 6, 2022: member

See also https://github.com/bitcoin/bitcoin/issues/24664

maflcko referenced this in commit 73b61717a9 on Oct 6, 2022

sidhujag referenced this in commit 7e057fbc1e on Oct 6, 2022

hebasto commented at 12:23 pm on October 10, 2022: member

Opened an issue upstream.

hebasto commented at 6:23 pm on October 18, 2022: member

An upstream bugfix has been submitted.

hebasto added the label Upstream on Oct 18, 2022

fanquake closed this on Nov 1, 2022

sidhujag referenced this in commit 0d85218568 on Nov 1, 2022

bitcoin locked this on Nov 1, 2023

test: minisketch_tests.cpp can fail when built with MSVC #26262

test: `minisketch_tests.cpp` can fail when built with MSVC #26262