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.

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

Add a condition sending a COMMIT message in handleCommit #888

Merged
merged 3 commits into from
Feb 24, 2021

Conversation

aidan-kwon
Copy link
Member

@aidan-kwon aidan-kwon commented Feb 8, 2021

Proposed changes

When CNs try to have a consensus on the hashed-locked block, they may fail to send a commit message. Non-hash-locked nodes need to change their state to prepared sending a commit message when they received more than 2f+1 prepare or commit messages. But, they don't change their state in the current logic when only commit messages are delivered after 2f messages since the state changing logic exists in prepare message handling logic.

With this change, committee nodes can change their state to prepared when they received 2f+1 agreement regardless of the message order.

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

  • This bug prolonged consensus time triggering round change more frequently for hash-locked proposals in Cypress network.

@aidan-kwon aidan-kwon added the issue/bug Issues with the code-level bugs. label Feb 8, 2021
@aidan-kwon aidan-kwon self-assigned this Feb 8, 2021
@ehnuje ehnuje self-requested a review February 8, 2021 05:15
@KimKyungup
Copy link
Contributor

KimKyungup commented Feb 17, 2021

@alanchchen @bailantaotao @markya0616 @tailingchen @yutelin
Hello. I invited you to ask for your opinion on this PR.
Seeing @yutelin writing ethereum/EIPs#650, I know you developed Quorum IBFT implementation.

First of all, briefly speaking about our Klaytn blockchain.
It is based on the Geth and Quorum IBFT. Then, we optimized for 1 second block time and added several feature like account/transaction type, fee delegation, etc. (For more information, visit https://docs.klaytn.com/klaytn, https://scope.klaytn.com/)

Recently, the Klaytn network has some round change problems. And we investigated and concluded that the IBFT code misses some code like this PR. And also we made a issue/PR(Consensys/quorum#1133, Consensys/quorum#1134) to Quorum.

So I think it's the most accurate and honorable to get a review as the original developer.
If possible, we would appreciate your feedback via this PR or the email below.

ethan.kim@groundx.xyz

@alanchchen
Copy link

@alanchchen @bailantaotao @markya0616 @tailingchen @yutelin

Hello. I invited you to ask for your opinion on this PR.

Seeing @yutelin writing ethereum/EIPs#650, I know you developed Quorum IBFT implementation.

First of all, briefly speaking about our Klaytn blockchain.

It is based on the Geth and Quorum IBFT. Then, we optimized for 1 second block time and added several feature like account/transaction type, fee delegation, etc. (For more information, visit https://docs.klaytn.com/klaytn, https://scope.klaytn.com/)

Recently, the Klaytn network has some round change problems. And we investigated and concluded that the IBFT code misses some code like this PR. And also we made a issue/PR(Consensys/quorum#1133, Consensys/quorum#1134) to Quorum.

So I think it's the most accurate and honorable to get a review as the original developer.

If possible, we would appreciate your feedback via this PR or the email below.

ethan.kim@groundx.xyz

Hi, it's honored to see that Klaytn chooses IBFT. Unfortunately our team haven't continued IBFT development for a long time. I suggest you contact the Quorum team.

BTW, Hyperledger Besu have implemented IBFT 2.0. It might be helpful.

Thank you 😀

@aidan-kwon aidan-kwon merged commit 7b152e9 into klaytn:dev Feb 24, 2021
@aidan-kwon aidan-kwon deleted the 210208-fixForHashLock branch February 24, 2021 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/bug Issues with the code-level bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants