Nodes coordinator with staking v4 #3883

mariusmihaic · 2022-03-07T10:39:47Z

Refactor

Created NodesCoordinatorRegistryHandler & EpochValidatorsHandler interfaces to be used instead of pointers to structs
Created EpochValidatorsWithAuction & NodesCoordinatorRegistryWithAuction proto structs which implement the interfaces above and will be used starting staking v4. To be noted that old code used simple structs and json marshaller

New code

Added staking v4 flag to save and load nodes registry using new proto structs
In NodeShuffler, starting with staking v4:

All nodes which which have been selected from staking v4 (based on their top up), will be distributed in the waiting list
All shuffled out nodes will not be moved into waiting list anymore, but saved in the nodes coordinator registry. Further PR will follow with how to handle them

…rdinator-staking-v4

…rdinator-staking-v4 # Conflicts: # epochStart/bootstrap/baseStorageHandler.go # epochStart/bootstrap/fromLocalStorage.go # epochStart/bootstrap/interface.go # epochStart/bootstrap/process.go # epochStart/bootstrap/syncValidatorStatus.go # factory/bootstrapParameters.go # factory/interface.go # factory/shardingFactory.go # sharding/interface.go # sharding/nodesCoordinator/indexHashedNodesCoordinator.go # sharding/nodesCoordinator/indexHashedNodesCoordinatorRegistry.go # testscommon/bootstrapMocks/bootstrapParamsStub.go # testscommon/shardingMocks/nodesCoordinatorStub.go

sasurobert · 2022-03-21T10:02:15Z

sharding/nodesCoordinator/common.go

+// NodesCoordinatorRegistry with a json marshaller; while the new version(from staking v4) uses NodesCoordinatorRegistryWithAuction
+// with proto marshaller
+func CreateNodesCoordinatorRegistry(marshaller marshal.Marshalizer, buff []byte) (NodesCoordinatorRegistryHandler, error) {
+	registry, err := createOldRegistry(buff)


do not unmarshall 2 times - push the epoch here - make a component which is NodesCoordinatorRegistryFactory - which knows depending on the epoch which registry has to be created

Very good suggestion. I've done this similar to how CreateHeader is done, which creates either Header/HeaderV2, but your suggestion is way better.

Therefore, I've done a pretty big refactored and created a NodesCoordinatorRegistryFactory component

sasurobert · 2022-03-21T12:57:50Z

sharding/nodesCoordinator/hashValidatorShuffler.go

-	if err != nil {
-		log.Warn("distributeValidators shuffledOut failed", "error", err)
+	if arg.flagStakingV4 {
+		err = distributeValidators(newWaiting, arg.auction, arg.randomness, false)


it is hard to understand the difference between the 2 versions. Rename this function to distributeValidatorsUsingAuction - or something more clear.

I do not think that creating an extra function here would bring any benefit, since the only thing it would do is to call distributeValidators -> which already does its job.

I think the way to go here is to add an explanatory comment

sasurobert · 2022-03-21T12:59:24Z

sharding/nodesCoordinator/hashValidatorShuffler.go

+		if err != nil {
+			log.Warn("distributeValidators auction list failed", "error", err)
+		}
+	} else {


I do not see why there is log.Warn everywhere - this errors are critical - so they should return error. Add a task to refactor this.
Plus I do not see the point of shuffleNodes - why is it like a function which gets a lot of arguments as input - I think it would be better that this is a component, with defined interface. Add a refactor task for this as well.

RIght, I was also thinking the same when I saw this, but decided to be consistent with the current implementation.
Added 2 tasks for this refactor !

sasurobert · 2022-03-21T13:08:24Z

sharding/nodesCoordinator/indexHashedNodesCoordinator.go

 		chanStopNode:                  arguments.ChanStopNode,
 		nodeTypeProvider:              arguments.NodeTypeProvider,
 		isFullArchive:                 arguments.IsFullArchive,
 	}
 	log.Debug("indexHashedNodesCoordinator: enable epoch for waiting waiting list", "epoch", ihnc.waitingListFixEnableEpoch)
+	log.Debug("indexHashedNodesCoordinator: staking v4", "epoch", ihnc.stakingV4EnableEpoch)


enable epoch for

Right, refactored it

sasurobert · 2022-03-21T13:09:19Z

sharding/nodesCoordinator/indexHashedNodesCoordinator.go

@@ -739,10 +751,15 @@ func (ihnc *indexHashedNodesCoordinator) computeNodesConfigFromList(
 			log.Debug("inactive validator", "pk", validatorInfo.PublicKey)
 		case string(common.JailedList):
 			log.Debug("jailed validator", "pk", validatorInfo.PublicKey)
+		case string(common.SelectedFromAuctionList):
+			if ihnc.flagStakingV4.IsSet() {


add a log.Error - in case of "else"

later after like refactoring - returning error here should be good as well.

Decided to return error right here, actually.

…aking-v4

AdoAdoAdo · 2022-03-23T09:40:47Z

factory/bootstrapComponents.go

@@ -160,27 +161,37 @@ func (bcf *bootstrapComponentsFactory) Create() (*bootstrapComponents, error) {

 	dataSyncerFactory := bootstrap.NewScheduledDataSyncerFactory()

+	nodesCoordinatorRegistryFactory, err := nodesCoordinator.NewNodesCoordinatorRegistryFactory(


can you add the epoch notifier as an argument to the constructor and have the registration of the nodes coordinator registry factory to the epoch notifier done on the constructor as well?

Refactored the code such that NodesCoordinatorRegistryFactory takes EpochNotifier as input param in constructor.

Good suggestion, thanks !

AdoAdoAdo · 2022-03-23T09:41:42Z

factory/bootstrapComponents.go

+	if err != nil {
+		return nil, err
+	}
+	bcf.coreComponents.EpochNotifier().RegisterNotifyHandler(nodesCoordinatorRegistryFactory)


this should be removed if the nodes coordinator registry factory registers itself to the epoch notifier.

Yes, this registration has been removed after refactor 👍

AdoAdoAdo · 2022-03-23T09:46:04Z

factory/shardingFactory.go

@@ -173,27 +174,33 @@ func CreateNodesCoordinator(
 		return nil, err
 	}

+	nodesCoordinatorRegistryFactory, err := nodesCoordinator.NewNodesCoordinatorRegistryFactory(marshalizer, stakingV4EnableEpoch)


this is not registered to the epoch notifier so it will not be notified on epoch changes.
Also there is no need to keep creating different instances of this factory, you should instead create it once and have it passed to the other components requiring it.
Please check all instantiations and pass it instead as an argument.

You're right, good suggestion !
Refactored all code, such that BootStrapCompHolder factory will create only one instance and provide it for other subcomps that need it

AdoAdoAdo · 2022-03-23T09:47:13Z

integrationTests/consensus/testInitializer.go

@@ -520,24 +520,26 @@ func createNodes(
 		bootStorer := integrationTests.CreateMemUnit()
 		consensusCache, _ := lrucache.NewCache(10000)

+		ncf, _ := nodesCoordinator.NewNodesCoordinatorRegistryFactory(&testscommon.MarshalizerMock{}, integrationTests.StakingV4Epoch)


would not work properly as it is not registered to the epoch notifier.
Will be fixed if epoch notifier is passed instead to the constructor, see my previous comments.

Right, however this is used for tests

AdoAdoAdo · 2022-03-23T09:52:33Z

sharding/nodesCoordinator/errors.go

+var ErrNilNodesCoordinatorRegistryFactory = errors.New("nil nodes coordinator registry factory has been given")
+
+// ErrReceivedAuctionValidatorsBeforeStakingV4 signals that auction nodes have been received from peer mini blocks before enabling staking v4
+var ErrReceivedAuctionValidatorsBeforeStakingV4 = errors.New("should no have received selected nodes from auction in peer mini blocks, since staking v4 is not enabled yet")


typo should no have received -> should not have received

Right, fixed it

AdoAdoAdo · 2022-03-23T10:45:54Z

sharding/nodesCoordinator/indexHashedNodesCoordinator.go

+	ihnc.flagStakingV4.SetValue(epoch >= ihnc.stakingV4EnableEpoch)
+	log.Debug("indexHashedNodesCoordinator: staking v4", "enabled", ihnc.flagStakingV4.IsSet())
+
+	ihnc.nodesCoordinatorRegistryFactory.EpochConfirmed(epoch, 0)


we should not need to manually call this here.

Right, this was just a "hack", but was not supposed to stay there. Removed it

AdoAdoAdo · 2022-03-23T12:34:12Z

sharding/nodesCoordinator/indexHashedNodesCoordinator_test.go

+	require.Equal(t, nc.shardIDAsObserver, computedShardId)
+	require.False(t, isValidator)
+
+	nc.flagStakingV4.SetReturningPrevious()


why not SetValue(true) as you are not checking the returned value anyway?

SetReturningPrevious was doing the same thing, but I guess it's better to use SetValue. Refactored as suggested

AdoAdoAdo · 2022-03-24T09:38:56Z

integrationTests/factory/processComponents/processComponents_test.go

@@ -49,6 +50,12 @@ func TestProcessComponents_Close_ShouldWork(t *testing.T) {
 	require.Nil(t, err)
 	nodesShufflerOut, err := mainFactory.CreateNodesShuffleOut(managedCoreComponents.GenesisNodesSetup(), configs.GeneralConfig.EpochStartConfig, managedCoreComponents.ChanStopNodeProcess())
 	require.Nil(t, err)
+	nodesCoordinatorRegistryFactory, err := nodesCoord.NewNodesCoordinatorRegistryFactory(


could you use the nodesCoordinatorRegistryFactory from managedBootstrapComponents instead?

Very good suggestion, applied it !

AdoAdoAdo · 2022-03-24T09:39:22Z

integrationTests/factory/statusComponents/statusComponents_test.go

@@ -49,6 +50,12 @@ func TestStatusComponents_Create_Close_ShouldWork(t *testing.T) {
 	require.Nil(t, err)
 	nodesShufflerOut, err := mainFactory.CreateNodesShuffleOut(managedCoreComponents.GenesisNodesSetup(), configs.GeneralConfig.EpochStartConfig, managedCoreComponents.ChanStopNodeProcess())
 	require.Nil(t, err)
+	nodesCoordinatorRegistryFactory, err := nodesCoord.NewNodesCoordinatorRegistryFactory(


please use the factory from managedBootstrapComponents

Very good suggestion, applied it !

AdoAdoAdo · 2022-03-24T09:39:57Z

integrationTests/factory/consensusComponents/consensusComponents_test.go

@@ -48,6 +49,12 @@ func TestConsensusComponents_Close_ShouldWork(t *testing.T) {
 	require.Nil(t, err)
 	nodesShufflerOut, err := mainFactory.CreateNodesShuffleOut(managedCoreComponents.GenesisNodesSetup(), configs.GeneralConfig.EpochStartConfig, managedCoreComponents.ChanStopNodeProcess())
 	require.Nil(t, err)
+	nodesCoordinatorRegistryFactory, err := nodesCoord.NewNodesCoordinatorRegistryFactory(


please use the factory from managedBootstrapComponents instead

Very good suggestion, applied it !

FEAT: Add first version

479692d

mariusmihaic changed the title ~~En 11664 nodes coordinator staking v4~~ Nodes coordinator with staking v4 Mar 7, 2022

mariusmihaic changed the base branch from EN-11662-ignore-staking-queue to move-waiting-list-from-staking March 7, 2022 10:40

mariusmihaic added 15 commits March 7, 2022 15:06

FEAT: ihnc with auction

8c1ed21

Merge branch 'move-waiting-list-from-staking' into EN-11664-nodes-coo…

40805f9

…rdinator-staking-v4

FEAT: Use flag to save with auction list

d87f063

FEAT: Use interface instead of *NodesCoordinatorRegistry

fe9db50

FIX: Build

34b4f01

FIX: Build 2

9664050

FEAT: Refactor LoadState to use interface

54087d9

FEAT: Use proto structs

55e09b3

FEAT: Add generated proto file

337a353

FIX: Refactor code structure

6e7b730

FIX: Remove SetEpochsConfig interface func

d6cf445

FEAT: Extract common code to getMinAndLastEpoch

e63f85b

FEAT: Add CreateNodesCoordinatorRegistry

82bf91e

FEAT: Use CreateNodesCoordinatorRegistry in nodesCoord

3ca3f89

Merge branch 'move-waiting-list-from-staking' into EN-11664-nodes-coo…

be3bc70

…rdinator-staking-v4

mariusmihaic self-assigned this Mar 9, 2022

mariusmihaic added 6 commits March 10, 2022 11:01

FIX: Use SelectedFromAuctionList instead of AuctionList

b4993df

FEAT: Add tmp test

e306d99

FEAT: Add nodes coord tests

0c6ae5e

FEAT: Add node shuffler tests

9815093

FIX: Small test refactor

0807341

Merge branch 'move-waiting-list-from-staking' into EN-11664-nodes-coo…

4fddf7b

…rdinator-staking-v4

mariusmihaic marked this pull request as ready for review March 11, 2022 14:11

mariusmihaic force-pushed the EN-11664-nodes-coordinator-staking-v4 branch from 40715e9 to 4fddf7b Compare March 16, 2022 08:54

mariusmihaic added 2 commits March 17, 2022 12:03

FIX: Merge conflicts

8dbcf97

Base automatically changed from move-waiting-list-from-staking to feat/liquid-staking March 21, 2022 13:22

sasurobert reviewed Mar 21, 2022

View reviewed changes

mariusmihaic added 5 commits March 22, 2022 11:15

FEAT: Create nodesCoordinatorRegistryFactory.go

068c23a

FIX: Test

ccea211

FIX: CreateNodesCoordinator

04b6888

FIX: Review findings

eca5854

Merge branch 'feat/liquid-staking' into EN-11664-nodes-coordinator-st…

09d6a4a

…aking-v4

sasurobert previously approved these changes Mar 22, 2022

View reviewed changes

FIX: Tests

fb6a3b9

mariusmihaic dismissed sasurobert’s stale review via fb6a3b9 March 23, 2022 09:04

AdoAdoAdo requested changes Mar 23, 2022

View reviewed changes

mariusmihaic added 4 commits March 23, 2022 16:36

FEAT: Save shuffled out in auction list + test

9f32944

FIX: Test

8f17265

FIX: Another test + typo

d58e550

FIX: Findings + tests

7dd0593

sasurobert previously approved these changes Mar 24, 2022

View reviewed changes

AdoAdoAdo requested changes Mar 24, 2022

View reviewed changes

FIX: Review findings

6092f80

mariusmihaic dismissed sasurobert’s stale review via 6092f80 March 24, 2022 10:13

AdoAdoAdo approved these changes Mar 24, 2022

View reviewed changes

sasurobert approved these changes Mar 24, 2022

View reviewed changes

mariusmihaic merged commit 8a122e8 into feat/liquid-staking Mar 24, 2022

mariusmihaic deleted the EN-11664-nodes-coordinator-staking-v4 branch March 24, 2022 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nodes coordinator with staking v4 #3883

Nodes coordinator with staking v4 #3883

mariusmihaic commented Mar 7, 2022 •

edited

sasurobert Mar 21, 2022

mariusmihaic Mar 22, 2022

sasurobert Mar 21, 2022

mariusmihaic Mar 22, 2022

sasurobert Mar 21, 2022

mariusmihaic Mar 22, 2022

sasurobert Mar 21, 2022

mariusmihaic Mar 22, 2022

sasurobert Mar 21, 2022

sasurobert Mar 21, 2022

mariusmihaic Mar 22, 2022

AdoAdoAdo Mar 23, 2022

mariusmihaic Mar 23, 2022 •

edited

AdoAdoAdo Mar 23, 2022

mariusmihaic Mar 23, 2022

AdoAdoAdo Mar 23, 2022

mariusmihaic Mar 23, 2022

AdoAdoAdo Mar 23, 2022

mariusmihaic Mar 23, 2022

AdoAdoAdo Mar 23, 2022

mariusmihaic Mar 23, 2022

AdoAdoAdo Mar 23, 2022 •

edited

mariusmihaic Mar 23, 2022

AdoAdoAdo Mar 23, 2022

mariusmihaic Mar 23, 2022

AdoAdoAdo Mar 24, 2022

mariusmihaic Mar 24, 2022

AdoAdoAdo Mar 24, 2022

mariusmihaic Mar 24, 2022

AdoAdoAdo Mar 24, 2022

mariusmihaic Mar 24, 2022

		@@ -160,27 +161,37 @@ func (bcf bootstrapComponentsFactory) Create() (bootstrapComponents, error) {

		dataSyncerFactory := bootstrap.NewScheduledDataSyncerFactory()

		nodesCoordinatorRegistryFactory, err := nodesCoordinator.NewNodesCoordinatorRegistryFactory(

Nodes coordinator with staking v4 #3883

Nodes coordinator with staking v4 #3883

Conversation

mariusmihaic commented Mar 7, 2022 • edited

Refactor

New code

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

mariusmihaic Mar 23, 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

AdoAdoAdo Mar 23, 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

mariusmihaic commented Mar 7, 2022 •

edited

mariusmihaic Mar 23, 2022 •

edited

AdoAdoAdo Mar 23, 2022 •

edited