test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable

hebasto commented at 7:45 pm on May 2, 2025: member

This PR adds a way to suppress the abort message box when running test_bitcoin.exe and fuzz.exe built with the debug runtime library on Windows.

Otherwise, a failing assert() triggers the abort routine, which displays a message box and causes a timeout in CI.

Here are CI jobs for the “Debug” configuration:

on the master branch: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/14759346479/job/41435857332
with this PR: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15038743417/job/42265371841

Addresses this comment.

According to Microsoft’s docs, this behavior is MSCV-specific.

hebasto added the label Windows on May 2, 2025

hebasto added the label Tests on May 2, 2025

DrahtBot commented at 7:45 pm on May 2, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32409.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	shahsb

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in src/test/fuzz/fuzz.cpp:248 in f7f803f4e2 outdated

244@@ -245,6 +245,10 @@ int main(int argc, char** argv)
245         test_one_input(buffer);
246         return 0;
247     }
248+#ifdef WIN32

shahsb commented at 2:35 am on May 3, 2025:

0#ifdef _WIN32

_WIN32 is a compiler flag and could be used for both windows platform i.e windows_x86 (32-bit) as well as windows_x64 (64-bit)

hebasto commented at 6:44 am on May 3, 2025:

The current code is consistent with our entire code base.

shahsb commented at 2:36 am on May 3, 2025: none

Concept ACK

maflcko commented at 11:53 am on May 4, 2025: member

No objection, but assert/Assert seems to be used widely in the codebase, so shouldn’t this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.

hebasto force-pushed on May 4, 2025

hebasto renamed this:
~~fuzz: Suppress abort message on Windows~~
test: Suppress Windows abort message in CI
on May 4, 2025

hebasto commented at 3:56 pm on May 4, 2025: member

@maflcko

No objection, but assert/Assert seems to be used widely in the codebase, so shouldn’t this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.

Thanks! Reworked.

hebasto force-pushed on May 4, 2025

in src/test/util/setup_common.cpp:76 in 191bdd1f8e outdated

73@@ -73,6 +74,18 @@ using node::LoadChainstate;
74 using node::RegenerateCommitments;
75 using node::VerifyLoadedChainstate;
76 
77+#if defined(_MSC_VER)

fanquake commented at 1:33 pm on May 6, 2025:

The commit message says “On Windows”, but the code is using _MSC_VER, not WIN32. Is that a typo, or does this issue only affect MSVC compiled binaries; in which case, why is there a difference in runtime behaviour between the MSVC compiled test binaries & our release compiled test binaries?

hebasto commented at 12:37 pm on May 9, 2025:

This behaviour is MSVC-specific (I’ve added a note to the PR description). And I cannot reproduce it when running cross-compiled binaries.

fanquake commented at 3:36 pm on May 9, 2025:

I think the commit message should still be fixed to say When compiling with MSVC, rather than On Windows?

(I’ve added a note to the PR description)

Is it a bug in mingw-w64 that it doesn’t match the MSVC behaviour? It would be good to an explanation of the difference in the PR description/commit message, as it isn’t really clear from going to the link you’ve provided.

hebasto commented at 3:40 pm on May 9, 2025:

Is it a bug in mingw-w64 that it doesn’t match the MSVC behaviour?

I don’t think so, given the different runtime libraries used. I must admit I haven’t tested cross-compiled binaries linked to UCRT.

hebasto commented at 4:42 pm on May 6, 2025: member

Not related to this PR, but the error in the wallet_fees target:

0wallet_fees: succeeded against 23 files in 0s.
1Run wallet_fees with args ['D:\\a\\bitcoin-core-nightly\\bitcoin-core-nightly\\build\\bin\\Debug\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_corpora/wallet_fees')]
2\u26a0\ufe0f Failure generated from target with exit code 3221225477: ['D:\\a\\bitcoin-core-nightly\\bitcoin-core-nightly\\build\\bin\\Debug\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_corpora/wallet_fees')]

