Waiting list length fix #3534

iulianpascalau · 2021-10-25T08:29:13Z

added a new functionality that will fix the length on the waiting list length.

…ist length.

sasurobert · 2021-10-25T08:32:09Z

vm/systemSmartContracts/staking.go

@@ -1898,15 +1900,90 @@ func (s *stakingSC) getFirstElementsFromWaitingList(numNodes uint32) (*waitingLi
 		blsKeysToStake = append(blsKeysToStake, element.BLSPublicKey)
 		stakedDataList = append(stakedDataList, stakedData)
 		index++
+		if len(element.NextKey) == 0 {


add in one more place

there is another place Line 1296
where a
for len(nextKey) != 0 && index <= waitingListHead.Length { is used. Should it be also added there?

sasurobert · 2021-10-25T08:32:48Z

vm/systemSmartContracts/staking.go

 		copy(nextKey, element.NextKey)
 	}

+	if len(blsKeysToStake) != int(waitingListHead.Length) {


if numNodes >= waitingListHead.Length &&

sasurobert · 2021-10-25T08:34:45Z

vm/systemSmartContracts/staking.go

+	if waitingListHead.Length == 0 {
+		waitingListHead.Length = 0
+
+		return s.saveWaitingListReturningErrorCode(waitingListHead)


just return here. without save.

sasurobert · 2021-10-25T08:35:23Z

vm/systemSmartContracts/staking.go

+	return s.saveWaitingListReturningErrorCode(waitingListHead)
+}
+
+func (s *stakingSC) saveWaitingListReturningErrorCode(waitingList *WaitingList) vmcommon.ReturnCode {


no need for this function

right, removed

sasurobert · 2021-10-25T08:41:52Z

vm/systemSmartContracts/staking.go

+		return vmcommon.UserError
+	}
+
+	if waitingListHead.Length == 0 {


could we have length = 0 but still have something on the FirstKey? Could there be a case where the Length is smaller than the actual length.

- added unit test

vm/systemSmartContracts/staking.go

raduchis · 2021-10-25T09:21:32Z

vm/systemSmartContracts/staking.go

+		return vmcommon.UserError
+	}
+
+	if waitingListHead.Length == 0 {


could we have length = 0 but still have something on the FirstKey? Could there be a case where the Length is smaller than the actual length.

raduchis · 2021-10-25T09:30:46Z

vm/systemSmartContracts/staking.go

@@ -1898,15 +1900,90 @@ func (s *stakingSC) getFirstElementsFromWaitingList(numNodes uint32) (*waitingLi
 		blsKeysToStake = append(blsKeysToStake, element.BLSPublicKey)
 		stakedDataList = append(stakedDataList, stakedData)
 		index++
+		if len(element.NextKey) == 0 {


there is another place Line 1296
where a
for len(nextKey) != 0 && index <= waitingListHead.Length { is used. Should it be also added there?

- added more unit tests

vm/systemSmartContracts/staking.go

codecov · 2021-10-25T10:54:35Z

Codecov Report

Merging #3534 (bc9efa4) into master (356fc93) will increase coverage by 0.00%.
The diff coverage is 76.99%.

@@           Coverage Diff            @@
##           master    #3534    +/-   ##
========================================
  Coverage   73.88%   73.88%            
========================================
  Files         581      581            
  Lines       74603    74706   +103     
========================================
+ Hits        55119    55197    +78     
- Misses      15079    15095    +16     
- Partials     4405     4414     +9

Impacted Files	Coverage Δ
vm/systemSmartContracts/staking.go	`63.12% <76.99%> (+0.97%)`	⬆️
p2p/libp2p/netMessenger.go	`74.72% <0.00%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 356fc93...bc9efa4. Read the comment docs.

vm/systemSmartContracts/staking.go

SebastianMarian · 2021-10-26T09:19:02Z

vm/systemSmartContracts/staking.go

+		return vmcommon.OutOfGas
+	}
+	if len(args.Arguments) != 1 {
+		s.eei.AddReturnMessage("not enough arguments")


s.eei.AddReturnMessage(fmt.Sprintf("invalid number of arguments: expected %d, got %d", 1, len(args.Arguments)))

would leave as it is.

Ok, but != 1 could be 2,3 ... 100. Not enough arguments doesn't match in these cases

iulianpascalau · 2021-10-26T11:44:55Z

vm/systemSmartContracts/staking.go

 	index := uint32(1)
 	nextKey := make([]byte, len(waitingListHead.FirstKey))
 	copy(nextKey, waitingListHead.FirstKey)
 	for len(nextKey) != 0 && index <= waitingListHead.Length {
 		element, errGet := s.getWaitingListElement(nextKey)
 		if errGet != nil {
-			s.eei.AddReturnMessage(err.Error())
+			s.eei.AddReturnMessage(errGet.Error())


iulianpascalau · 2021-10-26T11:45:07Z

vm/systemSmartContracts/staking.go

 		_, errGet = s.getOrCreateRegisteredData(element.BLSPublicKey)
 		if errGet != nil {
-			s.eei.AddReturnMessage(err.Error())
+			s.eei.AddReturnMessage(errGet.Error())


vm/systemSmartContracts/staking.go

AdoAdoAdo · 2021-10-26T11:38:22Z

vm/systemSmartContracts/staking.go

+		return vmcommon.Ok
+	}
+
+	foundLastJailedKey := len(waitingListHead.LastJailedKey) == 0


this gives true if there is no jailed element, but flag is called foundLastJailed
shouldn't you change to
foundLastJailedKey := len(waitingListHead.LastJailedKey) != 0

LucianMincu

System tests passed.

- added a new functionality that will fix the length on the waiting l…

31619d9

…ist length.

iulianpascalau marked this pull request as draft October 25, 2021 08:29

sasurobert reviewed Oct 25, 2021

View reviewed changes

sasurobert previously approved these changes Oct 25, 2021

View reviewed changes

- fixes

7fc0cf3

- added unit test

iulianpascalau dismissed sasurobert’s stale review via 7fc0cf3 October 25, 2021 09:18

- more fixes

8de3414

sasurobert previously approved these changes Oct 25, 2021

View reviewed changes

raduchis reviewed Oct 25, 2021

View reviewed changes

- more fixes

011e5eb

- added more unit tests

iulianpascalau dismissed sasurobert’s stale review via 011e5eb October 25, 2021 10:06

raduchis previously approved these changes Oct 25, 2021

View reviewed changes

vm/systemSmartContracts/staking.go Show resolved Hide resolved

sasurobert previously approved these changes Oct 25, 2021

View reviewed changes

- fixed unit tests

a47d44c

iulianpascalau dismissed stale reviews from sasurobert and raduchis via a47d44c October 25, 2021 10:45

iulianpascalau marked this pull request as ready for review October 25, 2021 10:45

raduchis previously approved these changes Oct 25, 2021

View reviewed changes

fixed lastJailedKey

57ef41d

raduchis dismissed their stale review via 57ef41d October 25, 2021 18:25

sasurobert added 3 commits October 26, 2021 11:15

Merge branch 'master' into fix-waiting-list-queue-size

7d818e3

added new function to correct values

cbfd5d3

Merge branch 'master' into fix-waiting-list-queue-size

7850f74

raduchis reviewed Oct 26, 2021

View reviewed changes

vm/systemSmartContracts/staking.go Show resolved Hide resolved

SebastianMarian reviewed Oct 26, 2021

View reviewed changes

added new function to correct values

fb688eb

SebastianMarian previously approved these changes Oct 26, 2021

View reviewed changes

fix and unit tests

14b56dc

sasurobert dismissed SebastianMarian’s stale review via 14b56dc October 26, 2021 10:19

Merge branch 'master' into fix-waiting-list-queue-size

bc9efa4

SebastianMarian approved these changes Oct 26, 2021

View reviewed changes

iulianpascalau commented Oct 26, 2021

View reviewed changes

AdoAdoAdo approved these changes Oct 26, 2021

View reviewed changes

LucianMincu approved these changes Oct 26, 2021

View reviewed changes

LucianMincu merged commit 2ef5218 into master Oct 26, 2021

LucianMincu deleted the fix-waiting-list-queue-size branch October 26, 2021 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Waiting list length fix #3534

Waiting list length fix #3534

iulianpascalau commented Oct 25, 2021 •

edited

sasurobert Oct 25, 2021

iulianpascalau Oct 25, 2021

raduchis Oct 25, 2021

sasurobert Oct 25, 2021

iulianpascalau Oct 25, 2021

sasurobert Oct 25, 2021

iulianpascalau Oct 25, 2021

sasurobert Oct 25, 2021

iulianpascalau Oct 25, 2021

sasurobert Oct 25, 2021

iulianpascalau Oct 25, 2021

raduchis Oct 25, 2021

raduchis Oct 25, 2021

raduchis Oct 25, 2021

codecov bot commented Oct 25, 2021 •

edited

SebastianMarian Oct 26, 2021

sasurobert Oct 26, 2021

SebastianMarian Oct 26, 2021

iulianpascalau Oct 26, 2021

iulianpascalau Oct 26, 2021

AdoAdoAdo Oct 26, 2021

LucianMincu left a comment

Waiting list length fix #3534

Waiting list length fix #3534

Conversation

iulianpascalau commented Oct 25, 2021 • 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

codecov bot commented Oct 25, 2021 • edited

Codecov Report

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

iulianpascalau commented Oct 25, 2021 •

edited

codecov bot commented Oct 25, 2021 •

edited