Add Serde #189

UserAB1236872 · 2017-10-27T14:18:48Z

This addresses #84 by making ChaCha, Isaac, XorShift, and StdRng serializable and deserializable with serde.

This was extremely tedious, and basically comes down entirely to the fact that serde doesn't implement its traits for Wrapping, and Isaac's array length is 256 and thus above the autogenerated trait impls. Fixing both of those from the serde and Rust side would remove a lot of code and let you just derive them.

Luckily, since IsaacRng and Isaac64Rng underlie StdRng, we get that one via derive.

Currently this is set up as a feature. In order to use it someone would have to use rand = { version="1.2.3", features=["serde-1"] } in their Cargo.toml.

This introduces a minor dev-dependency on bincode because I needed something to serialize with for unit tests. Unfortunately there's no way currently to gate dev-dependencies behind features (See here).

I can also push a commit to bump the version if desired, but I figured I should leave that to you instead of scribbling around version numbers myself.

UserAB1236872 · 2017-10-27T14:26:48Z

Oh crud, a bunch of rustfmt nonsense got into the tree. That'll take a sec to fix

dhardy · 2017-10-28T09:05:12Z

200 lines for each of the ISAAC variants — painful. I gave up trying to implement Serde support before precisely because of the Wrapping thing.

Can you please remove the rustfmt changes to existing code? (Probably easiest to use git rebase -i and git checkout -p.)

UserAB1236872 · 2017-10-29T14:43:02Z

Got it. Also rebased and resolved merge conflicts.

UserAB1236872 · 2017-10-30T03:08:11Z

Hmm, looks like the failing test is the Rust 1.15 version. How important is that? I didn't even remember struct field shorthands being a recent(-ish) feature. It's not a big deal to fix, but if I don't have to I'll leave it.

dhardy · 2017-10-30T10:02:07Z

Struct field shorthands are certainly recent; I can't answer the question about the importance of old compiler versions.

I think there's general agreement that feature-gated Serde support is wanted (it's mentioned in the redesign RFC; unfortunately I can't find any discussion on this).

I had a quick look at the code; looks fine and well tested.

UserAB1236872 · 2017-10-31T12:20:00Z

I think I can actually clean this up quite a bit. Firstly, whenever serde-rs/serde#1072 is merged ChaCha and XorShift become derivable. Secondly, we can use Serde's with attribute to decorate the two Isaac fields that still won't be derivable which should reduce code duplication quite a bit.

UserAB1236872 · 2017-10-31T13:31:03Z

Managed to about halve the lines with the serde(with= attribute. The pieces of ChaCha and XorShift can even further be reduced dependent on my PR being merged to Serde.

dtolnay

I published @LinearZoetrope's Wrapping impls in Serde 1.0.17.

UserAB1236872 · 2017-10-31T20:12:57Z

Yay, now it's down about as low as we can get it without blanket impls for arbitrary constants.

dhardy · 2017-10-31T20:22:07Z

Very nice.

I should point out that there's been some discussion of whether CSPRNGs should implement serialization; see here: dhardy#31.

dhardy

dhardy#31 still doesn't have many comments, so possibly wait some more, but at least there is no disagreement yet.

After the changes mentioned, I'd like to see this merged.

dhardy · 2017-11-06T10:25:50Z

src/isaac.rs

+
+#[cfg(feature="serde-1")]
+mod rand_size_64_serde {
+    use super::RAND_SIZE_64;


Isn't this module identical to the usize version? I doubt we'll be changing RAND_SIZE_USIZE or RAND_SIZE_64 — they're fundamental to the algorithms — so it seems silly duplicating the code here.

Yeah, I thought the same thing, but I did it just to be safe. I can remove it.

RAND_SIZE_USIZE was just there to avoid a few casts. In https://github.com/dhardy/rand/pull/4/files I removed it.

dhardy · 2017-11-06T10:29:48Z

src/lib.rs

@@ -840,6 +844,7 @@ pub struct Closed01<F>(pub F);
 /// The standard RNG. This is designed to be efficient on the current
 /// platform.
 #[derive(Copy, Clone, Debug)]
+#[cfg_attr(feature="serde-1",derive(Serialize,Deserialize))]
 pub struct StdRng {


I personally think StdRng should not be serializable — we wish to be able to replace the algorithm in the future without breaking code, and if someone is serializing the RNG it implies they wish output to remain the same. StdRng is also architecture dependent (serialize on a 32-bit machine, deserialize on 64-bit — whoops). Users may use IsaacRng or the 64-bit version directly.

That's a fair point.

pitdicker · 2017-12-18T07:30:37Z

I think it would be nice to land this in some form with the other improvements to PRNG's for 0.4.1.

I do have some comments/opinions:

It is probably best rebase on Cherry-pick changes to PRNG algorithms #209
I don't think it is a good idea to implement serialisation for cryptographic RNG's. It is basically the opposite of what they are designed for and what sets them apart from faster RNG's: be unpredictable because there is no way to recover the internal state. Then ChaCha and HC-128 should not implement serialisation, but Xorshift should. Also I think it is ok for ISAAC to implement it, as we can't advertise ISAAC as cryptographically good.
@dhardy already made the good point that serialisation should only be implemented for the RPNG's, not for a wrapper like StdRng.
Basically everyone agrees Xorshift should be replaced by a different small RNG, and the list is down to only a few candidates. And if it is up to me, ISAAC will be dropped in favour of HC-128. Then there is not much left anymore of the serialisation added by this PR. Still I see good benefits in this PR because it is well done and sets up the 'infrastructure': tests, CI, Cargo feature. And I suppose the RNG's will not just be dropped, but moved somewhere to stay available.

@dhardy Does this sound like a way forward to you?
@LinearZoetrope Great work! Would you be willing to do a rebase?

dhardy · 2017-12-18T08:37:43Z

Mostly I agree with @pitdicker; I'm not convinced on the CSPRNGs but I suspect most demand for serialization will be with the non-crypto PRNGs, so I guess his plan works.

Anyway, thanks again @LinearZoetrope for doing the leg-work including getting Serde updated.

UserAB1236872 · 2018-01-02T00:08:20Z

Sorry I've been silent, things have been busy. I'd be more okay with omitting the CSPRNGs if there were better standing options for regular PRNGs implemented. As it stands, limiting yourself to XorShift just because you need serialization makes it almost useless to have the feature. I'd have fewer complaints if we had options like Mersenne Twister or whatever else is being considered available right now. Though I suppose if I can put it on Isaac due to the noted lack of it being known to be CS I can live.

That said, I'm also sensitive to the fact that we can't add serialization for CSPRNGs and then remove it once we have better non-CS options (okay, fine, pre-1.0 we CAN but a pre-planned regression before a feature is even added seems like an awful idea).

I can address the other issues such as rebase/removing StdRng/etc.

dhardy · 2018-01-02T08:49:22Z

That's okay. Yes, I think we will merge #209 and possibly #210 first, so if you rebase on top of #209 it would be better. (Unfortunately it's not a simple rebase because ISAAC got moved around a lot.)

StdRng shouldn't implement serialisation because it could break just going from a 32-bit machine to a 64-bit one at the moment. We're talking about switching it to HC128 across the board; after that maybe it could; I'm not sure.

I still think hiding internal state is a weak argument for not implementing serialization, personally.

dhardy · 2018-01-02T08:57:28Z

I should also mention that @pitdicker has been working on collecting a number of alternatives to Xorshift: dhardy#60. We aim to make a companion crate available (currently https://github.com/pitdicker/small-rngs) and replace Xorshift: dhardy#52

pitdicker · 2018-01-02T13:44:30Z

I still think hiding internal state is a weak argument for not implementing serialization, personally.

Yes. To be honest I worded it a bit stronger than that I feel about it. Because I am not sure it is a good idea and actually needed, not implementing it feels like a conservative position to start from.

You also made the good point in several places that anyone is free to use a cryptographic RNG for applications that do not strictly need one (like many games). Who are we to deny it?

I think @LinearZoetrope say's it well that a reason to want serialization for CSPRNG's now is because the quality of the current alternative, Xorshift, is not much. And also that once we add serialisation, we can't just remove it.

Personally I would wait to add serialisation to ChaCha until someone opens an issue for it together with his use case. But I won't stand in the way to add it now.

B.t.w. @LinearZoetrope. Is there some guideline as to how ChaCha should be serialized (I know next to nothing about that)? In theory it is not necessary to serialize the results buffer, that can easily be recalculated.

dhardy · 2018-01-02T14:22:18Z

To be fair to your point about hiding the internals, serialisation for HashSet and HashMap only serialises elements, not the hash key. That means element order can change after deserialisation and better protection from hashtable DoS attacks. But for PRNGs maintaining the exact same internal state can be important.

I guess we can apply this by making StdRng (and ReseedingRng) omit internal state and set themselves so that they need immediate reseeding after deserialisation.

aidanhs · 2018-01-17T01:35:05Z

So where is this PR is up to (since I have some interest in it)? Does it just need a rebase and stdrng removing?

dhardy · 2018-01-17T10:15:50Z

I think so, yes.

pitdicker · 2018-01-18T08:32:37Z

I guess we can apply this by making StdRng (and ReseedingRng) omit internal state and set themselves so that they need immediate reseeding after deserialisation.

StdRng doesn't use reseeding.

So where is this PR is up to (since I have some interest in it)? Does it just need a rebase and stdrng removing?

Quite some work has already been put into it, so I think that is it.

In dhardy#58 (comment) I listed some of the changes planned for the PRNG's. I think we could merge this PR when it is rebased, but everything it adds serialisation for will be different soon(ish).

UserAB1236872 · 2018-01-22T05:07:40Z

Okay, that should address all the criticism. I rebased on master since all the changes had been merged into there.

Currently just the Isaacs and Xorshift are implemented.

dhardy

Looks good to me other than the header block. Thanks @LinearZoetrope!

dhardy · 2018-01-22T10:04:58Z

src/prng/isaac_serde.rs

@@ -0,0 +1,68 @@
+#[cfg(feature="serde-1")]


This file needs a header. Copy the copyright/licence block and add a one-line //! comment please.

Also I think this cfg line is redundant since it was also applied to mod isaac_serde;.

UserAB1236872 · 2018-01-22T11:13:25Z

Done

dhardy · 2018-01-22T11:29:39Z

src/prng/isaac_serde.rs

@@ -1,4 +1,15 @@
-#[cfg(feature="serde-1")]
+// Copyright 2013 The Rust Project Developers. See the COPYRIGHT


It's 2018 now; use that or 2017-2018 (since you started working on it last year)

UserAB1236872 · 2018-01-22T11:38:12Z

Yeah, I was wondering but deferred changing it until I was told to.

Add Serde

UserAB1236872 force-pushed the serde branch from 24842c7 to 2f8a9a2 Compare October 27, 2017 14:24

UserAB1236872 force-pushed the serde branch 3 times, most recently from 1c37e96 to c96c235 Compare October 29, 2017 14:42

UserAB1236872 force-pushed the serde branch from c96c235 to 37d915a Compare October 29, 2017 14:47

dhardy mentioned this pull request Oct 30, 2017

Add XorShiftRng::extract_seed #119

Closed

UserAB1236872 mentioned this pull request Oct 31, 2017

Add std::num::Wrapping impl serde-rs/serde#1072

Merged

dhardy mentioned this pull request Oct 31, 2017

What common traits should RNG implementations implement? dhardy/rand#13

Closed

dtolnay reviewed Oct 31, 2017

View reviewed changes

UserAB1236872 force-pushed the serde branch from f293ca3 to 5ab1bfc Compare October 31, 2017 20:12

dhardy reviewed Nov 6, 2017

View reviewed changes

pitdicker mentioned this pull request Jan 2, 2018

To provide (or not) specific PRNGs? dhardy/rand#58

Open

UserAB1236872 force-pushed the serde branch 3 times, most recently from 3b9b943 to 0a1b4ee Compare January 22, 2018 05:06

UserAB1236872 added 3 commits January 21, 2018 21:23

Add serde infrastructure

73ec0f7

Isaac Serde

44455f0

XorShift serde

b7d04ee

UserAB1236872 force-pushed the serde branch from 0a1b4ee to b7d04ee Compare January 22, 2018 05:24

dhardy reviewed Jan 22, 2018

View reviewed changes

Add a header and remove redundant gate

a0248b2

Feature gate some stuff in Xor test to prevent warnings

969f411

dhardy reviewed Jan 22, 2018

View reviewed changes

Fix copyright year

0ded624

dhardy merged commit 9116c6e into rust-random:master Jan 22, 2018

pitdicker pushed a commit that referenced this pull request Apr 4, 2018

Merge pull request #189 from LinearZoetrope/serde

6fb3413

Add Serde

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Serde #189

Add Serde #189

UserAB1236872 commented Oct 27, 2017 •

edited

Loading

UserAB1236872 commented Oct 27, 2017

dhardy commented Oct 28, 2017

UserAB1236872 commented Oct 29, 2017

UserAB1236872 commented Oct 30, 2017

dhardy commented Oct 30, 2017 •

edited

Loading

UserAB1236872 commented Oct 31, 2017 •

edited

Loading

UserAB1236872 commented Oct 31, 2017 •

edited

Loading

dtolnay left a comment

UserAB1236872 commented Oct 31, 2017

dhardy commented Oct 31, 2017

dhardy left a comment

dhardy Nov 6, 2017

UserAB1236872 Nov 6, 2017

pitdicker Nov 10, 2017 •

edited

Loading

dhardy Nov 6, 2017

UserAB1236872 Nov 6, 2017

pitdicker commented Dec 18, 2017

dhardy commented Dec 18, 2017

UserAB1236872 commented Jan 2, 2018 •

edited

Loading

dhardy commented Jan 2, 2018

dhardy commented Jan 2, 2018

pitdicker commented Jan 2, 2018

dhardy commented Jan 2, 2018

aidanhs commented Jan 17, 2018

dhardy commented Jan 17, 2018

pitdicker commented Jan 18, 2018

UserAB1236872 commented Jan 22, 2018 •

edited

Loading

dhardy left a comment

dhardy Jan 22, 2018

UserAB1236872 commented Jan 22, 2018

dhardy Jan 22, 2018

UserAB1236872 commented Jan 22, 2018

		@@ -1,4 +1,15 @@
		#[cfg(feature="serde-1")]
		// Copyright 2013 The Rust Project Developers. See the COPYRIGHT

Add Serde #189

Add Serde #189

Conversation

UserAB1236872 commented Oct 27, 2017 • edited Loading

UserAB1236872 commented Oct 27, 2017

dhardy commented Oct 28, 2017

UserAB1236872 commented Oct 29, 2017

UserAB1236872 commented Oct 30, 2017

dhardy commented Oct 30, 2017 • edited Loading

UserAB1236872 commented Oct 31, 2017 • edited Loading

UserAB1236872 commented Oct 31, 2017 • edited Loading

dtolnay left a comment

Choose a reason for hiding this comment

UserAB1236872 commented Oct 31, 2017

dhardy commented Oct 31, 2017

dhardy left a comment

Choose a reason for hiding this comment

dhardy Nov 6, 2017

Choose a reason for hiding this comment

UserAB1236872 Nov 6, 2017

Choose a reason for hiding this comment

pitdicker Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

dhardy Nov 6, 2017

Choose a reason for hiding this comment

UserAB1236872 Nov 6, 2017

Choose a reason for hiding this comment

pitdicker commented Dec 18, 2017

dhardy commented Dec 18, 2017

UserAB1236872 commented Jan 2, 2018 • edited Loading

dhardy commented Jan 2, 2018

dhardy commented Jan 2, 2018

pitdicker commented Jan 2, 2018

dhardy commented Jan 2, 2018

aidanhs commented Jan 17, 2018

dhardy commented Jan 17, 2018

pitdicker commented Jan 18, 2018

UserAB1236872 commented Jan 22, 2018 • edited Loading

dhardy left a comment

Choose a reason for hiding this comment

dhardy Jan 22, 2018

Choose a reason for hiding this comment

UserAB1236872 commented Jan 22, 2018

dhardy Jan 22, 2018

Choose a reason for hiding this comment

UserAB1236872 commented Jan 22, 2018

UserAB1236872 commented Oct 27, 2017 •

edited

Loading

dhardy commented Oct 30, 2017 •

edited

Loading

UserAB1236872 commented Oct 31, 2017 •

edited

Loading

UserAB1236872 commented Oct 31, 2017 •

edited

Loading

pitdicker Nov 10, 2017 •

edited

Loading

UserAB1236872 commented Jan 2, 2018 •

edited

Loading

UserAB1236872 commented Jan 22, 2018 •

edited

Loading