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

crypto: replace rand<T>()%N idiom with unbiased rand_idx(N) #5392

Merged
merged 1 commit into from Apr 15, 2019

Conversation

Projects
None yet
4 participants
@stoffu
Copy link
Contributor

stoffu commented Apr 3, 2019

No description provided.

@vtnerd
Copy link
Contributor

vtnerd left a comment

There is also std::uniform_int_distribution. The light wallet server implemented a random_device then used uniform_int_distribution with it. Being a C++ functor make its slightly easier to compose in certain situations, although those use cases seem to be rare in this codebase.

/* Generate a random value between 0 and range_max-1 in an unbiased way
*/
template<typename T>
typename std::enable_if<std::is_pod<T>::value, T>::type rand_range(T range_max) {

This comment has been minimized.

Copy link
@vtnerd

vtnerd Apr 3, 2019

Contributor

is_pod is not sufficient, is_integral is a bit better.

{
res = rand<T>();
}
while (res >= max_value);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Apr 3, 2019

Contributor

Is the lower bound of the range implied 0 or implied std::numeric_limits<T>::min() ? This implementation is currently the former, but I'm not sure if allowing signed types is a good thing, it seems like the implementation could bust in those situations.

@stoffu stoffu force-pushed the stoffu:rand-range branch from ef68bc3 to ade2956 Apr 4, 2019

@stoffu

This comment has been minimized.

Copy link
Contributor Author

stoffu commented Apr 4, 2019

@vtnerd
I've replaced is_pod with is_unsigned to limit the usage to the very common idiom (rand()%N) addressed here. If a need for handling more general cases arises later, a new function can then be added.

@vtnerd

This comment has been minimized.

Copy link
Contributor

vtnerd commented Apr 4, 2019

Sounds like I was unable to persuade you to use the STL function. Humbug. Might be worth handling the 0 case too. Or perhaps the documentation implies 0 should be filtered by caller?

@stoffu stoffu force-pushed the stoffu:rand-range branch from ade2956 to 559f2a5 Apr 4, 2019

@stoffu

This comment has been minimized.

Copy link
Contributor Author

stoffu commented Apr 4, 2019

Now I'm persuaded to use the STL :)

@vtnerd
Copy link
Contributor

vtnerd left a comment

Well, I'm a terrible person. Your initial implementation was decent and didn't require - 1 everywhere. But "inclusion" ranges is more natural for this use case.

Anyway, another suggestion (sorry).

std::mt19937 gen(rd());
std::uniform_int_distribution<int> dis(0, dns_urls.size() - 1);
size_t first_index = dis(gen);
size_t first_index = crypto::rand_range<size_t>(0, dns_urls.size() - 1);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Apr 4, 2019

Contributor

Well, getting to update this at the same time is nice.

*/
template<typename T>
typename std::enable_if<std::is_integral<T>::value, T>::type rand_range(T range_min, T range_max) {
std::random_device rd;

This comment has been minimized.

Copy link
@vtnerd

vtnerd Apr 4, 2019

Contributor

We might want to define our own random generator to keep the behavior the same. The mt19937 isn't a crypto-secure generator:

  struct random_device
  {
    typedef uint64_t result_type;
    static constexpr result_type min() { return 0; }
    static constexpr result_type max() { return result_type(-1); }
    result_type operator()() const { return crypto::rand<result_type>(); }
  };

will work directly with uniform_int_distribution and doesn't do a syscall each time (similar to mt133937 usage).

@stoffu stoffu force-pushed the stoffu:rand-range branch from 559f2a5 to ab1d80f Apr 4, 2019

@stoffu

This comment has been minimized.

Copy link
Contributor Author

stoffu commented Apr 4, 2019

Thank you so much for the suggestion, I think the patch is a lot better than original now :)

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Apr 4, 2019

I liked the original version with a single param. Maybe it can be added back as a simple forwarder to the (0, N-1) call ?

@stoffu stoffu force-pushed the stoffu:rand-range branch from ab1d80f to a2195b9 Apr 4, 2019

@stoffu

This comment has been minimized.

Copy link
Contributor Author

stoffu commented Apr 4, 2019

@moneromooo-monero
I've added such a forwarder called rand_idx.

@stoffu stoffu changed the title crypto: replace rand<T>()%N idiom with unbiased rand_range crypto: replace rand<T>()%N idiom with unbiased rand_idx(N) Apr 4, 2019

@moneromooo-monero
Copy link
Contributor

moneromooo-monero left a comment

Thanks.

@fluffypony
Copy link
Collaborator

fluffypony left a comment

Reviewed

@fluffypony fluffypony merged commit a2195b9 into monero-project:master Apr 15, 2019

1 of 10 checks passed

buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
buildbot/monero-linux-armv8 Build done.
Details

fluffypony added a commit that referenced this pull request Apr 15, 2019

Merge pull request #5392
a2195b9 crypto: replace rand<T>()%N idiom with unbiased rand_idx(N) (stoffu)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.