Rebased version of #4236 by @arowser, changed to use begin_ptr from #4293. Also use vXXXX variable name syntax for the vectors...
Testing: I'm a bit wary to merge as it affects entropy collection for randomness. I'd like someone with windows to verify that this does the right thing (so, start with -debug=rand and look for the RandAddSeed() XXX bytes message).
Tried testing on wine, but the RegQueryValueExA(HKEY_PERFORMANCE_DATA) fails there :( (no such key)
A bitcoind.exe to test with is available here: https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-gbe873f6.zip (gpg sigs https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-gbe873f6.zip.sig)
Testing on Windows 8 64bit with @laanwj's binary, my log shows
2014-06-23 18:04:36 RandAddSeedPerfmon: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code 234
I'm not sure how to further debug this. My regular Bitcoin 0.9.1 GUI does not seem to show any randomness-related debug messages (started with -debug).
@leofidus Thanks for testing!
The regular client doesn't show any messages if the call fails, only if it succeeds. In the second commit of this pull I added that warning. So - it's failing in 0.9.1 as well.
But the error is different than mine on wine. Error 234 is ERROR_MORE_DATA. I doubt this ever worked at all.
According to the doc:
If the buffer specified by lpData parameter is not large enough to hold the data, the function returns ERROR_MORE_DATA and stores the required buffer size in the variable pointed to by lpcbData. In this case, the contents of the lpData buffer are undefined.
So handle this error properly we need to resize the buffer to the returned size and re-call the function. Edit: no, that doesn't work specifically for HKEY_PERFORMANCE_DATA according to the above doc.
My official 0.9.1 client shows, that it's working here 2014-06-24 07:19:08 RandAddSeed() 219352 bytes. Didn't yet try this patch...
@diapolo Thanks for testing, although at this point I doubt that it depends on this patch. So at least it works in some cases.
Do you perhaps know if HKEY_PERFORMANCE_DATA returns a variable number of bytes that may be >250000?
Ah, got it:
If hKey specifies
HKEY_PERFORMANCE_DATAand thelpDatabuffer is not large enough to contain all of the returned data,RegQueryValueExreturnsERROR_MORE_DATAand the value returned through the lpcbData parameter is undefined. This is because the size of the performance data can change from one call to the next. In this case, you must increase the buffer size and callRegQueryValueExagain passing the updated buffer size in the lpcbData parameter. Repeat this until the function succeeds. You need to maintain a separate variable to keep track of the buffer size, because the value returned by lpcbData is unpredictable.
So we need a loop.
@laanwj We had a debate about that part of the code in 09. 2012, see https://bitcointalk.org/index.php?topic=113496.msg1225880#msg1225880
@laanwj What do you think about removing that source of entropy and use the debug.log with RAND_load_file() (https://www.openssl.org/docs/crypto/RAND_load_file.html)? The data should be quite random after the initial startup.
Wut? It is known since 2012? I'm extremely disappointed and surprised here. Why did no one ever fix that or escalate it beyond a silly forum topic? Getting the performance data properly is simple, an example is even given in the MSDN documentation that I link above.
I don't think debug.log is a good source of entropy as it is - up to a point - controllable by the outside world through the network. But I'm not an expert on that. If you implement an extra entropy source for that be sure to just use part of the file, not all of it, or it could slow down start-up.
@laanwj I proposed a change (https://bitcointalk.org/index.php?topic=113496.msg1228012#msg1228012) and if I remember correctly also had a pull open for this. It was decided to leave as is.
Are you going to rework this to include a loop? I tend to just remove it, but have no strong enough feeling yet.
Yes, I'm going to turn it into a loop. We have a dearth of entropy sources on windows so removing any sounds extremely unwise.
Ok, it should always collect now unless there is >10MB of performance data.
A bitcoind.exe to test with is available here: https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-g3b8c1da.zip (gpg sigs https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-g3b8c1da.zip.sig)
Again, can someone with windows test the above bitcoin-win-v0.9.99.0-g3b8c1da.zip and start with -debug=rand and look for the RandAddSeedPerfmon() XXX bytes message in debug.log?
195 | + LogPrint("rand", "%s: %lu bytes\n", __func__, nSize); 196 | + } else { 197 | + static bool warned = false; // Warn only once 198 | + if (!warned) 199 | + { 200 | + LogPrintf("%s: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);
Nit: Can we define to add a space before the function name :)? Also should this be part of the "rand" category or not? Which is a thing I need to consider with my OpenSSL related pulls also...
I don't understand, the function name is at the beginning of the line, adding a space there makes no sense.
And this is not in the rand category on purpose because people need to see it. It is not just for debugging. And it only emits the warning once anyway.
Just talking about this changed to this anyway:
LogPrintf("%s : Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);
We seem to use that syntax more often.
That's inconsistent too. The second : has no space before it. Also looks strange to me. In English you write like this: there is never a space before a (semi)colon. Anyhow, I'm not going to waste time discussing the formatting of log messages, sorry.
167 | @@ -168,16 +168,31 @@ void RandAddSeedPerfmon() 168 | #ifdef WIN32 169 | // Don't need this on Linux, OpenSSL automatically uses /dev/urandom 170 | // Seed with the entire set of perfmon data 171 | - unsigned char pdata[250000]; 172 | - memset(pdata, 0, sizeof(pdata)); 173 | - unsigned long nSize = sizeof(pdata); 174 | - long ret = RegQueryValueExA(HKEY_PERFORMANCE_DATA, "Global", NULL, NULL, pdata, &nSize); 175 | + std::vector <unsigned char> vData(250000,0); 176 | + long ret;
Nit: I would like to see these 2 vars initialized with 0, what do you think?
Fine with me...
I'm currently testing this...
Currently we use a fixed buffer of 250000 bytes to request
HKEY_PERFORMANCE_DATA. In many cases this is not enough, causing the
entropy collection to be skipped.
Use a loop that grows the buffer as specified in the RegQueryValueEx
documentation:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724911%28v=vs.85%29.aspx
(as the size of the performance data can differ for every call, the
normal solution of requesting the size then allocating that can't work)
<pre> 2014-06-26 13:16:33 RandAddSeedPerfmon : 215208 bytes 2014-06-26 13:26:33 RandAddSeedPerfmon : 205416 bytes </pre>
Seems to work :).
ACK
Thanks for testing. I added the 0 initializations. Ready for merge now.
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4392_8ae973c00cfb02161bb2c2a6c839e510cd409278/ for binaries and test log.
This could happen for one of several reasons:
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.