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

Write minValidatorCount to genesis.json #1535

Conversation

jp-imx
Copy link
Contributor

@jp-imx jp-imx commented May 22, 2023

Write the value from the genesis '--min-validator-count' flag to the genesis.json output

Description

Please provide a detailed description of what was done in this PR

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)

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it

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

I ran the following command and verified the file:

./polygon-edge genesis --block-gas-limit 10000000 --epoch-size 10 \                                     
--validators-path ./ --validators-prefix test-chain- \
--consensus polybft \
--reward-wallet ... \
--transactions-allow-list-admin ... \
--transactions-allow-list-enabled ... \
--block-gas-limit ... \
--block-time ... \
--min-validator-count 3 \
--max-validator-count 100

Documentation update

Please link the documentation update PR in this section if it's present, otherwise delete it

Additional comments

Please post additional comments in this section if you have them, otherwise delete it

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Curious how you envision using the MinValidatorSetSize parameter. Currently, it is not used for the polybft consensus protocol, so does it make sense to exist in the first place?

consensus/polybft/polybft_config.go Outdated Show resolved Hide resolved
Co-authored-by: Stefan Negovanović <93934272+Stefan-Ethernal@users.noreply.github.com>
@jp-imx
Copy link
Contributor Author

jp-imx commented May 23, 2023

@Stefan-Ethernal, we are doing some prep work to understand the configurable parameters, how they influence the behaviour of our chain, and make decisions on what the genesis for our public testnet and mainnet should look like. While doing that, I found that the genesis.json file was not documenting this setting. I have yet to look into how it would apply to our chain, if at all.

Can you explain what the intended use of MinValidatorSetSize is ATM?

@goran-ethernal
Copy link
Collaborator

@Stefan-Ethernal, we are doing some prep work to understand the configurable parameters, how they influence the behaviour of our chain, and make decisions on what the genesis for our public testnet and mainnet should look like. While doing that, I found that the genesis.json file was not documenting this setting. I have yet to look into how it would apply to our chain, if at all.

Can you explain what the intended use of MinValidatorSetSize is ATM?

We will definitely use it in on-chain governance, since we are starting to implement that feature for the next release. We will be using it to update the current active validator set. We also added it to PolybftConfig in that governance PR we started: https://github.com/0xPolygon/polygon-edge/pull/1519/files#diff-8291393e7671084ce64a724f76f24df16cf0e262de84905168c4667f94a320abR43

@jp-imx
Copy link
Contributor Author

jp-imx commented May 23, 2023

@goran-ethernal seems like the PR you referenced already includes the changes in the branch. I will close this PR as redundant.

@jp-imx jp-imx closed this May 23, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 23, 2023
@goran-ethernal
Copy link
Collaborator

@goran-ethernal seems like the PR you referenced already includes the changes in the branch. I will close this PR as redundant.

Nah, we can merge your PR ASAP. No need to wait for ours.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants