RegQueryValueExA(HKEY_PERFORMANCE_DATA, ...) sometimes hangs bitcoind.exe on Windows during startup, at least on CI.
We have other sources of entropy to seed randomness with on Windows, so should be alright removing this. Might resurrect if less drastic fix is found.
Hopefully sufficient to fix #30390.
CI debugged with temporary Windows stack trace dumping + Symbols in #30956.
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.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
For brief context, in 3 of 3 checked Windows stack dumps, the main callstack where it hangs has been:
0 ntdll.dll!NtReleaseSemaphore() Unknown
1 KERNELBASE.dll!ReleaseSemaphore() Unknown
2 WmiApRpl.dll!WmiReverseGuard::LeaveRead(long) Unknown
3 WmiApRpl.dll!WmiReverseMemoryExt<class WmiReverseGuard>::Read(void *,unsigned long,unsigned long *,unsigned long) Unknown
4 WmiApRpl.dll!WmiAdapterWrapper::GetValidity(unsigned long) Unknown
5 WmiApRpl.dll!WmiAdapterWrapper::CollectObjects(unsigned short const *,void * *,unsigned long *,unsigned long *) Unknown
6 WmiApRpl.dll!WmiAdapterWrapper::Collect(unsigned short const *,void * *,unsigned long *,unsigned long *) Unknown
7 WmiApRpl.dll!WmiCollectPerfData(unsigned short const *,void * *,unsigned long *,unsigned long *) Unknown
8 advapi32.dll!CallExtObj() Unknown
9 advapi32.dll!QueryV1Provider() Unknown
10 advapi32.dll!QueryExtensibleData() Unknown
11 advapi32.dll!PerfRegQueryValueEx() Unknown
12 advapi32.dll!PerfRegQueryValue() Unknown
13 KERNELBASE.dll!BaseRegQueryValueInternal() Unknown
14> KERNELBASE.dll!RegQueryValueExA() Unknown
15 bitcoind.exe!`anonymous namespace'::RandAddSeedPerfmon(CSHA512 & hasher) Line 86 C++
16 bitcoind.exe!RandAddDynamicEnv(CSHA512 & hasher) Line 235 C++
17 bitcoind.exe!`anonymous namespace'::SeedStartup(CSHA512 & hasher, `anonymous-namespace'::RNGState & rng) Line 624 C++
18 bitcoind.exe!`anonymous namespace'::ProcRand(unsigned char * out, int num, `anonymous-namespace'::RNGLevel level, bool always_use_real_rng) Line 662 C++
19 [Inline Frame] bitcoind.exe!kernel::Context::{ctor}::__l2::<lambda_1>::operator()() Line 21 C++
20 [Inline Frame] bitcoind.exe!std::invoke(kernel::Context::{ctor}::__l2::<lambda_1> &&) Line 1704 C++
21 bitcoind.exe!std::call_once<`kernel::Context::Context'::`2'::<lambda_1>>(std::once_flag & _Once, kernel::Context::{ctor}::__l2::<lambda_1> && _Fx) Line 103 C++
22 bitcoind.exe!kernel::Context::Context() Line 23 C++
23 [Inline Frame] bitcoind.exe!std::make_unique() Line 3597 C++
24 bitcoind.exe!AppInit(node::NodeContext & node) Line 188 C++
25 bitcoind.exe!main(int argc, char * * argv) Line 277 C++
26 [Inline Frame] bitcoind.exe!invoke_main() Line 78 C++
27 bitcoind.exe!__scrt_common_main_seh() Line 288 C++
28 kernel32.dll!BaseThreadInitThunk() Unknown
29 ntdll.dll!RtlUserThreadStart() Unknown
…suggesting some kind of (semaphore-related) deadlock sometimes occurs.
187@@ -227,8 +188,6 @@ void AddAllCPUID(CSHA512& hasher)
188
189 void RandAddDynamicEnv(CSHA512& hasher)
190 {
191- RandAddSeedPerfmon(hasher);
SeedPeriodic() in random.cpp.
Adjusted in latest push, new comment:
0// Dynamic environment data (clocks, resource usage, ...)
winreg.h.
RegQueryValueExA(HKEY_PERFORMANCE_DATA, ...) sometimes hangs bitcoind.exe on Windows during startup, at least on CI.
We have other sources of entropy to seed randomness with on Windows, so should be alright removing this. Might resurrect if less drastic fix is found.
post-merge utACK https://github.com/bitcoin/bitcoin/pull/31124/commits/9bb92c0e7ffe2426b4afb80ddd4b4c96968316b5
I inspected the call stacks from the dumps of the failed runs hodlinator caused in Visual Studio and confirmed that the RegQueryValueExA call was responsible for every failure, I imagine that this bug may also be infrequently experienced by real windows users outside of CI.
I haven’t thought very seriously about what high quality entropy sources are and whether or not we have enough of them on Windows without this performance data, but I’m interested in the potential follow-ups suggested by @hodlinator (https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2427711698) that reintroduce performance data entropy on Windows.
I haven’t thought very seriously about what high quality entropy sources are and whether or not we have enough of them on Windows without this performance data
The high quality OS entropy source on windows is CryptGenRandom, and we use that.
Not opposed to collecting other ancillary data to mix in, though let’s try to avoid something as crude as pulling data from all drivers and processes on the system.
The high quality OS entropy source on windows is
CryptGenRandom, and we use that.
A Microsoft article from 2021 on CryptGenRandom states:
Important This API is deprecated. New and existing software should start using Cryptography Next Generation APIs. Microsoft may remove this API in future releases.
So maybe we could create an issue to migrate to a more modern API?