Cosmos-sdk: Switch Auth AccountNumbers and SequenceNumbers to be uint64
They are currently int64, and there's no reason they should be negative, so we should switch this.
sunnya97
All 7 comments
Wasn't there some discussion recently around this @ValarDragon?
alexanderbez
on 6 Nov 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.
ValarDragon
on 6 Nov 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.
cwgoes
on 6 Nov 2018
We switched amino to not use zig-zag encoding by default. Because of that, the encoding will stay the same between ints and uints.
ValarDragon
on 6 Nov 2018
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
https://github.com/cosmos/cosmos-sdk/issues/2173#issuecomment-419548238
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.
rigelrozanski
on 6 Nov 2018
Thanks @rigelrozanski that was the issue I was looking for!
alexanderbez
on 6 Nov 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: https://github.com/tendermint/tendermint/issues/2684
ValarDragon
on 6 Nov 2018
Related issues
jackzampolin
路
3Comments
rigelrozanski
路
3Comments
mossid
路
3Comments
faboweb
路
3Comments
ValarDragon
路
3Comments
Most helpful comment
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: https://github.com/tendermint/tendermint/issues/2684