Apply strict uint64 casting #1746

hwwhww · 2020-04-23T16:14:21Z

Issue

We've already stated that all calculations should be in uint64 domain (#1739). However, it doesn't apply for Python native int.

This draft applies some strict uint64 castings to known function parameter typings by fixing #1707 int v.s. uint64 incompatible errors that mypy found.

Pros:

We can have more safety checks from SSZ classes (remerkleable) layer!

Cons:

Annoying casting everywhere.
Clients may already handle the uint64 in different ways with different languages' features. This PR may cause some implementation burden.
Note that this PR probably didn't find all calculations that need casting.

Set it to draft since I'm wondering if it would introduce a serious implementation burden for the client teams.

djrtwo · 2020-04-23T17:17:43Z

I doubt this would introduce any requisite changes on clients.

Note that this PR probably didn't find all calculations that need casting.

What do you mean? It did add a bunch of casts. Are you saying that the linter didn't force any of these?

Annoying casting everywhere.

Slightly resistant for this reason. Does the uint64 provide any additional functionality other than static analysis? Does the type actually overflow or does can it still grow past 2**64?

hwwhww · 2020-04-24T04:47:53Z

@djrtwo

Does the uint64 provide any additional functionality other than static analysis? Does the type actually overflow or does can it still grow past 2**64?

Thanks to @protolambda, it does check the boundaries and raise ValueError. For example:

>>> uint64(2**64)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/hwwang/.pyenv/versions/env38a/lib/python3.8/site-packages/remerkleable/basic.py", line 43, in __new__
    raise ValueError(f"value out of bounds for {cls}")
ValueError: value out of bounds for <class 'remerkleable.basic.uint64'>

>>> uint64(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/hwwang/.pyenv/versions/env38a/lib/python3.8/site-packages/remerkleable/basic.py", line 40, in __new__
    raise ValueError(f"unsigned type {cls} must not be negative")
ValueError: unsigned type <class 'remerkleable.basic.uint64'> must not be negative

What do you mean? It did add a bunch of casts. Are you saying that the linter didn't force any of these?

I only added casting for the variables where mypy panicked. It's still possible that we may write the computations in some Python native variables that cause overflow/underflow to uint64. For example the edge case:

x = 1
y = -1
return unit64(x + y)

The linter can't detect that y is not uint64 type.

We could cover more cases once (i) remerkleable also checks __mul__ and __floordiv__ (ii) all constants are converted to uint64 as this PR.

protolambda · 2020-04-24T12:21:09Z

@hwwhww Remerkleable already does that, I implemented that shortly after that issue. See changelog: https://github.com/protolambda/remerkleable/blob/master/CHANGELOG.rst
Just update the version referenced in the spec 👍

protolambda · 2020-05-14T21:06:46Z

Is this ready to go out of draft mode? And which release would we like to target with this?

hwwhww · 2020-05-15T14:01:03Z

@protolambda @djrtwo
Ready to review now!

It does make the spec a bit more lengthy.

I'd say we should at least add castings to the constant definition section.
For the rest castings, it's good to have the protection from remerkleable with these casting. What do you think?

protolambda

Agree with some of the changes, but feel uneasy about some of the other changes. Would like a review from others here. At least it's all cosmetic/presentation/type checks.

protolambda · 2020-05-15T14:04:03Z

specs/phase0/beacon-chain.md

@@ -729,14 +728,18 @@ def compute_shuffled_index(index: uint64, index_count: uint64, seed: Bytes32) ->

    # Swap or not (https://link.springer.com/content/pdf/10.1007%2F978-3-642-32009-5_1.pdf)
    # See the 'generalized domain' algorithm on page 3
-    for current_round in range(SHUFFLE_ROUND_COUNT):
-        pivot = bytes_to_int(hash(seed + int_to_bytes(current_round, length=1))[0:8]) % index_count
+    for current_round in map(uint64, range(SHUFFLE_ROUND_COUNT)):


This is the type of casting I'm most uneasy with. It just feels like a Python hack.
Does the following work for mypy?

current_round: uint64 for current_round in range(SHUFFLE_ROUND_COUNT):

Or is that too verbose for spec also?

It doesn't work for mypy since the issue is that the range(SHUFFLE_ROUND_COUNT) generator returns int.

The current_round is also only converted to a single byte (see other comment about uint8). Avoiding this map would be nice.

What about changing the type of the first parameter of int_to_bytes to int ?
current_round can be kept int then.
Or the goal is to get read of unnecessary ints?

Alternatively one can introduce something like urange method, which will be the same map applied to range, though.

def urange(*args): return map(uint64, range(*args))

As uint64 is heavily used in pyspec, introducing unsigned range looks pretty reasonable.

@protolambda

Agreed with that current_round can be uint8.

Currently, uint8 is not compatible with uint64 for mypy in remerkerble layer when we want to pass it to int_to_bytes. Although it's fine to set current_round to uint8 type in pyspec right now since we haven't changed our CI linter setting to as strict as Add SSZ utils to mypy checking scope #1707.

For implementation-wise, I feel having int_to_bytes be an instance function in SSZ level would be neater.

We can define uint.uint_to_bytes in remerkerble:

def uint_to_bytes(self, byteorder: str) -> bytes: return self.to_bytes(self.type_byte_length, byteorder=byteorder)

To decrease the requirements for pyspec readers to understand remerkerble, we can modify int_to_bytes to:

def int_to_bytes(n: uint) -> bytes: """ Return the serialization of ``n`` in ``ENDIANNESS``-endian in the byte length of ``uint`` type. """ return n.uint_to_bytes(byteorder=ENDIANNESS)

^^^^ Updated: should be n.uint_to_bytes(byteorder=ENDIANNESS) instead of uint.uint_to_bytes(byteorder=ENDIANNESS)

What do you think?

@ericsson49
Hi!

To me, the goal is setting a boundary of all variables in pyspec since Python int is unbounded. I hope it would decrease the ambiguity and make other implementers' lives better?

IMHO It seems better to have urange supports all uint types. Especially if we want to set current_round to uint8 as @protolambda suggested. I'd say if we really want to avoid map for readability, the simplest way is:

for current_round in range(SHUFFLE_ROUND_COUNT): current_round = uint8(current_round) # cast `int` to `uint8` type

What do you think?

What do you think about the above proposal for int_to_bytes change?

Thank you both! 🙂

We can define uint.uint_to_bytes in remerkerble:

We already have that, as well as stream variants, for every type :).

But for spec style and less hidden things, we generally avoid it.

The methods:

def encode_bytes(self) -> bytes:

def serialize(self, stream: BinaryIO) -> int:

def decode_bytes(cls: Type[V], bytez: bytes) -> V:

def deserialize(cls: Type[V], stream: BinaryIO, scope: int) -> V:

So uint8(123).encode_bytes() would just work to get it as bytes.

And there are a ton more util methods for size bounds, working with merkle backings, etc., but it's meant for tooling more than the spec.

@protolambda

Oh great!

But for spec style and less hidden things, we generally avoid it.

Agreed with that we should generally avoid exposing SSZ APIs. Another solution is that we can describe int_to_bytes(n: uint) in the same way we describe hash_tree_root.

Created an alternative solution PR: #1935

@hwwhww
Hi! Sorry, I missed a notification, so responding somewhat late.

To me, the goal is setting a boundary of all variables in pyspec since Python int is unbounded. I hope it would decrease the ambiguity and make other implementers' lives better?

That's a great goal. It definitely helps. Basically, a typical developer would prefer using a bounded integer type whenever possible. And resolving possible range can be a problem.
In the particular case, it's however obvious that current_round is bounded.
But as current_round is intended to be uint8 it definitely worth using an explicit type/conversion here.

However, unbounded int cannot be completely avoided, I guess, e.g. legendre_bit uses it and private keys are sometimes used. But it would be definitely great to separate cases where bounded ints are enough from those, where arbitrary precision ints are more appropriate.

IMHO It seems better to have urange supports all uint types. Especially if we want to set current_round to uint8 as @protolambda suggested. I'd say if we really want to avoid map for readability, the simplest way is:

I think it's a matter of taste. Basically, all variants are clear, but some slightly less concise.

specs/phase0/beacon-chain.md

hwwhww · 2020-05-15T14:46:02Z

specs/phase0/beacon-chain.md

        index=state.eth1_deposit_index,
        root=state.eth1_data.deposit_root,
    )

    # Deposits must be processed in order
-    state.eth1_deposit_index += 1
+    state.eth1_deposit_index = uint64(state.eth1_deposit_index + 1)


Hmmm it's so strange. exit_queue_epoch += Epoch(1) is okay, but state.eth1_deposit_index += uint64(1) returns error:

Incompatible types in assignment (expression has type "uint", variable has type "uint64")

Strange dark mypy hole. 🤦‍♀️

The integer additions etc. are implemented on the uint base class in remerkleable, to not duplicate it everywhere. I will try to add typevar annotations that might help mypy understand it, then you can try this in the spec.

Would this help? https://github.com/protolambda/remerkleable/compare/uint_type

If that epoch thing works, then state.eth1_deposit_index += DepositIndex(1) should also work.

Oh nvm, DepositIndex doesn't have its own type in the pyspec. Hmm, that error is weird.

I still wonder about this thing, but suppose the other typing fixes outweigh this style issue.

protolambda · 2020-06-18T00:32:46Z

Hey, we didn't include this issue since the v0.12 release was kept small where possible, but it could still be useful for better type checking. I'll try and resolve the merge conflicts, and I think we can define a def copy(v: V) -> V helper function to maybe clean up that added BeaconState type annotation.

protolambda · 2020-06-18T01:31:19Z

See strict-uint64-merge-dev branch to resolve merge conflict. This PR builds on a fork of the specs repository, so I pushed it to another branch.

protolambda

The typing fixes are generally good, but we need to iterate some more on things like the shuffle rounds type, otherwise we only confuse implementers with the new typing.

protolambda · 2020-06-19T09:31:06Z

specs/phase0/beacon-chain.md

@@ -729,14 +728,18 @@ def compute_shuffled_index(index: uint64, index_count: uint64, seed: Bytes32) ->

    # Swap or not (https://link.springer.com/content/pdf/10.1007%2F978-3-642-32009-5_1.pdf)
    # See the 'generalized domain' algorithm on page 3
-    for current_round in range(SHUFFLE_ROUND_COUNT):
-        pivot = bytes_to_int(hash(seed + int_to_bytes(current_round, length=1))[0:8]) % index_count
+    for current_round in map(uint64, range(SHUFFLE_ROUND_COUNT)):


The current_round is also only converted to a single byte (see other comment about uint8). Avoiding this map would be nice.

protolambda · 2020-06-19T09:31:40Z

specs/phase0/beacon-chain.md

-        source = hash(seed + int_to_bytes(current_round, length=1) + int_to_bytes(position // 256, length=4))
+        source = hash(
+            seed
+            + int_to_bytes(current_round, length=1)


Does this count as a limitation to the shuffle-round counts, making its maximum value 255 (inclusive)? Should we change it to a uint8 type? And the SHUFFLE_ROUND_COUNT too? I don't think clients actually use uint64 here at least. E.g. lighthouse uses uint8.

see my response above #1746 (comment)

protolambda · 2020-06-19T16:14:46Z

For some of the int -> uintN casts, it may be unnecessary in a safe way when protolambda/remerkleable#4 is tested and integrated. We have to find some balance, in some places the explicit conversions still help the reader.

protolambda · 2020-06-24T20:51:36Z

Merged new arithmetic operators for better typing into remerkleable master branch. Let's try them out here, and if it works well, I'll cut a remerkleable release with the feature and we can do final polish + merge here.

hwwhww · 2020-06-25T09:35:25Z

@protolambda

There are some issues of the remerkleable master branch:

It disallows the Python sugar syntax like:

    num_validators = spec.SLOTS_PER_EPOCH * 8
    return [spec.MAX_EFFECTIVE_BALANCE] * num_validators

Error message:

eth2spec/test/context.py:120: in default_balances
    return [spec.MAX_EFFECTIVE_BALANCE] * num_validators
../../../../remerkleable/remerkleable/basic.py:95: in __rmul__
    return self.__mul__(other)
../../../../remerkleable/remerkleable/basic.py:92: in __mul__
    return self.__class__(super().__mul__(self.__class__.coerce_view(other)))
../../../../remerkleable/remerkleable/basic.py:178: in coerce_view
    return cls(v)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

cls = <class 'remerkleable.basic.uint64'>, value = [32000000000]

    def __new__(cls, value: int):
>       if value < 0:
E       TypeError: '<' not supported between instances of 'list' and 'int'

../../../../remerkleable/remerkleable/basic.py:72: TypeError

We can change it to:

    num_validators = spec.SLOTS_PER_EPOCH * 8
    default_balances = []
    for _ in range(num_validators):
        default_balances.append(spec.MAX_EFFECTIVE_BALANCE)
    return default_balances

But it would be much verbose than before. Is it possible to allow [spec.MAX_EFFECTIVE_BALANCE] * num_validators?

protolambda · 2020-06-25T13:47:48Z

Ah yes, forgot about non-integer __mul__ there. I'll fix that after the eth2 call today.

protolambda · 2020-06-25T17:23:09Z

Updated remerkleable to do multiplication python-style if the other input is not an int. And tested with lists.

protolambda · 2020-06-25T23:07:04Z

Made remerkleable pass mypy checks (within remerkleable, not yet the spec). Hopefully it helps sort out some typing issues here. PR: protolambda/remerkleable#6

protolambda · 2020-06-26T13:48:26Z

Update: releasing a new remerkleable version which infers a lot of the types here, making the changes in the spec less invasive.

…exing, avoid negative comparison in test

protolambda · 2020-06-26T14:18:46Z

Released remerkleable 0.1.17, integrated into spec, removed redundant type casts, added type coercions for new places. Test speed took a hit however, the constant coercions in uints and length checks make things slower. At least it catches more things now though.

See type-infer branch, branched off from this PR. https://github.com/ethereum/eth2.0-specs/tree/type-infer

remerkleable==0.1.17 + infer type

protolambda · 2020-07-08T11:48:30Z

Updating some python code that uses the spec, and noticed that with these new changes, we can change previous v0.12 type hacks like state.slot = Slot(state.slot + 1) in the spec back to state.slot += 1

djrtwo · 2020-07-15T14:37:40Z

Where are we at on this PR @hwwhww?

I'm interested in getting this into v0.12.2

eisaeidy · 2020-07-15T14:43:04Z

Ok

hwwhww · 2020-07-15T15:37:43Z

@djrtwo
It is ready for me! I just merge dev back to it again in case anything has changed these days.
Need approval from you or @protolambda here.

I'm slightly inclined to merge this one before #1935. But it's okay either way.

Also, to double confirm with you and @protolambda, do you think #1935 is ready to go? I haven't seen any objections from client teams so far. 😄

protolambda

LGTM.
@djrtwo I think this can go into next release, when would that be?

djrtwo · 2020-07-15T17:09:39Z

within the week.
Want to get in gossipsub params #1958
and clean up phase 1 test gen #1957

hwwhww added general:enhancement New feature or request phase0 general:RFC Request for Comments labels Apr 23, 2020

hwwhww mentioned this pull request Apr 24, 2020

It would be great if we could clarify the types of the constants in the beacon chain "Misc" and other parts #1750

Closed

hwwhww force-pushed the hwwhww/strict-uint64 branch from 051a9ee to b8e5941 Compare April 28, 2020 10:33

hwwhww force-pushed the hwwhww/strict-uint64 branch from b8e5941 to 224ef35 Compare May 15, 2020 13:50

hwwhww marked this pull request as ready for review May 15, 2020 13:54

protolambda reviewed May 15, 2020

View reviewed changes

hwwhww commented May 15, 2020

View reviewed changes

hwwhww requested a review from djrtwo May 18, 2020 13:48

hwwhww added 5 commits May 20, 2020 03:04

Apply strict uint64 casting

2bf020d

Fix typing

88d7315

(i) Fix leftover (ii) SHARD_COMMITTEE_PERIOD should be uint64

9f340d5

PR feedback from Proto

8f70453

PR feedback from proto: set length back to int

cd91380

hwwhww force-pushed the hwwhww/strict-uint64 branch from 9a789cd to cd91380 Compare May 19, 2020 19:05

hwwhww changed the base branch from v012x to dev May 19, 2020 19:05

Merge branch 'dev' into hwwhww/strict-uint64

41ebd51

Fix table

4428a6a

hwwhww force-pushed the hwwhww/strict-uint64 branch from 50252ec to 4428a6a Compare June 18, 2020 05:50

Fix get_min_new_period_epochs helper

26c540f

hwwhww requested a review from protolambda June 18, 2020 07:03

Merge branch 'dev' into hwwhww/strict-uint64

76840c9

protolambda reviewed Jun 19, 2020

View reviewed changes

This was referenced Jun 20, 2020

More uint operators and r-op versions, for better typing in spec protolambda/remerkleable#4

Merged

get_custody_atoms: avoid application of modulo operation (%) to a negative value #1923

Closed

Merge branch 'dev' into hwwhww/strict-uint64

b239f61

hwwhww mentioned this pull request Jun 25, 2020

Proposal: generic uint_to_bytes #1935

Merged

Infer types where possible, e.g. uint64+uint64=uint64

531184f

protolambda added 2 commits June 26, 2020 15:57

update remerkleable for uint and mypy improvements

3fb0257

make extracted byte uint8 for bitshift, do not use negative slice ind…

3b7617f

…exing, avoid negative comparison in test

hwwhww mentioned this pull request Jun 26, 2020

remerkleable==0.1.17 + infer type #1940

Merged

Merge pull request #1940 from ethereum/type-infer

9af0cf9

remerkleable==0.1.17 + infer type

hwwhww mentioned this pull request Jun 30, 2020

Avoid negative values and minor refactoring #1943

Merged

protolambda added the general:typing Spec typing, no breaking changes label Jul 8, 2020

Merge branch 'dev' into hwwhww/strict-uint64-2

6e3a7ad

protolambda approved these changes Jul 15, 2020

View reviewed changes

djrtwo merged commit 6a7a47d into dev Jul 15, 2020

djrtwo deleted the hwwhww/strict-uint64 branch July 15, 2020 17:10

terencechain mentioned this pull request Jul 24, 2020

Align to v0.12.2 prysmaticlabs/prysm#6716

Closed

15 tasks

Apply strict uint64 casting #1746

Apply strict uint64 casting #1746

Conversation

hwwhww commented Apr 23, 2020 • edited Loading

Issue

Pros:

Cons:

djrtwo commented Apr 23, 2020

hwwhww commented Apr 24, 2020 • edited Loading

protolambda commented Apr 24, 2020

protolambda commented May 14, 2020

hwwhww commented May 15, 2020

protolambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsson49 Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

hwwhww Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protolambda commented Jun 18, 2020

protolambda commented Jun 18, 2020

protolambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

protolambda commented Jun 19, 2020

protolambda commented Jun 24, 2020

hwwhww commented Jun 25, 2020

protolambda commented Jun 25, 2020

protolambda commented Jun 25, 2020

protolambda commented Jun 25, 2020

protolambda commented Jun 26, 2020

protolambda commented Jun 26, 2020

protolambda commented Jul 8, 2020

djrtwo commented Jul 15, 2020

eisaeidy commented Jul 15, 2020

hwwhww commented Jul 15, 2020

protolambda left a comment

Choose a reason for hiding this comment

djrtwo commented Jul 15, 2020

hwwhww commented Apr 23, 2020 •

edited

Loading

hwwhww commented Apr 24, 2020 •

edited

Loading

ericsson49 Jun 19, 2020 •

edited

Loading

hwwhww Jun 22, 2020 •

edited

Loading

hwwhww Jun 22, 2020 •

edited

Loading