En 7194/unJail-with-option #2230

sasurobert · 2020-08-19T14:28:00Z

unJail with option.
If the node was jailed for the first time, it will get in to the top of the waiting list.

… list.

andrei-marinica · 2020-08-19T14:35:22Z

vm/systemSmartContracts/auction.go

-	if len(args.Arguments) == 0 {
-		s.eei.AddReturnMessage("invalid number of arguments: expected min 1, got 0")
+	if len(args.Arguments) == 0 || len(args.Arguments)%2 != 0 {
+		s.eei.AddReturnMessage("invalid number of arguments: expected paired number")


even number

andrei-marinica · 2020-08-19T14:37:51Z

vm/systemSmartContracts/staking.go

@@ -342,38 +300,42 @@ func (r *stakingSC) unJail(args *vmcommon.ContractCallInput) vmcommon.ReturnCode
 		r.eei.AddReturnMessage("unJail function not allowed to be called by address " + string(args.CallerAddr))
 		return vmcommon.UserError
 	}
+	if len(args.Arguments) != 2 || len(args.Arguments[1]) != 1 {
+		r.eei.AddReturnMessage("wrong arguments number")


wrong number of arguments

… into EN-7194/unJail-with-option

SebastianMarian · 2020-08-20T19:50:22Z

vm/systemSmartContracts/auction.go

-	for _, blsKey := range blsKeys {
-		vmOutput, err := s.executeOnStakingSC([]byte("unJail@" + hex.EncodeToString(blsKey)))
-		if err != nil {
-			s.eei.AddReturnMessage(err.Error())


Not need to add this message ?

SebastianMarian · 2020-08-20T19:56:24Z

vm/systemSmartContracts/staking.go

@@ -342,38 +300,42 @@ func (r *stakingSC) unJail(args *vmcommon.ContractCallInput) vmcommon.ReturnCode
 		r.eei.AddReturnMessage("unJail function not allowed to be called by address " + string(args.CallerAddr))
 		return vmcommon.UserError
 	}
+	if len(args.Arguments) != 2 || len(args.Arguments[1]) != 1 {
+		r.eei.AddReturnMessage("wrong number of arguments, wanted 2")


the message could be not correct if there are 2 arguments but the len of the second one is not 1

yes. last argument length must be 1.

This is what I said, that the message could be misleading, if the unJail is called with two arguments but the last one has the len different than 1

SebastianMarian · 2020-08-20T20:04:01Z

vm/systemSmartContracts/staking.go

@@ -441,6 +407,7 @@ func (r *stakingSC) jail(args *vmcommon.ContractCallInput) vmcommon.ReturnCode {
 		stakedData.JailedRound = r.eei.BlockChainHook().CurrentRound()
 		stakedData.JailedNonce = r.eei.BlockChainHook().CurrentNonce()
 		stakedData.Jailed = true
+		stakedData.NumJailed = stakedData.NumJailed + 1


stakedData.NumJailed++

SebastianMarian · 2020-08-20T20:12:01Z

vm/systemSmartContracts/staking.go

 	registrationData.JailedRound = r.eei.BlockChainHook().CurrentRound()
 	registrationData.JailedNonce = r.eei.BlockChainHook().CurrentNonce()
 	registrationData.Jailed = true
+	registrationData.NumJailed = registrationData.NumJailed + 1


registrationData.NumJailed++

SebastianMarian · 2020-08-20T20:17:16Z

vm/systemSmartContracts/staking.go

@@ -879,6 +809,67 @@ func (r *stakingSC) addToWaitingList(blsKey []byte) error {
 	return r.saveElementAndList(inWaitingListKey, elementInWaiting, waitingList)
 }

+func (r *stakingSC) insertAfter(


Need some unit tests for this method

vm/systemSmartContracts/auction.go

raduchis · 2020-08-21T09:40:21Z

vm/systemSmartContracts/auction.go

-		if err != nil {
-			s.eei.AddReturnMessage(err.Error())
+	for i, blsKey := range blsKeys {
+		vmOutput, err := s.executeOnStakingSC([]byte("unJail@" + hex.EncodeToString(blsKey) + "@" + hex.EncodeToString(args.Arguments[2*i+1])))


what does the"args.Arguments[2*i+1]" represent?

unJial can be called with unJail@blsKey1@0@blsKey2@1 etc. For each blsKey you need to define whether you want to stake / unstake.

ok... not really easy to understand...

vm/systemSmartContracts/auction.go

vm/systemSmartContracts/staking.go

raduchis · 2020-08-21T10:03:29Z

vm/systemSmartContracts/staking.go


-		err = r.saveStakingData(argument, stakedData)
+	if args.Arguments[1][0] == 1 {


What does it check here?

if it is unJail with stake

vm/systemSmartContracts/staking.go

SebastianMarian · 2020-08-21T13:35:40Z

vm/systemSmartContracts/staking.go

+		registrationData.UnStakedEpoch = r.eei.BlockChainHook().CurrentEpoch()
+		registrationData.UnStakedNonce = r.eei.BlockChainHook().CurrentNonce()
+		registrationData.StakedNonce = math.MaxUint64
+		registrationData.Jailed = true


not needed, already set above

SebastianMarian · 2020-08-21T13:40:14Z

vm/systemSmartContracts/auction.go

 			s.eei.Finish(blsKey)
 			s.eei.Finish([]byte{failed})
 			continue
 		}
 	}

+	err = s.eei.Transfer(args.CallerAddr, args.RecipientAddr, transferBack, nil, 0)
+	if err != nil {
+		s.eei.AddReturnMessage("transfer error on unBond function")


unJail instead unBond ?

SebastianMarian · 2020-08-21T13:46:39Z

vm/systemSmartContracts/staking_test.go

@@ -1150,25 +1150,25 @@ func TestStakingSc_ExecuteStakeStakeStakeJailJailUnJailTwice(t *testing.T) {
 	callerAddress := []byte("data")

 	// do stake should work
-	doStake(t, stakingSmartContract, stakingAccessAddress, stakerAddress, []byte("firstKey"))
+	doStake(t, stakingSmartContract, stakingAccessAddress, stakerAddress, []byte("firsstKey"))


Why firsstKey instead firstKey and fifthhKey instead fifthKey ?

to make it easier the key copies. all keys in the contract has the same length.

SebastianMarian · 2020-09-14T12:13:57Z

epochStart/metachain/systemSCs.go

@@ -311,6 +366,9 @@ func (s *systemSCProcessor) getSortedJailedNodes(validatorInfos map[uint32][]*st

 	sort.Slice(jailedValidators, func(i, j int) bool {
 		if jailedValidators[i].TempRating == jailedValidators[i].TempRating {


There is a wrongly use of "i" instead "j" in the right side of this comparation. Probably an old bug. Please check if there exists other places where these sorts with TempRating are used with this pattern -> i, i

SebastianMarian · 2020-09-14T12:17:30Z

epochStart/metachain/systemSCs.go

@@ -40,6 +40,9 @@ type systemSCProcessor struct {
 	validatorInfoCreator    epochStart.ValidatorInfoCreator
 	endOfEpochCallerAddress []byte
 	stakingSCAddress        []byte
+
+	mapNumSwitchedPerShard   map[uint32]uint32


These maps are used without mutex protection. Please check if they should be used under the mutex or not.

Not possible.

It is used in serial way only

…ator.

SebastianMarian · 2020-09-14T16:03:34Z

process/scToProtocol/stakingToPeer.go

@@ -238,6 +238,11 @@ func (stp *stakingToPeer) updatePeerState(
 		if isNewValidator {
 			account.SetListAndIndex(account.GetShardId(), string(core.NewList), uint32(stakingData.UnJailedNonce))
 		}
+
+		if account.GetList() == string(core.JailedList) {


This shouldn't be done on else branch, if is not new validator ? Could be the situation when isNewValidator == true && account.GetList() == string(core.JailedList) ? In this case account.SetListAndIndex will be executed twice here, first time with core.NewList and second time with core.InactiveList.

Agreed. Must add to else branch.

No need to change. It is good as it is. If it is a new validator, it will set to new list. Thus it will not get into this branch.

SebastianMarian · 2020-09-14T18:53:59Z

process/scToProtocol/stakingToPeer.go

@@ -238,6 +238,11 @@ func (stp *stakingToPeer) updatePeerState(
 		if isNewValidator {
 			account.SetListAndIndex(account.GetShardId(), string(core.NewList), uint32(stakingData.UnJailedNonce))
 		}
+
+		if account.GetList() == string(core.JailedList) {


vm/systemSmartContracts/auction_test.go

epochStart/metachain/systemSCs.go

…och - which leads to panic in node.

LucianMincu

System tests passed! 💯 🔥

sasurobert added 3 commits August 18, 2020 18:39

if jailed only once, after stake it will go to the top of the waiting…

3d49303

… list.

slash, stakevalue, unstake

0fb7ad2

implemented unJail with option to stake / unStake

a4c09b3

sasurobert changed the base branch from master to development August 19, 2020 14:28

small improvement

250a2d2

andrei-marinica reviewed Aug 19, 2020

View reviewed changes

SebastianMarian self-requested a review August 19, 2020 20:55

sasurobert added 5 commits August 20, 2020 09:30

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

e72d0c2

string typo fixes

320a743

Merge remote-tracking branch 'origin/EN-7194/auction-at-end-of-epoch'…

e3d7158

… into EN-7194/unJail-with-option

add jailed nodes on top of jailed list.

a03f6e4

small fix

ca9fcce

SebastianMarian reviewed Aug 20, 2020

View reviewed changes

sasurobert added 2 commits August 21, 2020 12:49

fixed after adding a big unit test

ffac9aa

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

415c34c

raduchis requested changes Aug 21, 2020

View reviewed changes

sasurobert added 2 commits August 21, 2020 15:45

fix after review

e170c33

fix after review

d4f71b4

SebastianMarian reviewed Aug 21, 2020

View reviewed changes

small fixes

003f3bc

SebastianMarian previously approved these changes Aug 21, 2020

View reviewed changes

raduchis previously approved these changes Aug 21, 2020

View reviewed changes

LucianMincu and others added 8 commits August 23, 2020 12:09

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

ffa9706

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

ef3e0b6

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

4505330

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

f7871b2

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

9ea9531

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

3d2c902

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

63d3943

config change

af57d69

SebastianMarian previously approved these changes Sep 10, 2020

View reviewed changes

numRegistered must be decreased only on unBond.

949cabf

sasurobert dismissed SebastianMarian’s stale review via 949cabf September 11, 2020 08:52

SebastianMarian previously approved these changes Sep 11, 2020

View reviewed changes

no need to set unStakedEpoch.

bf48cc9

sasurobert dismissed SebastianMarian’s stale review via bf48cc9 September 14, 2020 07:48

SebastianMarian previously approved these changes Sep 14, 2020

View reviewed changes

switch jail with new only the waiting list num of validators

c7b4983

sasurobert dismissed SebastianMarian’s stale review via c7b4983 September 14, 2020 10:19

SebastianMarian reviewed Sep 14, 2020

View reviewed changes

sasurobert added 2 commits September 14, 2020 15:25

fixed sorting bug

e4dadbb

small fixes. setting as jailed directly when saving from nodescoordin…

e2e81cd

…ator.

SebastianMarian reviewed Sep 14, 2020

View reviewed changes

SebastianMarian previously approved these changes Sep 14, 2020

View reviewed changes

raduchis reviewed Sep 14, 2020

View reviewed changes

vm/systemSmartContracts/auction_test.go Show resolved Hide resolved

epochStart/metachain/systemSCs.go Outdated Show resolved Hide resolved

added logs and small fixes

3485db5

sasurobert dismissed SebastianMarian’s stale review via 3485db5 September 15, 2020 08:14

changed tests to work with go 1.15

5423d3d

raduchis previously approved these changes Sep 15, 2020

View reviewed changes

SebastianMarian previously approved these changes Sep 15, 2020

View reviewed changes

fixing scenario when too many nodes are switched at the end of the ep…

feceb84

…och - which leads to panic in node.

sasurobert dismissed stale reviews from SebastianMarian and raduchis via feceb84 September 15, 2020 12:10

SebastianMarian approved these changes Sep 15, 2020

View reviewed changes

sasurobert and others added 2 commits September 15, 2020 17:03

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

f952d99

Merge branch 'development' into EN-7194/auction-at-end-of-epoch

5ecc864

raduchis approved these changes Sep 15, 2020

View reviewed changes

LucianMincu approved these changes Sep 16, 2020

View reviewed changes

LucianMincu merged commit 0f16068 into development Sep 16, 2020

LucianMincu deleted the EN-7194/auction-at-end-of-epoch branch September 16, 2020 07:02


		err = r.saveStakingData(argument, stakedData)
		if args.Arguments[1][0] == 1 {

		@@ -311,6 +366,9 @@ func (s systemSCProcessor) getSortedJailedNodes(validatorInfos map[uint32][]st

		sort.Slice(jailedValidators, func(i, j int) bool {
		if jailedValidators[i].TempRating == jailedValidators[i].TempRating {

En 7194/unJail-with-option #2230

En 7194/unJail-with-option #2230

Conversation

sasurobert commented Aug 19, 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

SebastianMarian Aug 21, 2020 • edited

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

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

SebastianMarian Aug 21, 2020 •

edited