fix: use portable C++ RNG #11627

bryanhonof · 2024-10-01T20:09:11Z

Motivation

Context

I took inspiration from "A Tour of C++, Third Edition" (ISBN-10 0136816487) to implement the Random class. Making it a bit easier for users to generate random numbers using C++'s portable RNG. The use of std::random_device might be a bit overkill, and unnecessary, but I was having fun. 😅

I'm also aware that this constantly creates instances of Random, and then instantly throws them away again. I thought about making this a singleton in some way, but then again, maybe the compiler is smart enough about this.

There are other distribution mechanisms that we could use, I just went with std::uniform_int_distribution, since that's what the book uses.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Ericson2314 · 2024-10-01T20:40:10Z

@bryanhonof can you try the thing I suggested in the issue of making IndirectRootStore own the random number generator?

Also, can you remove the srand and srandom we don't need anymore?

bryanhonof · 2024-10-01T21:02:00Z

@bryanhonof can you try the thing I suggested in the issue of making IndirectRootStore own the random number generator?

Sure, although I didn't quite understand why it'd need to be part of that t.b.h. I liked the way you could use this implementation as a functor, and have the constructor define the range, giving people working on the Nix codebase a nice way to generate random numbers in the future, without doing the whole init dance. We'll lose that behavior if I'm going to instantiate it in IndirectRootStore, I think.

Also, can you remove the srand and srandom we don't need anymore?

I believe I already did, rg -- 'srand\(' returned nothing for me from the root of the project.

bryanhonof · 2024-10-01T21:10:22Z

Added a generic Random as a public member. lmkwyt.

src/libstore/indirect-root-store.cc

Ericson2314 · 2024-10-01T21:35:34Z

src/libstore/sqlite.cc

@@ -258,7 +259,7 @@ void handleSQLiteBusy(const SQLiteBusy & e, time_t & nextWarning)
       is likely to fail again. */
    checkInterrupt();
    /* <= 0.1s */
-    std::this_thread::sleep_for(std::chrono::milliseconds { rand() % 100 });
+    std::this_thread::sleep_for(std::chrono::milliseconds { Random{0, 100}() });


Can you make SQLite require a reference to a Random and then for the LocalStore use the one you just added, and then add one on NarInfoDiskCacheImpl (the other user of SQLite)?

Make sure to add some doxygen to the Random & rng saying what it is used for :)

The function calling that Random, handleSQLiteBusy(), isn't a member of the SQLite struct. Would it be okay to just keep the functor call here?

I think I'll need a bit of help on how to resolve this one. I'm not quite sure where to initialize the RandomNumberGenerator when it comes to this specific function. Maybe we could create a thread_local global rng for use-cases like this, but is that a suitable solution?

src/libstore/build/derivation-goal.cc

bryanhonof · 2024-10-01T21:38:23Z

Should I also replace the code in src/libstore/filetransfer.cc with a Random?

nix/src/libstore/filetransfer.cc

Lines 45 to 46 in 96ba7f9

    
           std::random_device rd; 
        
           std::mt19937 mt19937;

Ericson2314 · 2024-10-01T21:40:34Z

@bryanhonof

Sure, although I didn't quite understand why it'd need to be part of that t.b.h. I liked the way you could use this implementation as a functor, and have the constructor define the range, giving people working on the Nix codebase a nice way to generate random numbers in the future, without doing the whole init dance. We'll lose that behavior if I'm going to instantiate it in IndirectRootStore, I think.

I think it is good if the call-site specifies the distribution, the state is just to avoid consulting the somewhat icky device random global variable. Feel free to change Random to separate the "storing the seed" part, from the "choosing what distribution to sample from" part.

Ericson2314 · 2024-10-01T21:42:35Z

@bryanhonof Sure feel free to change FileTransfer

I guess getFileTransfer and makeFileTransfer would need to be changed to pass in the seed?

NaN-git · 2024-10-01T23:26:39Z

I don't think that using std::random_device is a good choice. Its behavior is basically undefined:

std::random_device may be implemented in terms of an implementation-defined pseudo-random number engine if a non-deterministic source (e.g. a hardware device) is not available to the implementation. In this case each std::random_device object may generate the same number sequence.

The question is: Which properties shall this RNG have? For testing a PRNG is useful, i.e. the same seed shall generate the same sequence of numbers. If the generator is non-deterministic, then this is not possible.
Is anything else than uniformly distributed numbers needed?
Please stay away from Mersenne-Twister, especially std::mt19937, because it's huge and slow. If not seeded correctly, then it will output a long sequence of "bad" random numbers.

