ApproximateBestSubset internal RNG to prevent degenerate behavior. #2312

Merged
merged 1 commit into from Feb 22, 2013

Conversation

Projects
None yet
5 participants
Member

gmaxwell commented Feb 15, 2013

This fixes test_bitcoin failures on openbsd reported by dhill on IRC.

On some systems rand() is a simple LCG over 2^31 and so it produces
an even-odd sequence. ApproximateBestSubset was only using the least
significant bit and so every run of the iterative solver would be the
same for some inputs, resulting in some pretty dumb decisions.

Using something other than the least significant bit would paper over
the issue but who knows what other way a system's rand() might get us
here. Instead we use an internal RNG with a period of something like
2^60 which is well behaved. This also makes it possible to make the
selection deterministic for the tests, if we wanted to implement that.

+extern uint32_t insecure_rand_Rw;
+static inline uint32_t insecure_rand(void)
+{
+ insecure_rand_Rz=36969*(insecure_rand_Rz&65535)+(insecure_rand_Rz>>16);
@Diapolo

Diapolo Feb 16, 2013

Indentation :)

-
+uint32_t insecure_rand_Rz = 11;
+uint32_t insecure_rand_Rw = 11;
+void seed_insecure_rand(bool fDeterministic)
@Diapolo

Diapolo Feb 16, 2013

Nit, I'm missing some spaces here and there, I think it's terrible to read no?

Automatic sanity-testing: WARNING, see http://jenkins.bluematt.me/pull-tester/9a0864b9ae3777d229694ab6d20736f05e4f9ca7 for binaries and test log.

This pull decreases total test coverage, please add unit tests to test all new code and help us add test cases for existing code.
Coverage report can be found at http://jenkins.bluematt.me/pull-tester/9a0864b9ae3777d229694ab6d20736f05e4f9ca7/bitcoin/src/total.coverage/

Automatic sanity-testing: WARNING, see http://jenkins.bluematt.me/pull-tester/74ba33905791b34153e8ad0e469c878b4900a5ef for binaries and test log.

This pull decreases total test coverage, please add unit tests to test all new code and help us add test cases for existing code.
Coverage report can be found at http://jenkins.bluematt.me/pull-tester/74ba33905791b34153e8ad0e469c878b4900a5ef/bitcoin/src/total.coverage/

Internal RNG for approximateBestSubset to prevent degenerate behavior.
This fixes test_bitcoin failures on openbsd reported by dhill on IRC.

  On some systems rand() is a simple LCG over 2^31 and so it produces
an even-odd sequence.  ApproximateBestSubset was only using the least
significant bit and so every run of the iterative solver would be the
same for some inputs, resulting in some pretty dumb decisions.

Using something other than the least significant bit would paper over
the issue but who knows what other way a system's rand() might get us
here.  Instead we use an internal RNG with a period of something like
2^60 which is well behaved.  This also makes it possible to make the
selection deterministic for the tests, if we wanted to implement that.
Member

gmaxwell commented Feb 18, 2013

@BitcoinPullTester Because you asked so nicely…

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/907a2aa4c78833ce93455567ae10ff2f506e752e for binaries and test log.

gavinandresen added a commit that referenced this pull request Feb 22, 2013

Merge pull request #2312 from gmaxwell/random_random
ApproximateBestSubset internal RNG to prevent degenerate behavior.

@gavinandresen gavinandresen merged commit aaeb443 into bitcoin:master Feb 22, 2013

Diapolo commented Feb 22, 2013

Really, I can be a fucking damn nit-picker, but that indentation thing could have been fixed. I'm thinking about never doing anything coding style related in core (if that is what you guys want, then win-win :-P).

Member

gmaxwell commented Feb 22, 2013

@Diapolo I did change the indentation. Please don't be rude about it, I am listening and I do care... but as much as people shouldn't be offended when you make nits, you shouldn't be offended by people's responses.

Personally, I'm fine to have your style nits, my preferred style is something else no one here wants— and sometimes a bit of it leaks through, so I'm happy to have people point it out and I'll change it. (which I did here!)

Owner

laanwj commented Feb 22, 2013

That's probably best for your heart-rate Diapolo :) Learn to stop worrying and love the inconsistent indentation. On the other hand, if you submit a pull that only fixes the indentation on those few lines I'll merge it for you.

Diapolo commented Feb 22, 2013

No offence to anyone :), I just though out loud ^^. I think @laanwj your suggestion to keep a look at my blood-pressure and heart-rate is the best I can and will do :D.

laudney pushed a commit to reddcoin-project/reddcoin that referenced this pull request Mar 19, 2014

Merge pull request #2312 from gmaxwell/random_random
ApproximateBestSubset internal RNG to prevent degenerate behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment