In the code which selects which coins to use, the following appears:
if (nPass == 0 ? rand() % 2 : !vfIncluded[i])
but rand() is never seeded.
Should we use GetRandInt(2) instead to get unpredictable results?
Probably. It is the only place in the source where rand() is used.
I see it in commented code in main.cpp too:
//while (rand() % 3 == 0)
I wonder if perhaps it's deliberate, because the wallet rand() code is in a tight loop and we don't want to exhaust our entropy source.
That loop execute a great many times— a proper cryptographically secure RNG is probably going to be noticeably slow here. If it is, then would anyone object to using a fast PRNG seeded by RandBytes at the start of the function?
In general I don't think there is a big privacy concern here— even knowing the random sequence completely shouldn't matter unless you also know the inputs under consideration.
@gmaxwell also, rand() is not thread-safe according to its man page, using rand_r(*seed) is recommended in that case is as it has explicit state.
If there is no privacy concern here, a simple solution would be using rand_r with the state (just one int) generated at the beginning of the function with GetRandInt().
We don't use rand() anymore but our own insecure_rand() for non-security-critical randomness that does get seeded. Closing this.