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

Fix: update transitions for old config #1373

Merged
merged 3 commits into from
Apr 7, 2022

Conversation

achraf17
Copy link
Contributor

@achraf17 achraf17 commented Apr 7, 2022

When updating the genesis it is possible to have a situation where the chain will continue to work but will fail when you re-sync your node.

For example:

Initialise testQBFTBlock=100 you reach block 101
Change testQBFTBlock=99 restart you node and it continues from 102 ...

Something happens and you need to re-sync, at block 99/100 you will have a failure/fork.

@antonydenyer antonydenyer merged commit e57d19b into Consensys:master Apr 7, 2022
@achraf17 achraf17 deleted the fix/transitions branch April 7, 2022 12:33
Comment on lines +719 to +735
c1RecsBelowHead := 0
for _, data := range c1.Transitions {
if data.Block.Cmp(head) <= 0 {
c1RecsBelowHead++
} else {
break
}
}

c2RecsBelowHead := 0
for _, data := range c2.Transitions {
if data.Block.Cmp(head) <= 0 {
c2RecsBelowHead++
} else {
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we should factorize these to a single function to avoid duplicate


// validate that each past record is matching exactly. if not return error
for i := 0; i < c1RecsBelowHead; i++ {
isSameBlock := c1.Transitions[i].Block.Cmp(c2.Transitions[i].Block) != 0
Copy link
Contributor

Choose a reason for hiding this comment

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

if it is not the same block, that means some block(s) has(ve) disappear?
should we raise an error?

@@ -724,6 +788,11 @@ func (c *ChainConfig) CheckCompatible(newcfg *ChainConfig, height uint64, isQuor
if err != nil {
return newCompatError(err.Error(), cBlock, newCfgBlock)
}
// compare the transitions data between the old and new config
err, cBlock, newCfgBlock = isTransitionsConfigCompatible(c, newcfg, bhead)
Copy link
Contributor

Choose a reason for hiding this comment

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

so cBlock and newCfgBlock are used for creating a compat error, why not returning this error directly from the isTransiti...Compatible()?

also isTransi...Compatible(), I thought it would return a bool, could it be named checkCompatibilityOfConfigs()?

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.

3 participants