Add PreviousList to peersAccount/validatorInfo/shardValidatorInfo #4961

mariusmihaic · 2023-02-06T11:22:46Z

Reasoning behind the pull request

There is no easy way to find a peersAccount/validatorInfo/shardValidatorInfo previous list, especially for a node that is starting in epoch or restarting without db.
There are currently two bugs in indexHashedNodesCoordinator when computing the new nodes config from current epoch.

In case of a node restart without db/start in epoch, previousConfig was actually using the nodes config from epoch 0.
In case of a leaving/unstaked node, which would be forced to remain, the wrongful previousConfig would be used to make nodes remain eligibile (wrongful nodes)

Proposed changes

Added PreviousList in peersAccount/validatorInfo/shardValidatorInfo and changed their used interfaces + regenerated proto files.
In order to maintain backwards compatibility, when setting a validator list, added an extra flag updatePreviousList, e.g.: SetList(list string, updatePreviousList bool), which will be called with StakingV4StartedFlag - meaning this fix will be enabled only with stakingV4
Removed previousConfig from indexHashedNodesCoordinator and used PreviousList to compute new nodes config

Testing procedure

System test scenario with unstaked eligible/waiting/auction nodes with restarts without db.

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

codecov-commenter · 2023-02-06T12:54:41Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/staking-v4@e6410fc). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head f240a85 differs from pull request most recent head c68c30f. Consider uploading reports for the commit c68c30f to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@                Coverage Diff                 @@
##             feat/staking-v4    #4961   +/-   ##
==================================================
  Coverage                   ?   70.87%           
==================================================
  Files                      ?      662           
  Lines                      ?    86501           
  Branches                   ?        0           
==================================================
  Hits                       ?    61304           
  Misses                     ?    20647           
  Partials                   ?     4550

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

sasurobert · 2023-02-07T09:55:38Z

state/peerAccountData.proto

