New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

start using reproducible random numbers #98

Merged
merged 4 commits into from Feb 5, 2019

Conversation

Projects
None yet
4 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Feb 4, 2019

Currently we have Travis running random.random() to determine if someone has won. This of course gives different values each time you run it. This is basically fine, but it would be better to use a hash of something that (a) we all can verify and (b) we can't predict. Then we don't have to trust Travis or worry about people re-running Travis builds.

This PR switches us to using the hash of the merge commit as our source of randomness.

As a sanity check on the math I generated the random numbers corresponding to the 194 commits we have on master, to see that it really does give the full range of values:

screen shot 2019-02-04 at 10 04 53 am

https://docs.google.com/spreadsheets/d/1PwRT8q5OdjWsZsGTHwZSlw4zCjy2yaUvKi0UX--7Wtk/edit#gid=0

start using reproducible random numbers
Currently we have Travis running random.random() to determine if someone has won.  This of course gives different values each time you run it.  This is basically fine, but it would be better to use a hash of something that (a) we all can verify and (b) we can't predict.  Then we don't have to trust Travis or worry about people re-running Travis builds.

This PR switches us to using the hash of the merge commit as our source of randomness.  We only get one of these, so this depends on the fix in #97.

@jeffkaufman jeffkaufman added the reviewme label Feb 4, 2019

@@ -94,9 +93,33 @@ def determine_if_mergeable(pr):
def determine_if_winner():
print_points()

# Pick a winner at random with a single random number. We divide the number

This comment has been minimized.

@pavellishin

pavellishin Feb 4, 2019

Collaborator

#97 changes leaked in.

This comment has been minimized.

@jeffkaufman

jeffkaufman Feb 4, 2019

Author Owner

The #97 changes are here intentionally (see PR description). Without the #97 change we call random() multiple times, but util.random() only returns a single random number at any given master commit.

This comment has been minimized.

@jeffkaufman

jeffkaufman Feb 4, 2019

Author Owner

We can wait to review this until #97 is merged, though, if you like.

This comment has been minimized.

@pavellishin

pavellishin Feb 4, 2019

Collaborator

I almost want to throw caution to the wind; this isn't work, this is a game! But yeah, we probably should :P

This comment has been minimized.

@jeffkaufman

jeffkaufman Feb 4, 2019

Author Owner

No problem!

(This isn't really a caution-to-the-wind sort of game...)

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Feb 4, 2019

We can't reasonably predict this because we can't control Github's merge timestamp, right?

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 4, 2019

We can't reasonably predict this because we can't control Github's merge timestamp, right?

Right, otherwise this wouldn't work.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 4, 2019

Looks good but let's merge #97 first.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 4, 2019

@tnelling @csvoss @pavellishin ready for review, now that #97 is in

Show resolved Hide resolved validate.py

@tnelling tnelling requested a review from csvoss Feb 5, 2019

@csvoss

csvoss approved these changes Feb 5, 2019

Copy link
Contributor

csvoss left a comment

Might be vulnerable to timing attacks?

@csvoss csvoss merged commit 1d6c32e into master Feb 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

csvoss added a commit that referenced this pull request Feb 5, 2019

Merge pull request #98 from jeffkaufman/consistent-random
start using reproducible random numbers
@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 5, 2019

Congratulations @csvoss!

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 5, 2019

Was that actually a timing attack, or coincidence?

@csvoss

This comment has been minimized.

Copy link
Contributor

csvoss commented Feb 5, 2019

Definitely a timing attack. :)

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 5, 2019

Care to explain? Was "We can't reasonably predict this because we can't control Github's merge timestamp, right?" incorrect?

@jeffkaufman jeffkaufman referenced this pull request Feb 5, 2019

Closed

@csvoss wins! #100

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 5, 2019

Was "We can't reasonably predict this because we can't control Github's merge timestamp, right?" incorrect?

Totally incorrect: the timestamp just goes to the second. Merging to a specific second seems pretty doable.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 5, 2019

The bigger problem is I can't figure out how @csvoss avoided having github sign the commit, which makes the hash unpredictable.

@csvoss

This comment has been minimized.

Copy link
Contributor

csvoss commented Feb 5, 2019

edit: explanation moved to #100

@tnelling tnelling referenced this pull request Feb 5, 2019

Closed

Play again? #105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment