diff --git a/CHANGELOG.md b/CHANGELOG.md index c30dcb6ff38..b7f7727e055 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve ### Removed - Removed finalized validator index cache, no longer needed. +- Removed validator queue position log on key reload and wait for activation. ### Fixed diff --git a/proto/engine/v1/electra.go b/proto/engine/v1/electra.go index 628373fe1c7..c6e3af5554a 100644 --- a/proto/engine/v1/electra.go +++ b/proto/engine/v1/electra.go @@ -28,16 +28,16 @@ const ( func (ebe *ExecutionBundleElectra) GetDecodedExecutionRequests() (*ExecutionRequests, error) { requests := &ExecutionRequests{} - var prevTypeNum uint8 + var prevTypeNum *uint8 for i := range ebe.ExecutionRequests { requestType, requestListInSSZBytes, err := decodeExecutionRequest(ebe.ExecutionRequests[i]) if err != nil { return nil, err } - if prevTypeNum > requestType { - return nil, errors.New("invalid execution request type order, requests should be in sorted order") + if prevTypeNum != nil && *prevTypeNum >= requestType { + return nil, errors.New("invalid execution request type order or duplicate requests, requests should be in sorted order and unique") } - prevTypeNum = requestType + prevTypeNum = &requestType switch requestType { case DepositRequestType: drs, err := unmarshalDeposits(requestListInSSZBytes) diff --git a/proto/engine/v1/electra_test.go b/proto/engine/v1/electra_test.go index e6aa2c783d1..bab9f3912b0 100644 --- a/proto/engine/v1/electra_test.go +++ b/proto/engine/v1/electra_test.go @@ -83,6 +83,36 @@ func TestGetDecodedExecutionRequests(t *testing.T) { _, err = ebe.GetDecodedExecutionRequests() require.ErrorContains(t, "invalid execution request, length less than 1", err) }) + t.Run("a duplicate request should fail", func(t *testing.T) { + withdrawalRequestBytes, err := hexutil.Decode("0x6400000000000000000000000000000000000000" + + "6500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040597307000000") + require.NoError(t, err) + withdrawalRequestBytes2, err := hexutil.Decode("0x6400000000000000000000000000000000000000" + + "6500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040597307000000") + require.NoError(t, err) + ebe := &enginev1.ExecutionBundleElectra{ + ExecutionRequests: [][]byte{append([]byte{uint8(enginev1.WithdrawalRequestType)}, withdrawalRequestBytes...), append([]byte{uint8(enginev1.WithdrawalRequestType)}, withdrawalRequestBytes2...)}, + } + _, err = ebe.GetDecodedExecutionRequests() + require.ErrorContains(t, "requests should be in sorted order and unique", err) + }) + t.Run("a duplicate withdrawals ( non 0 request type )request should fail", func(t *testing.T) { + depositRequestBytes, err := hexutil.Decode("0x610000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + + "620000000000000000000000000000000000000000000000000000000000000000" + + "4059730700000063000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + + "00000000000000000000000000000000000000000000000000000000000000000000000000000000") + require.NoError(t, err) + depositRequestBytes2, err := hexutil.Decode("0x610000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + + "620000000000000000000000000000000000000000000000000000000000000000" + + "4059730700000063000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + + "00000000000000000000000000000000000000000000000000000000000000000000000000000000") + require.NoError(t, err) + ebe := &enginev1.ExecutionBundleElectra{ + ExecutionRequests: [][]byte{append([]byte{uint8(enginev1.DepositRequestType)}, depositRequestBytes...), append([]byte{uint8(enginev1.DepositRequestType)}, depositRequestBytes2...)}, + } + _, err = ebe.GetDecodedExecutionRequests() + require.ErrorContains(t, "requests should be in sorted order and unique", err) + }) t.Run("If a request type is provided, but the request list is shorter than the ssz of 1 request we error", func(t *testing.T) { consolidationRequestBytes, err := hexutil.Decode("0x6600000000000000000000000000000000000000" + "670000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + diff --git a/validator/client/BUILD.bazel b/validator/client/BUILD.bazel index 2fceda41cf5..e873a131b3f 100644 --- a/validator/client/BUILD.bazel +++ b/validator/client/BUILD.bazel @@ -42,7 +42,6 @@ go_library( "//consensus-types/blocks:go_default_library", "//consensus-types/interfaces:go_default_library", "//consensus-types/primitives:go_default_library", - "//consensus-types/validator:go_default_library", "//crypto/bls:go_default_library", "//crypto/hash:go_default_library", "//crypto/rand:go_default_library", diff --git a/validator/client/key_reload.go b/validator/client/key_reload.go index 48871787dd1..52ae4006adb 100644 --- a/validator/client/key_reload.go +++ b/validator/client/key_reload.go @@ -16,10 +16,5 @@ func (v *validator) HandleKeyReload(ctx context.Context, currentKeys [][fieldpar return false, err } - valCount, err := v.getValidatorCount(ctx) - if err != nil { - return false, err - } - - return v.checkAndLogValidatorStatus(valCount), nil + return v.checkAndLogValidatorStatus(), nil } diff --git a/validator/client/key_reload_test.go b/validator/client/key_reload_test.go index 41fb4d2466c..f9abd2ffc8b 100644 --- a/validator/client/key_reload_test.go +++ b/validator/client/key_reload_test.go @@ -6,12 +6,10 @@ import ( "github.com/pkg/errors" fieldparams "github.com/prysmaticlabs/prysm/v5/config/fieldparams" - validator2 "github.com/prysmaticlabs/prysm/v5/consensus-types/validator" ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1" "github.com/prysmaticlabs/prysm/v5/testing/assert" "github.com/prysmaticlabs/prysm/v5/testing/require" validatormock "github.com/prysmaticlabs/prysm/v5/testing/validator-mock" - "github.com/prysmaticlabs/prysm/v5/validator/client/iface" "github.com/prysmaticlabs/prysm/v5/validator/client/testutil" logTest "github.com/sirupsen/logrus/hooks/test" "go.uber.org/mock/gomock" @@ -48,11 +46,6 @@ func TestValidator_HandleKeyReload(t *testing.T) { PublicKeys: [][]byte{inactive.pub[:], active.pub[:]}, }, ).Return(resp, nil) - prysmChainClient.EXPECT().ValidatorCount( - gomock.Any(), - "head", - []validator2.Status{validator2.Active}, - ).Return([]iface.ValidatorCount{}, nil) anyActive, err := v.HandleKeyReload(context.Background(), [][fieldparams.BLSPubkeyLength]byte{inactive.pub, active.pub}) require.NoError(t, err) @@ -85,11 +78,6 @@ func TestValidator_HandleKeyReload(t *testing.T) { PublicKeys: [][]byte{kp.pub[:]}, }, ).Return(resp, nil) - prysmChainClient.EXPECT().ValidatorCount( - gomock.Any(), - "head", - []validator2.Status{validator2.Active}, - ).Return([]iface.ValidatorCount{}, nil) anyActive, err := v.HandleKeyReload(context.Background(), [][fieldparams.BLSPubkeyLength]byte{kp.pub}) require.NoError(t, err) diff --git a/validator/client/validator.go b/validator/client/validator.go index 45e67a43fd0..53628e99af1 100644 --- a/validator/client/validator.go +++ b/validator/client/validator.go @@ -10,7 +10,6 @@ import ( "encoding/json" "fmt" "io" - "math" "strconv" "strings" "sync" @@ -349,9 +348,9 @@ func (v *validator) WaitForSync(ctx context.Context) error { } } -func (v *validator) checkAndLogValidatorStatus(activeValCount int64) bool { +func (v *validator) checkAndLogValidatorStatus() bool { nonexistentIndex := primitives.ValidatorIndex(^uint64(0)) - var validatorActivated bool + var someAreActive bool for _, s := range v.pubkeyToStatus { fields := logrus.Fields{ "pubkey": fmt.Sprintf("%#x", bytesutil.Trunc(s.publicKey)), @@ -369,29 +368,11 @@ func (v *validator) checkAndLogValidatorStatus(activeValCount int64) bool { case ethpb.ValidatorStatus_UNKNOWN_STATUS: log.Info("Waiting for deposit to be observed by beacon node") case ethpb.ValidatorStatus_DEPOSITED: - if s.status.PositionInActivationQueue != 0 { - log.WithField( - "positionInActivationQueue", s.status.PositionInActivationQueue, - ).Info("Deposit processed, entering activation queue after finalization") - } + log.Info("Validator deposited, entering activation queue after finalization") case ethpb.ValidatorStatus_PENDING: - if activeValCount >= 0 && s.status.ActivationEpoch == params.BeaconConfig().FarFutureEpoch { - activationsPerEpoch := - uint64(math.Max(float64(params.BeaconConfig().MinPerEpochChurnLimit), float64(uint64(activeValCount)/params.BeaconConfig().ChurnLimitQuotient))) - secondsPerEpoch := uint64(params.BeaconConfig().SlotsPerEpoch.Mul(params.BeaconConfig().SecondsPerSlot)) - expectedWaitingTime := - time.Duration((s.status.PositionInActivationQueue+activationsPerEpoch)/activationsPerEpoch*secondsPerEpoch) * time.Second - log.WithFields(logrus.Fields{ - "positionInActivationQueue": s.status.PositionInActivationQueue, - "expectedWaitingTime": expectedWaitingTime.String(), - }).Info("Waiting to be assigned activation epoch") - } else if s.status.ActivationEpoch != params.BeaconConfig().FarFutureEpoch { - log.WithFields(logrus.Fields{ - "activationEpoch": s.status.ActivationEpoch, - }).Info("Waiting for activation") - } + log.Info("Waiting for activation... Check validator queue status in a block explorer") case ethpb.ValidatorStatus_ACTIVE, ethpb.ValidatorStatus_EXITING: - validatorActivated = true + someAreActive = true log.WithFields(logrus.Fields{ "index": s.index, }).Info("Validator activated") @@ -401,11 +382,11 @@ func (v *validator) checkAndLogValidatorStatus(activeValCount int64) bool { log.Warn("Invalid Eth1 deposit") default: log.WithFields(logrus.Fields{ - "activationEpoch": s.status.ActivationEpoch, + "status": s.status.Status.String(), }).Info("Validator status") } } - return validatorActivated + return someAreActive } // CanonicalHeadSlot returns the slot of canonical block currently found in the diff --git a/validator/client/validator_test.go b/validator/client/validator_test.go index 90ecee079d2..ec2189ee36e 100644 --- a/validator/client/validator_test.go +++ b/validator/client/validator_test.go @@ -806,7 +806,7 @@ func TestCheckAndLogValidatorStatus_OK(t *testing.T) { PositionInActivationQueue: 30, }, }, - log: "Deposit processed, entering activation queue after finalization\" positionInActivationQueue=30 prefix=client pubkey=0x000000000000 status=DEPOSITED validatorIndex=30", + log: "Validator deposited, entering activation queue after finalization\" prefix=client pubkey=0x000000000000 status=DEPOSITED validatorIndex=30", active: false, }, { @@ -820,21 +820,7 @@ func TestCheckAndLogValidatorStatus_OK(t *testing.T) { PositionInActivationQueue: 6, }, }, - log: "Waiting to be assigned activation epoch\" expectedWaitingTime=12m48s positionInActivationQueue=6 prefix=client pubkey=0x000000000000 status=PENDING validatorIndex=50", - active: false, - }, - { - name: "PENDING", - status: &validatorStatus{ - publicKey: pubKeys[0], - index: 89, - status: ðpb.ValidatorStatusResponse{ - Status: ethpb.ValidatorStatus_PENDING, - ActivationEpoch: 60, - PositionInActivationQueue: 5, - }, - }, - log: "Waiting for activation\" activationEpoch=60 prefix=client pubkey=0x000000000000 status=PENDING validatorIndex=89", + log: "Waiting for activation... Check validator queue status in a block explorer\" prefix=client pubkey=0x000000000000 status=PENDING validatorIndex=50", active: false, }, { @@ -889,7 +875,7 @@ func TestCheckAndLogValidatorStatus_OK(t *testing.T) { pubkeyToStatus: make(map[[48]byte]*validatorStatus), } v.pubkeyToStatus[bytesutil.ToBytes48(test.status.publicKey)] = test.status - active := v.checkAndLogValidatorStatus(100) + active := v.checkAndLogValidatorStatus() require.Equal(t, test.active, active) if test.log != "" { require.LogsContain(t, hook, test.log) diff --git a/validator/client/wait_for_activation.go b/validator/client/wait_for_activation.go index 3f25ff461f4..c40a4a0d811 100644 --- a/validator/client/wait_for_activation.go +++ b/validator/client/wait_for_activation.go @@ -6,12 +6,10 @@ import ( "github.com/pkg/errors" fieldparams "github.com/prysmaticlabs/prysm/v5/config/fieldparams" - validator2 "github.com/prysmaticlabs/prysm/v5/consensus-types/validator" "github.com/prysmaticlabs/prysm/v5/math" "github.com/prysmaticlabs/prysm/v5/monitoring/tracing" "github.com/prysmaticlabs/prysm/v5/monitoring/tracing/trace" "github.com/prysmaticlabs/prysm/v5/time/slots" - "github.com/prysmaticlabs/prysm/v5/validator/client/iface" octrace "go.opentelemetry.io/otel/trace" ) @@ -61,14 +59,8 @@ func (v *validator) internalWaitForActivation(ctx context.Context, accountsChang return v.retryWaitForActivation(ctx, span, err, "Connection broken while waiting for activation. Reconnecting...", accountsChangedChan) } - // Step 4: Fetch validator count. - valCount, err := v.getValidatorCount(ctx) - if err != nil { - return err - } - - // Step 5: Check and log validator statuses. - someAreActive := v.checkAndLogValidatorStatus(valCount) + // Step 4: Check and log validator statuses. + someAreActive := v.checkAndLogValidatorStatus() if !someAreActive { // Step 6: If no active validators, wait for accounts change, context cancellation, or next epoch. select { @@ -88,22 +80,6 @@ func (v *validator) internalWaitForActivation(ctx context.Context, accountsChang return nil } -// getValidatorCount is an api call to get the current validator count. -// "-1" indicates that validator count endpoint is not supported by the beacon node. -func (v *validator) getValidatorCount(ctx context.Context) (int64, error) { - // TODO: revisit https://github.com/prysmaticlabs/prysm/pull/12471#issuecomment-1568320970 to review if ValidatorCount api can be removed. - - var valCount int64 = -1 - valCounts, err := v.prysmChainClient.ValidatorCount(ctx, "head", []validator2.Status{validator2.Active}) - if err != nil && !errors.Is(err, iface.ErrNotSupported) { - return -1, errors.Wrap(err, "could not get active validator count") - } - if len(valCounts) > 0 { - valCount = int64(valCounts[0].Count) - } - return valCount, nil -} - func (v *validator) retryWaitForActivation(ctx context.Context, span octrace.Span, err error, message string, accountsChangedChan <-chan [][fieldparams.BLSPubkeyLength]byte) error { tracing.AnnotateError(span, err) attempts := activationAttempts(ctx) diff --git a/validator/client/wait_for_activation_test.go b/validator/client/wait_for_activation_test.go index 50f179ae724..05e67eb917b 100644 --- a/validator/client/wait_for_activation_test.go +++ b/validator/client/wait_for_activation_test.go @@ -14,7 +14,6 @@ import ( "github.com/prysmaticlabs/prysm/v5/testing/require" validatormock "github.com/prysmaticlabs/prysm/v5/testing/validator-mock" walletMock "github.com/prysmaticlabs/prysm/v5/validator/accounts/testing" - "github.com/prysmaticlabs/prysm/v5/validator/client/iface" "github.com/prysmaticlabs/prysm/v5/validator/client/testutil" "github.com/prysmaticlabs/prysm/v5/validator/keymanager/derived" constant "github.com/prysmaticlabs/prysm/v5/validator/testing" @@ -46,16 +45,6 @@ func TestWaitActivation_Exiting_OK(t *testing.T) { PublicKeys: [][]byte{kp.pub[:]}, }, ).Return(resp, nil) - prysmChainClient.EXPECT().ValidatorCount( - gomock.Any(), - "head", - gomock.Any(), - ).Return([]iface.ValidatorCount{ - { - Status: "EXITING", - Count: 1, - }, - }, nil).AnyTimes() require.NoError(t, v.WaitForActivation(ctx, nil)) require.Equal(t, 1, len(v.pubkeyToStatus)) @@ -93,16 +82,6 @@ func TestWaitForActivation_RefetchKeys(t *testing.T) { PublicKeys: [][]byte{kp.pub[:]}, }, ).Return(resp, nil) - prysmChainClient.EXPECT().ValidatorCount( - gomock.Any(), - "head", - gomock.Any(), - ).Return([]iface.ValidatorCount{ - { - Status: "ACTIVE", - Count: 1, - }, - }, nil) accountChan := make(chan [][fieldparams.BLSPubkeyLength]byte) sub := km.SubscribeAccountChanges(accountChan) @@ -163,11 +142,6 @@ func TestWaitForActivation_AccountsChanged(t *testing.T) { }, ).Return(activeResp, nil)) - prysmChainClient.EXPECT().ValidatorCount( - gomock.Any(), - "head", - gomock.Any(), - ).Return([]iface.ValidatorCount{}, nil).AnyTimes() chainClient.EXPECT().ChainHead( gomock.Any(), gomock.Any(), @@ -246,11 +220,6 @@ func TestWaitForActivation_AccountsChanged(t *testing.T) { }, ).Return(activeResp, nil)) - prysmChainClient.EXPECT().ValidatorCount( - gomock.Any(), - "head", - gomock.Any(), - ).Return([]iface.ValidatorCount{}, nil).AnyTimes() chainClient.EXPECT().ChainHead( gomock.Any(), gomock.Any(), @@ -295,11 +264,6 @@ func TestWaitForActivation_AttemptsReconnectionOnFailure(t *testing.T) { gomock.Any(), gomock.Any(), ).Return(activeResp, nil)) - prysmChainClient.EXPECT().ValidatorCount( - gomock.Any(), - "head", - gomock.Any(), - ).Return([]iface.ValidatorCount{}, nil).AnyTimes() chainClient.EXPECT().ChainHead( gomock.Any(), gomock.Any(),