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

[IBFT] Introduce quorum calculation switch #549

Merged
merged 19 commits into from
May 16, 2022
Merged

Conversation

dbrajovic
Copy link
Contributor

@dbrajovic dbrajovic commented May 12, 2022

Description

This PR enables users to specify a block number after which QuorumOptimal (2/3 * N) will be used to calculate the required number of messages for the given network size (N).

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

  1. Setup a 6-node cluster on commit 805f9ef
  2. Start 3 of the nodes
  3. Start a non-validator node on this branch
  4. Observe that the non-validator is unable to sync with the network
  5. Run ibft quorum --from XX --chain your_genesis_file.json to specify the block number after which quorum optimal calculation will be used
  6. Restart all (6) nodes with the binary built from this branch and the chain grow
  7. Run the non-validator node and observe it is able to sync up with the network

Documentation update

TODO

Additional comments

PR #513 introduced a new calculation for quorum size of a given validator set. In practical terms, this means the number of votes required for reaching consensus became more constrained. Consequentially, the PR implemented a breaking change for existing chains, as any new node joining the network would not be able to verify previously sealed blocks.

By specifying a --from value for the ibft quorum command, new nodes syncing with the chain will be able to correctly verify seals from older blocks. Old blocks (up until the value specified) will be verified using the old formula (LegacyQuorumSize) and all subsequent blocks with the new (OptimalQuorumSize).

How-to: applying the flag for existing chains

  1. Update all nodes to a version of Polygon-Edge that includes this PR
  2. Modify your genesis file by running: polygon-edge ibft quorum --from [your_quorum_switch_block_num] --chain [your_genesis_file]. Make sure that after your_quorum_switch_block_num an OptimalQuorumSize of validators are signing the blocks
  3. Observe that all (new) nodes are able to sync with the running network

Fixes EDGE-556

command/ibft/ibft.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks great 💯

I've left some minor nitpick comments, nothing major 👍

Great work on this PR 👏

@zivkovicmilos zivkovicmilos added this to the 0.4.0 milestone May 13, 2022
Copy link
Contributor

@Kourin1996 Kourin1996 left a comment

Choose a reason for hiding this comment

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

LGTM. I will approve after the fixes for @zivkovicmilos's reviews are pushed

Copy link
Contributor

@0xAleksaOpacic 0xAleksaOpacic left a comment

Choose a reason for hiding this comment

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

LGTM!

command/ibft/quorum/params.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Approving, again 💯

@dbrajovic dbrajovic merged commit b0f6f0c into develop May 16, 2022
@dbrajovic dbrajovic deleted the hotfix/legacy-quorum branch May 16, 2022 14:20
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New update to Polygon Edge hotfix Major bug fix that should be merged ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failed to write bulk sync blocks: failed to verify the header: not enough seals to seal block
4 participants