8000 consensus: the certain case with six validators or less by mckim19 · Pull Request #1462 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

consensus: the certain case with six validators or less #1462

Merged
merged 11 commits into from
Jul 5, 2022
Merged

consensus: the certain case with six validators or less #1462

merged 11 commits into from
Jul 5, 2022

Conversation

mckim19
Copy link
Contributor
@mckim19 mckim19 commented Jun 24, 2022 •
edited
Loading

Proposed changes

If the number of validators is 6 or less, the fork could be occurred by round change and byzantine case.
Becuase there are two quorum in the one block.
The quorum size is 2F+1 at the moment.
If the number of validators is 2 or 3, there is a possibility of multiple consensus in one block becuase F = 0 and quorum is 1.
Also, if the number of validators is 6, there is a possibility that there will be two consensus in one block becuase F = 1 and the quorum is 3.
In incident situation, the number of validators could be reduced for solving the problem.
These cases are very exceptional, but this PR is needed to completely eliminate the possibility of a fork.
In addition, when configuring the service chain and private network with a small number of nodes, this PR enhances network stability.

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

#1403

@mckim19 mckim19 changed the title Under six validators six validators or less Jun 24, 2022
jiseongnoh
jiseongnoh reviewed Jun 27, 2022
View reviewed changes
jiseongnoh
jiseongnoh reviewed Jun 27, 2022
View reviewed changes
@jiseongnoh
Copy link
Contributor

I believe that four consensus nodes should allow one faulty node.
I suggest creating a function like requiredMessageCount() that returns the required number of consensus messages.


// Minimum required number of consensus messages to proceed 
func requiredMessageCount(valset istanbul.ValidatorSet) int {
	size := valset.Size()
	switch size {
	// in the certain cases we must receive the messages from all consensus nodes to ensure finality...
	case 1, 2, 3, 5, 6:
		return int(size)
	default:
		return 2*valset.F() + 1
	}
}

if c.current.GetPrepareOrCommitSize() >= requiredMessageCount(c.valSet) {

@mckim19 mckim19 self-assigned this Jun 27, 2022
@mckim19
Copy link
Contributor Author
mckim19 commented Jun 27, 2022

@jiseongnoh
Thanks for your review.
I added the function that you suggested. ad5851a
That's the way better!

@mckim19 mckim19 force-pushed the under_six_validators branch from c55819d to 7021d00 Compare June 29, 2022 04:53
aidan-kwon
aidan-kwon reviewed Jun 30, 2022
View reviewed changes
Copy link
Member
@aidan-kwon aidan-kwon left a comment

Choose a reason for hiding this comment

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

@mckim19 Can I know why did you change > to >= ?

@aidan-kwon aidan-kwon added the need to merge Need to merge for the next time label Jul 1, 2022
@mckim19
Copy link
Contributor Author
mckim19 commented Jul 1, 2022 •
edited
Loading

@aidan-kwon
Thanks for your review.
Becuase requiredMessageCount func returns 2F+1, 2F+2 or the number of valSet that is enough for consensus.
I thought it would be good for readability if the function name and return value match.
If the func returns 2F like before, it can use >.

aidan-kwon
aidan-kwon reviewed Jul 4, 2022
View reviewed changes
jiseongnoh
jiseongnoh reviewed Jul 4, 2022
View reviewed changes
@mckim19 mckim19 changed the title six validators or less consensus with six validators or less Jul 4, 2022
@mckim19 mckim19 changed the title consensus with six validators or less certain consensus case with six validators or less Jul 4, 2022
@mckim19 mckim19 changed the title certain consensus case with six validators or less consensus: the certain case with six validators or less Jul 4, 2022
yoomee1313
yoomee1313 reviewed Jul 4, 2022
View reviewed changes
jiseongnoh
jiseongnoh reviewed Jul 5, 2022
View reviewed changes
@mckim19 mckim19 requested a review from jiseongnoh July 5, 2022 05:17
jiseongnoh
jiseongnoh approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor
@jiseongnoh jiseongnoh left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you.

@mckim19 mckim19 merged commit a120c12 into klaytn:dev Jul 5, 2022
@mckim19 mckim19 mentioned this pull request Jul 7, 2022
Closed
10 tasks
@mckim19 mckim19 mentioned this pull request Aug 24, 2022
Closed
@mckim19 mckim19 mentioned this pull request Sep 14, 2022
Closed
@blukat29 blukat29 removed the need to merge Need to merge for the next time label Feb 17, 2023
@hyunsooda hyunsooda mentioned this pull request Feb 23, 2023
Merged
9 tasks
@jiseongnoh jiseongnoh mentioned this pull request Nov 7, 2023
Closed
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jiseongnoh jiseongnoh jiseongnoh approved these changes

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

@yoomee1313 yoomee1313 yoomee1313 approved these changes

@KimKyungup KimKyungup Awaiting requested review from KimKyungup

@ethan-kr ethan-kr Awaiting requested review from ethan-kr

@jeongkyun-oh jeongkyun-oh Awaiting requested review from jeongkyun-oh

@ehnuje ehnuje Awaiting requested review from ehnuje

@henry-will henry-will Awaiting requested review from henry-will

@nohkwak nohkwak Awaiting requested review from nohkwak

@2dvorak 2dvorak Awaiting requested review from 2dvorak

Assignees

@mckim19 mckim19

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mckim19 @jiseongnoh @aidan-kwon @yoomee1313 @blukat29
0