Skip to content

types: return an error if voting power overflows#4764

Merged
mergify[bot] merged 3 commits intomasterfrom
anton/4755-int64-overflow
May 4, 2020
Merged

types: return an error if voting power overflows#4764
mergify[bot] merged 3 commits intomasterfrom
anton/4755-int64-overflow

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Apr 29, 2020

in VerifyCommitTrusting

Closes #4755

@auto-comment
Copy link

auto-comment bot commented Apr 29, 2020

👋 Thanks for creating a PR!

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer
  • Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

@melekes melekes self-assigned this Apr 29, 2020
@melekes melekes added C:light Component: Light R:minor Release: Minor labels Apr 29, 2020
@melekes melekes marked this pull request as ready for review April 29, 2020 07:39
@melekes melekes requested a review from tessr as a code owner April 29, 2020 07:39
@codecov-io
Copy link

Codecov Report

Merging #4764 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4764      +/-   ##
==========================================
- Coverage   64.59%   64.55%   -0.05%     
==========================================
  Files         216      216              
  Lines       20104    20104              
==========================================
- Hits        12987    12978       -9     
- Misses       6075     6083       +8     
- Partials     1042     1043       +1     
Impacted Files Coverage Δ
consensus/ticker.go 91.66% <0.00%> (-4.17%) ⬇️
consensus/state.go 74.65% <0.00%> (-1.38%) ⬇️
consensus/reactor.go 78.29% <0.00%> (-1.16%) ⬇️
blockchain/v0/reactor.go 73.68% <0.00%> (-0.88%) ⬇️
blockchain/v0/pool.go 78.98% <0.00%> (+0.31%) ⬆️
p2p/pex/addrbook.go 70.62% <0.00%> (+0.46%) ⬆️
consensus/replay.go 73.64% <0.00%> (+0.77%) ⬆️
blockchain/v2/reactor.go 36.36% <0.00%> (+0.82%) ⬆️
p2p/pex/pex_reactor.go 84.91% <0.00%> (+2.23%) ⬆️
crypto/sr25519/pubkey.go 39.28% <0.00%> (+7.14%) ⬆️
... and 1 more

Copy link
Contributor

@tessr tessr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Great test cases too

@melekes melekes added the S:automerge Automatically merge PR when requirements pass label May 4, 2020
@mergify mergify bot merged commit 336b929 into master May 4, 2020
@mergify mergify bot deleted the anton/4755-int64-overflow branch May 4, 2020 07:35
@tessr tessr removed the R:minor Release: Minor label May 27, 2020
absOfB--
}

if (b < 0 && a > 0) || (b < 0 && a < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be if b < 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. b < 0 && a != 0 should work though

return c
}

func safeMul(a, b int64) (int64, bool) {
Copy link
Contributor

@ebuchman ebuchman Jun 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be more efficient to just check that |a| < MaxInt/|b| like for safeAdd?

melekes added a commit that referenced this pull request Jun 29, 2020
Refs #4764 (comment)

also, multiplication via * should be more efficient.
@melekes melekes mentioned this pull request Jun 29, 2020
mergify bot pushed a commit that referenced this pull request Jun 29, 2020
Refs #4764 (comment)

also, multiplication using * should be more efficient.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:light Component: Light S:automerge Automatically merge PR when requirements pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integer Overflow in ValidatorSet.VerifyCommitTrusting

4 participants