@@ -52,4 +52,5 @@ message PeerAccountData {
    uint32      TotalValidatorIgnoredSignaturesRate = 16 [(gogoproto.jsontag) = "totalValidatorIgnoredSignaturesRate"];
    uint64      Nonce                               = 17 [(gogoproto.jsontag) = "nonce"];
    uint32      UnStakedEpoch                       = 18 [(gogoproto.jsontag) = "unStakedEpoch"];
+    string      PreviousList                        = 19 [(gogoproto.jsontag) = "previousList,omitempty"];


This is not backward compatible. peerAccount is written into the validator statistics trie.

I did some searching and testing and it seems it is backwards compatible.
According to this source:

maintaining backwards compatibility is crucial. Protocol buffers allow for the seamless support of changes, including the addition of new fields and the deletion of existing fields, to any protocol buffer without breaking existing services

New code will also transparently read old messages. New fields will not be present in old messages; in these cases protocol buffers provide a reasonable default value.

Also, I did a unit test which confirms this:

func TestDsa(t *testing.T) { str1 := &ShardValidatorInfo{ PublicKey: []byte("pk"), ShardId: 4, List: "list", Index: 5, TempRating: 6, PreviousList: "", } str2 := &ShardValidatorInfo2{ PublicKey: []byte("pk"), ShardId: 4, List: "list", Index: 5, TempRating: 6, } marshaller := &marshal.GogoProtoMarshalizer{} bytesStr1, err := marshaller.Marshal(str1) require.Nil(t, err) bytesStr2, err := marshaller.Marshal(str2) require.Nil(t, err) require.Equal(t, bytesStr1, bytesStr2) }

Both versions return the same marshalled byte array:

ShardValidatorInfo has PreviousList field

ShardValidatorInfo2 is the old version, without added field

sasurobert · 2023-02-07T10:03:52Z

state/peerAccount.go

@@ -108,7 +108,11 @@ func (pa *peerAccount) SetTempRating(rating uint32) {
 }

 // SetListAndIndex will update the peer's list (eligible, waiting) and the index inside it with journal
-func (pa *peerAccount) SetListAndIndex(shardID uint32, list string, index uint32) {
+func (pa *peerAccount) SetListAndIndex(shardID uint32, list string, index uint32, updatePreviousList bool) {
+	if updatePreviousList {


I think it is better to have a peerAccountV2 - where this update is true - and leave the old code as it is.

And you do not need to push the boolean in the argument list.

I would leave the existing structure with extension field instead of creating a new struct. This is also related to my comment above.

sasurobert · 2023-02-07T10:29:18Z

sharding/nodesCoordinator/indexHashedNodesCoordinator.go

 	// TODO: compare with previous nodesConfig if exists
-	newNodesConfig, err := ihnc.computeNodesConfigFromList(copiedPrevious, allValidatorInfo)
+	newNodesConfig, err := ihnc.computeNodesConfigFromList(allValidatorInfo)


how can this be backward compatible ? it you have no information ?

It is backwards compatible, since the previousConfig was never used on mainnet. It was supposed to be enabled with FixWaitingListEnableEpoch, which I've deleted in a previous PR.

Old version compatibility is maintained in addValidatorToPreviousMap :

if !ihnc.flagStakingV4Started.IsSet() { eligibleMap[shardId] = append(eligibleMap[shardId], currentValidator) return }

Here, eligibleMap is not the one from previousEpoch, is actually the received list from peerMiniBlocks

SebastianMarian · 2023-02-09T09:36:16Z

state/peerAccount.go

 		pa.PreviousList = pa.List
+		pa.PreviousIndexInList = pa.IndexInList


We don't need also a variable to hold PreviousShardID ?

Apparently, every time SetListAndIndex (jailed/leaving) is called, it uses the same shardID, so that doesn't change.
Also, for our use-case (leaving), I don't think there will be any future use-case where a leaving unstaked validator would be set with a different shard ID.

AdoAdoAdo · 2023-02-10T11:28:05Z

epochStart/metachain/legacySystemSCs.go

@@ -290,7 +290,7 @@ func (s *legacySystemSCProcessor) unStakeNodesWithNotEnoughFunds(
 		}

 		validatorLeaving := validatorInfo.ShallowClone()
-		validatorLeaving.SetList(string(common.LeavingList))
+		validatorLeaving.SetList(string(common.LeavingList), s.enableEpochsHandler.IsStakingV4Started())


you don't need to set the index as well?

Yes, very good catch.
Updated index as well

mariusmihaic added 2 commits February 6, 2023 13:21

FEAT: First version without activation flag

b390a95

FEAT: Version with enable epoch

c68c30f

FIX: computeNodesConfig in nodes coordinator

8b98616

mariusmihaic self-assigned this Feb 6, 2023

FIX: Delete previous config

6aa5d08

mariusmihaic marked this pull request as ready for review February 7, 2023 09:00

sasurobert self-requested a review February 7, 2023 09:10

sasurobert reviewed Feb 7, 2023

View reviewed changes

AdoAdoAdo self-requested a review February 7, 2023 11:33

SebastianMarian previously approved these changes Feb 7, 2023

View reviewed changes

FIX: Add PreviousIndex for validators

5eaf2f2

mariusmihaic dismissed SebastianMarian’s stale review via 5eaf2f2 February 9, 2023 08:40

SebastianMarian reviewed Feb 9, 2023

View reviewed changes

sasurobert previously approved these changes Feb 9, 2023

View reviewed changes

SebastianMarian previously approved these changes Feb 9, 2023

View reviewed changes

AdoAdoAdo previously approved these changes Feb 10, 2023

View reviewed changes

AdoAdoAdo reviewed Feb 10, 2023

View reviewed changes

FIX: Set PreviousIndex when setting validator to leaving

b0eb486

mariusmihaic dismissed stale reviews from AdoAdoAdo, SebastianMarian, and sasurobert via b0eb486 February 10, 2023 13:33

FIX: Linter

c08a8c1

SebastianMarian approved these changes Feb 10, 2023

View reviewed changes

AdoAdoAdo approved these changes Feb 10, 2023

View reviewed changes

sasurobert approved these changes Feb 10, 2023

View reviewed changes

mariusmihaic merged commit d08f285 into feat/staking-v4 Feb 10, 2023

mariusmihaic deleted the MX-13779-peers-previous-list branch February 10, 2023 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PreviousList to peersAccount/validatorInfo/shardValidatorInfo #4961

Add PreviousList to peersAccount/validatorInfo/shardValidatorInfo #4961

mariusmihaic commented Feb 6, 2023 •

edited

codecov-commenter commented Feb 6, 2023

sasurobert Feb 7, 2023

mariusmihaic Feb 7, 2023

sasurobert Feb 7, 2023

sasurobert Feb 7, 2023

mariusmihaic Feb 7, 2023

sasurobert Feb 7, 2023

mariusmihaic Feb 7, 2023

SebastianMarian Feb 9, 2023

mariusmihaic Feb 9, 2023 •

edited

SebastianMarian Feb 9, 2023

AdoAdoAdo Feb 10, 2023

mariusmihaic Feb 10, 2023

		pa.PreviousList = pa.List
		pa.PreviousIndexInList = pa.IndexInList

Add PreviousList to peersAccount/validatorInfo/shardValidatorInfo #4961

Add PreviousList to peersAccount/validatorInfo/shardValidatorInfo #4961

Conversation

mariusmihaic commented Feb 6, 2023 • edited

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov-commenter commented Feb 6, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmihaic Feb 9, 2023 • 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

mariusmihaic commented Feb 6, 2023 •

edited

mariusmihaic Feb 9, 2023 •

edited