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

Switch Auth AccountNumbers and SequenceNumbers to be uint64 #2701

Closed
sunnya97 opened this issue Nov 5, 2018 · 7 comments · Fixed by #2799
Closed

Switch Auth AccountNumbers and SequenceNumbers to be uint64 #2701

sunnya97 opened this issue Nov 5, 2018 · 7 comments · Fixed by #2799
Assignees

Comments

@sunnya97
Copy link
Member

sunnya97 commented Nov 5, 2018

They are currently int64, and there's no reason they should be negative, so we should switch this.

@sunnya97 sunnya97 self-assigned this Nov 5, 2018
@alexanderbez
Copy link
Contributor

Wasn't there some discussion recently around this @ValarDragon?

@ValarDragon
Copy link
Contributor

ValarDragon commented Nov 6, 2018

Yes. On the tendermint side we have decided we want to go through with such a switch, but it likely won't happen their prelaunch. (As it involves massive changes to the entirety of the codebase)

For the cases mentioned in the title, this should absolutely be changed. However this change is non-state breaking, so no pressure for it to be done prelaunch. Tagging as security, since the purpose of doing this would be to get compile time guarantees.

@cwgoes
Copy link
Contributor

cwgoes commented Nov 6, 2018

However this change is non-state breaking

Really? Won't we change serialized accounts by enacting this? I suppose we could write conversion wrappers, but it would be a bit messy.

@ValarDragon
Copy link
Contributor

We switched amino to not use zig-zag encoding by default. Because of that, the encoding will stay the same between ints and uints.

@rigelrozanski
Copy link
Contributor

Why does this keep coming up? We should create a ADR on this. Anytime we are performing arithmetic we ought to be using int64 as per many previous discussions. see this great discussion point from @jaekwon
#2173 (comment)

sequence numbers used in arithmetic are they not? Note adding +1 to a sequence number counts as arithmetic - if that's the only arithmetic that's used I could see some minor justification for using uint there but still seems like a bug. However if any other arithmetic whatsoever is used then we should for-sure stick to int

Generally not in favour of this idea.

@alexanderbez
Copy link
Contributor

Thanks @rigelrozanski that was the issue I was looking for!

@ValarDragon
Copy link
Contributor

ValarDragon commented Nov 6, 2018

The reason this keeps coming up is due to disagreement about it. The argument presented there, that it is easier to check for overflow with ints, has two things which I object to. The first is that even with ints we never check for overflows to begin with. The compile-time guarantee is quite valuable. Even with uints, instead of doing the overflow check after the addition, you just now put it before the addition. Its still rather simple.

Note that both of the numbers suggested within this issue are only getting incremented by 1. So the chance of overflow through natural causes is zero, and thus were only gaining an additional type safety guarantee if we fail to sanitize input somewhere. Currently we have no safety checks for these. We can still retain overflow safety checks with uints, but also get input sanitization for free. #postlaunch we can just make a checked uint type, but retain the compile-time guarantee.

See the discussion on tendermint: tendermint/tendermint#2684

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this issue Mar 1, 2024
* update mergify

* update dependabot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants