Skip to content
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

consensus/istanbul: add round to previous hash #997

Merged
merged 4 commits into from
Jul 21, 2021
Merged

consensus/istanbul: add round to previous hash #997

merged 4 commits into from
Jul 21, 2021

Conversation

yoomee1313
Copy link
Contributor

@yoomee1313 yoomee1313 commented Jun 24, 2021 •
edited

Proposed changes

  • This PR implements istanbul compatible change item. Hardfork blocknumber is nil, so it is not activated, yet.
  • Main idea
    • When selecting next committee set, a seed is provided to select committee set in random. However, ingredient of the seed is only previous block's hash. For this reason, committee set is very similar between two different rounds.
    • To solve this problem, round value is added to the ingredient of the seed.
  • This PR have three commits.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

  • Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@yoomee1313 yoomee1313 self-assigned this Jun 24, 2021
@yoomee1313 yoomee1313 marked this pull request as ready for review June 30, 2021 08:02
@yoomee1313 yoomee1313 marked this pull request as draft July 2, 2021 08:43
@yoomee1313 yoomee1313 force-pushed the next-committee branch 2 times, most recently from 8b6d388 to 2fbf9fb Compare July 18, 2021 10:34
@yoomee1313 yoomee1313 marked this pull request as ready for review July 19, 2021 01:08
@yoomee1313 yoomee1313 removed the draft label Jul 19, 2021
@yoomee1313
Copy link
Contributor Author

yoomee1313 commented Jul 19, 2021 •
edited

Dear reviewers, revision of #997 and #1001 is done. Please take a look. It would be good to see it as commit by commit.
This PR will be reviewed first, and then we will review the 1001 PR.
cc. @aidan-kwon @jeongkyun-oh @hyochan-brown @winnie-byun @ehnuje

joowon-byun
joowon-byun reviewed Jul 19, 2021
View reviewed changes
fork/fork.go Outdated Show resolved Hide resolved
fork/fork.go Show resolved Hide resolved
@yoomee1313 yoomee1313 mentioned this pull request Jul 20, 2021
Merged
9 tasks
@yoomee1313 yoomee1313 force-pushed the next-committee branch 2 times, most recently from 39a8a71 to 756541e Compare July 20, 2021 13:46
@yoomee1313 yoomee1313 force-pushed the next-committee branch 2 times, most recently from f37b6ed to 7b268b6 Compare July 20, 2021 16:41
aidan-kwon
aidan-kwon reviewed Jul 21, 2021
View reviewed changes
fork/fork.go Outdated Show resolved Hide resolved
@yoomee1313 yoomee1313 mentioned this pull request Jul 21, 2021
Closed
9 tasks
ehnuje
ehnuje reviewed Jul 21, 2021
View reviewed changes
Copy link
Contributor

@ehnuje ehnuje left a comment

Choose a reason for hiding this comment

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

Some of your changes are not reflected in github (or just for me)
Could you please check?

fork/fork.go Outdated Show resolved Hide resolved
consensus/istanbul/validator/weighted.go Show resolved Hide resolved
consensus/istanbul/config.go Outdated Show resolved Hide resolved
@aidan-kwon aidan-kwon requested a review from ehnuje July 21, 2021 09:26
@yoomee1313 yoomee1313 merged commit a91a363 into klaytn:dev Jul 21, 2021
@yoomee1313 yoomee1313 deleted the next-committee branch July 21, 2021 10:02
@aidan-kwon aidan-kwon mentioned this pull request Oct 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehnuje ehnuje ehnuje approved these changes

@joowon-byun joowon-byun joowon-byun approved these changes

@jeongkyun-oh jeongkyun-oh jeongkyun-oh approved these changes

@aidan-kwon aidan-kwon aidan-kwon approved these changes

@hyochan-brown hyochan-brown Awaiting requested review from hyochan-brown

@KimKyungup KimKyungup Awaiting requested review from KimKyungup

Assignees

@yoomee1313 yoomee1313

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yoomee1313 @ehnuje @joowon-byun @jeongkyun-oh @aidan-kwon