New `ssv_types` #69

Zacholme7 · 2024-12-09T22:11:21Z

Issue Addressed

N/A

Proposed Changes

This PR introduces a new ssv_types system.

The types follow a hierarchical structure of roughly the following. This cleanly represents all SSV specific information.

Operator
- Operator ID
- Operator Public Key
Cluster
- Cluster ID
- Validator Metadata
- Cluster Members
  - Identifiers
  - Share

It does not contain related methods as we still need to determine what is needed, but this is an initial step to get the ball rolling.

jking-aus

I think this is the way to go - let's get it passing CI and have a chat about a few things then merge it down

edit: seeing the CI flags are dependencies -- let's just have a chat on the below and we'll merge anyway

jking-aus · 2024-12-10T03:52:38Z

anchor/common/ssv_types/src/cluster.rs

+    /// The number of faulty operator in the Cluster
+    pub faulty: u64,
+    /// If the Cluster is liquidated or active
+    pub liquidated: bool,


I don't see a liquidated flag in the specs but can understand why it would be useful. Is there no analogous implementation in the go client?

Its on the metadata here in the client.

jking-aus · 2024-12-10T04:06:13Z

anchor/common/ssv_types/src/cluster.rs

+
+/// A Cluster is a group of Operators that are acting on behalf of a Validator
+#[derive(Debug, Clone)]
+pub struct Cluster {


I think domain needs to be included on the Cluster unless you had another plan for that implementation

I left domain and some other specific Validator data off since a lot of it is going to be included via the lighthouse types. If we realize it makes more sense to add it here we can do that.

jking-aus · 2024-12-10T04:09:52Z

anchor/common/ssv_types/src/cluster.rs

+    /// Unique identifier for the Operator this member represents
+    pub operator_id: OperatorId,
+    /// Unique identifier for the Cluster this member is a part of
+    pub cluster_id: ClusterId,


I dont think we need clusterid here since ClusterMember is already contained within a Cluster -- suggest drop unless there's a reason

Good point, but I think this depends on how we want to structure the maps for the database. If we want to do something like OperatorID => Vec of ClusterMembers, then this ClusterID is important. If we do ClusterID => Vec of ClusterMembers, then it is not important. The design of the database quite tightly influences the design of the types and it's hard to anticipate what the right structure is at this point.

jking-aus · 2024-12-10T04:17:02Z

anchor/common/ssv_types/src/share.rs

+#[derive(Debug, Clone)]
+pub struct Share {
+    /// The public key of this Share
+    pub share_pubkey: PublicKey,


would we not need operatorID here?

The Share is part of ClusterMember, and the operator ID can be found there

Something I considered but this is also a it depends on the structure of the database. We do have the access to the OperatorID from the ClusterMember. My idea was to go from ClusterID => Share where the Share is owned by the current operator, so storing the ID on the share is not needed in this case.

jking-aus · 2024-12-10T04:19:49Z

anchor/common/ssv_types/src/cluster.rs

+
+/// Unique identifier for a cluster
+#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Hash, From, Deref)]
+pub struct ClusterId(pub u64);


I know there is some terminology overlap but it's probably better we stick with "committee" for now so we're using the same terminology as the other client team

An argument for "cluster" is that "committee" might be confusable with the committees from Ethereum consensus

Yep Daniel mentioned my idea behind Cluster. Im good to change it but in that case we should name it something like SSVCommittee just to be explicit about the difference.

jking-aus · 2024-12-10T04:21:21Z

anchor/common/ssv_types/src/cluster.rs

+
+/// General Metadata about a Validator
+#[derive(Debug, Clone)]
+pub struct ValidatorMetadata {


is there a reason we've pulled out the committee ID and domaintype from here?

I think this type is supposed to basically represent the metadata of the validator operated by the cluster, not the cluster itself, so I think I like leaving it out here.

Yep, this is just about the Validator the cluster is acting on behalf of. Can be stored in database as ClusterID => ValidatorMetadata so this info still gets included. As for the domain, I left it off because I believe it is being included via lighthouse types, but I have to double check to confirm what it gets included through.

jking-aus · 2024-12-10T04:23:28Z

anchor/common/ssv_types/src/operator.rs

+
+/// Client responsible for maintaining the overall health of the network.
+#[derive(Debug, Clone)]
+pub struct Operator {


is there any benefit to tracking our cluster memberships here or is that being handled in the DB?

IMO that should be handled in the db

Yep, I recommend keeping Operator self contained and handling this in the DB

dknopik · 2024-12-10T08:09:10Z

anchor/common/ssv_types/src/share.rs

+#[derive(Debug, Clone)]
+pub struct Share {
+    /// The public key of this Share
+    pub share_pubkey: PublicKey,


The Share is part of ClusterMember, and the operator ID can be found there

dknopik · 2024-12-10T08:10:16Z

anchor/common/ssv_types/src/share.rs

+
+/// One of N shares of a split validator key.
+#[derive(Debug, Clone)]
+pub struct Share {


Should we add the (encrypted) private key here?

I had it in but removed it right before the PR as I convinced myself it wasn't needed in the Share for some reason. Thinking on it now it should probably be here. I'll add it back in thanks!

dknopik · 2024-12-10T08:13:03Z

anchor/common/ssv_types/src/operator.rs

+
+/// Client responsible for maintaining the overall health of the network.
+#[derive(Debug, Clone)]
+pub struct Operator {


IMO that should be handled in the db

dknopik · 2024-12-10T08:15:47Z

anchor/common/ssv_types/src/cluster.rs

+
+/// General Metadata about a Validator
+#[derive(Debug, Clone)]
+pub struct ValidatorMetadata {


I think this type is supposed to basically represent the metadata of the validator operated by the cluster, not the cluster itself, so I think I like leaving it out here.

jking-aus

lgtm

diegomrsantos · 2025-01-09T16:44:19Z

anchor/common/ssv_types/src/operator.rs

+    /// Base-64 encoded PEM RSA public key
+    pub rsa_pubkey: Rsa<Public>,
+    /// Owner of the operator
+    pub owner: Address,


Is this necessary? I think I didn't see it in the go code

The public key is used for initial operator identification. When you first join as an operator and do not yet know your id, the public key is used to match to event logs and figure out which id is yours.

The owner address is where the SSV you get paid for running an operator is sent to

newtypes

04943e5

jking-aus reviewed Dec 10, 2024

View reviewed changes

dknopik reviewed Dec 10, 2024

View reviewed changes

Zacholme7 added 3 commits December 11, 2024 14:44

fix ci audit and add encrypted secret key

673537f

move to byte array for encrypted private key

bd0e451

Merge branch 'unstable' into new-ssvtypes

dfa4c5c

Zacholme7 mentioned this pull request Dec 11, 2024

NetworkDatabase impl with new ssv_types system #67

Merged

jking-aus approved these changes Dec 12, 2024

View reviewed changes

jking-aus merged commit 4182bf6 into sigp:unstable Dec 12, 2024
8 checks passed

Zacholme7 deleted the new-ssvtypes branch December 12, 2024 12:55

diegomrsantos reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `ssv_types` #69

New `ssv_types` #69

Zacholme7 commented Dec 9, 2024

jking-aus left a comment •

edited

Loading

jking-aus Dec 10, 2024

Zacholme7 Dec 10, 2024

jking-aus Dec 10, 2024

Zacholme7 Dec 10, 2024

jking-aus Dec 10, 2024

Zacholme7 Dec 10, 2024 •

edited

Loading

jking-aus Dec 10, 2024

dknopik Dec 10, 2024

Zacholme7 Dec 10, 2024

jking-aus Dec 10, 2024

dknopik Dec 10, 2024

Zacholme7 Dec 10, 2024

jking-aus Dec 10, 2024

dknopik Dec 10, 2024

Zacholme7 Dec 10, 2024

jking-aus Dec 10, 2024

dknopik Dec 10, 2024

Zacholme7 Dec 10, 2024

dknopik Dec 10, 2024

dknopik Dec 10, 2024

Zacholme7 Dec 10, 2024

dknopik Dec 10, 2024

dknopik Dec 10, 2024

jking-aus left a comment

diegomrsantos Jan 9, 2025 •

edited

Loading

Zacholme7 Jan 9, 2025

New ssv_types #69

New ssv_types #69

Conversation

Zacholme7 commented Dec 9, 2024

Issue Addressed

Proposed Changes

jking-aus left a comment • edited Loading

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

Zacholme7 Dec 10, 2024 • edited Loading

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

jking-aus left a comment

Choose a reason for hiding this comment

diegomrsantos Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

New `ssv_types` #69

New `ssv_types` #69

jking-aus left a comment •

edited

Loading

Zacholme7 Dec 10, 2024 •

edited

Loading

diegomrsantos Jan 9, 2025 •

edited

Loading