check for overflow and underflow while choosing proposer by melekes · Pull Request #997 · tendermint/tendermint

melekes · 2017-12-21T20:06:14Z

codecov-io · 2017-12-21T20:31:11Z

Codecov Report

Merging #997 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #997      +/-   ##
===========================================
+ Coverage    59.98%   60.01%   +0.03%     
===========================================
  Files          109      109              
  Lines        10198    10245      +47     
===========================================
+ Hits          6117     6149      +32     
- Misses        3524     3537      +13     
- Partials       557      559       +2

melekes · 2017-12-22T18:59:20Z

Should I write a comment that the reason for these checks is that it is critical code?
@jaekwon is this what you had in mind by "mind the overflow". What's the word mind mean? Be mindful or actually add the checks plus tests like a did?
Do we need more tests here?

jaekwon · 2017-12-25T19:50:20Z

types/validator_set.go

-		val.Accum += val.VotingPower * int64(times) // TODO: mind overflow
+		res, overflow := signedMulWithOverflowCheck(val.VotingPower, int64(times))
+		// check for overflow both multiplication and sum
+		if !overflow && val.Accum <= mostPositive-res {


lets make addSafe().

jaekwon · 2017-12-25T19:50:51Z

types/validator_set.go

 	validatorsHeap := cmn.NewHeap()
 	for _, val := range valSet.Validators {
-		val.Accum += val.VotingPower * int64(times) // TODO: mind overflow
+		res, overflow := signedMulWithOverflowCheck(val.VotingPower, int64(times))


s/signedMulWithOverflowCheck/mulSafe/g
I don't think we'd check overflow with unsigned numbers.

or multiply unsigned numbers for that matter. if we do it's probably for bit-twiddling and we'll prob want to create custom functions for those kinds of operations, I think.

jaekwon · 2017-12-25T19:52:11Z

types/validator_set.go

 		}
-		mostest.Accum -= int64(valSet.TotalVotingPower())
+
+		// mind underflow


I'd add underflow protection to addSafe() as well. maybe we can have subSafe() too, which handles both over/underflow protection.

jaekwon · 2017-12-25T19:52:20Z

types/validator_set.go

 	if valSet.totalVotingPower == 0 {
 		for _, val := range valSet.Validators {
-			valSet.totalVotingPower += val.VotingPower
+			// mind overflow


Refs #919

melekes · 2017-12-26T00:43:03Z

types/validator_set.go

 	for _, val := range valSet.Validators {
-		val.Accum += val.VotingPower * int64(times) // TODO: mind overflow
+		// check for overflow both multiplication and sum
+		res, overflow := safeMul(val.VotingPower, int64(times))


maybe we can simplify this somehow?

safeMulClip could automatically set to math.MaxInt64 or math.MinInt64 appropriately, and return a single value. safeAddClip likewise.

jaekwon · 2017-12-26T06:16:43Z

types/validator_set.go

+///////////////////////////////////////////////////////////////////////////////
+// Safe multiplication and addition/subtraction
+
+const mostNegative int64 = -mostPositive - 1


math.MaxInt64 and math.MinInt64

ebuchman · 2017-12-26T15:25:06Z

Is this really how we should deal with the overflows - ie. just use the max ints? Or should we consider big int arithmetic ?

* add mempool English and Quint specifications * some changes * Update spec/mempool/mempool.md Co-authored-by: Lasaro <lasaro@gmail.com> * Update spec/mempool/mempool.md Co-authored-by: Lasaro <lasaro@gmail.com> * small change to the text * add changelog entry * some minor fixes --------- Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com> Co-authored-by: Lasaro <lasaro@gmail.com>

…endermint#1010) * Move senders from txs to txSenders * Move resCbFirstTime to globalCb * Fix typo * Remove callback argument from CheckTx * Fix lint * make mocks * lock isSender * Change sync.Map for map with lock; add test * fix lint * comments * move senders to reactor and add txsRemoved channel * Record sender only on valid txs * fix MConnection panicked * notifyTxRemoved when removeAllTxs * Add TxsRemoved and EnableTxsRemoved to interface * Increase channel buffer size * forgot emptyMempool * Change Mempool interface * Revert "Change Mempool interface" This reverts commit d3468a12843b11269a180c9f889f1ec79c55b1d3. * Simplify if/else * Channel buffer size * notify txRemoved even when txKey is not in txsMap * Remove redundant update to map * add callback for removing txs * Add tests * Use Mutex intead of RWMutex * Fix TestMempoolNoCacheOverflow * Fix lint * Fix TestMempoolTxConcurrentWithCommit * Fix TestReactorConcurrency * Comment * Remove references to implemenation details (cache) * Rename removeTxOnReactor * Comment * Rename removeFromCache * Comment * Remove test line forgotten in tendermint#934. * Comment * revert mempool test size * ci: Trigger workflows on merge group (tendermint#1118) Signed-off-by: Thane Thomson <connect@thanethomson.com> * Revert "config: add bootstrap peers (tendermint#9680)" (tendermint#1109) * Revert "config: add bootstrap peers (tendermint#9680)" This reverts commit f12588a. * docs/p2p: bootstrap_peers config flag removed * node: Revert removal of public reactor accessors (tendermint#1120) * Revert "Remove unused code (tendermint#286)" This reverts commit a2d9915. Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Remove access to consensus state Consensus state should only ever be accessible via the consensus reactor. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix mistake in changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> * ci: Disable CodeQL check in merge queues (tendermint#1123) Signed-off-by: Thane Thomson <connect@thanethomson.com> * p2p: Remove UPnP functionality (tendermint#1114) * Remove UPnP functionality Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update documentation and specs to reflect UPnP removal Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> * ADR 107: Rename proto versions to pre-v1 betas (tendermint#1110) * ADR 107: Rename proto versions to pre-v1 betas * ADR 107: fix hyperlinks to ADR 103 * ADR 107: authorship of the revision Co-authored-by: Thane Thomson <connect@thanethomson.com> * ADR 107: change status to Accepted --------- Co-authored-by: Thane Thomson <connect@thanethomson.com> * RFC 104: Internal messaging using the actor model (tendermint#1092) * Add first draft Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment on actor receive method Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix grammar Signed-off-by: Thane Thomson <connect@thanethomson.com> * Emphasize/clarify conclusions Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> * build(deps): Bump github.com/bufbuild/buf from 1.23.1 to 1.24.0 (tendermint#1131) Bumps [github.com/bufbuild/buf](https://github.com/bufbuild/buf) from 1.23.1 to 1.24.0. - [Release notes](https://github.com/bufbuild/buf/releases) - [Changelog](https://github.com/bufbuild/buf/blob/main/CHANGELOG.md) - [Commits](bufbuild/buf@v1.23.1...v1.24.0) --- updated-dependencies: - dependency-name: github.com/bufbuild/buf dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): Bump github.com/vektra/mockery/v2 from 2.31.1 to 2.32.0 (tendermint#1132) Bumps [github.com/vektra/mockery/v2](https://github.com/vektra/mockery) from 2.31.1 to 2.32.0. - [Release notes](https://github.com/vektra/mockery/releases) - [Changelog](https://github.com/vektra/mockery/blob/master/docs/changelog.md) - [Commits](vektra/mockery@v2.31.1...v2.32.0) --- updated-dependencies: - dependency-name: github.com/vektra/mockery/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): Bump docker/setup-buildx-action from 2.9.0 to 2.9.1 (tendermint#1133) Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 2.9.0 to 2.9.1. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](docker/setup-buildx-action@v2.9.0...v2.9.1) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): Bump bufbuild/buf-setup-action from 1.23.1 to 1.24.0 (tendermint#1134) Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.23.1 to 1.24.0. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.23.1...v1.24.0) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * spec: Add mempool specification in English and Quint (tendermint#997) * add mempool English and Quint specifications * some changes * Update spec/mempool/mempool.md Co-authored-by: Lasaro <lasaro@gmail.com> * Update spec/mempool/mempool.md Co-authored-by: Lasaro <lasaro@gmail.com> * small change to the text * add changelog entry * some minor fixes --------- Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com> Co-authored-by: Lasaro <lasaro@gmail.com> * Add NewRandomTxs * Fix TestReactorTxSendersMultiNode * Fix TestDontExhaustMaxActiveIDs * Fix unused parameter * Add changelog * Update UPGRADING.md * Remove unused link in doc --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Mikhail Zabaluev <mikhail.zabaluev@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pierre Sutra <0track@gmail.com> Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com> Co-authored-by: Lasaro <lasaro@gmail.com>

melekes requested a review from ebuchman as a code owner December 21, 2017 20:06

melekes force-pushed the 919-careful-with-validator-voting branch 4 times, most recently from 51a1781 to a9bc325 Compare December 22, 2017 18:57

melekes requested a review from jaekwon December 22, 2017 18:57

jaekwon reviewed Dec 25, 2017

View reviewed changes

melekes added 2 commits December 25, 2017 18:39

check for overflow and underflow while choosing proposer

e8b0458

Refs #919

add safeAdd & safeSub plus quickcheck tests

69c3a76

melekes force-pushed the 919-careful-with-validator-voting branch from a9bc325 to 69c3a76 Compare December 26, 2017 00:39

melekes commented Dec 26, 2017

View reviewed changes

jaekwon reviewed Dec 26, 2017

View reviewed changes

add safe*Clip funcs

1339a44

ebuchman approved these changes Jan 7, 2018

View reviewed changes

ebuchman mentioned this pull request Jan 7, 2018

Document the maximum supported voting power due to overflow #1075

Closed

ebuchman merged commit cf42611 into develop Jan 7, 2018

ebuchman deleted the 919-careful-with-validator-voting branch January 7, 2018 21:48

adrianbrink mentioned this pull request Jan 8, 2018

careful with validator voting power overflow #919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check for overflow and underflow while choosing proposer#997

check for overflow and underflow while choosing proposer#997
ebuchman merged 3 commits intodevelopfrom
919-careful-with-validator-voting

melekes commented Dec 21, 2017

Uh oh!

codecov-io commented Dec 21, 2017 •

edited

Loading

Uh oh!

melekes commented Dec 22, 2017 •

edited

Loading

Uh oh!

jaekwon Dec 25, 2017

Uh oh!

jaekwon Dec 25, 2017

Uh oh!

jaekwon Dec 25, 2017

Uh oh!

jaekwon Dec 25, 2017

Uh oh!

jaekwon Dec 25, 2017

Uh oh!

melekes Dec 26, 2017

Uh oh!

jaekwon Dec 26, 2017

Uh oh!

jaekwon Dec 26, 2017

Uh oh!

ebuchman commented Dec 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

melekes commented Dec 21, 2017

Uh oh!

codecov-io commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melekes commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Dec 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Dec 21, 2017 •

edited

Loading

melekes commented Dec 22, 2017 •

edited

Loading