Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Staking v4: soft auction list selection #4083

Merged
merged 32 commits into from Jun 8, 2022

Conversation

mariusmihaic
Copy link
Contributor

@mariusmihaic mariusmihaic commented May 17, 2022

Implemented soft(dynamic) auction selection for nodes in auction list. "Core" implementation can be found in calcSoftAuctionNodesConfig func, which implements the algorithm proposed here (see Pseudo code of topUp calculation)

What's new

  • Added SoftAuctionConfig config for soft auction selection
  • Extended StakingDataProvider to provide GetNumStakedNodes & GetTotalTopUp of an owner. Note that it is required that these information are pre-filled (which happens for all nodes in ProcessSystemSC) before calling SelectNodesFromAuctionList
  • SelectNodesFromAuctionList now expects to receive all unqualified owners, of whom auction nodes will not participate at selection. These unqualified owners are the ones which do not have enough stake for their nodes, therefore one or more of their nodes will be removed at the end of the epoch.
  • Each node in auction will have its ownerData computed and filled(active nodes, auction nodes, topUp per node, etc...) before applying the dynamic selection. After soft auction computation is done, each owner's auction nodes will be sorted (BY blsKey) and qualified ones will be marked as selected. If two/more nodes, which belong to different owners, have the same qualifiedTopUpPerNode, sorting will be done based on blsKey XOR randomness. See TestSystemSCProcessor_ProcessSystemSmartContractStakingV4Enabled test which contains an example with graphic tables

Bugfixes

  • Fixed bug in epochStart/metachain/systemSCs.go when preparing staking data (L134). It was done for all nodes instead of eligible nodes only, causing rewards to be distributed to non-eligible nodes.

@mariusmihaic mariusmihaic changed the title FEAT: First ugly version Staking v4: soft auction list selection May 17, 2022
@mariusmihaic mariusmihaic self-assigned this May 17, 2022
Base automatically changed from EN-12187-auction-list-selection-subcomp to feat/liquid-staking May 23, 2022 11:48
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022

Codecov Report

Merging #4083 (2623fb7) into feat/liquid-staking (363865a) will increase coverage by 0.61%.
The diff coverage is 89.16%.

@@                   Coverage Diff                   @@
##           feat/liquid-staking    #4083      +/-   ##
=======================================================
+ Coverage                75.38%   75.99%   +0.61%     
=======================================================
  Files                      622      658      +36     
  Lines                    83275    86766    +3491     
=======================================================
+ Hits                     62774    65936    +3162     
- Misses                   15776    15988     +212     
- Partials                  4725     4842     +117     
Impacted Files Coverage Δ
dataRetriever/interface.go 12.12% <ø> (ø)
epochStart/bootstrap/baseStorageHandler.go 55.55% <ø> (ø)
epochStart/metachain/common.go 0.00% <0.00%> (ø)
factory/bootstrapComponentsHandler.go 55.55% <0.00%> (ø)
factory/consensusComponentsHandler.go 60.56% <0.00%> (+4.71%) ⬆️
factory/cryptoComponentsHandler.go 66.21% <0.00%> (ø)
factory/dataComponentsHandler.go 71.08% <0.00%> (ø)
factory/heartbeatComponentsHandler.go 64.17% <0.00%> (ø)
factory/stateComponentsHandler.go 72.04% <0.00%> (ø)
factory/statusComponentsHandler.go 16.45% <0.00%> (ø)
... and 145 more

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 47ee099...2623fb7. Read the comment docs.

@mariusmihaic mariusmihaic marked this pull request as ready for review May 26, 2022 13:10
@sasurobert sasurobert self-requested a review May 27, 2022 06:44
# Conflicts:
#	cmd/node/config/config.toml
#	config/config.go
#	testscommon/generalConfig.go
Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partial review, mostly on calcSoftAuctionNodesConfig function

sw.Start("auctionListSelector.sortAuctionList")
defer func() {
sw.Stop("auctionListSelector.sortAuctionList")
log.Debug("time measurements", sw.GetMeasurements()...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏


ret[string(pubKey)] = topUp
// TODO: Move this in elrond-go-core
func safeSub(a, b uint32) (uint32, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have this in core already I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for uint32, only for uint64 and I didn't want to cast the values

numNodesQualifyingForTopUp := int64(0)
previousConfig = copyOwnersData(ownersData)

for ownerPubKey, owner := range ownersData {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion to extract L335-L357 in a separate function that will output the required map & numNodesQualifyingForTopUp value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted separate function

@raduchis raduchis self-requested a review May 30, 2022 06:51

// GetTotalTopUp returns owner's total top up
func (sdp *stakingDataProvider) GetTotalTopUp(owner []byte) (*big.Int, error) {
ownerInfo, ok := sdp.cache[string(owner)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have some mutexes for other gets. add here as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on all gets from the cache

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now obsolete, since this func is removed in future refactor PR

return selectedFromAuction[:numAvailableSlots]
}

func getPubKeyLen(ownersData map[string]*ownerData) int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this ? you do not need to iterate and return the lenght of data like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not iterate over all values; it stops at the first element; I need it to find the blsKeyLen so we can normalize randomness


ret[string(pubKey)] = topUp
// TODO: Move this in elrond-go-core
func safeSub(a, b uint32) (uint32, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have this in core already I think.

func (als *auctionListSelector) getAuctionDataAndNumOfValidators(
validatorsInfoMap state.ShardValidatorsInfoMapHandler,
unqualifiedOwners map[string]struct{},
) (map[string]*ownerData, uint32, uint32, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make a structure please instead of 3 returns and the error. So one struct and the error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now obsolete, since this func is removed in future refactor PR

numOfValidators := uint32(0)
numOfNodesInAuction := uint32(0)

for _, validator := range validatorsInfoMap.GetAllValidatorsInfo() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is like double iteration - you already have the owner data in the stakingProvider component. Everything is there. You have to iterate per owner. Not per key. There are a lot of useless checks per key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, refactored all code in a separate PR: #4155

continue
}

err = als.addOwnerData(owner, validator, ownersData)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should start from data from the stakingProvider component and add the necessary information. You need to know the keys which are in the auction list for this owner and the total topup and other global info about the owner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, refactored all code in a separate PR: #4155

numNodesQualifyingForTopUp := int64(0)
previousConfig = copyOwnersData(ownersData)

for ownerPubKey, owner := range ownersData {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

cmd/node/config/config.toml Show resolved Hide resolved
epochStart/metachain/systemSCs.go Show resolved Hide resolved
func (als *auctionListSelector) getAuctionDataAndNumOfValidators(
validatorsInfoMap state.ShardValidatorsInfoMapHandler,
unqualifiedOwners map[string]struct{},
) (map[string]*ownerData, uint32, uint32, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

numNodesQualifyingForTopUp := int64(0)
previousConfig = copyOwnersData(ownersData)

for ownerPubKey, owner := range ownersData {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+2

epochStart/metachain/auctionListSorting.go Show resolved Hide resolved
epochStart/metachain/auctionListDisplayer.go Show resolved Hide resolved

// GetTotalTopUp returns owner's total top up
func (sdp *stakingDataProvider) GetTotalTopUp(owner []byte) (*big.Int, error) {
ownerInfo, ok := sdp.cache[string(owner)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on all gets from the cache

@mariusmihaic mariusmihaic merged commit b546bdb into feat/liquid-staking Jun 8, 2022
@mariusmihaic mariusmihaic deleted the EN-12196-implement-soft-auction branch June 8, 2022 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants