validator: listen and get metadata only for committee validators #1787

nkryuchkov · 2024-10-10T23:42:08Z

… validators belonging to own committee

y0sher

I know this pr is still immature. quoting the issue here to clarify my original intentions -

- Consider to pull metadata from beacon node only about validators that are part of your listened subnets.
- Reject/ignore all messages that are not part of your subnet validators (this might happen automatically since you won't have their status and duties)

So about first point I see you started it, but your using the ValidatorSubnet calculation which is pre-alan way to find your subnets. We now use CommiteeSubnet and have a different way of dividing our validators to subnets (according to committees).

About second point I don't see that you did anything about message validation, though if we don't have any metadata about this operator's validators than we might just naturally reject/ignore it? please make sure what happens in this case.

nkryuchkov · 2024-10-16T01:32:20Z

So about first point I see you started it, but your using the ValidatorSubnet calculation which is pre-alan way to find your subnets. We now use CommiteeSubnet and have a different way of dividing our validators to subnets (according to committees).

Added post-fork handling, please review again

About second point I don't see that you did anything about message validation, though if we don't have any metadata about this operator's validators than we might just naturally reject/ignore it? please make sure what happens in this case.

In this case the message gets ignored:

	ErrNoShareMetadata                         = Error{text: "share has no metadata"}

operator/validator/controller.go

y0sher

Remove prefork code, as I explained its not needed anymore
to make this PR final we need to POC that it works on stage
Compare cpu mem usage
compare how this impacts the overload on beacon node
We probably need to ignore messages that we don't have duties or its validator we don't see as active (because they're not ours). instead of rejecting them.
@MatheusFranco99 can you chime in in terms of protocol and networking, other than more well spread messages, any reason why ignoring these message will impact us badly?

MatheusFranco99 · 2024-10-16T07:30:07Z

@MatheusFranco99 can you chime in in terms of protocol and networking, other than more well-spread messages, any reason why ignoring these messages will impact us badly?

@y0sher
Do you mean ignoring messages from validators that we think are liquidated / not active / exited or whatever similar? I think it's not a problem to ignore instead of reject. We also have ignoring protection against an attack so we should be fine.

But we must not do this if you meant ignoring messages about validators that we are not part of the committee.

nkryuchkov · 2024-10-17T01:33:45Z

Remove prefork code, as I explained its not needed anymore

done

to make this PR final we need to POC that it works on stage

I'm still testing it but so far it seems to be working well

Compare cpu mem usage

It was deployed between 23:46 and 00:28 and since 01:13

CPU dropped from ~2 to ~0.4-0.5:

Memory might have dropped a little bit but the difference is not that huge, also it differs between runs:

Receive bandwidth dropped from ~1.3 to ~0.4-0.5, transmit bandwidth just a little bit:

Although, for another node the transmit bandwidth difference is significant:

compare how this impacts the overload on beacon node

I don't see any difference

CPU:

Mem:

We probably need to ignore messages that we don't have duties or its validator we don't see as active (because they're not ours). instead of rejecting them.

Well, we ignore messages if there's no metadata, so if they're not ours, we should have no metadata for them, hence the message is ignored

MatheusFranco99 · 2024-10-17T07:32:48Z

Well, we ignore messages if there's no metadata, so if they're not ours, we should have no metadata for them, hence the message is ignored

@nkryuchkov
When you say "if they're not ours", you mean non SSV validators, right? Or do you mean non-committee validators? Or validators from topics we're not subscribed to? 😅
I think that's the part I got a bit confused

Great job!

nkryuchkov · 2024-10-17T12:38:33Z

@MatheusFranco99 sorry for the confusion, I meant validators from topics we're not subscribed to

operator/validator/controller.go

MatusKysel

👍

y0sher · 2024-10-20T12:30:26Z

operator/validator/controller.go

 	for _, share := range shares {
 		if c.operatorDataStore.GetOperatorID() != 0 && share.BelongsToOperator(c.operatorDataStore.GetOperatorID()) {
 			ownShares = append(ownShares, share)
 		}
-		allPubKeys = append(allPubKeys, share.ValidatorPubKey[:])
+		committeeSubnet := byte(commons.CommitteeSubnet(share.CommitteeID()))
+		if bytes.Contains(activeSubnets, []byte{committeeSubnet}) {


I believe we have SharedSubnets and some other subnets utility functions. its best to use a function across the code to make changing and bug fixing easier.

SharedSubnets checks intersections between two subnet sets but we have a single value here, so I think it's not a good fit here. However, there was a bug in my implementation, I don't need bytes.Contains, I can just check the index

However, I think we need to refactor how we work with subnets in future PRs: Create a type for it and add methods

y0sher · 2024-10-20T12:37:00Z

operator/validator/controller.go

@@ -1139,6 +1148,12 @@ func (c *controller) UpdateValidatorMetaDataLoop() {
 			if share.Liquidated {
 				return true
 			}
+
+			committeeSubnet := byte(commons.CommitteeSubnet(share.CommitteeID()))
+			if !bytes.Contains(activeSubnets, []byte{committeeSubnet}) {


moshe-blox · 2024-10-21T12:09:32Z

operator/validator/controller.go

@@ -1131,6 +1139,7 @@ func (c *controller) UpdateValidatorMetaDataLoop() {
 	const batchSize = 512
 	var sleep = 2 * time.Second

+	activeSubnets := c.network.ActiveSubnets()


for performance reasons, because this would be checked >44k times on Mainnet, we should change it to return [SubnetCount]bool or convert into it here, and then we can just check the array at the subnet index instead of using bytes.Contains below

@moshe-blox There was a bug in my implementation, I don't need to use bytes.Contains actually, I can just check the index. I don't think we need to have an array in this PR just in this method, but we need to refactor subnets to have array as an underlying type

validator: listen and get metadata only for committee validators

c2a0697

nkryuchkov requested a review from y0sher October 10, 2024 23:42

nkryuchkov added 4 commits October 11, 2024 16:24

fix tests

602c350

fetch metadata from validators belonging to active subnets instead of…

709c6a4

… validators belonging to own committee

use uber's gomock

cac920e

fix TestSetupValidatorsExporter

0476e21

y0sher requested changes Oct 15, 2024

View reviewed changes

calculate subnet for committee instead of validator

62b6950

nkryuchkov requested a review from y0sher October 16, 2024 01:32

y0sher reviewed Oct 16, 2024

View reviewed changes

operator/validator/controller.go Outdated Show resolved Hide resolved

y0sher requested changes Oct 16, 2024

View reviewed changes

remove pre-fork code

0ae7821

nkryuchkov marked this pull request as ready for review October 16, 2024 23:42

nkryuchkov requested a review from y0sher October 17, 2024 01:34

nkryuchkov requested a review from MatusKysel October 17, 2024 12:38

MatusKysel reviewed Oct 17, 2024

View reviewed changes

operator/validator/controller.go Outdated Show resolved Hide resolved

MatusKysel reviewed Oct 17, 2024

View reviewed changes

operator/validator/controller.go Outdated Show resolved Hide resolved

call ActiveSubnets before the loop

4b30cc6

MatusKysel approved these changes Oct 17, 2024

View reviewed changes

y0sher reviewed Oct 20, 2024

View reviewed changes

moshe-blox reviewed Oct 21, 2024

View reviewed changes

nkryuchkov added 3 commits October 21, 2024 11:44

fix check for subnet

9e41edf

fix TestSetupValidatorsExporter

74c19ef

check bounds

df6e732

nkryuchkov requested review from y0sher, iurii-ssv and moshe-blox October 26, 2024 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validator: listen and get metadata only for committee validators #1787

validator: listen and get metadata only for committee validators #1787

nkryuchkov commented Oct 10, 2024 •

edited

Loading

y0sher left a comment

nkryuchkov commented Oct 16, 2024 •

edited

Loading

y0sher left a comment •

edited

Loading

MatheusFranco99 commented Oct 16, 2024

nkryuchkov commented Oct 17, 2024

MatheusFranco99 commented Oct 17, 2024

nkryuchkov commented Oct 17, 2024

MatusKysel left a comment

y0sher Oct 20, 2024

nkryuchkov Oct 21, 2024

nkryuchkov Oct 21, 2024

y0sher Oct 20, 2024

moshe-blox Oct 21, 2024 •

edited

Loading

nkryuchkov Oct 21, 2024

validator: listen and get metadata only for committee validators #1787

Are you sure you want to change the base?

validator: listen and get metadata only for committee validators #1787

Conversation

nkryuchkov commented Oct 10, 2024 • edited Loading

y0sher left a comment

Choose a reason for hiding this comment

nkryuchkov commented Oct 16, 2024 • edited Loading

y0sher left a comment • edited Loading

Choose a reason for hiding this comment

MatheusFranco99 commented Oct 16, 2024

nkryuchkov commented Oct 17, 2024

MatheusFranco99 commented Oct 17, 2024

nkryuchkov commented Oct 17, 2024

MatusKysel left a comment

Choose a reason for hiding this comment

y0sher Oct 20, 2024

Choose a reason for hiding this comment

nkryuchkov Oct 21, 2024

Choose a reason for hiding this comment

nkryuchkov Oct 21, 2024

Choose a reason for hiding this comment

y0sher Oct 20, 2024

Choose a reason for hiding this comment

moshe-blox Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

nkryuchkov Oct 21, 2024

Choose a reason for hiding this comment

nkryuchkov commented Oct 10, 2024 •

edited

Loading

nkryuchkov commented Oct 16, 2024 •

edited

Loading

y0sher left a comment •

edited

Loading

moshe-blox Oct 21, 2024 •

edited

Loading