UUID partition change #4914

bluesign · 2023-10-31T19:12:21Z

Changing partition of uuid to 3rd byte for compatibility with database types and javascript MAX_SAFE_INTEGER

janezpodhostnik

Looks great! Thank you.

fvm/environment/uuids.go

zhangchiqing · 2023-10-31T23:17:11Z

fvm/environment/uuids_test.go


 	value, err = uuids.getUint64()
 	require.NoError(t, err)
 	require.Equal(t, value, maxUint56Minus2+1)

 	value, err = uuids.GenerateUUID()
 	require.NoError(t, err)
-	require.Equal(t, value, uint64(0xdefffffffffffffe))
+	require.Equal(t, value, uint64(0xffffdefffffffffe))



I'm not sure about the implementation.

We know that the UUID generator generates UUID for the same partition, and the partition is determined by the block ID and transaction index.

In other words, given a UUID Generator, if we keep calling GenerateUUID, the returned value always the same byte at the partition byte position, which is the 3rd byte.

However, L201 takes the stored uuid value and add 1 to it:

err = generator.setUint56(value + 1)

This works if partition is the 1st byte, but won't work if partition is 3rd byte.
See this case: if now the UUID for partition 2 is: 3298534883327 (hex: 00 00 02 FF FF FF FF FF)

C7 C6 P C5 C4 C3 C2 C1 00 00 02 FF FF FF FF FF

Then calling GenerateUUID() again, will call setUint56(value + 1), which becomes 00 00 03 00 00 00 00 00, which would not increment C6, and causing the counter to go back to 0 again.

We need to have a test case, for a UUID generator for partition 2 and it's value is 00 00 02 FF FF FF FF FF, the next two UUIDs should be 00 01 02 00 00 00 00 00, and 00 01 02 00 00 00 00 01.

What happens is that we first call generator.getUint64() which gets the stored counter for that partition (00 00 00 FF FF FF FF FF). then stores it back as incremented by 1 generator.setUint56(value + 1) ( 00 00 01 00 00 00 00 00). Only after this is the value cut up and the partition bytes inserted.

value: 00 00 00 FF FF FF FF FF
partition: 02

(value & 0xFF_FF00_0000_0000) << 8) -> 00 00 00 00 00 00 00 00 (partition << 40) -> 00 00 02 00 00 00 00 00 (value & 0xFF_FFFF_FFFF) -> 00 00 00 FF FF FF FF FF 00 00 02 FF FF FF FF FF

and the next one:
value: 00 00 01 00 00 00 00 00
partition: 02

(value & 0xFF_FF00_0000_0000) << 8) -> 00 01 00 00 00 00 00 00 (partition << 40) -> 00 00 02 00 00 00 00 00 (value & 0xFF_FFFF_FFFF) -> 00 00 00 00 00 00 00 00 00 01 02 00 00 00 00 00

I do agree that a test for this continuation would be great!

@zhangchiqing
actually problem is naming of the function, I didn't want to touch that. Actually getuint64 is just returning the counter ( without partition byte )

We need to have a test case, for a UUID generator for partition 2 and it's value is 00 00 02 FF FF FF FF FF, the next two UUIDs should be 00 01 02 00 00 00 00 00, and 00 01 02 00 00 00 00 01.

@bluesign could you at least add this test case? Just want to make sure we have test case covering it.

sure, of course, I missed that part of the comment

Oh, I know where I got confused. I thought the stored register value contains the partition and the counter, but actually it is just the counter. The register ID contains the partition already.

yeah naming of the functions are confusing, setUint56 and getUint64 makes you feel like you are setting 56 bits of 64 bit int, and then getting it as 64 bit with partition.

Co-authored-by: Janez Podhostnik <67895329+janezpodhostnik@users.noreply.github.com>

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

codecov-commenter · 2023-11-03T14:30:17Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (7f0980e) 55.61% compared to head (a3b336c) 55.60%.

Files	Patch %	Lines
utils/unittest/execution_state.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4914      +/-   ##
==========================================
- Coverage   55.61%   55.60%   -0.01%     
==========================================
  Files        1002     1002              
  Lines       96600    96600              
==========================================
- Hits        53724    53716       -8     
- Misses      38820    38826       +6     
- Partials     4056     4058       +2