can be fixed with the following patch:

 0--- a/src/wallet/test/fuzz/fees.cpp
 1+++ b/src/wallet/test/fuzz/fees.cpp
 2@@ -15,22 +15,22 @@
 3 namespace wallet {
 4 namespace {
 5 const TestingSetup* g_setup;
 6-static std::unique_ptr<CWallet> g_wallet_ptr;
 7 
 8 void initialize_setup()
 9 {
10     static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
11     g_setup = testing_setup.get();
12-    const auto& node{g_setup->m_node};
13-    g_wallet_ptr = std::make_unique<CWallet>(node.chain.get(), "", CreateMockableWalletDatabase());
14 }
15 
16 FUZZ_TARGET(wallet_fees, .init = initialize_setup)
17 {
18+    SeedRandomStateForTest(SeedRand::ZEROS);
19     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
20+    SetMockTime(ConsumeTime(fuzzed_data_provider));
21     const auto& node{g_setup->m_node};
22     Chainstate* chainstate = &node.chainman->ActiveChainstate();
23-    CWallet& wallet = *g_wallet_ptr;
24+    std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(node.chain.get(), "", CreateMockableWalletDatabase())};
25+    CWallet& wallet{*wallet_ptr};
26     {
27         LOCK(wallet.cs_wallet);
28         wallet.SetLastBlockProcessed(chainstate->m_chain.Height(), chainstate->m_chain.Tip()->GetBlockHash());

cc @dergoegge @marcofleon

fanquake commented at 4:46 pm on May 6, 2025: member

Not sure about compiler specific changes, that rely on the presence of an undocumented environment variable (which we don’t explicitly set as far as I can tell?), to make tests work properly.

maflcko commented at 6:07 pm on May 6, 2025: member

Not related to this PR, but the error in the wallet_fees target:

0wallet_fees: succeeded against 23 files in 0s.
1Run wallet_fees with args ['D:\\a\\bitcoin-core-nightly\\bitcoin-core-nightly\\build\\bin\\Debug\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_corpora/wallet_fees')]
2\u26a0\ufe0f Failure generated from target with exit code 3221225477: ['D:\\a\\bitcoin-core-nightly\\bitcoin-core-nightly\\build\\bin\\Debug\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_corpora/wallet_fees')]

The patch looks odd. Is this just an issue about the destruction order between wallet and node? (See also siof: https://en.cppreference.com/w/cpp/language/siof). If yes, could just ensure the wallet is destructed before the node?

Edit: Though, if the exec/s are not decreasing with the patch, it seems fine as well. Could create a pull with it?

hebasto commented at 4:30 pm on May 8, 2025: member

Not sure about compiler specific changes, that rely on the presence of an undocumented environment variable (which we don’t explicitly set as far as I can tell?), to make tests work properly.

It is documented, and it is pre-defined by CI itself:

in GitHub Actions:

CI | Always set to true.

in Cirrus CI:

CI | [pre-defined to] true

And I assume it’s a common practice among major CIs.

fanquake commented at 3:37 pm on May 9, 2025: member

It is documented, and it is pre-defined by CI itself:

It’s not in our docs, or in our CI code, which means our CI would “work”, because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a .cpp file.

Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If so, how would they figure that out?

hebasto commented at 3:48 pm on May 9, 2025: member

It is documented, and it is pre-defined by CI itself:

It’s not in our docs, or in our CI code, which means our CI would “work”, because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a .cpp file.

What are you suggesting should be changed to improve or avoid this situation?

Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If so, how would they figure that out?

Only CI jobs defined by scripts in the ci/test/ directory are intended to be runnable locally. This does not apply to the “raw” GitHub Actions workflows, where this code takes effect.

fanquake commented at 3:49 pm on May 9, 2025: member

This does not apply to the “raw” GitHub Actions workflows, where this code takes effect.

Can’t someone just compile and run the same binaries locally?

hebasto commented at 3:53 pm on May 9, 2025: member

This does not apply to the “raw” GitHub Actions workflows, where this code takes effect.

Can’t someone just compile and run the same binaries locally?

Certainly, they can. But displaying a message box isn’t an issue when running binaries locally, so there’s no need to alter the environment in that case.

hebasto commented at 3:58 pm on May 9, 2025: member

As an alternative, we could explicitly set our own SUPPRESS_ABORT_MESSAGE_BOX environment variable in the workflow, and gate the code with this variable instead, ~~removing the #if defined(_MSC_VER) condition~~.

fanquake referenced this in commit 8a65f03894 on May 14, 2025

test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable

This change adds a way to suppress the abort message box when running
test or fuzz executables built with the debug runtime library on
Windows. Without this, the message box may cause timeouts in CI.

8d674717df

hebasto force-pushed on May 15, 2025

hebasto renamed this:
~~test: Suppress Windows abort message in CI~~
test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable
on May 15, 2025

hebasto commented at 6:56 am on May 15, 2025: member

It is documented, and it is pre-defined by CI itself:

It’s not in our docs, or in our CI code, which means our CI would “work”, because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a .cpp file.

Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If so, how would they figure that out?

Reworked to avoid relying on third-party set environment variables.

hebasto commented at 10:31 am on May 15, 2025: member

This issue seems related: https://developercommunity.visualstudio.com/t/Debug-CRT-shows-abort-message-box-instea/10904782.

fanquake commented at 11:53 am on May 21, 2025: member

This PR adds a way to suppress the abort message box when running test_bitcoin.exe and fuzz.exe built with the debug runtime library on Windows.

Given this isn’t being used anywhere in our CI, it’s no-longer clear why this is needed (we don’t generally add unused code to the project). This also still isn’t documented anywhere, so it’s not clear how anyone who would need to use this, would know that it exists.

test: Introduce SUPPRESS_ABORT_MESSAGE environment variable #32409

Code Coverage & Benchmarks

Reviews

test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable #32409