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

Fixed backwards compatibility issues on genesis #2612

Merged
merged 13 commits into from Dec 21, 2020

Conversation

iulianpascalau
Copy link
Contributor

@iulianpascalau iulianpascalau commented Dec 18, 2020

  • fixed a bug when saving validator data in validatorSaveLoad.go
  • fixed the situation when the genesis process constructs the system SC container that will lead into creating more accounts than expected on metachain
  • fixed a receipt bug related to gas consumption
  • fixed a backwards compatibility issue related to maximum allowed gas consumption of a transaction
  • made the developer rewards fully backwards compatible by having the old code into a new file
  • turned off the consensus watchdog when running in import-db mode

Testing procedures: start the import-db process, should sync every block produced on the mainnet.

- fixed the situation when the genesis process constructs the system SC container that will lead into creating more accounts than expected
@iulianpascalau iulianpascalau marked this pull request as draft December 18, 2020 22:16
@iulianpascalau iulianpascalau marked this pull request as ready for review December 19, 2020 21:44
func createGenesisConfig() config.GeneralSettingsConfig {
return config.GeneralSettingsConfig{
BuiltInFunctionsEnableEpoch: 0,
SCDeployEnableEpoch: unreachableEpoch,
Copy link
Contributor

Choose a reason for hiding this comment

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

So, scDeployEnableEpoch was modified from 0 to unreachableEpoch because was introduced also the genesisProcessing condition ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. This was done as to not activate the gas computation changes that now depend on the same flag.

log.LogIfError(err)
}

//TODO remove functions initDelegationManager, processSCOutputAccounts, getUserAccount when TODO from L1368 is done
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you should replace "L1368" with "initMetaInnerProcessors" as this line is already L1370 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes - this could be moved inside initMetaInnerProcessors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

node/node.go Outdated
@@ -237,6 +238,11 @@ func (n *Node) StartConsensus() error {
"it is incompatible with the indexing process.")
n.watchdog = &watchdog.DisabledWatchdog{}
}
if !n.isInImportMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

if n.isInImportMode instead if !n.isInImportMode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, changed

@@ -128,7 +128,7 @@ func (v *validatorSC) saveRegistrationData(key []byte, validator *ValidatorDataV
}

func (v *validatorSC) saveRegistrationDataV1(key []byte, validator *ValidatorDataV2) error {
validatorDataV1 := &ValidatorDataV2{
validatorDataV1 := &ValidatorDataV1{
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@sasurobert sasurobert self-requested a review December 21, 2020 07:32
integrationTests/testProcessorNode.go Outdated Show resolved Hide resolved
genesis/process/shardGenesisBlockCreator.go Show resolved Hide resolved
log.LogIfError(err)
}

//TODO remove functions initDelegationManager, processSCOutputAccounts, getUserAccount when TODO from L1368 is done
Copy link
Contributor

Choose a reason for hiding this comment

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

yes - this could be moved inside initMetaInnerProcessors.

process/smartContract/process.go Outdated Show resolved Hide resolved
vm/factory/systemSCFactory.go Show resolved Hide resolved
@@ -127,7 +127,24 @@ func (vmf *vmContainerFactory) Create() (process.VirtualMachinesContainer, error
return container, nil
}

func (vmf *vmContainerFactory) createSystemVM() (vmcommon.VMExecutionHandler, error) {
// CreateForGenesis sets up all the needed virtual machines returning a container of all the VMs to be used in the genesis process
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment that genesis contains only a,b,c and d contracts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
err = sc.updateDeveloperRewardsProxy(tx, vmOutput, builtInFuncGasUsed)
if err != nil {
log.Error("updateDeveloper rewards error", "error", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid printing twice the word "error" you can replace this line with:
log.Error("updateDeveloperRewardsProxy", "error", err.Error())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
err = sc.updateDeveloperRewardsProxy(tx, vmOutput, 0)
if err != nil {
log.Debug("updateDeveloper rewards error", "error", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid printing twice the word "error" you can replace this line with:
log.Error("updateDeveloperRewardsProxy", "error", err.Error())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

) error {
if !sc.flagDeploy.IsSet() {
return sc.updateDeveloperRewardsV1(tx, vmOutput, builtInFuncGasUsed)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove else branch and return directly V2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, done

Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

Import-DB passed ✅

@LucianMincu LucianMincu merged commit 3a2a281 into development Dec 21, 2020
@iulianpascalau iulianpascalau deleted the fix-backwards-compatibility-genesis branch December 25, 2020 14:33
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.

None yet

4 participants