Flag	Coverage Δ
unittests	`55.60% <66.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zhangchiqing

Looks good.

I added some comments and minor suggestion. Great work!

fvm/environment/uuids.go

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

…low-go into uuid_partition_change

janezpodhostnik · 2024-01-24T21:05:34Z

We forgot about this one.
The test faulire was that the state commitment changed. This is expected. I updated the state commitment hashes: a3b336c

bluesign added 2 commits October 31, 2023 19:49

change partition byte for uuid to 3rd byte

c28cdda

udid partition tests

462b9cc

bluesign requested review from ramtinms and janezpodhostnik as code owners October 31, 2023 19:12

janezpodhostnik approved these changes Oct 31, 2023

View reviewed changes

fvm/environment/uuids.go Outdated Show resolved Hide resolved

ramtinms requested a review from zhangchiqing October 31, 2023 22:01

zhangchiqing requested changes Oct 31, 2023

View reviewed changes

bluesign and others added 2 commits November 1, 2023 09:24

Update uuids.go

20f0dc5

Co-authored-by: Janez Podhostnik <67895329+janezpodhostnik@users.noreply.github.com>

Update fvm/environment/uuids.go

0d02488

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

bluesign added 2 commits November 3, 2023 16:57

continuation test

48520a1

test

000ea72

bluesign requested a review from zhangchiqing November 3, 2023 21:23

zhangchiqing approved these changes Nov 3, 2023

View reviewed changes

fvm/environment/uuids.go Outdated Show resolved Hide resolved

fvm/environment/uuids.go Show resolved Hide resolved

bluesign and others added 5 commits November 3, 2023 23:09

Update fvm/environment/uuids.go

2a27781

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

Update fvm/environment/uuids.go

efe1c6f

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

rename

8eb7df2

Merge branch 'uuid_partition_change' of https://github.com/bluesign/f…

2c71696

…low-go into uuid_partition_change

rename methods to getCounter/setCounter

4f4c564

zhangchiqing approved these changes Nov 3, 2023

View reviewed changes

janezpodhostnik enabled auto-merge November 7, 2023 18:48

janezpodhostnik and others added 2 commits January 24, 2024 21:44

Merge branch 'master' into uuid_partition_change

0fd79a8

change expected state comitments

a3b336c

janezpodhostnik added this pull request to the merge queue Jan 24, 2024

Merged via the queue into onflow:master with commit f0b7f2a Jan 24, 2024
51 checks passed

janezpodhostnik mentioned this pull request Jan 24, 2024

UUID partition change - port #5287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UUID partition change #4914

UUID partition change #4914

bluesign commented Oct 31, 2023

janezpodhostnik left a comment

zhangchiqing Oct 31, 2023

janezpodhostnik Nov 1, 2023

bluesign Nov 1, 2023

zhangchiqing Nov 3, 2023

bluesign Nov 3, 2023

zhangchiqing Nov 3, 2023 •

edited

Loading

bluesign Nov 3, 2023

zhangchiqing Nov 3, 2023

codecov-commenter commented Nov 3, 2023 •

edited

Loading

zhangchiqing left a comment

janezpodhostnik commented Jan 24, 2024

UUID partition change #4914

UUID partition change #4914

Conversation

bluesign commented Oct 31, 2023

janezpodhostnik left a comment

Choose a reason for hiding this comment

zhangchiqing Oct 31, 2023

Choose a reason for hiding this comment

janezpodhostnik Nov 1, 2023

Choose a reason for hiding this comment

bluesign Nov 1, 2023

Choose a reason for hiding this comment

zhangchiqing Nov 3, 2023

Choose a reason for hiding this comment

bluesign Nov 3, 2023

Choose a reason for hiding this comment

zhangchiqing Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

bluesign Nov 3, 2023

Choose a reason for hiding this comment

zhangchiqing Nov 3, 2023

Choose a reason for hiding this comment

codecov-commenter commented Nov 3, 2023 • edited Loading

Codecov Report

zhangchiqing left a comment

Choose a reason for hiding this comment

janezpodhostnik commented Jan 24, 2024

zhangchiqing Nov 3, 2023 •

edited

Loading

codecov-commenter commented Nov 3, 2023 •

edited

Loading