fix incorrect timestamp in uuid v6

[ls4154]

The timestamp part of the uuid v6 implementation is incorrect.

The only lower 60 bits of the timestamp from GetTime should be used,
but current implementation put entire 64-bit timestamp and overwrite the lower 4-bit part with the version ID.

Therefore, it is not field-compatible with version 1.

V1 timestamp
     <----high---> <------mid------> <----------------low-------------->
|xxxx0000|00000000|00000000|00000000|00000000|00000000|00000000|00000000|

V6 timestamp
     <----------------high--------------><-------mid------><----low---->
|xxxx0000|00000000|00000000|00000000|00000000|00000000|00000000|00000000|

Current implementation for V6 timestamp
 <---------------high--------------> <-------mid----->     <----low---->
|xxxx0000|00000000|00000000|00000000|00000000|00000000|00000000|00000000|

Also, we don't need to add variant field at uuid[8], as it is already filled by getTime.

[bormanp]

Thank you for noticing this.

While your fix appears correct your bit layout diagram was a bit confusing in were you placed your xxxx.

V6 layout as in the standard:

The format for the 16-octet, 128-bit UUIDv6 is shown in Figure 1

     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                           time_high                           |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |           time_mid            |      time_low_and_version     |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |clk_seq_hi_res |  clk_seq_low  |         node (0-1)            |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                         node (2-5)                            |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Figure 1: UUIDv6 Field and Bit Layout

A V6 diagram that is less confusing (the author of the patch would probably have gotten it correct with this one):

     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                           time_high                           |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |           time_mid            |  ver  |       time_low        |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |clk_seq_hi_res |  clk_seq_low  |         node (0-1)            |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                         node (2-5)                            |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

I will note this is not compatible with v1 and should not be (which was the point of v6). V1 looks like:

 0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                          time_low                             |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |       time_mid                |         time_hi_and_version   |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |clk_seq_hi_res |  clk_seq_low  |         node (0-1)            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                         node (2-5)                            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Again, this probably should have been written as:

 0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                          time_low                             |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |       time_mid                |  ver  |       time_hi         |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |clk_seq_hi_res |  clk_seq_low  |         node (0-1)            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                         node (2-5)                            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[bormanp]

The & 0xffffffff and & 0xffff are not needed as typecasting to uint32 and uint16 automatically do that but hopefully the compiler will figure that out and elide the extra operation

[ls4154]

That's right. bitmasking is borrowed from the existing uuid v1 code for consistent code style.
Would it be better to remove that part?

[bormanp]

Hah! I wrote the V1 code back in 2011 when Go was pretty new then and I was being paranoid. Probably would do many things different if I were to rewrite it (which I do not plan on doing).

You can choose if you want to leave it in or take it out. I am okay with either.

[ls4154]

Thanks for the review.

While your fix appears correct your bit layout diagram was a bit confusing in were you placed your xxxx.

The diagram I drew is about timestamp slicing.
This illustrates that existing implementation discards the middle 4 bits of the timestamp, and includes the 4 bits beyond the valid 60-bit timestamp range.