Improvement to Fee Recipient UX (#11307)

* updating mock * adding new internal api * adding generated code * converting validator index to pubkey * adding new API endpoint * fixing mock related new API * fixing unit tests * preventing failure on startup, replacing with warnings * improving log message * changing warn log to error log * fixing error formatting * improve log on beacon node side on startup * fixing deepsource issue * improving logs * fixing unit tests * fixing more tests * adding error check * adding in new case for fee recipient to avoid conflicts on changing beacon node suggested fee recipient * adding default fee recipient check for gas limit as well * adding improved unit tests * accidently checked in tmp folder * adding more unit tests * fixing gas limit unit test * Update validator/rpc/standard_api_test.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/standard_api_test.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/standard_api_test.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update beacon-chain/node/config.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update beacon-chain/node/config.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update proto/prysm/v1alpha1/validator.proto Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/standard_api.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/client/runner.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * addressing comments * updating proto generated files * fixing linting and addressign review comments * fixing unit test * improve error handling * accidently added tmp folder * improving function error returns * realizing i am wrapping error incorrectly * fixing unit test * addressing review comment * fixing linting * fixing unit test * improving ux around enable builder Co-authored-by: Radosław Kapka <rkapka@wp.pl> Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
2026-01-09 15:37:56 -05:00 · 2022-09-22 11:35:35 -05:00
parent 61f6b27548
commit 20e99fd1f9
20 changed files with 1113 additions and 471 deletions
--- a/validator/rpc/standard_api.go
+++ b/validator/rpc/standard_api.go
@@ -14,6 +14,7 @@ import (
 	validatorServiceConfig "github.com/prysmaticlabs/prysm/v3/config/validator/service"
 	"github.com/prysmaticlabs/prysm/v3/encoding/bytesutil"
 	ethpbservice "github.com/prysmaticlabs/prysm/v3/proto/eth/service"
+	eth "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
 	"github.com/prysmaticlabs/prysm/v3/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/v3/validator/keymanager/derived"
 	slashingprotection "github.com/prysmaticlabs/prysm/v3/validator/slashing-protection-history"
@@ -24,6 +25,8 @@ import (
 	"google.golang.org/grpc/status"
 )

+const nonExistantPublicKey = "0x0"
+
 // ListKeystores implements the standard validator key management API.
 func (s *Server) ListKeystores(
 	ctx context.Context, _ *empty.Empty,
@@ -448,11 +451,26 @@ func (s *Server) SetGasLimit(ctx context.Context, req *ethpbservice.SetGasLimitR
 	pOption.BuilderConfig = pBuilderConfig

 	if s.validatorService.ProposerSettings == nil {
-		s.validatorService.ProposerSettings = &validatorServiceConfig.ProposerSettings{
-			ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*validatorServiceConfig.ProposerOption{
-				bytesutil.ToBytes48(validatorKey): &pOption,
-			},
-			DefaultConfig: &defaultOption,
+		// get the default fee recipient defined with an invalid public key from beacon node
+		resp, err := s.beaconNodeValidatorClient.GetFeeRecipientByPubKey(ctx, &eth.FeeRecipientByPubKeyRequest{
+			PublicKey: []byte(nonExistantPublicKey),
+		})
+		if resp == nil || len(resp.FeeRecipient) == 0 || err != nil {
+			s.validatorService.ProposerSettings = &validatorServiceConfig.ProposerSettings{
+				ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*validatorServiceConfig.ProposerOption{
+					bytesutil.ToBytes48(validatorKey): &pOption,
+				},
+				DefaultConfig: &defaultOption,
+			}
+		} else {
+			s.validatorService.ProposerSettings = &validatorServiceConfig.ProposerSettings{
+				ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*validatorServiceConfig.ProposerOption{
+					bytesutil.ToBytes48(validatorKey): &pOption,
+				},
+				DefaultConfig: &validatorServiceConfig.ProposerOption{
+					FeeRecipient: common.BytesToAddress(resp.FeeRecipient),
+				},
+			}
 		}
 	} else if s.validatorService.ProposerSettings.ProposeConfig == nil {
 		s.validatorService.ProposerSettings.ProposeConfig = make(map[[fieldparams.BLSPubkeyLength]byte]*validatorServiceConfig.ProposerOption)
@@ -510,7 +528,7 @@ func (s *Server) DeleteGasLimit(ctx context.Context, req *ethpbservice.DeleteGas
 }

 // ListFeeRecipientByPubkey returns the public key to eth address mapping object to the end user.
-func (s *Server) ListFeeRecipientByPubkey(_ context.Context, req *ethpbservice.PubkeyRequest) (*ethpbservice.GetFeeRecipientByPubkeyResponse, error) {
+func (s *Server) ListFeeRecipientByPubkey(ctx context.Context, req *ethpbservice.PubkeyRequest) (*ethpbservice.GetFeeRecipientByPubkeyResponse, error) {
 	if s.validatorService == nil {
 		return nil, status.Error(codes.FailedPrecondition, "Validator service not ready")
 	}
@@ -519,34 +537,34 @@ func (s *Server) ListFeeRecipientByPubkey(_ context.Context, req *ethpbservice.P
 		return nil, status.Error(codes.FailedPrecondition, err.Error())
 	}
 	defaultFeeRecipient := params.BeaconConfig().DefaultFeeRecipient.Bytes()
-	if s.validatorService.ProposerSettings == nil {
-		return &ethpbservice.GetFeeRecipientByPubkeyResponse{
-			Data: &ethpbservice.GetFeeRecipientByPubkeyResponse_FeeRecipient{
-				Pubkey:     validatorKey,
-				Ethaddress: defaultFeeRecipient,
-			},
-		}, nil
-	}
-	if s.validatorService.ProposerSettings.ProposeConfig != nil {
-		proposerOption, found := s.validatorService.ProposerSettings.ProposeConfig[bytesutil.ToBytes48(validatorKey)]
-		if found {
-			return &ethpbservice.GetFeeRecipientByPubkeyResponse{
-				Data: &ethpbservice.GetFeeRecipientByPubkeyResponse_FeeRecipient{
-					Pubkey:     validatorKey,
-					Ethaddress: proposerOption.FeeRecipient.Bytes(),
-				},
-			}, nil
-		}
-	}
-	if s.validatorService.ProposerSettings.DefaultConfig != nil {
-		defaultFeeRecipient = s.validatorService.ProposerSettings.DefaultConfig.FeeRecipient.Bytes()
-	}
-	return &ethpbservice.GetFeeRecipientByPubkeyResponse{
+	finalResp := &ethpbservice.GetFeeRecipientByPubkeyResponse{
 		Data: &ethpbservice.GetFeeRecipientByPubkeyResponse_FeeRecipient{
 			Pubkey:     validatorKey,
 			Ethaddress: defaultFeeRecipient,
 		},
-	}, nil
+	}
+	if s.validatorService.ProposerSettings == nil {
+		resp, err := s.beaconNodeValidatorClient.GetFeeRecipientByPubKey(ctx, &eth.FeeRecipientByPubKeyRequest{
+			PublicKey: validatorKey,
+		})
+		if resp != nil && len(resp.FeeRecipient) != 0 && err == nil {
+			finalResp.Data.Ethaddress = resp.FeeRecipient
+		}
+		return finalResp, nil
+	}
+	if s.validatorService.ProposerSettings.ProposeConfig != nil {
+		hexv := hexutil.Encode(validatorKey)
+		fmt.Println(hexv)
+		proposerOption, found := s.validatorService.ProposerSettings.ProposeConfig[bytesutil.ToBytes48(validatorKey)]
+		if found {
+			finalResp.Data.Ethaddress = proposerOption.FeeRecipient.Bytes()
+			return finalResp, nil
+		}
+	}
+	if s.validatorService.ProposerSettings.DefaultConfig != nil {
+		finalResp.Data.Ethaddress = s.validatorService.ProposerSettings.DefaultConfig.FeeRecipient.Bytes()
+	}
+	return finalResp, nil
 }

 // SetFeeRecipientByPubkey updates the eth address mapped to the public key.
@@ -568,12 +586,23 @@ func (s *Server) SetFeeRecipientByPubkey(ctx context.Context, req *ethpbservice.
 	pOption.FeeRecipient = common.BytesToAddress(req.Ethaddress)
 	switch {
 	case s.validatorService.ProposerSettings == nil:
-		s.validatorService.ProposerSettings = &validatorServiceConfig.ProposerSettings{
+		// get the default fee recipient defined with an invalid public key from beacon node
+		resp, err := s.beaconNodeValidatorClient.GetFeeRecipientByPubKey(ctx, &eth.FeeRecipientByPubKeyRequest{
+			PublicKey: []byte(nonExistantPublicKey),
+		})
+		settings := &validatorServiceConfig.ProposerSettings{
 			ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*validatorServiceConfig.ProposerOption{
 				bytesutil.ToBytes48(validatorKey): &pOption,
 			},
-			DefaultConfig: &defaultOption,
 		}
+		if resp == nil || len(resp.FeeRecipient) == 0 || err != nil {
+			settings.DefaultConfig = &defaultOption
+		} else {
+			settings.DefaultConfig = &validatorServiceConfig.ProposerOption{
+				FeeRecipient: common.BytesToAddress(resp.FeeRecipient),
+			}
+		}
+		s.validatorService.ProposerSettings = settings
 	case s.validatorService.ProposerSettings.ProposeConfig == nil:
 		pOption.BuilderConfig = s.validatorService.ProposerSettings.DefaultConfig.BuilderConfig
 		s.validatorService.ProposerSettings.ProposeConfig = map[[fieldparams.BLSPubkeyLength]byte]*validatorServiceConfig.ProposerOption{
--- a/validator/rpc/standard_api_test.go
+++ b/validator/rpc/standard_api_test.go
@@ -9,6 +9,7 @@ import (

 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/golang/mock/gomock"
 	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/google/uuid"
 	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
@@ -18,8 +19,10 @@ import (
 	"github.com/prysmaticlabs/prysm/v3/crypto/bls"
 	"github.com/prysmaticlabs/prysm/v3/encoding/bytesutil"
 	ethpbservice "github.com/prysmaticlabs/prysm/v3/proto/eth/service"
+	eth "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
 	validatorpb "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1/validator-client"
 	"github.com/prysmaticlabs/prysm/v3/testing/assert"
+	mock2 "github.com/prysmaticlabs/prysm/v3/testing/mock"
 	"github.com/prysmaticlabs/prysm/v3/testing/require"
 	"github.com/prysmaticlabs/prysm/v3/validator/accounts"
 	"github.com/prysmaticlabs/prysm/v3/validator/accounts/iface"
@@ -704,6 +707,7 @@ func TestServer_ListFeeRecipientByPubkey(t *testing.T) {
 		name    string
 		args    *validatorserviceconfig.ProposerSettings
 		want    *want
+		cached  *eth.FeeRecipientByPubKeyResponse
 		wantErr bool
 	}{
 		{
@@ -724,23 +728,60 @@ func TestServer_ListFeeRecipientByPubkey(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "empty settings",
+			name: "happy path test cached",
+			args: &validatorserviceconfig.ProposerSettings{
+				ProposeConfig: map[[48]byte]*validatorserviceconfig.ProposerOption{
+					bytesutil.ToBytes48(byteval): {
+						FeeRecipient: common.HexToAddress("0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9"),
+					},
+				},
+				DefaultConfig: &validatorserviceconfig.ProposerOption{
+					FeeRecipient: common.HexToAddress("0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9"),
+				},
+			},
+			want: &want{
+				EthAddress: "0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9",
+			},
+			cached: &eth.FeeRecipientByPubKeyResponse{
+				FeeRecipient: common.HexToAddress("0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9").Bytes(),
+			},
+			wantErr: false,
+		},
+		{
+			name: "empty settings non cached",
 			args: nil,
 			want: &want{
 				EthAddress: params.BeaconConfig().DefaultFeeRecipient.Hex(),
 			},
 			wantErr: false,
 		},
+		{
+			name: "empty settings cached",
+			args: nil,
+			want: &want{
+				EthAddress: common.HexToAddress("0x055Fb65722E7b2455012BFEBf6177F1D2e97387").Hex(),
+			},
+			wantErr: false,
+			cached: &eth.FeeRecipientByPubKeyResponse{
+				FeeRecipient: common.HexToAddress("0x055Fb65722E7b2455012BFEBf6177F1D2e97387").Bytes(),
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			mockValidatorClient := mock2.NewMockBeaconNodeValidatorClient(ctrl)
 			vs, err := client.NewValidatorService(ctx, &client.Config{
 				Validator:        &mock.MockValidator{},
 				ProposerSettings: tt.args,
 			})
 			require.NoError(t, err)
+			if tt.args == nil || tt.args.ProposeConfig == nil {
+				mockValidatorClient.EXPECT().GetFeeRecipientByPubKey(gomock.Any(), gomock.Any()).Return(tt.cached, nil)
+			}
 			s := &Server{
-				validatorService: vs,
+				validatorService:          vs,
+				beaconNodeValidatorClient: mockValidatorClient,
 			}
 			got, err := s.ListFeeRecipientByPubkey(ctx, &ethpbservice.PubkeyRequest{Pubkey: byteval})
 			require.NoError(t, err)
@@ -749,11 +790,21 @@ func TestServer_ListFeeRecipientByPubkey(t *testing.T) {
 	}
 }
 func TestServer_SetFeeRecipientByPubkey(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	beaconClient := mock2.NewMockBeaconNodeValidatorClient(ctrl)
 	ctx := grpc.NewContextWithServerTransportStream(context.Background(), &runtime.ServerTransportStream{})
 	byteval, err := hexutil.Decode("0xaf2e7ba294e03438ea819bd4033c6c1bf6b04320ee2075b77273c08d02f8a61bcc303c2c06bd3713cb442072ae591493")
+	wantAddress := "0x055Fb65722e7b2455012Bfebf6177f1d2e9738d7"
+	cachedAddress := "0x055Fb65722E7b2455012BFEBf6177F1D2e97387"
 	require.NoError(t, err)
 	type want struct {
-		EthAddress string
+		valEthAddress     string
+		defaultEthaddress string
+	}
+	type beaconResp struct {
+		resp  *eth.FeeRecipientByPubKeyResponse
+		error error
 	}
 	tests := []struct {
 		name             string
@@ -761,12 +812,66 @@ func TestServer_SetFeeRecipientByPubkey(t *testing.T) {
 		proposerSettings *validatorserviceconfig.ProposerSettings
 		want             *want
 		wantErr          bool
+		beaconReturn     *beaconResp
 	}{
 		{
 			name: "Happy Path Test",
 			args: "0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9",
 			want: &want{
-				EthAddress: "0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9",
+				valEthAddress:     "0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9",
+				defaultEthaddress: params.BeaconConfig().DefaultFeeRecipient.Hex(),
+			},
+			wantErr: false,
+			beaconReturn: &beaconResp{
+				resp:  nil,
+				error: nil,
+			},
+		},
+		{
+			name: "Happy Path Test Beacon Cached",
+			args: "0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9",
+			want: &want{
+				valEthAddress:     "0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9",
+				defaultEthaddress: common.HexToAddress(cachedAddress).Hex(),
+			},
+			wantErr: false,
+			beaconReturn: &beaconResp{
+				resp: &eth.FeeRecipientByPubKeyResponse{
+					FeeRecipient: common.HexToAddress(cachedAddress).Bytes(),
+				},
+				error: nil,
+			},
+		},
+		{
+			name: "Happy Path Test Beacon Cached preexisting proposer data",
+			args: wantAddress,
+			want: &want{
+				valEthAddress:     wantAddress,
+				defaultEthaddress: common.HexToAddress(cachedAddress).Hex(),
+			},
+			proposerSettings: &validatorserviceconfig.ProposerSettings{
+				ProposeConfig: map[[48]byte]*validatorserviceconfig.ProposerOption{
+					bytesutil.ToBytes48(byteval): {
+						FeeRecipient: common.HexToAddress("0x055Fb65722e7b2455012Bfebf6177f1d2e9738d8"),
+					},
+				},
+				DefaultConfig: &validatorserviceconfig.ProposerOption{
+					FeeRecipient: common.HexToAddress(cachedAddress),
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "Happy Path Test Beacon Cached preexisting default data",
+			args: wantAddress,
+			want: &want{
+				valEthAddress:     wantAddress,
+				defaultEthaddress: common.HexToAddress(cachedAddress).Hex(),
+			},
+			proposerSettings: &validatorserviceconfig.ProposerSettings{
+				DefaultConfig: &validatorserviceconfig.ProposerOption{
+					FeeRecipient: common.HexToAddress(cachedAddress),
+				},
 			},
 			wantErr: false,
 		},
@@ -777,13 +882,21 @@ func TestServer_SetFeeRecipientByPubkey(t *testing.T) {
 				Validator:        &mock.MockValidator{},
 				ProposerSettings: tt.proposerSettings,
 			})
+			if tt.beaconReturn != nil {
+				beaconClient.EXPECT().GetFeeRecipientByPubKey(
+					gomock.Any(),
+					gomock.Any(),
+				).Return(tt.beaconReturn.resp, tt.beaconReturn.error)
+			}
 			require.NoError(t, err)
 			s := &Server{
-				validatorService: vs,
+				validatorService:          vs,
+				beaconNodeValidatorClient: beaconClient,
 			}
 			_, err = s.SetFeeRecipientByPubkey(ctx, &ethpbservice.SetFeeRecipientByPubkeyRequest{Pubkey: byteval, Ethaddress: common.HexToAddress(tt.args).Bytes()})
 			require.NoError(t, err)
-			assert.Equal(t, tt.want.EthAddress, s.validatorService.ProposerSettings.ProposeConfig[bytesutil.ToBytes48(byteval)].FeeRecipient.Hex())
+			assert.Equal(t, tt.want.valEthAddress, s.validatorService.ProposerSettings.ProposeConfig[bytesutil.ToBytes48(byteval)].FeeRecipient.Hex())
+			assert.Equal(t, tt.want.defaultEthaddress, s.validatorService.ProposerSettings.DefaultConfig.FeeRecipient.Hex())
 		})
 	}
 }
@@ -905,12 +1018,18 @@ func TestServer_GetGasLimit(t *testing.T) {
 }

 func TestServer_SetGasLimit(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	beaconClient := mock2.NewMockBeaconNodeValidatorClient(ctrl)
 	ctx := grpc.NewContextWithServerTransportStream(context.Background(), &runtime.ServerTransportStream{})
 	pubkey1, err := hexutil.Decode("0xaf2e7ba294e03438ea819bd4033c6c1bf6b04320ee2075b77273c08d02f8a61bcc303c2c06bd3713cb442072ae591493")
 	pubkey2, err2 := hexutil.Decode("0xbedefeaa94e03438ea819bd4033c6c1bf6b04320ee2075b77273c08d02f8a61bcc303c2cdddddddddddddddddddddddd")
 	require.NoError(t, err)
 	require.NoError(t, err2)
-
+	type beaconResp struct {
+		resp  *eth.FeeRecipientByPubKeyResponse
+		error error
+	}
 	type want struct {
 		pubkey   []byte
 		gaslimit uint64
@@ -922,6 +1041,7 @@ func TestServer_SetGasLimit(t *testing.T) {
 		newGasLimit      uint64
 		proposerSettings *validatorserviceconfig.ProposerSettings
 		w                []want
+		beaconReturn     *beaconResp
 	}{
 		{
 			name:        "update existing gas limit",
@@ -986,7 +1106,25 @@ func TestServer_SetGasLimit(t *testing.T) {
 			},
 		},
 		{
-			name:        "create new gas limit value for nil proposerSettings",
+			name:        "create new gas limit value for nil proposerSettings with beacon node fee recipient",
+			pubkey:      pubkey1,
+			newGasLimit: 7777,
+			// proposerSettings is not set - we need to create proposerSettings and set gaslimit properly
+			w: []want{
+				want{
+					pubkey:   pubkey1,
+					gaslimit: 7777,
+				},
+			},
+			beaconReturn: &beaconResp{
+				resp: &eth.FeeRecipientByPubKeyResponse{
+					FeeRecipient: common.HexToAddress("0x055Fb65722E7b2455012BFEBf6177F1D2e97387").Bytes(),
+				},
+				error: nil,
+			},
+		},
+		{
+			name:        "create new gas limit value for nil proposerSettings without beacon node fee recipient",
 			pubkey:      pubkey1,
 			newGasLimit: 7777,
 			// proposerSettings is not set - we need to create proposerSettings and set gaslimit properly
@@ -996,23 +1134,39 @@ func TestServer_SetGasLimit(t *testing.T) {
 					gaslimit: 7777,
 				},
 			},
+			beaconReturn: &beaconResp{
+				resp:  nil,
+				error: nil,
+			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			feeRecipient := params.BeaconConfig().DefaultFeeRecipient.Hex()
 			vs, err := client.NewValidatorService(ctx, &client.Config{
 				Validator:        &mock.MockValidator{},
 				ProposerSettings: tt.proposerSettings,
 			})
 			require.NoError(t, err)
 			s := &Server{
-				validatorService: vs,
+				validatorService:          vs,
+				beaconNodeValidatorClient: beaconClient,
+			}
+			if tt.beaconReturn != nil {
+				beaconClient.EXPECT().GetFeeRecipientByPubKey(
+					gomock.Any(),
+					gomock.Any(),
+				).Return(tt.beaconReturn.resp, tt.beaconReturn.error)
+				if tt.beaconReturn.resp != nil {
+					feeRecipient = common.BytesToAddress(tt.beaconReturn.resp.FeeRecipient).Hex()
+				}
 			}
 			_, err = s.SetGasLimit(ctx, &ethpbservice.SetGasLimitRequest{Pubkey: tt.pubkey, GasLimit: tt.newGasLimit})
 			require.NoError(t, err)
 			for _, w := range tt.w {
 				assert.Equal(t, w.gaslimit, uint64(s.validatorService.ProposerSettings.ProposeConfig[bytesutil.ToBytes48(w.pubkey)].BuilderConfig.GasLimit))
 			}
+			assert.Equal(t, s.validatorService.ProposerSettings.DefaultConfig.FeeRecipient.Hex(), feeRecipient)
 		})
 	}
 }