bryanhonof · 2024-10-01T23:49:50Z

@NaN-git

I don't think that using std::random_device is a good choice. Its behavior is basically undefined:

I did see that comment, I honestly wouldn't know what else to use. Do you maybe think having a second constructor that accepts a std::seed_seq might be better? Or perhaps even a plain int, and use that in combination with some time mechanism?

The question is: Which properties shall this RNG have? For testing a PRNG is useful, i.e. the same seed shall generate the same sequence of numbers. If the generator is non-deterministic, then this is not possible.

I'm honestly not sure, might be that @Ericson2314 knows?
But, when it comes to testing, that class has the seed() member function. So that we can just set the seed to something we know.

Is anything else than uniformly distributed numbers needed?

As far as I can see, no. I don't expect to support generating random characters any time soon. Maybe floats or doubles.

Please stay away from Mersenne-Twister, especially std::mt19937, because it's huge and slow. If not seeded correctly, then it will output a long sequence of "bad" random numbers.

I've used std::default_random_engine, which seems to default to std::minstd_rand, at least on LLVM. The code in src/libstore/filetransfer.cc does use std::mt19937, but I'm trying to replace that with just the Random class.

Ericson2314 · 2024-10-02T17:11:06Z

If it uses std::mt19937 today, I think it could be fine to keep it that way (but also fine to change it). I care more about avoiding global variables for seeding than sampling at this point, since the rand/random baseline is also somewhat vaguely defined.

src/libstore/unix/build/local-derivation-goal.hh

src/libutil/rng.hh

Mic92 · 2024-10-06T07:59:14Z

src/libutil/rng.hh

+
+// Inspired by the book "A Tour of C++, Third Edition" (ISBN-10 0136816487)
+template<typename T, typename Distribution, typename Engine>
+struct RandomNumberGenerator


This is a pseudo random generator, right? We should probably put this in the name so people not use it in the wrong place.

Is any random number generator really a true random number generator? 😁
No, in all seriousness, I'm not sure. If people would want to use this for cryptographic reasons, I don't think this header is the right choice. Linking against openssl, and using its facilities, would be way better. But PseudoRandomNumberGenerator starts becoming very verbose as well. Maybe mention it in the comments of this header that it isn't meant for cryptographic reasons? Perhaps use Doxygen docs?

Maybe mentioning in the header that this should not be used for cryptographic purpose would be a good start.

Mic92 · 2024-10-06T08:12:52Z

src/libstore/unix/build/local-derivation-goal.cc

+       tmpPath (the replacement), so we have to move it out of the
+       way first.  We'd better not be interrupted here, because if
+       we're repairing (say) Glibc, we end up with a broken system. */
+    Path oldPath = fmt("%1%.old-%2%-%3%", storePath, getpid(), getLocalStore().rng());


Shouldn't be part of this PR, but on linux, we can use renameat2 with RENAME_EXCHANGE to make the whole thing atomic without using a temporary directory.

Could you make a seperate issue for this?

Ericson2314 · 2024-10-21T15:41:10Z

I'm going to push a commit with the things I want.

Ericson2314 · 2024-11-04T21:27:00Z

@bryanhonof I am afraid I am a bit back to the drawing board now that I don't see any PRNG "splitting" of the sort mentioned in https://www.tweag.io/blog/2020-06-29-prng-test/ in the C++ standard library.

Enzime · 2024-11-13T00:24:54Z

There's a related Lix commit that may fix #7273 that you may wish to backport instead

https://gerrit.lix.systems/c/lix/+/2100

I used "A Tour of C++, Third Edition" (ISBN-10 0136816487) as inspiration for the Random class. This replaces all occurrences of `rand()` and `srand()`. Co-authored-by: John Ericson <[email protected]> Co-authored-by: Eelco Dolstra <[email protected]>

bryanhonof requested review from Ericson2314 and edolstra as code owners October 1, 2024 20:09

github-actions bot added the store Issues and pull requests concerning the Nix store label Oct 1, 2024

bryanhonof force-pushed the bryanhonof.use-cpp-prng branch from dfd1ce7 to 563a6dd Compare October 1, 2024 21:08

Ericson2314 reviewed Oct 1, 2024

View reviewed changes

src/libstore/indirect-root-store.cc Outdated Show resolved Hide resolved

bryanhonof force-pushed the bryanhonof.use-cpp-prng branch 2 times, most recently from 8400a1f to 5d60ccc Compare October 1, 2024 21:29

