abci: update Header to match amino encoding of types.Header#2681
abci: update Header to match amino encoding of types.Header#2681
Conversation
| // basic block info | ||
| Version version = 1 [(gogoproto.nullable)=false]; | ||
| string chain_id = 2 [(gogoproto.customname)="ChainID"]; | ||
| int64 height = 3; |
There was a problem hiding this comment.
I hope its not too late to revert this decision prelaunch, but I think height being signed is the wrong decision. Feel free to close this comment if the ship has sailed.
There was a problem hiding this comment.
Did you read https://blog.cosmos.network/choosing-a-type-for-blockchain-height-beware-of-unsigned-integers-714804dddf1d ? Can you argue against it ? Having to add an s here made me doubt our decision for the first time, but I still think it's probably right.
There was a problem hiding this comment.
I have read it, will respond more later. I do disagree with it, and think that not choosing uint does increase chance of error. (I'll have to double check later, but I think we even have evidence of it doing so in our project)
There was a problem hiding this comment.
uints by design mitigate classes of bugs where the height would be negative. This could potentially be an issue on input received by a peer. Ref #2683. Its better to have this be a compile time guarantee.
I don't agree with the argumented presented in the blog post. I believe it mostly stems from a sub-optimally safe implementation at the time. Note that the blog post is essentially stating that "in the event of underflow when our logic does subtraction, its harder to tell if underflow happened."
When we do things like evidenceAge := height - evidence.Height(), we should have an explicit handler for the negative case anyways. I.e. To sanitize inputs, ensure that evidence.Height() <= height. In every situation where this underflowI could occur, I assert that it ought to be handled separately regardless. (The fact that the code currently doesn't do this is a separate issue) It makes more structural sense to sanitize this case before assigning to a variable. Doing the negative check afterwards (which already is lacking in every spot I've seen) seems worse, as inputs should be sanitized ASAP.
When doing things like foo < height - 1, I also think we need separate logic for the height = 0 case. Again us not having code handling the height=0 in the current code is a second bug. If the code were written more safely / with better errors, there would be no issue switching and we would get the compile-time guarantee / input sanitization for free. The fact that we need to account for these underflows separately is something that should be treated separately anyway, its an issue with the current code that it is not. This what I think led up to the decision to make these ints, but instead we should write the logic to handle these cases explicitly. Then we get the uint compile time guarantee.
To phrase this another way, the current implementation is handling these underflow cases ungracefully (i.e. same error message), and also requiring sanitization of all received input. Instead we should write it to handle underflow gracefully, and then switch to compile-time guarantees to remove that particular input sanitation.
There was a problem hiding this comment.
Another option with uints: implement SafeSub, SafeAdd, SafeMul, and SafeDiv, wrappers for checked arithmetic, and lint for any usage of bare operators in the codebase - that gives both compile-time compiler guarantees of correct signage and compile-time linter prevention of accidental check omission. It also protects better against cases where inputs might be checked initially when received from a peer, but are then added or combined in ways which could result in overflow/underflow if not explicitly checked then.
There was a problem hiding this comment.
The linter you're proposing sounds like the "Code Jesus" proposed in https://blog.cosmos.network/choosing-a-type-for-blockchain-height-beware-of-unsigned-integers-714804dddf1d.
I've summarized some thoughts about integers in Tendermint given recent events in: #2684
| // EvidenceParams contains limits on the evidence. | ||
| message EvidenceParams { | ||
| // Note: must be greater than 0 | ||
| int64 max_age = 1; |
There was a problem hiding this comment.
IMO, This really should be a uint, since we even say must be greater than 0.
Codecov Report
@@ Coverage Diff @@
## develop #2681 +/- ##
==========================================
Coverage ? 61.62%
==========================================
Files ? 207
Lines ? 16895
Branches ? 0
==========================================
Hits ? 10412
Misses ? 5622
Partials ? 861
|
| block.AppHash = state.AppHash | ||
| block.LastResultsHash = state.LastResultsHash | ||
|
|
||
| // NOTE: we can't use the state.Validators because we don't |
There was a problem hiding this comment.
Agree - it's commented in the functions godoc. But maybe we should bring it back here too?
There was a problem hiding this comment.
it's commented in the functions godoc.
where? 👀
|
Not thrilled about having to change these all to |
| [[constraint]] | ||
| name = "github.com/tendermint/go-amino" | ||
| version = "v0.12.0-rc0" | ||
| version = "v0.13.0-rc0" |
There was a problem hiding this comment.
The release candidate is created from the "prepare release PR" which doesn't contain the commit for the renaming of MarshalBinary -> MarshalBinaryLenPrefixed (tendermint/go-amino#222 got merged after the PR was opened).
Turns out the amino encoding of the types.Header didn't match the proto3 encoding of the abci.Header - ref #2682.
The problem was that the ABCI header wasn't using signed integers properly -
int64in proto files maps touint64in Go. Need to usesint64in the proto files instead.We also needed to update to amino v0.13.0 so that the Timestamp encoding would be compatible.
This PR fixes all of that - now the aminoEncoding(types.Header) and proto3Encoding(abci.Header) match.