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

Update Web3Q validator set from PoS staking contract in remote chain #78

Merged
merged 16 commits into from
May 12, 2022

Conversation

ping-ke
Copy link

@ping-ke ping-ke commented Apr 28, 2022

consensus/tendermint/tendermint.go Outdated Show resolved Hide resolved
consensus/tendermint/tendermint.go Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
consensus/tendermint/tendermint.go Outdated Show resolved Hide resolved
consensus/tendermint/gov/gov.go Outdated Show resolved Hide resolved
consensus/tendermint/adapter/store.go Outdated Show resolved Hide resolved
consensus/tendermint/gov/gov.go Outdated Show resolved Hide resolved
consensus/tendermint/tendermint.go Outdated Show resolved Hide resolved
consensus/tendermint/tendermint.go Outdated Show resolved Hide resolved
consensus/tendermint/tendermint.go Outdated Show resolved Hide resolved
consensus/tendermint/adapter/store.go Outdated Show resolved Hide resolved
consensus/tendermint/adapter/store.go Show resolved Hide resolved
consensus/tendermint/adapter/store.go Outdated Show resolved Hide resolved
consensus/tendermint/adapter/store.go Show resolved Hide resolved
Copy link

@qizhou qizhou left a comment

Choose a reason for hiding this comment

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

The code looks good. Just a few comments regarding the race of calling external contract and some cosmetic changes.

consensus/tendermint/gov/gov.go Outdated Show resolved Hide resolved
consensus/tendermint/adapter/store.go Outdated Show resolved Hide resolved
consensus/tendermint/adapter/store.go Outdated Show resolved Hide resolved
consensus/tendermint/tendermint.go Show resolved Hide resolved
epochId := header.Number.Uint64() / s.config.Epoch
remoteChainNumber := uint64(0)
hash, emptyHash := common.Hash{}, common.Hash{}
if s.config.ValidatorChangeEpochId > 0 && s.config.ValidatorChangeEpochId <= epochId {
Copy link

Choose a reason for hiding this comment

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

This condition is used twice in the call

  • here
  • in NextValidatorsAndPowersAt(...)

In fact, I think it is better to use the condition once, i.e., adding
} else { header := g.chain.GetHeaderByNumber(0) validators = header.NextValidators; powers = header.NextValidatorPowers; }

and simplify the logic of NextValidatorsAndPowersAt (and move the call of NextValidatorsAndPowersAt() of course).

This also removes a wired call that if this condition is not satisfied, NextValidatorsAndPowersAt is called remoteChainNumber 0 and emptyHash.

remoteChainNumber := uint64(0)
hash := common.Hash{}

header.NextValidators, header.NextValidatorPowers, remoteChainNumber, hash, err =
Copy link

Choose a reason for hiding this comment

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

A thought for better readability: since s.config.ValidatorChangeEpochId and s.config.ValidatorChangeEpochId <= epochId are already used in ValidateBlock(), we should have the mirrored logic here, i.e., if the condition is not satisfied, use genesis validators.

@ping-ke ping-ke merged commit f981768 into tm_w3q May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants