Filter auction nodes list #3822

mariusmihaic · 2022-02-21T13:31:38Z

Staking v4 nodes filtering

All nodes which are in the auction list will be sorted based on their top up per node. If >= 2 bls keys have the same top up, they will be sorted based on XOR with previous block rand seed. The number of available node slots will be computed and only the first nodes in the sorted auction list will fill these slots.
Added a display func to see the whole auction list and all nodes which will be moved to new list (to be further used by nodes coordinator to select them as eligible), e.g. from an auction of 5 bls keys, only 3 will be selected (pubKey2, pubKey9, pubKey4):

		+--------+----------------+----------------+
		| Owner  | Registered key | TopUp per node |
		+--------+----------------+----------------+
		| owner1 | pubKey2        | 1000           |
		| owner4 | pubKey9        | 500            |
		| owner2 | pubKey4        | 0              |
		+--------+----------------+----------------+
		| owner2 | pubKey5        | 0              |
		| owner3 | pubKey7        | 0              |
		+--------+----------------+----------------+

Refactor

In stakingDataProvider.go, refactored GetBlsKeyOwner to be an exported func
In systemSCs_test refactored test funcs which added validator data and staking data, so that we can easily register an owner and its staked bls keys and top up per node
ProcessSystemSmartContract from EpochStartSystemSCProcessor interface to accept an extra param: randomness []byte for sorting bls keys

…node

…N-11662-filter-new-nodes

iulianpascalau

Good job! 👍
Some findings that will improve performance

cmd/node/config/enableEpochs.toml

epochStart/metachain/systemSCs.go

epochStart/metachain/systemSCs_test.go

iulianpascalau · 2022-03-01T14:51:00Z

cmd/node/config/enableEpochs.toml

@@ -191,6 +191,9 @@
    # all nodes from staking queue are moved in the auction list
    StakingV4InitEnableEpoch = 4

+    # StakingV4EnableEpoch represents the epoch when staking v4 is enabled. Should have a greater value than StakingV4InitEnableEpoch
+    StakingV4EnableEpoch = 5


so this should always be StakingV4InitEnableEpoch + 1? What happens if we wrongly configure this?

Theoretically, code should not panic or anything. If EnableEpoch < InitEpoch => auction list will be empty => no sorting/selection will be made.
Logic-wise, if they are inverted it shouldn't cause any bugs for NOW. I'm not sure how the implementation on nodes coordinator side will look like to give an educated opinion. Depending how it will be designed, it might cause some bugs

sasurobert · 2022-03-02T13:28:14Z

epochStart/metachain/systemSCs.go

@@ -214,6 +220,7 @@ func (s *systemSCProcessor) ProcessSystemSmartContract(
 	validatorInfos map[uint32][]*state.ValidatorInfo,
 	nonce uint64,
 	epoch uint32,
+	randomness []byte,
 ) error {
 	if s.flagHystNodesEnabled.IsSet() {
 		err := s.updateSystemSCConfigMinNodes()


we need a sort of refactor here - like adding a separate function with old / inactive flags - all those things which were once activated on the mainnet. That we we make it easier to read this function. like flagHystNodes, flagSetOwner, flagChangeMaxNodes, flagCorrectlastUnJailed, flagDelegationEnabled, flagCorrectNumNodesToStake

Those are called only once - so you could add flagESDTEnabled there. Move to new activation function which are not yet enabled, like flagGovernance, flagBuiltInOnMetachain, flagStakingV4Init

Decided to split them in 2 functions with pretty "trivial" names:

checkOldFlags (which contains: flagHystNodes, flagSetOwner,...)

checkNewFlags(which contains flagGovernance, flagStakingV4Init,...)

epochStart/metachain/systemSCs.go

sasurobert · 2022-03-02T13:36:58Z

process/block/metablock.go

@@ -403,7 +403,7 @@ func (mp *metaProcessor) processEpochStartMetaBlock(
 	}

 	if mp.isRewardsV2Enabled(header) {
-		err = mp.epochSystemSCProcessor.ProcessSystemSmartContract(allValidatorsInfo, header.Nonce, header.Epoch)
+		err = mp.epochSystemSCProcessor.ProcessSystemSmartContract(allValidatorsInfo, header.Nonce, header.Epoch, header.GetPrevRandSeed())


it would be better to send the whole header. instead of 3 arguments.

Probably in this case would be better; since maybe in the future we might need additional data from header;
Will refactor it

sasurobert · 2022-03-02T13:46:02Z

epochStart/metachain/systemSCs.go

+	}
+
+	auctionListSize := uint32(len(auctionList))
+	numOfAvailableNodeSlots := core.MinUint32(auctionListSize, s.maxNodes-numOfValidators)


make a check on numValidators < s.maxNodes -> return -> do not do anything - it means there are not enough nodes.

still add the sanity check.

Added sanity check as an early exit point

sasurobert · 2022-03-02T13:48:36Z

epochStart/metachain/systemSCs.go

+			return compareByXORWithRandomness(pubKey1, pubKey2, randomness)
+		}
+
+		return nodeTopUpPubKey1.Cmp(nodeTopUpPubKey2) == 1


0 instead of == 1

Why 0?

We want to compare which node has the highest top up. Cmp documentation says:

// Cmp compares x and y and returns: // // -1 if x < y // 0 if x == y // +1 if x > y

use bigger than 0. like ">0"

Changed it to >0

sasurobert · 2022-03-02T13:48:50Z

epochStart/metachain/systemSCs.go

+	s.displayAuctionList(auctionList, numOfAvailableNodeSlots)
+
+	for i := uint32(0); i < numOfAvailableNodeSlots; i++ {
+		auctionList[i].List = string(common.NewList)


I think we need to somewhat rewrite the map[uint32][]*state.ValidatorInfo - like use an interface and set new values there. It is ok for now - but it is super easy to make a mistake - modifying a value under a wrong pointer - hard to catch. Add a task for this rework.

Agree, current implementation it is pretty prone to errors; however I'm not sure how this interface might look like -> seems a bit of overhead; will add a TODO

add Jira task as well.

Will add task in Jira as well

sasurobert · 2022-03-02T14:33:16Z

epochStart/metachain/systemSCs.go

+
+	for _, validator := range validators {
+		pubKey := validator.PublicKey
+		topUp, err := s.stakingDataProvider.GetNodeStakedTopUp(pubKey)


discussion is needed here in order to see if we want the following: one entity might register 10 nodes with 3000eGLD, meaning 500 topup per node. but if the topUp limit is at 2500 - that entity might have 5 nodes which enter and 5 which will not. However the first auction system was designed like that - selecting the most possible for an account. So add a task for this - let's discuss and it should be another task if we decide that.

Will leave this topic open and create a new Jira task

sasurobert · 2022-03-03T15:16:19Z

epochStart/metachain/systemSCs.go

+	validatorsInfoMap map[uint32][]*state.ValidatorInfo,
+	header data.HeaderHandler,
+) error {
+	err := s.checkOldFlags(validatorsInfoMap, header.GetNonce(), header.GetEpoch())


rename processingWithOldFlags

epochStart/metachain/systemSCs.go

sasurobert · 2022-03-03T15:22:54Z

epochStart/metachain/systemSCs.go

 		if err != nil {
 			return err
 		}
 	}

 	if s.flagStakingV2Enabled.IsSet() {
-		err := s.prepareRewardsData(validatorInfos)
+		numUnStaked, err := s.prepareStakingAndUnStakeNodesWithNotEnoughFunds(validatorsInfoMap, epoch)


please leave functions as it was.

and make completely new functions

Refactored this code

sasurobert · 2022-03-03T15:27:16Z

epochStart/metachain/systemSCs.go

-		return err
+func (s *systemSCProcessor) prepareStakingData(validatorsInfoMap map[uint32][]*state.ValidatorInfo) error {
+	nodes := make(map[uint32][][]byte)
+	if s.flagStakingV2Enabled.IsSet() {


no 2 flags on the same function. it is better to have duplicated code.

Refactored it so we don't have 2 flags

common/constants.go

sasurobert · 2022-03-09T13:32:15Z

epochStart/metachain/systemSCs.go

 		if err != nil {
 			return err
 		}

-		numUnStaked, err := s.unStakeNodesWithNotEnoughFunds(validatorInfos, epoch)
+		err = s.stakeNodesFromQueue(validatorsInfoMap, numUnStaked, nonce, common.NewList)


flag is missing here

stakeNodesFromQueue (numUnstaked) is missing

Right, forgot about the flag, added it;
stakeNodesFromQueue is called on L301

sasurobert · 2022-03-09T13:32:40Z

epochStart/metachain/systemSCs.go

 		if err != nil {
 			return err
 		}

-		err = s.fillStakingDataForNonEligible(validatorInfos)
+		numUnStaked, err := s.unStakeNonEligibleNodes(validatorsInfoMap, epoch)


add "withNotEnoughFunds"

Renamed it to unStakeNonEligibleNodesWithNotEnoughFunds

iulianpascalau · 2022-03-09T14:35:59Z

epochStart/metachain/systemSCs.go

+	validatorsInfoMap map[uint32][]*state.ValidatorInfo,
+	header data.HeaderHandler,
+) error {
+	err := s.processWithOldFlags(validatorsInfoMap, header.GetNonce(), header.GetEpoch())


what does "new" and "old" mean? They are so relative based on the epoch flags.
This just became a hard to comprehend function

iulianpascalau

according to my last comment

iulianpascalau

Code separation in a different PR

FEAT: Add first ugly version

a0cdfc5

mariusmihaic changed the title ~~FEAT: Add first ugly version~~ Filter auction nodes list Feb 21, 2022

mariusmihaic self-assigned this Feb 21, 2022

mariusmihaic added 14 commits February 22, 2022 16:58

Merge branch 'EN-11661-staking-v4-init' into EN-11662-filter-new-nodes

200f17d

FEAT: Add temporary test

60e4e3a

FEAT: Change ProcessSystemSmartContract func interface to accept rand

bd9d101

FEAT: Sort by pubKey XOR rand if multiple nodes have same top up per …

a838a7f

…node

FIX: Top up per node in tests

ef30472

FEAT: Display auction list, refactor interface + tests

caa682d

FIX: Refactor tests

40ff5a7

FIX: Refactor code pt. 1

02160ad

FIX: Refactor code pt. 2

5ae3d73

FEAT: Add tests for error paths

2a2dc29

FIX: Small refactor

473896e

FEAT: Add flag in toml file

e51f952

Merge branch 'EN-11661-staking-v4-init' into EN-11662-filter-new-nodes

638a23b

Merge remote-tracking branch 'origin/EN-11661-staking-v4-init' into E…

a8fef77

…N-11662-filter-new-nodes

mariusmihaic marked this pull request as ready for review March 1, 2022 06:38

iulianpascalau self-requested a review March 1, 2022 11:08

iulianpascalau reviewed Mar 1, 2022

View reviewed changes

mariusmihaic added 2 commits March 1, 2022 15:42

Merge branch 'EN-11661-staking-v4-init' into EN-11662-filter-new-nodes

27aa5f8

FIX: Review findings

69bc7c5

iulianpascalau previously approved these changes Mar 1, 2022

View reviewed changes

Base automatically changed from EN-11661-staking-v4-init to feat/liquid-staking March 2, 2022 13:45

Merge branch 'feat/liquid-staking' into EN-11662-filter-new-nodes

53befc7

sasurobert requested changes Mar 2, 2022

View reviewed changes

FIX: Review findings

23675b0

mariusmihaic dismissed iulianpascalau’s stale review via 23675b0 March 3, 2022 11:09

sasurobert reviewed Mar 3, 2022

View reviewed changes

FIX: Review findings pt. 2

9639aa5

mariusmihaic added 5 commits March 4, 2022 11:55

Merge branch 'feat/liquid-staking' into EN-11662-filter-new-nodes

196ffdb

FIX: One review finding

42b0528

FIX: Integration test

3df6cfb

FIX: Broken tests

c3abbdb

FEAT: Move selected nodes from AuctionList to SelectedFromAuctionList

47c7712

sasurobert reviewed Mar 9, 2022

View reviewed changes

FIX: Review findings

20535f3

sasurobert approved these changes Mar 9, 2022

View reviewed changes

iulianpascalau reviewed Mar 9, 2022

View reviewed changes

iulianpascalau requested changes Mar 9, 2022

View reviewed changes

iulianpascalau approved these changes Mar 9, 2022

View reviewed changes

iulianpascalau merged commit 8f09c2d into feat/liquid-staking Mar 9, 2022

iulianpascalau deleted the EN-11662-filter-new-nodes branch March 9, 2022 17:28

Filter auction nodes list #3822

Filter auction nodes list #3822

Conversation

mariusmihaic commented Feb 21, 2022 • edited

Staking v4 nodes filtering

Refactor

iulianpascalau left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmihaic Mar 1, 2022 • 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

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

iulianpascalau left a comment

Choose a reason for hiding this comment

mariusmihaic commented Feb 21, 2022 •

edited

iulianpascalau left a comment •

edited

mariusmihaic Mar 1, 2022 •

edited