Substantial VC cleanup #13593

rkapka · 2024-02-07T15:58:10Z

What type of PR is this?

Cleanup

What does this PR do? Why is it needed?

Certain parts of the validator client's code have not been for a long time. The code is messy, non-elegant and oftentimes hard to update. This PR is the first one in a series of PRs that aim to improve the overall code quality of the module.

Changes overview:

Renamed ValidatorClient interface to Coordinator. Given that Validator Client is the name for the whole component of Prysm, it is confusing that the interface has the same name. The main reason for choosing this particular name is that functions defined on this interface define the behavior of the VC and actions performed on managed keys.
Removed the Beacon part from BeaconChainClient and PrysmBeaconChainClient because it's obvious we mean the beacon chain, not some other chain.
Organized, renamed and cleaned up fields in service/config structs.
Removed unused SyncChecker and GenesisFetcher interfaces and their implementations.
Removed TLS support from the RPC server. The certificate flags that we have are used for values regarding the connection from the VC to the BN, and there is no way to currently define a certificate for the VC server (the fields that we check are never set to anything). Nobody ever complained about it and I think such a requirement would be very niche.

james-prysm · 2024-02-07T22:42:29Z

validator/client/iface/validator_client.go

@@ -62,7 +62,7 @@ func (b *BeaconCommitteeSelection) UnmarshalJSON(input []byte) error {
 	return nil
 }

-type ValidatorClient interface {
+type Coordinator interface {


I'm not sold on the word coordinator, to me coordinator would also define when these are called as well.
Most of these functions are actions for participating in consensus, what about ConsensusToolkit or ConsensusAgent or ConsensusOperator or RoleOperator/DutyOperator - ( since the validator gets the duties first to perform the tasks in it.) there's probably some design pattern to enforce that the duties are up to date in the state to perform the actions inside.

I reverted this change. We should probably split the interface into a few smaller interfaces, each responsible for its own this. Mixing GetDuties with EventStreamIsRunning doesn't feel right.

james-prysm · 2024-02-07T22:56:07Z

validator/rpc/beacon.go

 		log.Info("Established secure gRPC connection")
 	}
-	s.beaconNodeHealthClient = ethpb.NewHealthClient(grpcConn)
+	s.healthClient = ethpb.NewHealthClient(grpcConn)


we need a todo to get rid of this thing i think

CLAassistant · 2024-02-12T11:42:47Z

All committers have signed the CLA.

# Conflicts: # validator/client/service.go # validator/client/validator.go # validator/rpc/server.go

validator/client/validator_test.go

james-prysm · 2024-02-12T19:20:56Z

validator/client/validator.go

 	genesisTime                        uint64
+	highestValidSlot                   primitives.Slot


why did we move some of these fields around? is it a linting issue?

I moved fields so that they are grouped together by functionality

james-prysm · 2024-02-12T19:38:52Z

validator/rpc/handlers_keymanager.go

@@ -414,7 +414,7 @@ func (s *Server) ListRemoteKeys(w http.ResponseWriter, r *http.Request) {
 	for i := 0; i < len(pubKeys); i++ {
 		keystoreResponse[i] = &RemoteKey{
 			Pubkey:   hexutil.Encode(pubKeys[i][:]),
-			Url:      s.validatorService.Web3SignerConfig.BaseEndpoint,
+			Url:      s.validatorService.SignerConfig().BaseEndpoint,


maybe it makes more sense as RemoteSignerConfig()

james-prysm · 2024-02-12T19:41:17Z

validator/rpc/server.go

@@ -270,5 +240,5 @@ func (s *Server) Stop() error {

 // Status returns nil or credentialError.
 func (s *Server) Status() error {
-	return s.credentialError
+	return nil


does this function do anything? lol is there a reason to keep it or at least add some log?

This is part of the Service interface contract. It returns an error when something is wrong with the service. Which means that currently we always treat the service as healthy. If we want to add some logic here, it would be better to do it in another PR.

# Conflicts: # testing/validator-mock/chain_client_mock.go # testing/validator-mock/prysm_beacon_chain_client_mock.go # validator/client/attest_test.go # validator/client/beacon-api/beacon_api_validator_client.go # validator/client/beacon-api/duties_test.go # validator/client/propose_test.go # validator/client/service.go # validator/client/validator.go # validator/client/validator_test.go # validator/node/node.go # validator/rpc/auth_token.go # validator/rpc/beacon.go # validator/rpc/handlers_keymanager.go # validator/rpc/handlers_keymanager_test.go # validator/rpc/handlers_slashing.go # validator/rpc/handlers_slashing_test.go # validator/rpc/server.go

james-prysm · 2024-05-21T16:00:38Z

validator/rpc/server.go

@@ -109,7 +110,7 @@ func NewServer(ctx context.Context, cfg *Config) *Server {
 		if err := server.initializeAuthToken(); err != nil {
 			log.WithError(err).Error("Could not initialize web auth token")
 		}
-		validatorWebAddr := fmt.Sprintf("%s:%d", server.validatorGatewayHost, server.validatorGatewayPort)
+		validatorWebAddr := fmt.Sprintf("%s:%d", server.grpcGatewayHost, server.grpcGatewayPort)


we should look at changing this naming again in the future. we want to get rid of gateway asap

james-prysm · 2024-05-21T16:26:02Z

validator/client/service.go

@@ -268,6 +240,11 @@ func (v *ValidatorService) Keymanager() (keymanager.IKeymanager, error) {
 	return v.validator.Keymanager()
 }

+// RemoteSignerConfig returns the web3signer configuration
+func (v *ValidatorService) RemoteSignerConfig() *remoteweb3signer.SetupConfig {
+	return v.web3SignerConfig


should we change this name too?

* Cleanup part 1 * Cleanup part 2 * Cleanup part 3 * remove lock field init * doc for SignerConfig * remove vars * use full Keymanager word in function * revert interface rename * linter * fix build issues * review (cherry picked from commit 30cc23c)

rkapka added 2 commits February 6, 2024 20:46

Cleanup part 1

e16d6ac

Cleanup part 2

88ee17d

rkapka added Cleanup Code health! Validator Client labels Feb 7, 2024

rkapka requested a review from a team as a code owner February 7, 2024 15:58

rkapka requested review from nalepae, saolyn and potuz February 7, 2024 15:58

Cleanup part 3

7e0b507

rkapka force-pushed the simplify-vc branch from 8e36291 to 7e0b507 Compare February 7, 2024 16:11

rkapka added 4 commits February 7, 2024 17:17

remove lock field init

29af68d

doc for SignerConfig

480a4fd

remove vars

8eba33d

use full Keymanager word in function

20d3909

rkapka added the Blocked Blocked by research or external factors label Feb 7, 2024

james-prysm reviewed Feb 7, 2024

View reviewed changes

revert interface rename

2f4da7b

rkapka force-pushed the simplify-vc branch from 9d6f4c7 to 2f4da7b Compare February 12, 2024 11:42

rkapka and others added 2 commits February 12, 2024 13:01

Merge branch '__develop' into simplify-vc

3388e58

# Conflicts: # validator/client/service.go # validator/client/validator.go # validator/rpc/server.go

linter

eecbc09

james-prysm reviewed Feb 12, 2024

View reviewed changes

validator/client/validator_test.go Outdated Show resolved Hide resolved

james-prysm reviewed Feb 12, 2024

View reviewed changes

rkapka added 3 commits May 21, 2024 13:08

fix build issues

d0830fb

review

6b88684

rkapka removed the Blocked Blocked by research or external factors label May 21, 2024

james-prysm reviewed May 21, 2024

View reviewed changes

james-prysm approved these changes May 21, 2024

View reviewed changes

james-prysm reviewed May 21, 2024

View reviewed changes

rkapka added this pull request to the merge queue May 21, 2024

Merged via the queue into develop with commit 30cc23c May 21, 2024
16 of 17 checks passed

rkapka deleted the simplify-vc branch May 21, 2024 16:46

james-prysm mentioned this pull request May 22, 2024

WebFix develop #14040

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Substantial VC cleanup #13593

Substantial VC cleanup #13593

rkapka commented Feb 7, 2024 •

edited

Loading

james-prysm Feb 7, 2024 •

edited

Loading

rkapka Feb 12, 2024

james-prysm Feb 7, 2024

CLAassistant commented Feb 12, 2024 •

edited

Loading

james-prysm Feb 12, 2024

rkapka May 21, 2024

james-prysm Feb 12, 2024

rkapka May 21, 2024

james-prysm Feb 12, 2024

rkapka May 21, 2024

james-prysm May 21, 2024

james-prysm May 21, 2024

Substantial VC cleanup #13593

Substantial VC cleanup #13593

Conversation

rkapka commented Feb 7, 2024 • edited Loading

james-prysm Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Feb 12, 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

rkapka commented Feb 7, 2024 •

edited

Loading

james-prysm Feb 7, 2024 •

edited

Loading

CLAassistant commented Feb 12, 2024 •

edited

Loading