Ericson2314 reviewed Oct 1, 2024

View reviewed changes

src/libstore/build/derivation-goal.cc Outdated Show resolved Hide resolved

bryanhonof force-pushed the bryanhonof.use-cpp-prng branch from 5d60ccc to 6677a68 Compare October 2, 2024 18:43

bryanhonof commented Oct 2, 2024

View reviewed changes

src/libstore/unix/build/local-derivation-goal.hh Outdated Show resolved Hide resolved

bryanhonof force-pushed the bryanhonof.use-cpp-prng branch 4 times, most recently from 42acd1c to 0b0d5d8 Compare October 3, 2024 01:27

edolstra reviewed Oct 4, 2024

View reviewed changes

src/libutil/rng.hh Outdated Show resolved Hide resolved

bryanhonof force-pushed the bryanhonof.use-cpp-prng branch from 9dc7cd5 to 94cb5a7 Compare October 6, 2024 01:49

Mic92 reviewed Oct 6, 2024

View reviewed changes

bryanhonof force-pushed the bryanhonof.use-cpp-prng branch from 94cb5a7 to 71ab474 Compare October 7, 2024 13:09

edolstra approved these changes Oct 16, 2024

View reviewed changes

fix: use portable C++ RNG

fc4cc5a

I used "A Tour of C++, Third Edition" (ISBN-10 0136816487) as inspiration for the Random class. This replaces all occurrences of `rand()` and `srand()`. Co-authored-by: John Ericson <[email protected]> Co-authored-by: Eelco Dolstra <[email protected]>

bryanhonof force-pushed the bryanhonof.use-cpp-prng branch from 71ab474 to fc4cc5a Compare November 25, 2024 07:54

Mic92 approved these changes Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use portable C++ RNG #11627

fix: use portable C++ RNG #11627

bryanhonof commented Oct 1, 2024

Ericson2314 commented Oct 1, 2024

bryanhonof commented Oct 1, 2024 •

edited

Loading

bryanhonof commented Oct 1, 2024 •

edited

Loading

Ericson2314 Oct 1, 2024 •

edited

Loading

bryanhonof Oct 2, 2024 •

edited

Loading

bryanhonof Oct 3, 2024

bryanhonof commented Oct 1, 2024

Ericson2314 commented Oct 1, 2024

Ericson2314 commented Oct 1, 2024

NaN-git commented Oct 1, 2024

bryanhonof commented Oct 1, 2024

Ericson2314 commented Oct 2, 2024

Mic92 Oct 6, 2024 •

edited

Loading

bryanhonof Oct 7, 2024 •

edited

Loading

Mic92 Oct 8, 2024

Mic92 Oct 6, 2024 •

edited

Loading

bryanhonof Oct 7, 2024

Mic92 Oct 8, 2024

Ericson2314 commented Oct 21, 2024

Ericson2314 commented Nov 4, 2024

Enzime commented Nov 13, 2024

fix: use portable C++ RNG #11627

Are you sure you want to change the base?

fix: use portable C++ RNG #11627

Conversation

bryanhonof commented Oct 1, 2024

Motivation

Context

Priorities and Process

Ericson2314 commented Oct 1, 2024

bryanhonof commented Oct 1, 2024 • edited Loading

bryanhonof commented Oct 1, 2024 • edited Loading

Ericson2314 Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

bryanhonof Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

bryanhonof Oct 3, 2024

Choose a reason for hiding this comment

bryanhonof commented Oct 1, 2024

Ericson2314 commented Oct 1, 2024

Ericson2314 commented Oct 1, 2024

NaN-git commented Oct 1, 2024

bryanhonof commented Oct 1, 2024

Ericson2314 commented Oct 2, 2024

Mic92 Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

bryanhonof Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Mic92 Oct 8, 2024

Choose a reason for hiding this comment

Mic92 Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

bryanhonof Oct 7, 2024

Choose a reason for hiding this comment

Mic92 Oct 8, 2024

Choose a reason for hiding this comment

Ericson2314 commented Oct 21, 2024

Ericson2314 commented Nov 4, 2024

Enzime commented Nov 13, 2024

bryanhonof commented Oct 1, 2024 •

edited

Loading

bryanhonof commented Oct 1, 2024 •

edited

Loading

Ericson2314 Oct 1, 2024 •

edited

Loading

bryanhonof Oct 2, 2024 •

edited

Loading

Mic92 Oct 6, 2024 •

edited

Loading

bryanhonof Oct 7, 2024 •

edited

Loading

Mic92 Oct 6, 2024 •

edited

Loading