Re: [tsvwg] Benjamin Kaduk's No Objection on draft-ietf-tsvwg-rlc-fec-scheme-14: (with COMMENT)

[Vincent Roca <[e-mail]>]

IESG Members, all,

Thanks a lot for this second IESG review round (last time it was part of the RLC I-D).
I have copied below all Discuss/Comments made for this -03 version of the document
(previous comments for -02 have already been considered) and added our answers.

Many thanks, the document quality has been further improved.
Cheers,

   Vincent, on behalf of the authors


==========

> Roman Danyliw Discuss
>
> Discuss (2019-05-28)
>
> (1) Section 1.  Per “The TinyMT32 PRNG initialization depends, among other things, on a parameter set -- namely (mat1, mat2, tmat) -- that needs to be well chosen (pre-calculated values are available in the official web site).”, why is there a reference to an external website when Section 3.1 explicitly lists the constant and mandates their usage?  I’m elevating this to a discuss as there should be no ambiguity on the location of these constants.

[VR] You're right, it brings confusion. We have removed this
part of the sentence.

NEW:
   The TinyMT32 PRNG initialization depends, among other things, on a
   parameter set, namely (mat1, mat2, tmat).  In order to facilitate the
   use of this PRNG and make the sequence of pseudo-random numbers
   depend only on the seed value, this specification requires the use of
   a specific parameter set (see Section 3.1).  This is a major
   difference with respect to the implementation version 1.1
   (2015/04/24) that leaves this parameter set unspecified.


> (2) Section 3.1.  A comment in tinymt32_next_state() notes code changes made to the “original code … [so it is] not depend on the representation of negative integers by 2's complements.”  The following code in tinymt32_temper() appears to suffer from this same, original portability issue:
> t0 ^= -((int32_t)(t1 & 1)) & s->tmat;

[VR] Shame on me, I've omitted this line.
You're perfectly right and here is the fix. Thanks!

NEW:
       /*
        * The if (t1 & 1) {...} block below replaces:
        *     t0 ^= -((int32_t)(t1 & 1)) & s->tmat;
        * The adopted code is equivalent to the original code
        * but does not depend on the representation of negative
        * integers by 2's complements. It is therefore more
        * portable, but includes an if-branch which may slow
        * down the generation speed.
        */
       if (t1 & 1) {
           t0 ^= s->tmat;
       }


==========

> Comment (2019-05-28)
>
> (1) Section 1.  “BigCrush in TestU01 and AdaptiveCrush” needs a reference.

[VR] I have added the two URLs for the academic publicatons behind these
two PRNG statistical tests, keeping also the two URLs to the related
software web sites.


> (2) Abstract and Section 1.  I found characterizing the quality of the randomness as “pretty good” imprecise (relative to what?).  Can this statement be qualified or qualified in some way?

[VR] Yes, I've clarified the abstract by refering to Park-Miller PRNG.
I think the new Section I paragraph contains enough information to
further support this claim.

NEW (abstract):
   The main
   advantage of TinyMT32 over MT is the use of a small internal state,
   compatible with most target platforms including embedded devices,
   while keeping a reasonably good randomness that represents a
   sigificant improvement compared to the Park-Miller Linear
   Congruential PRNG.


> (3) Section 1 (“This is the first difference …”) and Section 3.1 (“The TinyMT32 PRNG reference implementation is reproduced in Figure 1, with the following differences …”) both enumerate a list of differences between the github repo and the code in this text.  Why are both required? Section 3.1 seems more precise.

[VR] You're right, we now only refer to the specialization of the
(mat1, mat2, tmat) tuple in Section 1 (because of its importance in
the way we use the PRNG), leaving the details for Section 3.1.


> (4) Section 1.  I am confused by the terminology of “official web site” and “official github site”.  What makes them official?

[VR] Right, and we changed the wording:

NEW:

   This document specifies the TinyMT32 PRNG, as a specialization of the
   reference implementation version 1.1 (2015/04/24) by Mutsuo Saito and
   Makoto Matsumoto, from Hiroshima University, that can be found at
   [TinyMT-web] (TinyMT web site) and [TinyMT-dev] (Github site).  This
   specialisation aims at having a simple-to-use and deterministic PRNG,
   as explained below.

> (5) Section 1.  With due respect to the first two authors, it seems odd to mention them by name three times in this section.  Also, the fact that this document is derived from a github repo is mentioned twice in this section and once in the code comments of Section 3.1.

