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

Validator registration filter non active keys before submission #12322

Merged
merged 16 commits into from
Apr 27, 2023

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Apr 24, 2023

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

some relays do not accept validators that are not in active status such as validators that are exited.
currently, we send validator registrations as long as the public key has an index but it seems that some relays return 502 if the validator has exited. this pr adds additional filtering for these cases.

currently the user will receive this kind of error

[2023-04-25 12:08:38]  WARN validator: Push proposer settings error error=could not submit signed registrations to beacon node: rpc error: code = InvalidArgument desc = Could not register block builder: could not register validator(s): not a known validator: 0x8b91f34c039c348f73c2dda492998e268c2815a433f59a6667258267739947dcfc8ade6823b3f1f3cfef824497eb113c: recv 400 BadRequest response from API: did not receive 200 response from API: Builder API validator registration unsuccessful

now the non active status validators should be filtered and not included when submitting builder validator registration

[2023-04-25 14:26:43]  INFO validator: Validator exited index=392139 pubKey=0x8b91f34c039c status=EXITED
[2023-04-25 14:26:43]  INFO validator: Validator exited index=392137 pubKey=0x855ae9c6184d status=EXITED
[2023-04-25 14:26:43]  INFO validator: Validator activated index=392138 publicKey=0xaa0ef7404c3a
[2023-04-25 14:26:43]  INFO validator: Attestation schedule attesterDutiesAtSlot=1 pubKeys=[0xaa0ef7404c3a] slot=5495244 slotInEpoch=12 timeTillDuty=2m5s totalAttestersInEpoch=1
[2023-04-25 14:26:43]  INFO validator: Validator client started with provided proposer settings that sets options such as fee recipient and will periodically update the beacon node and custom builder (if --enable-builder)
[2023-04-25 14:26:43]  INFO validator: Submitted builder validator registration settings for custom builders

the most expensive call added here is MultipleValidatorStatus , below is a screenshot of the api called in another place in the code with 1000 validators on prater resulting in 20millisecond return
image

@james-prysm james-prysm changed the title Validator registration filter Validator registration filter non active keys before submission Apr 24, 2023
@james-prysm james-prysm added the Builder PR or issue that supports builder related work label Apr 24, 2023
@james-prysm james-prysm marked this pull request as ready for review April 24, 2023 22:24
@james-prysm james-prysm requested a review from a team as a code owner April 24, 2023 22:24
Comment on lines +1153 to +1157
// map is populated before this function in buildPrepProposerReq
_, ok := v.pubkeyToValidatorIndex[k]
if !ok {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you need a read lock to access this map?

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 currently, is this something that should have a read lock and write lock for updating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

access to this map should be sequential only so read and writes shouldn't overlap in this case I think we won't need the lock but will test.

v.pubkeyToValidatorIndex[k] = i
}
status, err := v.validatorClient.ValidatorStatus(ctx, &ethpb.ValidatorStatusRequest{PublicKey: k[:]})
Copy link
Member

Choose a reason for hiding this comment

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

doing this for every validator seems expensive, each request will be a separate roundtrip to the beacon node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated using our internal API which will minimize the requests.

@james-prysm james-prysm merged commit f9f4097 into develop Apr 27, 2023
@delete-merged-branch delete-merged-branch bot deleted the validator-registration-filter branch April 27, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builder PR or issue that supports builder related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants