ABCI tighter type constraints
tomtau opened this issue · 6 comments
Some of the message types generated by protobuf have a bit imprecise types -- for example, the block height is returned as i64 even though it should never be negative
I believe this issue may be relevant in tendermint to change the int64 types to uint64 types in proto files where needed?
I agree with you @tomtau. Also, @marbar3778 is right that the proto files (and hence the generated code) should match the tendermint repo. Actually discussing signed vs unsinged integers has a long tradition:
tendermint/tendermint#2684
https://blog.cosmos.network/choosing-a-type-for-blockchain-height-beware-of-unsigned-integers-714804dddf1d
IIRC, we wanted to switch to unsigned where values could never go negative but focused on other things.
Ad that block height discussion: Rust does under/overflow assertion checks in debug releases by default; one could enable them in production builds in a Cargo profile configuration.
Anyway, I guess except for "logic" (#49), there could be some value sanity checks/assertions as well
Besides block height (and perhaps time), there are a few other examples where the protobuf type doesn't indicate to what the specs say -- for example, for Tags: https://tendermint.com/docs/spec/abci/abci.html#tags it mentions keys/values must be UTF-8 strings, but they are Vec.
On the other hand, I've been abusing this particular part and it doesn't seem to have any effect on Tendermint (RPC happily returns them base64-encoded). I guess that only affects indexing / querying or the ABCI specs documented some no longer valid assumption?
Another thing is "Query": https://tendermint.com/docs/spec/abci/abci.html#query
Apps MUST interpret '/store' as a query by key on the underlying store.
Currently, the trait doesn't force implementors to do so -- on the other hand, I didn't observe any consequences if one doesn't do so?
Ad that block height discussion: Rust does under/overflow assertion checks in debug releases by default; one could enable them in production builds in a Cargo profile configuration.
Anyway, I guess except for "logic" (#49), there could be some value sanity checks/assertions as well
Any reason not having
[profile.release]
overflow-checks = true
in Cargo.toml?
@martinholovsky https://doc.rust-lang.org/cargo/reference/manifest.html#the-profile-sections
Any manifest may declare a profile, but only the top level package’s profiles are actually read. All dependencies’ profiles will be overridden.
rust-abci is a library, i.e. won't be top-level. My guess (from that description) is that regardless what's declared here, profiles of application crates override it (so it should be declared in Cargo.toml of applications rather than here).