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

[rand.eng.philox] Make the round states explicit. #7152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tkoeppe
Copy link
Contributor

@tkoeppe tkoeppe commented Jul 22, 2024

This clarifies which state is the final result, and avoids the use of the vaguely defined variable $X'$. It changes the index variable $q$ to be 1-based. The single sequence $V$ is replaced with the sequence of sequences $V^{(q)}$.

Currently, the definition of the "r-round network", "rounds", and how they fit together, is somewhat informal and imprecise. In particular, the statement that Philox "returns the sequence $Y = X'$" is needlessly ambiguous.

It would be clearer if all involved mathematical entities and operations were clearly spelled out.


Preview:

image

@tkoeppe tkoeppe requested a review from jwakely July 22, 2024 13:26
@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

@villevoutilainen, @iburyl, PTAL?

@villevoutilainen
Copy link
Member

This looks so non-editorial to me that it isn't even funny. :D

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

@villevoutilainen: Hm :-) Do you think this changes the normative content? Which part specifically?

@villevoutilainen
Copy link
Member

@tkoeppe I don't want to figure out whether it does change the normative content, and if so, how. I want LWG to do that.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

Got it, thanks!

@iburyl
Copy link

iburyl commented Jul 22, 2024

My problem with this change is that X is now being used

  1. without indices,
  2. with upper indices,
  3. with lower indices and
  4. with upper and lower indices.

Also not sure why q is being used with parenthesis for V and X and without for key.
While rounds are defined a tiny bit clearer, but it adds quite a bit of complexity to X definition.
Given the wording was not really broken before the changes, I am not sure that it is worth it.

@villevoutilainen
Copy link
Member

Right. The change is a spec revamp. While I have the utmost confidence that @tkoeppe can do it correctly, this sort of changes are in principle something that the spec review group, in this case, LWG, needs to ack.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

To be clear:

  • $X$ and $X^{(q)}$ are two distinct objects.
  • Both are sequences:
    • $X = (X_i)_{i=0}^{n-1}$
    • $X^{(q)} = (X_{i}^{(q)})_{i=0}^{n-1}$
    • If you will, $(X^{(q)})_{q=1}^r$ is a sequence of sequences.
  • For any sequence $S$, the name of the sequence names the entire sequence, and subscripting it names an individual element, so $S_i$ is a specific element, and $S$ is shorthand for $(S_i)_{i=a}^b$. This is already something we're doing in the approved paper without further explanation, e.g. when referring to the elements $V_i$ of the sequence $V$.

The upper index in parentheses numbers the rounds, and writing it in parentheses is a not too uncommon style to make it easier to distinguish this kind of "family index" from, say, an exponent. It's ultimately just another sequence index, but into a different kind of sequence (namely the sequence of rounds).

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

Given the wording was not really broken before the changes, I am not sure that it is worth it.

What I find unclear in the approved wording is what the result of Philox is. It says $X'$, but what is that?

One could argue further that the term "$r$-round substitution-permutation network" isn't defined :-)

@iburyl
Copy link

iburyl commented Jul 22, 2024

My point re paranthesis was, if we start using that for X and V, they should also be used for the key. Now usage is inconsistent.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

My point re paranthesis was, if we start using that for X and V, they should also be used for the key. Now usage is inconsistent.

OK, sure, we can either drop the parentheses, or also write $\mathit{key}^{(q)}_k$ if you like, that's fine!

@iburyl
Copy link

iburyl commented Jul 22, 2024

One could argue further that the term "$r$-round substitution-permutation network" isn't defined :-)

I can agree with that, but it is more a light reference to the original paper rather than something really used in the definition. This sentence can be removed without changing the algorithm definition.

Currently it is defined as:

A single round of the generation algorithm performs the following steps:
...
The output sequence X’ of the previous round (X in case of the first round)
...
After r applications of the single-round function, Philox returns the sequence Y = X’.

"single round of the generation algorithm" and "single-round function" could probably be called more consistently though.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

Yes, indeed, what is a "round", and why is there a "first round", etc. etc. I think "$r$-round network" is a good evocative name after the fact, but the name alone doesn't mean anything. In maths you should generally say what something is, and then you can give it a name for later reference. But names don't make meaning.

We can also use a totally different symbol instead of $X^{(q)}$, e.g. $W^{(q)}$, if you prefer.

Oh, by the way, note that the drafting directives forbid multiletter variable names, so we'll probably have to do something about $\mathit{key}$ at some point :-)

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

Given the wording was not really broken before the changes, I am not sure that it is worth it.

I would definitely call into question what the result of Philox is in the approved wording. The paper says "returns the sequence $Y = X'$", but $X'$ is defined as "the output sequence of the previous round". Again, "previous" seems a bit disconnected in this context, and in any case one could read this as "previous means that there's something else coming after, so the final round is just discarded". But surely that's not what we mean here?

@iburyl
Copy link

iburyl commented Jul 22, 2024

Looking into compiled preview it does seem rather readable and several versions of X are not too confusing.
If we rename key to K^{(n)}, saying it is a modifikation of K sequence for the given round (which it is), that would probably be also rather consistent with how we deal with X sequence.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

Good idea, done!

image

@iburyl
Copy link

iburyl commented Jul 22, 2024

Bullet list:

  • K^{(q)}_k is the k^{th} ... <- singular
  • K_k are the elements ... <- plural
  • M_k is multipliers[k], and ... <- singular

Since everything around became singular...

  • K_k is the k^{th} element of the key sequence K

@iburyl
Copy link

iburyl commented Jul 22, 2024

Other than that looks ok for me. But as @villevoutilainen said, there should be some bless from LWG on that change.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 22, 2024

  • K_k is the k^{th} element of the key sequence K

Yes, good call, done. I might even apply this change separately already, since ISO will require the renaming anyway.

Other than that looks ok for me. But as @villevoutilainen said, there should be some bless from LWG on that change.

Yes, indeed, but I wanted to get some sense of agreement first and flesh out the details -- thanks!

This clarifies which state is the final result, and avoids the use of
the vaguely defined variable $X'$. It changes the index variable $q$
to be 1-based. The single sequence $V$ is replaced with the sequence
of sequences $V^{(q)}$.

We also rename $\mathit{key}^q_k$ to $K^{(q)}_{k}$, since ISO requires
that variable names consist of only a single letter. This creates a
nice parallel between $X$/$X^{(q)}$ and $K$/$K^{(q)}$.
Copy link

@iburyl iburyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants