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

EVM-713 Fork manager params #1645

Merged
merged 4 commits into from
Jun 23, 2023
Merged

EVM-713 Fork manager params #1645

merged 4 commits into from
Jun 23, 2023

Conversation

igorcrevar
Copy link
Contributor

@igorcrevar igorcrevar commented Jun 21, 2023

Description

Forks inside genesis.json now will look like:

forks": {
            "Dummy1": {
                "block": 10,
                "params": {
                    "maxValidatorSetSize": 30
                }
            },
            "EIP150": {
                "block": 0,
            },
            ...
}

Currently there are 5 predefined parameters for forks

type ForkParams struct {
	// MaxValidatorSetSize indicates the maximum size of validator set
	MaxValidatorSetSize *uint64 `json:"maxValidatorSetSize,omitempty"`

	EpochSize *uint64 `json:"epochSize,omitempty"`

	// SprintSize is size of sprint
	SprintSize *uint64 `json:"sprintSize,omitempty"`

	// BlockTime is target frequency of blocks production
	BlockTime *common.Duration `json:"blockTime,omitempty"`

	// BlockTimeDrift defines the time slot in which a new block can be created
	BlockTimeDrift *uint64 `json:"blockTimeDrift,omitempty"`
}

We can specify some or all of those parameters for some fork in genesis.json. The parameters need to be pointers because some of them can be omitted. If some parameter is omitted for specific fork, then its values is picked from first fork with smaller block number or from initial fork parameters.

During the execution, parameter can be retrieved by using fork manager func (fm *forkManager) GetParams(blockNumber uint64) *chain.ForkParams

Initial parameters for polybft are set via builtin.go mapping function inside polybft.go

func ForkManagerInitialParamsFactory(config *chain.Chain) (*chain.ForkParams, error) {
	pbftConfig, err := GetPolyBFTConfig(config)
	if err != nil {
		return nil, err
	}

	return &chain.ForkParams{
		MaxValidatorSetSize: &pbftConfig.MaxValidatorSetSize,
		EpochSize:           &pbftConfig.EpochSize,
		SprintSize:          &pbftConfig.SprintSize,
		BlockTime:           &pbftConfig.BlockTime,
		BlockTimeDrift:      &pbftConfig.BlockTimeDrift,
	}, nil
}

If we want to add new parameter, then we need to add that parameter (as a pointer) to ForkParams and change its value in genesis.json. In that case its probably good to change ForkManagerInitialParamsFactory too

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

@igorcrevar igorcrevar force-pushed the feature/fork_manager_params branch 3 times, most recently from b2c0479 to 7704d0b Compare June 21, 2023 09:50
@igorcrevar igorcrevar force-pushed the feature/fork_manager_params branch 3 times, most recently from 86e7360 to 5976087 Compare June 21, 2023 12:56
@igorcrevar igorcrevar requested a review from a team June 21, 2023 13:01
@igorcrevar igorcrevar self-assigned this Jun 21, 2023
@igorcrevar igorcrevar added the feature New update to Polygon Edge label Jun 21, 2023
@igorcrevar igorcrevar force-pushed the feature/fork_manager_params branch 2 times, most recently from 8946346 to b2f757e Compare June 22, 2023 10:05
@igorcrevar igorcrevar marked this pull request as ready for review June 22, 2023 10:06
@igorcrevar igorcrevar changed the title Fork manager params poc Fork manager params Jun 22, 2023
@igorcrevar igorcrevar changed the title Fork manager params EVM-713 Fork manager params Jun 22, 2023
chain/params.go Outdated Show resolved Hide resolved
chain/params.go Outdated Show resolved Hide resolved
@igorcrevar igorcrevar force-pushed the feature/fork_manager_params branch 4 times, most recently from 088576f to b84662a Compare June 22, 2023 12:03
chain/params.go Show resolved Hide resolved
forkmanager/fork_manager.go Show resolved Hide resolved
forkmanager/fork_manager.go Outdated Show resolved Hide resolved
forkmanager/fork_manager.go Outdated Show resolved Hide resolved
forkmanager/fork.go Outdated Show resolved Hide resolved
consensus/polybft/polybft.go Show resolved Hide resolved
forkmanager/fork.go Outdated Show resolved Hide resolved
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.

I have a couple of questions:

  • as I understand, this is just a temporary solution since we haven't implemented on chain governance yet. When and if we implement it, this feature would be redundant and we can remove it if necessary?
  • we could change these parameters manually in the genesis.json and after server is restarted, those changes would take place. In that sense, it may look redundant to it. Although convenience for forking support is that you can set the parameters and delay activation by specifying exact block and we are good. Anyways is forking mechanism going to override it if somebody chooses to change some value by hand?
  • if I'm correct, this is only useful for us (Edge team) if we want to enfore some set of parameters along with their values, but not sure how it is going to be used by the end users? They would probably need to fork Edge repository and implement some fork on their own. Is that correct?

chain/params.go Outdated Show resolved Hide resolved
@igorcrevar igorcrevar merged commit 2df1b93 into develop Jun 23, 2023
4 of 6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2023
@vcastellm vcastellm deleted the feature/fork_manager_params branch June 26, 2023 16:47
@vcastellm
Copy link
Contributor

  • as I understand, this is just a temporary solution since we haven't implemented on chain governance yet. When and if we implement it, this feature would be redundant and we can remove it if necessary?
  • we could change these parameters manually in the genesis.json and after server is restarted, those changes would take place. In that sense, it may look redundant to it. Although convenience for forking support is that you can set the parameters and delay activation by specifying exact block and we are good. Anyways is forking mechanism going to override it if somebody chooses to change some value by hand?
  • if I'm correct, this is only useful for us (Edge team) if we want to enfore some set of parameters along with their values, but not sure how it is going to be used by the end users? They would probably need to fork Edge repository and implement some fork on their own. Is that correct?

Would be great to have answers for these questions @igorcrevar , how's that envisioned?

@Nemanja0x
Copy link
Contributor

  • as I understand, this is just a temporary solution since we haven't implemented on chain governance yet. When and if we implement it, this feature would be redundant and we can remove it if necessary?

Correct.

  • we could change these parameters manually in the genesis.json and after server is restarted, those changes would take place. In that sense, it may look redundant to it. Although convenience for forking support is that you can set the parameters and delay activation by specifying exact block and we are good. Anyways is forking mechanism going to override it if somebody chooses to change some value by hand?

We need to add code (similar to handlers) which will determine what is the proper value of the parameter based on the block.
For example, If epoch size is 50 before block 100 and 500 after block 1000 we need a handler to return a proper value based on the current block.

  • if I'm correct, this is only useful for us (Edge team) if we want to enfore some set of parameters along with their values, but not sure how it is going to be used by the end users? They would probably need to fork Edge repository and implement some fork on their own. Is that correct?

Correct.

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

Successfully merging this pull request may close these issues.

None yet

5 participants