Skip to content

Block double vote attempts #1800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Sep 30, 2019
Merged

Block double vote attempts #1800

merged 5 commits into from
Sep 30, 2019

Conversation

@foriequal0
Copy link
Contributor

Close: #1778

@foriequal0 foriequal0 self-assigned this Sep 26, 2019
remagpie
remagpie requested changes Sep 26, 2019
View reviewed changes
majecty
majecty requested changes Sep 26, 2019
View reviewed changes
remagpie
remagpie requested changes Sep 26, 2019
View reviewed changes
@kseo kseo changed the title Block to attempt to double vote Block double vote attemptions Sep 26, 2019
@remagpie remagpie changed the title Block double vote attemptions Block double vote attempts Sep 26, 2019
@foriequal0 foriequal0 force-pushed the fix/block-attempt-to-double-vote branch 2 times, most recently from ab7ccd5 to a934ba9 Compare September 26, 2019 10:35
majecty
majecty previously approved these changes Sep 26, 2019
View reviewed changes
@foriequal0 foriequal0 force-pushed the fix/block-attempt-to-double-vote branch from a934ba9 to e21bd6c Compare September 26, 2019 10:59
remagpie
remagpie previously approved these changes Sep 26, 2019
View reviewed changes
@foriequal0 foriequal0 force-pushed the fix/block-attempt-to-double-vote branch from e21bd6c to b654d1e Compare September 26, 2019 17:44
remagpie
remagpie requested changes Sep 27, 2019
View reviewed changes
@foriequal0
Add VoteRegressionChecker
57f220a
@foriequal0
Rename VoteCollector::vote to collect
03bfe3b
@foriequal0
Fix to use Err(DoubleVote) from VoteCollector
27cd4c5 
VoteCollector is changed to return Result<bool, DoubleVote>. Rust
compiler will force you to check whether there was a double vote.
@foriequal0 foriequal0 force-pushed the fix/block-attempt-to-double-vote branch from b654d1e to 27cd4c5 Compare September 29, 2019 16:38
@foriequal0
Copy link
Contributor Author

I'll remove @majecty from the reviewers since he is vacant. @HoOngEe Would you review this?
Here are the review points:

  • "Cleanup vote functions" is a refactoring. You should carefully review whether I've changed the behavior accidentally.
  • "Fix to use Err(DoubleVote) from VoteCollector" is adding missing assertions. It should not change the behavior unless the node tries to double vote. In that case, the node should crash immediately since there's no way to recover from the error.
  • Other commits: They have straightforward names and tests. I think checking styles and consistencies would be enough.

@foriequal0 foriequal0 removed the request for review from majecty September 30, 2019 02:55
@HoOngEe
Copy link
Contributor

HoOngEe commented Sep 30, 2019

Thank you for the summary, I started reviewing.

HoOngEe
HoOngEe approved these changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@HoOngEe HoOngEe left a comment

Choose a reason for hiding this comment

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

LGTM

@foriequal0
Copy link
Contributor Author

CI passed. I'll merge it manually.

@foriequal0 foriequal0 merged commit 8731fc4 into CodeChain-io:master Sep 30, 2019
@foriequal0 foriequal0 deleted the fix/block-attempt-to-double-vote branch September 30, 2019 08:57
@foriequal0 foriequal0 mentioned this pull request Sep 30, 2019
Merged
@HoOngEe HoOngEe mentioned this pull request Oct 18, 2019
Closed
@foriequal0 foriequal0 mentioned this pull request Nov 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@remagpie remagpie remagpie approved these changes

@HoOngEe HoOngEe HoOngEe approved these changes

@majecty majecty majecty left review comments

Assignees

@foriequal0 foriequal0

Labels

consensus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Prevent further double vote incidents

4 participants

@foriequal0 @HoOngEe @majecty @remagpie