jailed-node-backward-compatible #2303

sasurobert · 2020-09-18T14:29:53Z

making code backward compatible even if there was a jailed node

epochStart/metachain/systemSCs.go

iulianpascalau · 2020-09-18T14:43:44Z

process/peer/process.go

@@ -200,7 +211,7 @@ func (vs *validatorStatistics) saveUpdatesForList(
 			return err
 		}

-		if peerType == core.InactiveList && peerAcc.GetUnStakedEpoch() == core.DefaultUnstakedEpoch {
+		if vs.flagSwitchEnabled.IsSet() && peerType == core.InactiveList && peerAcc.GetUnStakedEpoch() == core.DefaultUnstakedEpoch {


Maybe extract in a bool variable?

iulianpascalau · 2020-09-18T14:44:32Z

process/peer/process.go

@@ -421,16 +432,38 @@ func (vs *validatorStatistics) getValidatorDataFromLeaves(
 	return validators, nil
 }

+func getActualList(peerAccount state.PeerAccountHandler) string {


guess we can't continue without this function :D

best function ever.

iulianpascalau · 2020-09-18T14:45:24Z

process/peer/process.go

@@ -609,6 +642,10 @@ func (vs *validatorStatistics) ResetValidatorStatisticsAtNewEpoch(vInfos map[uin
 			}
 			peerAccount.ResetAtNewEpoch()

+			if vs.flagSwitchEnabled.IsSet() && validator.List == string(core.JailedList) && peerAccount.GetList() != string(core.JailedList) {


iulianpascalau · 2020-09-18T14:50:21Z

process/peer/process.go

+
+// EpochConfirmed is called whenever a new epoch is confirmed
+func (vs *validatorStatistics) EpochConfirmed(epoch uint32) {
+	vs.flagSwitchEnabled.Toggle(epoch >= vs.switchEnableEpoch)


Wondering if there is an chicken-egg problem when using this mechanism in this particular implementation:
Does peer processor is called before or after the epoch change? Because if it is after the epoch change, the new start of epoch block will contain the modified peer changes. Otherwise, it will keep the old processing way. Since the shards do not care about the peer statuses I guess it's ok the processing there.

before any processing is called. thus is ok.

…unJail-full-backward-compatible

iulianpascalau

Will test this by doing an unjail tx on the testnet and then repaying everything using the import-db feature.

SebastianMarian · 2020-09-23T16:15:32Z

process/peer/process.go

@@ -609,6 +646,11 @@ func (vs *validatorStatistics) ResetValidatorStatisticsAtNewEpoch(vInfos map[uin
 			}
 			peerAccount.ResetAtNewEpoch()

+			shouldBeJailed := vs.flagJailedEnabled.IsSet() && validator.List == string(core.JailedList) && peerAccount.GetList() != string(core.JailedList)
+			if shouldBeJailed {
+				peerAccount.SetListAndIndex(validator.ShardId, validator.List, validator.Index)


Maybe you can replace validator.List with string(core.JailedList) to be more clear what is about to do here

SebastianMarian · 2020-09-23T16:19:45Z

epochStart/metachain/systemSCs.go

@@ -40,6 +44,8 @@ type systemSCProcessor struct {
 	validatorInfoCreator    epochStart.ValidatorInfoCreator
 	endOfEpochCallerAddress []byte
 	stakingSCAddress        []byte
+	switchUnJailEnableEpoch uint32


switchJailWaitingEnableEpoch or switchEnableEpoch ?

SebastianMarian · 2020-09-23T16:22:59Z

epochStart/metachain/systemSCs.go

+// EpochConfirmed is called whenever a new epoch is confirmed
+func (s *systemSCProcessor) EpochConfirmed(epoch uint32) {
+	s.flagSwitchEnabled.Toggle(epoch >= s.switchUnJailEnableEpoch)
+	log.Debug("systemSCProcessor: switch unJail", "enabled", s.flagSwitchEnabled.IsSet())


: switch jail waiting ?

SebastianMarian · 2020-09-23T16:25:01Z

process/peer/process.go

@@ -68,6 +71,8 @@ type validatorStatistics struct {
 	genesisNonce           uint64
 	ratingEnableEpoch      uint32
 	lastFinalizedRootHash  []byte
+	switchEnableEpoch      uint32
+	flagJailedEnabled      atomic.Flag


flagSwitchEnabled ?

renamed to jailedEnableEpoch

SebastianMarian · 2020-09-23T16:25:37Z

process/peer/process.go

+// EpochConfirmed is called whenever a new epoch is confirmed
+func (vs *validatorStatistics) EpochConfirmed(epoch uint32) {
+	vs.flagJailedEnabled.Toggle(epoch >= vs.switchEnableEpoch)
+	log.Debug("validatorStatistics: switch", "enabled", vs.flagJailedEnabled.IsSet())


: switch jail waiting ?

renamed to jailed

SebastianMarian · 2020-09-23T16:48:53Z

process/scToProtocol/stakingToPeer.go

+	}
+
+	if stakingData.JailedNonce == nonce && account.GetList() != string(core.InactiveList) {
+		account.SetListAndIndex(account.GetShardId(), string(core.LeavingList), uint32(stakingData.JailedNonce))


LeavingList ?

old code - not relevant - deleted

SebastianMarian · 2020-09-23T16:58:10Z

vm/systemSmartContracts/auction.go

+	totalUnJailPrice := big.NewInt(0).Mul(auctionConfig.UnJailPrice, big.NewInt(int64(len(args.Arguments))))
+
+	if totalUnJailPrice.Cmp(args.CallValue) != 0 {
+		s.eei.AddReturnMessage("insufficient funds sent for unJail")


insufficient or incorrect ?

this is copied from mainnet version and should be left as it is.

# Conflicts: # vm/errors.go # vm/systemSmartContracts/auction.go

vm/systemSmartContracts/auction.go

iulianpascalau · 2020-09-24T09:22:32Z

vm/systemSmartContracts/auction.go

+	}
+	if args.GenesisTotalSupply == nil || args.GenesisTotalSupply.Cmp(zero) <= 0 {
+		return nil, fmt.Errorf("%w, value is %v", vm.ErrInvalidGenesisTotalSupply, args.GenesisTotalSupply)
+	}


L210 is incompatible with the mainnet running version:
mainnet:

s.eei.AddReturnMessage("could not get all blsKeys from registration data: error " + err.Error()) err = errors.New("public key missmatch")

here:

s.eei.AddReturnMessage("could not get all blsKeys from registration data: " + vm.ErrBLSPublicKeyMissmatch.Error()) var ErrBLSPublicKeyMissmatch = errors.New("public key missmatch")

vm/systemSmartContracts/auction.go

iulianpascalau · 2020-09-24T09:33:11Z

vm/errors.go

@@ -50,6 +50,9 @@ var ErrInvalidMinStakeValue = errors.New("invalid min stake value")
 // ErrInvalidNodePrice signals that an invalid node price was provided
 var ErrInvalidNodePrice = errors.New("invalid node price")

+// ErrInvalidMinStepValue signals that an invalid min step value was provided
+var ErrInvalidMinStepValue = errors.New("invalid min step value")


Should add comments on exported items in vm/consts.go

that is from a pull request from Radu already merged in development./

# Conflicts: # cmd/node/factory/structs.go # integrationTests/testProcessorNode.go # process/scToProtocol/stakingToPeer.go # process/scToProtocol/stakingToPeer_test.go # vm/systemSmartContracts/staking.go

iulianpascalau · 2020-09-24T10:52:07Z

vm/systemSmartContracts/staking.go

@@ -1336,6 +1336,52 @@ func (r *stakingSC) getRemainingUnbondPeriod(args *vmcommon.ContractCallInput) v
 	return vmcommon.Ok
 }

+func (r *stakingSC) getWaitingListRegisterNonceAndRewardAddress(args *vmcommon.ContractCallInput) vmcommon.ReturnCode {


This should be unit tested in a future PR.

SebastianMarian · 2020-09-24T12:35:36Z

vm/consts.go

-	UnStakeNotEnabled                       = "unStake is not enabled"
-	TransactionValueMustBeZero              = "transaction value must be zero"
-	CannotGetOrCreateRegistrationData       = "cannot get or create registration data: error - "
+	// InsufficientGasLimit defined constant for return message


"defines a constant" instead "defined constant" in all comments

LucianMincu

👍

making code backward compatible even if there was a jailed node

6991947

iulianpascalau self-requested a review September 18, 2020 14:36

iulianpascalau reviewed Sep 18, 2020

View reviewed changes

sasurobert added 2 commits September 18, 2020 19:36

fix after review

955600b

changed enable nonce to enable epoch

3df978f

iulianpascalau previously approved these changes Sep 21, 2020

View reviewed changes

save as leaving always.

7a007ed

sasurobert dismissed iulianpascalau’s stale review via 7a007ed September 22, 2020 09:50

iulianpascalau previously approved these changes Sep 22, 2020

View reviewed changes

sasurobert added 2 commits September 23, 2020 17:53

unjail fix - no need for ugly hack.

bb7d3f2

full backward compatibility

07bad04

sasurobert dismissed iulianpascalau’s stale review via 07bad04 September 23, 2020 14:54

sasurobert added 3 commits September 23, 2020 18:40

small fix

a9adf94

Merge remote-tracking branch 'origin/enable-epoch-for-systemSC' into …

a7cc901

…unJail-full-backward-compatible

Merge branch 'development' into unJail-full-backward-compatible

a72a10f

iulianpascalau previously approved these changes Sep 23, 2020

View reviewed changes

SebastianMarian self-requested a review September 23, 2020 16:44

SebastianMarian reviewed Sep 23, 2020

View reviewed changes

sasurobert added 2 commits September 23, 2020 20:15

Merge branch 'development' into unJail-full-backward-compatible

34bdcc0

fixed after review

0c0f2f9

sasurobert dismissed iulianpascalau’s stale review via 0c0f2f9 September 23, 2020 17:27

iulianpascalau previously approved these changes Sep 23, 2020

View reviewed changes

SebastianMarian previously approved these changes Sep 23, 2020

View reviewed changes

LucianMincu and others added 3 commits September 23, 2020 21:50

Merge branch 'development' into unJail-full-backward-compatible

7392aeb

Merge branch 'development' into unJail-full-backward-compatible

cf75100

# Conflicts: # vm/errors.go # vm/systemSmartContracts/auction.go

fixed after merge

0c0c005

sasurobert dismissed stale reviews from SebastianMarian and iulianpascalau via 0c0c005 September 24, 2020 07:48

SebastianMarian previously approved these changes Sep 24, 2020

View reviewed changes

Merge branch 'development' into unJail-full-backward-compatible

db33dcb

iulianpascalau requested changes Sep 24, 2020

View reviewed changes

fixed after merge

1104212

sasurobert dismissed SebastianMarian’s stale review via 1104212 September 24, 2020 09:40

Merge branch 'development' into unJail-full-backward-compatible

98b8c30

# Conflicts: # cmd/node/factory/structs.go # integrationTests/testProcessorNode.go # process/scToProtocol/stakingToPeer.go # process/scToProtocol/stakingToPeer_test.go # vm/systemSmartContracts/staking.go

iulianpascalau approved these changes Sep 24, 2020

View reviewed changes

SebastianMarian approved these changes Sep 24, 2020

View reviewed changes

LucianMincu approved these changes Sep 24, 2020

View reviewed changes

LucianMincu merged commit 98b3820 into development Sep 24, 2020

LucianMincu deleted the unJail-full-backward-compatible branch September 24, 2020 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jailed-node-backward-compatible #2303

jailed-node-backward-compatible #2303

sasurobert commented Sep 18, 2020

iulianpascalau Sep 18, 2020

sasurobert Sep 18, 2020

iulianpascalau Sep 18, 2020

sasurobert Sep 18, 2020

iulianpascalau Sep 18, 2020

sasurobert Sep 18, 2020

iulianpascalau Sep 18, 2020

sasurobert Sep 18, 2020

iulianpascalau left a comment

SebastianMarian Sep 23, 2020

SebastianMarian Sep 23, 2020

sasurobert Sep 23, 2020

SebastianMarian Sep 23, 2020

sasurobert Sep 23, 2020

SebastianMarian Sep 23, 2020

sasurobert Sep 23, 2020

SebastianMarian Sep 23, 2020

sasurobert Sep 23, 2020

SebastianMarian Sep 23, 2020

sasurobert Sep 23, 2020

SebastianMarian Sep 23, 2020

sasurobert Sep 23, 2020

SebastianMarian Sep 23, 2020

iulianpascalau Sep 24, 2020

sasurobert Sep 24, 2020

iulianpascalau Sep 24, 2020

sasurobert Sep 24, 2020

iulianpascalau Sep 24, 2020

SebastianMarian Sep 24, 2020

LucianMincu left a comment

jailed-node-backward-compatible #2303

jailed-node-backward-compatible #2303

Conversation

sasurobert commented Sep 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iulianpascalau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianMincu left a comment

Choose a reason for hiding this comment