<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Thanks, sounds mostly like an overly finicky test. So I concur
that you should make it soft for now, then figure out the right
test. I appreciate the clarification... <br>
</p>
<div class="moz-cite-prefix">On 1/18/2019 6:31 PM, Junxiao Shi
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAOFH+OZCT05nQWd=q83s80KyRR1Tuj9PG-uAZgwn_Uf+L5tX5g@mail.gmail.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div>
<div dir="auto">Hi Fred</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">There are two RNGs involved:</div>
<div dir="auto">1. C++ STL’s mt19937 seeded with 256 bits of
entropy; a developer wanted to conserve OS entropy source, so
that it did not use more entropy.</div>
<div dir="auto">2. OpenSSL’s RAND_bytes function.</div>
<div dir="auto"><br>
</div>
<div dir="auto">The original statistics method is a K-S test for
goodness of fit. The implementation checks lower 5 bits of the
random number over 35 samples.</div>
<div dir="auto">The test rewritten in attempted fix D is also K-S
test. It checks upper 8 bits of the random number over 50
samples.</div>
<div dir="auto"><br>
</div>
<div dir="auto">My vote for short term would be E: revert it to
soft failure, so that it does not block other commits; but the
warning is kept as a reminder of this problem.</div>
<div dir="auto">For longer term, I could prefer F or G over D, but
this needs someone that actually understands statistics.
Developer who wrote D is a beginner in statistics.</div>
<div dir="auto"><br>
</div>
<div>
<div dir="auto">Yours, Junxiao</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Fri, Jan 18, 2019 at 18:11 Fred Douglis <<a
href="mailto:fdouglis@perspectalabs.com"
moz-do-not-send="true">fdouglis@perspectalabs.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<p>Can I vote "none of the above"?</p>
<p>I think checking the RNG was a good idea, and simply
removing the test sounds bad. If in fact it's not
really conforming to the expected distribution, this is
a problem. You haven't really said much about the
statistical method that declared it was failing, other
than that you could repeat the test a few times and
dramatically bring down the failure rate. If you trust
the test, it sounds like the RNG is in fact broken, but
maybe it's more a bad test than a bad RNG. If the test
is good, then it seems like you missed opinion F, which
is to keep it as a hard failure and <b>force a fix or
change of libraries</b>. If the test is bad, then
opinion G is, <b>find a better test</b>. <br>
</p>
<p>Not that my opinion counts much, I'm new to the list,
and so far simply a lurker. <br>
</p>
<p>Fred<br>
</p>
</div>
<div>
<div class="m_-4037743747416091094moz-cite-prefix">On
1/18/2019 6:00 PM, Junxiao Shi wrote:<br>
</div>
</div>
<div>
<blockquote type="cite">
<div dir="auto">Dear folks</div>
<div dir="auto"><br>
</div>
<div dir="auto">There is an urgent developer
disagreement during code review related to issue 4808.
It seems that this could not wait until the next NFD
call, so I’ll explain the facts here.</div>
<div dir="auto"><br>
</div>
<div dir="auto">The technical problem: ndn-cxx has a
random number generator implemented by calling into
third party libraries. There was a unit test using
statistics method to check that the generated random
number conforms to a uniform distribution. Given it’s
a randomized test, the unit test fails “softly”: it
creates a warning when fails, not an error.</div>
<div dir="auto">Recent changes: developer A made a
commit changing the soft failure to hard failure. As a
result, many Jenkins builds are failing. An
independent test indicates the failure rate is 14% or
more.</div>
<div dir="auto"><br>
</div>
<div dir="auto">Attempted fix A: delete the unit test
outright because it’s “semi-broken”. Afterwards, there
would be no unit test to check the numbers are random.</div>
<div dir="auto">Opinion B: “testing random number
generator is not ndn-cxx’s business”, so the unit test
can be deleted.</div>
<div dir="auto">Opinion C: every line of code requires
at least one failing test. Therefore, without any unit
test on the random number generator, one could just
make “return 0;” the random number generator.</div>
<div dir="auto">Attempted fix D: rewrite the “goodness
of fit” unit test, loosening numerical requirements.
Execute the test five times, and declare a hard
failure only if the test fails three or more times out
of five. The failure rate of this method is 0.01%.</div>
<div dir="auto">Opinion E: revert to soft failure.</div>
<div dir="auto"><br>
</div>
<div dir="auto">I am one of the parties involved but I
tried to summerize the facts. I hope everyone
(including myself) can calm down and make a decision
at next NFD call, and don’t rush with merging one of
the changes. Please keep all discussions on the
mailing list by using reply-all only.</div>
<div dir="auto"><br>
</div>
<div dir="auto">Yours, Junxiao</div>
</blockquote>
</div>
<div> </div>
</blockquote>
</div>
</div>
</blockquote>
<div class="moz-signature">-- <br>
<img src="cid:part2.64FDA6A2.89E8EBCC@perspectalabs.com"
border="0"></div>
</body>
</html>