[VR] Yes, you're right. Their names now appear only once in text, once in source code, and of course in the authors list and in references. This is fair.


> (6) Section 3.1.  There are normative C code blocks in this text.  The obvious fact that this is C code needs to be specified – consider C99 (ISO/IEC 9899:1999), C11 (ISO/IEC 9899:2011), C18 (ISO/IEC 9899:2018)

[VR] Yes, you're right. I've checked with the -c99, -c11, -c17
(I'm using CLANG and -c18 does not exist yet) C dialect flags
and it compiles/runs without any problem.

NEW:
   The TinyMT32 PRNG reference implementation is reproduced in Figure 1.
   This is a C language implementation, compatible with the C99 (ISO/IEC
   9899:1999), C11 (ISO/IEC 9899:2011) and C18 (ISO/IEC 9899:2018)
   versions of the C language.  ...


> (7) Section 3.1.  I agree with ekr’s original comment that hard coded a constant in tinymt32_init() makes the code less readable -- “s->status[i & 3] ^= i + UINT32_C(1812433253)”

[VR] In general I would agree but in this particular case I have
no idea what this constant means. Hard to assign it a meaningful
constant name. Is that really a problem?


> (8) Editorial Nits
>
> Abstract.  No references are permitted in the abstract.

[VR] removed (the reference still appears in Section I so this is not an issue).

> Section 1.  Typo.  s/variatnt/variant/
> Section 1.  Typo.  s/randomnes/randomness/
> Section 3.1.  Typo.  s/licence/license/

[VR] done.


==========

> Adam Roach Discuss
> Discuss (2019-05-29)
>
> The normative portion of this document is the machine code in section 3.1. There
> is no definition or citation for the formal language used in this document. The
> document needs to either define or normatively cite the definition of this
> language.
>
> I suspect that the citation you want is ISO/IEC 9899:1999 (or a newer document
> in that same series).

[VR] Yes, you're perfectly right. See my answer to Roman Danyliw's comment.
We now explicitely refer to the C99 to C18 flavors of C (all three work).


==========
> Benjamin Kaduk No Objection
>
> Comment (2019-05-28)
>
> In Section 3.1:
>
>    o  the original copyright and licence have been removed, in
>       accordance with BCP 78 and the IETF Trust's Legal Provisions
>       Relating to IETF Documents (http://trustee.ietf.org/license-info);
>
> The original copyright holder's permission is needed to do this!  Perhaps
> it is appropriate to note that the original authors are now listed as
> authors on this document.

[VR] Done.


> Please also specify what language the code in Section 3.1 is written in,
> including reference (so as to specify which version of C it is to be
> intepreted as).

[VR] Done (see answer above).


> I agree with the secdir reviewer and Mirja that some more references
> would be good.
>
> Section-by-section comments follow.
>
> Abstract
>    while keeping a reasonably good randomness.
>
> please add the "non-cryptographic" qualifier.

[VR] Done (it's suggested in the following comment anyway).


> Section 1
>
>                           According to statistical tests (BigCrush in
>    TestU01 and AdaptiveCrush), the quality of the outputs of TinyMT
>    seems pretty good in terms of randomnes (in particular the uniformity
>    of generated numbers), taking the small size of the internal state
>    into consideration (see [TinyMT-web]).  [...]
>
> I'd suggest rewording as "The outputs of TinyMT satisfy several
> statistical tests for (non-cryptographic) randomness, including BigCrush
> in TestU01 and AdaptiveCrush), leaving it well-placed for
> non-cryptographic usage, especially given the small size of its internal
> state." or similar.  (This avoids the "pretty good" construct and moves
> the emphasis from the tests to the usability of the algorithm.)

[VR] Done. Much better like that.


> Section 3.1
>
>    o  mat1 = 0x8f7011ee = 2406486510
>    o  mat2 = 0xfc78ff1f = 4235788063
>    o  tmat = 0x3793fdff = 932445695
>
> side note: these first two have the MSB set, and thus when the
> decimal form of the constant is used in C code, the implicit type will
> be a signed integer type of width strictly greater than 32, as opposed
> to an unsigned integer type (which would presumably have width 32 bits
> on most modern sytsems).

[VR] The above three definitions are from the textual part of the document.
In the C implementation, UINT32_C(...) is used, which brings us to your
comment below.


>        for (int i = 1; i < MIN_LOOP; i++) {
>            s->status[i & 3] ^= i + UINT32_C(1812433253)
>                * (s->status[(i - 1) & 3]
>                   ^ (s->status[(i - 1) & 3] >> 30));
>
> side note: I note that the UINT32_C macro produces a result of type
> uint_least32_t, which in theory could be wider and of greater conversion
> rank than uint32_t.  This would result in the intermediate calculations
> being done in a type other than uint32_t, though for the specific
> calculation here this should not cause any difference in the final
> output after truncation to 32 bits.

[VR] I agree with your analysis. This least32 mapping is pretty subtle!


> Section 3.2
>
>    This PRNG MUST first be initialized with the following function:
>
>       void tinymt32_init (tinymt32_t * s, uint32_t seed);
>
> nit: the pointer declaration style here was missed in the style pass
> done in a recent revision.

[VR] Fixed.


>    It takes as input a 32-bit unsigned integer used as a seed (note that
>    value 0 is authorized by TinyMT32).  [...]
>
> nit: perhaps "permitted" is a better word than "authorized", which can
> have some different connotations to IETF readers.

[VR] Done.


>                                     The use of this structure authorizes
>    several instances of this PRNG to be used in parallel, each of them
>    having its own instance of the structure.
>
> nit: here, "admits" is a better replacement for "authorizes" (but
> "permits" would be fine, too).

[VR] Done.


> Section 3.3
>
>                Consequently, any implementation of the TinyMT32 PRNG in
>    line with this specification MUST comply with the following criteria.
>    Using a seed value of 1, the first 50 values returned by
>    tinymt32_generate_uint32(s) as 32-bit unsigned integers MUST be equal
>    to values provided in Figure 2.  [...]
>
> I don't think it's appropriate to attempt to use "agreement with these
> test vectors" as a compliance mechanism (i.e., normative "MUST") -- the
> normative requirement must surely be to have the same observable
> behavior as the algorithm specified in the C code of Section 3.1.  These
> vectors are just test vectors to give an implementor some confidence
> that they agree with the reference behavior.
>
> (You also need to say whether I read rows first or columns first in the
> Figure's contents.)

[VR] Yes to both comments. We changed as suggested.

NEW:
   Consequently, any implementation of the TinyMT32 PRNG in
   line with this specification MUST have the same output as that
   provided by the reference implementation of Figure 1.  In order to
   increase the compliancy confidence, this document proposes the
   following criteria.  Using a seed value of 1, the first 50 values
   returned by tinymt32_generate_uint32(s) as 32-bit unsigned integers
   are equal to values provided in Figure 2, to be read line by line.


===========

> Barry Leiba No Objection
> Comment (2019-05-28)
>
> — Abstract —
>
>    The main
>    advantage of TinyMT32 over MT is the use of a small internal state,
>    compatible with most target platforms including embedded devices,
>    while keeping a reasonably good randomness.
>
> The difference between “most target platforms including embedded devices,” and “most target platforms, including embedded devices,” is too subtle to be sure that readers get the meaning right.  I think the sentence would be clearer if it said, “most target platforms that include embedded devices,”.

[VR] Yes, it's subtle. We changed as suggested.


> — Section 1 —
>
>    the quality of the outputs of TinyMT
>    seems pretty good in terms of randomnes
>
> Make it “randomness”.

[VR] Done.


>    However, neither the TinyMT
>    nor MT PRNG are meant to be used for cryptographic applications.
>
> This seems a bit more buried than it should be, as it’s important.  I suggest putting some version of this in the Abstract, and also moving it more toward the beginning of the Introduction (probably in the first paragraph.

[VR] You're right, it's essential. We added a sentence at the end
of the Abstract, and clarified it at the end of 1st paragraph of
Section 1.


===========

> Martin Vigoureux No Objection
> Comment (2019-05-28)
>
> Hi,
>
> I only have a minor question:
> In which direction should the numbers of Figure 2 be read, per column or per line?

[VR] Good point. It's clarified.

NEW:
   Figure 2: First 50 decimal values (to be read per line) returned by
   tinymt32_generate_uint32(s) as 32-bit unsigned integers, with a seed
                                value of 1.


> nit:
> s/will then updated/will then be updated/

[VR] Done.