Skip to content
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

The randomness algorithm implements Sattolo's algorithm instead of Fisher-Yates #1

Open
cleanunicorn opened this issue Oct 6, 2021 · 0 comments

Comments

@cleanunicorn
Copy link
Member

Description

The randomness function is called once to shuffle the tickets.

https://github.com/monoceros-alpha/review-elrond-launchpad-2021-10/blob/a32fe0b2ef7430e9b86d6b22da60b697f8eaac3c/code/launchpad/src/launchpad.rs#L123-L125

This method needs to loop over each position, select a random ticket and put the ticket at that position. The method shuffle_single_ticket is called to pick a random ticket and place it in the current position.

https://github.com/monoceros-alpha/review-elrond-launchpad-2021-10/blob/a32fe0b2ef7430e9b86d6b22da60b697f8eaac3c/code/launchpad/src/launchpad.rs#L136-L143

After each swap, the position is incremented by 1.

https://github.com/monoceros-alpha/review-elrond-launchpad-2021-10/blob/a32fe0b2ef7430e9b86d6b22da60b697f8eaac3c/code/launchpad/src/launchpad.rs#L136-L143

The method shuffle_single_ticket picks a random ticket and places it in the current position (current_ticket_position).

https://github.com/monoceros-alpha/review-elrond-launchpad-2021-10/blob/a32fe0b2ef7430e9b86d6b22da60b697f8eaac3c/code/launchpad/src/launchpad.rs#L438-L446

This part selects a number between current_ticket_position + 1 and last_ticket_position inclusive.

https://github.com/monoceros-alpha/review-elrond-launchpad-2021-10/blob/a32fe0b2ef7430e9b86d6b22da60b697f8eaac3c/code/launchpad/src/launchpad.rs#L447-L448

The Fisher-Yates algorithm states that you should pick a number between the current position and the last position. The current position should be included in the selection range.

Wikipedia describes a variation of this algorithm, named Sattolo's algorithm.

A very similar algorithm was published in 1986 by Sandra Sattolo for generating uniformly distributed cycles of (maximal) length n.[6][7] The only difference between Durstenfeld's and Sattolo's algorithms is that in the latter, in step 2 above, the random number j is chosen from the range between 1 and i−1 (rather than between 1 and i) inclusive. This simple change modifies the algorithm so that the resulting permutation always consists of a single cycle.

The current code actually implements the Sattolo's algorithm because it selects items to swap from the current_ticket_position + 1 instead of current_ticket_position.

The Wikipedia page warns this can be easy to accidentally implement.

In fact, as described below, it is quite easy to accidentally implement Sattolo's algorithm when the ordinary Fisher–Yates shuffle is intended. This will bias the results by causing the permutations to be picked from the smaller set of (n−1)! cycles of length N, instead of from the full set of all n! possible permutations.

Technically the current code version implements Sattolo's algorithm instead of Fisher-Yates which is mentioned in the comments.

https://github.com/monoceros-alpha/review-elrond-launchpad-2021-10/blob/a32fe0b2ef7430e9b86d6b22da60b697f8eaac3c/code/launchpad/src/launchpad.rs#L438-L440

Recommendation

Change the selection interval to include the current_ticket_position when selecting a random number.

This code should use current_ticket_position instead of current_ticket_position + 1.

https://github.com/monoceros-alpha/review-elrond-launchpad-2021-10/blob/a32fe0b2ef7430e9b86d6b22da60b697f8eaac3c/code/launchpad/src/launchpad.rs#L447-L448

References

@cleanunicorn cleanunicorn changed the title The randomness algorithm implements Sattolo's algorithm The randomness algorithm implements Sattolo's algorithm instead of Fisher-Yates Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant