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

Monkeypatch the minus operator #1029

Closed
JustinDrake opened this issue May 2, 2019 · 4 comments
Closed

Monkeypatch the minus operator #1029

JustinDrake opened this issue May 2, 2019 · 4 comments

Comments

@JustinDrake
Copy link
Collaborator

JustinDrake commented May 2, 2019

See #1017 for context—cc @mkalinin @djrtwo

Monkeypatch the minus operator - so that if a - b is ever negative we assert False. That will allow the executable spec to automatically report uint64 underflows. May prove useful in the context of fuzz testing.

@djrtwo
Copy link
Contributor

djrtwo commented Jun 20, 2019

This is better handled in boxing all int types. @protolambda, are we safe with the new uint types everywhere?

@protolambda
Copy link
Collaborator

protolambda commented Jun 20, 2019

The new uint types are subclasses of uint. So you could override __add__/__sub__, returning the new value. And then raise if below zero / over maximum (casting the output will do that for us). So effectively 4 lines added to uint base class in #1180 would do it:

def __add__(self, other):
  return self.__class__(super().__add__(other))

def __sub__(self, other):
  return self.__class__(super().__sub__(other))

Current behavior is already close:

print(uint8(0) - uint8(123))  # int: -123
print(uint8(uint8(0) - uint8(123)))  # ValueError: unsigned types must not be negative

And then when you try to put the first somewhere, and the int is coerced to uint8, you get the error. You lose the type in between though, overriding the __add__ etc. is required to keep it the same type.

Do you want it in #1180?

@protolambda
Copy link
Collaborator

And what do we want for: uint8(0) + uint32(255)? With the above override, this results in uint8(255). But uint8(0) + uint32(256) would error. We could make it very strict. But it may affect spec readability.

@protolambda
Copy link
Collaborator

slot_to_attest = post_state.slot - spec.MIN_ATTESTATION_INCLUSION_DELAY + 1 ValueError: unsigned types must not be negative

Yay, it catched something. In old testing code though. Hope we find more with the ongoing fuzzing.

@djrtwo djrtwo closed this as completed in b7b2fee Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants