From abef94d7addf53df152bc8b8958628c30152cddd Mon Sep 17 00:00:00 2001 From: Potuz Date: Mon, 15 Jan 2024 15:50:33 -0300 Subject: [PATCH] do not check optimistic status if cached attestation (#13462) * do not check optimistic status if cached attestation * Gazelle * Gazelle again * fix nil panics * more nil checks * more nil checks --------- Co-authored-by: terence tsao --- beacon-chain/rpc/core/service.go | 21 ++++---- beacon-chain/rpc/core/validator.go | 10 ++++ beacon-chain/rpc/eth/validator/handlers.go | 4 -- .../rpc/eth/validator/handlers_test.go | 50 +++++++++++-------- .../rpc/prysm/v1alpha1/validator/BUILD.bazel | 5 +- .../rpc/prysm/v1alpha1/validator/attester.go | 6 --- .../validator/attester_mainnet_test.go | 9 ++-- .../prysm/v1alpha1/validator/attester_test.go | 41 ++++++++------- beacon-chain/rpc/service.go | 21 ++++---- 9 files changed, 91 insertions(+), 76 deletions(-) diff --git a/beacon-chain/rpc/core/service.go b/beacon-chain/rpc/core/service.go index aba524899..5a9facb4b 100644 --- a/beacon-chain/rpc/core/service.go +++ b/beacon-chain/rpc/core/service.go @@ -11,14 +11,15 @@ import ( ) type Service struct { - HeadFetcher blockchain.HeadFetcher - FinalizedFetcher blockchain.FinalizationFetcher - GenesisTimeFetcher blockchain.TimeFetcher - SyncChecker sync.Checker - Broadcaster p2p.Broadcaster - SyncCommitteePool synccommittee.Pool - OperationNotifier opfeed.Notifier - AttestationCache *cache.AttestationCache - StateGen stategen.StateManager - P2P p2p.Broadcaster + HeadFetcher blockchain.HeadFetcher + FinalizedFetcher blockchain.FinalizationFetcher + GenesisTimeFetcher blockchain.TimeFetcher + SyncChecker sync.Checker + Broadcaster p2p.Broadcaster + SyncCommitteePool synccommittee.Pool + OperationNotifier opfeed.Notifier + AttestationCache *cache.AttestationCache + StateGen stategen.StateManager + P2P p2p.Broadcaster + OptimisticModeFetcher blockchain.OptimisticModeFetcher } diff --git a/beacon-chain/rpc/core/validator.go b/beacon-chain/rpc/core/validator.go index 12b39bd14..a20f02506 100644 --- a/beacon-chain/rpc/core/validator.go +++ b/beacon-chain/rpc/core/validator.go @@ -32,6 +32,8 @@ import ( "golang.org/x/sync/errgroup" ) +var errOptimisticMode = errors.New("the node is currently optimistic and cannot serve validators") + // AggregateBroadcastFailedError represents an error scenario where // broadcasting an aggregate selection proof failed. type AggregateBroadcastFailedError struct { @@ -368,6 +370,14 @@ func (s *Service) GetAttestationData( }, }, nil } + // cache miss, we need to check for optimistic status before proceeding + optimistic, err := s.OptimisticModeFetcher.IsOptimistic(ctx) + if err != nil { + return nil, &RpcError{Reason: Internal, Err: err} + } + if optimistic { + return nil, &RpcError{Reason: Unavailable, Err: errOptimisticMode} + } headRoot, err := s.HeadFetcher.HeadRoot(ctx) if err != nil { diff --git a/beacon-chain/rpc/eth/validator/handlers.go b/beacon-chain/rpc/eth/validator/handlers.go index bdb06e70e..cecd6b8f3 100644 --- a/beacon-chain/rpc/eth/validator/handlers.go +++ b/beacon-chain/rpc/eth/validator/handlers.go @@ -388,10 +388,6 @@ func (s *Server) GetAttestationData(w http.ResponseWriter, r *http.Request) { return } - if isOptimistic, err := shared.IsOptimistic(ctx, w, s.OptimisticModeFetcher); isOptimistic || err != nil { - return - } - _, slot, ok := shared.UintFromQuery(w, r, "slot", true) if !ok { return diff --git a/beacon-chain/rpc/eth/validator/handlers_test.go b/beacon-chain/rpc/eth/validator/handlers_test.go index 9178c520b..1cfeb2350 100644 --- a/beacon-chain/rpc/eth/validator/handlers_test.go +++ b/beacon-chain/rpc/eth/validator/handlers_test.go @@ -831,10 +831,11 @@ func TestGetAttestationData(t *testing.T) { TimeFetcher: chain, OptimisticModeFetcher: chain, CoreService: &core.Service{ - HeadFetcher: chain, - GenesisTimeFetcher: chain, - FinalizedFetcher: chain, - AttestationCache: cache.NewAttestationCache(), + HeadFetcher: chain, + GenesisTimeFetcher: chain, + FinalizedFetcher: chain, + AttestationCache: cache.NewAttestationCache(), + OptimisticModeFetcher: chain, }, } @@ -914,10 +915,11 @@ func TestGetAttestationData(t *testing.T) { TimeFetcher: chain, OptimisticModeFetcher: chain, CoreService: &core.Service{ - AttestationCache: cache.NewAttestationCache(), - GenesisTimeFetcher: chain, - HeadFetcher: chain, - FinalizedFetcher: chain, + AttestationCache: cache.NewAttestationCache(), + GenesisTimeFetcher: chain, + HeadFetcher: chain, + FinalizedFetcher: chain, + OptimisticModeFetcher: chain, }, } @@ -959,8 +961,9 @@ func TestGetAttestationData(t *testing.T) { TimeFetcher: chain, OptimisticModeFetcher: chain, CoreService: &core.Service{ - GenesisTimeFetcher: chain, - FinalizedFetcher: chain, + GenesisTimeFetcher: chain, + OptimisticModeFetcher: chain, + FinalizedFetcher: chain, }, } @@ -1017,10 +1020,11 @@ func TestGetAttestationData(t *testing.T) { TimeFetcher: chain, OptimisticModeFetcher: chain, CoreService: &core.Service{ - HeadFetcher: chain, - GenesisTimeFetcher: chain, - StateGen: stategen.New(db, doublylinkedtree.New()), - FinalizedFetcher: chain, + HeadFetcher: chain, + GenesisTimeFetcher: chain, + OptimisticModeFetcher: chain, + StateGen: stategen.New(db, doublylinkedtree.New()), + FinalizedFetcher: chain, }, } @@ -1065,10 +1069,11 @@ func TestGetAttestationData(t *testing.T) { TimeFetcher: chain, OptimisticModeFetcher: chain, CoreService: &core.Service{ - AttestationCache: cache.NewAttestationCache(), - HeadFetcher: chain, - GenesisTimeFetcher: chain, - FinalizedFetcher: chain, + AttestationCache: cache.NewAttestationCache(), + OptimisticModeFetcher: chain, + HeadFetcher: chain, + GenesisTimeFetcher: chain, + FinalizedFetcher: chain, }, } @@ -1151,10 +1156,11 @@ func TestGetAttestationData(t *testing.T) { TimeFetcher: chain, OptimisticModeFetcher: chain, CoreService: &core.Service{ - AttestationCache: cache.NewAttestationCache(), - HeadFetcher: chain, - GenesisTimeFetcher: chain, - FinalizedFetcher: chain, + AttestationCache: cache.NewAttestationCache(), + OptimisticModeFetcher: chain, + HeadFetcher: chain, + GenesisTimeFetcher: chain, + FinalizedFetcher: chain, }, } diff --git a/beacon-chain/rpc/prysm/v1alpha1/validator/BUILD.bazel b/beacon-chain/rpc/prysm/v1alpha1/validator/BUILD.bazel index 471320cf2..e9aefbeed 100644 --- a/beacon-chain/rpc/prysm/v1alpha1/validator/BUILD.bazel +++ b/beacon-chain/rpc/prysm/v1alpha1/validator/BUILD.bazel @@ -38,7 +38,6 @@ go_library( "//beacon-chain/builder:go_default_library", "//beacon-chain/cache:go_default_library", "//beacon-chain/cache/depositcache:go_default_library", - "//beacon-chain/core/altair:go_default_library", "//beacon-chain/core/blocks:go_default_library", "//beacon-chain/core/feed:go_default_library", "//beacon-chain/core/feed/block:go_default_library", @@ -66,7 +65,6 @@ go_library( "//config/features:go_default_library", "//config/fieldparams:go_default_library", "//config/params:go_default_library", - "//consensus-types:go_default_library", "//consensus-types/blocks:go_default_library", "//consensus-types/interfaces:go_default_library", "//consensus-types/payload-attribute:go_default_library", @@ -91,13 +89,13 @@ go_library( "//time/slots:go_default_library", "@com_github_ethereum_go_ethereum//common:go_default_library", "@com_github_ethereum_go_ethereum//common/hexutil:go_default_library", + "@com_github_golang_protobuf//ptypes/empty", "@com_github_pkg_errors//:go_default_library", "@com_github_prometheus_client_golang//prometheus:go_default_library", "@com_github_prometheus_client_golang//prometheus/promauto:go_default_library", "@com_github_prysmaticlabs_fastssz//:go_default_library", "@com_github_prysmaticlabs_go_bitfield//:go_default_library", "@com_github_sirupsen_logrus//:go_default_library", - "@io_bazel_rules_go//proto/wkt:empty_go_proto", "@io_opencensus_go//trace:go_default_library", "@org_golang_google_grpc//codes:go_default_library", "@org_golang_google_grpc//status:go_default_library", @@ -141,6 +139,7 @@ common_deps = [ "//beacon-chain/sync/initial-sync/testing:go_default_library", "//config/fieldparams:go_default_library", "//config/params:go_default_library", + "//consensus-types:go_default_library", "//consensus-types/blocks:go_default_library", "//consensus-types/interfaces:go_default_library", "//consensus-types/primitives:go_default_library", diff --git a/beacon-chain/rpc/prysm/v1alpha1/validator/attester.go b/beacon-chain/rpc/prysm/v1alpha1/validator/attester.go index ea97088b1..7e402d0b6 100644 --- a/beacon-chain/rpc/prysm/v1alpha1/validator/attester.go +++ b/beacon-chain/rpc/prysm/v1alpha1/validator/attester.go @@ -31,12 +31,6 @@ func (vs *Server) GetAttestationData(ctx context.Context, req *ethpb.Attestation if vs.SyncChecker.Syncing() { return nil, status.Errorf(codes.Unavailable, "Syncing to latest head, not ready to respond") } - - // An optimistic validator MUST NOT participate in attestation. (i.e., sign across the DOMAIN_BEACON_ATTESTER, DOMAIN_SELECTION_PROOF or DOMAIN_AGGREGATE_AND_PROOF domains). - if err := vs.optimisticStatus(ctx); err != nil { - return nil, err - } - res, err := vs.CoreService.GetAttestationData(ctx, req) if err != nil { return nil, status.Errorf(core.ErrorReasonToGRPC(err.Reason), "Could not get attestation data: %v", err.Err) diff --git a/beacon-chain/rpc/prysm/v1alpha1/validator/attester_mainnet_test.go b/beacon-chain/rpc/prysm/v1alpha1/validator/attester_mainnet_test.go index d4e13e0ee..be6495386 100644 --- a/beacon-chain/rpc/prysm/v1alpha1/validator/attester_mainnet_test.go +++ b/beacon-chain/rpc/prysm/v1alpha1/validator/attester_mainnet_test.go @@ -61,10 +61,11 @@ func TestAttestationDataAtSlot_HandlesFarAwayJustifiedEpoch(t *testing.T) { OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, TimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, CoreService: &core.Service{ - AttestationCache: cache.NewAttestationCache(), - HeadFetcher: &mock.ChainService{TargetRoot: blockRoot, Root: blockRoot[:]}, - GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, - FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + AttestationCache: cache.NewAttestationCache(), + HeadFetcher: &mock.ChainService{TargetRoot: blockRoot, Root: blockRoot[:]}, + GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, + FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, }, } diff --git a/beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go b/beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go index e1a5cfd5c..9a238addc 100644 --- a/beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go +++ b/beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go @@ -116,8 +116,9 @@ func TestGetAttestationData_OK(t *testing.T) { GenesisTimeFetcher: &mock.ChainService{ Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second), }, - FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, - AttestationCache: cache.NewAttestationCache(), + FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + AttestationCache: cache.NewAttestationCache(), + OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, }, } @@ -176,7 +177,8 @@ func BenchmarkGetAttestationDataConcurrent(b *testing.B) { GenesisTimeFetcher: &mock.ChainService{ Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second), }, - FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, + FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, }, } @@ -222,9 +224,10 @@ func TestGetAttestationData_Optimistic(t *testing.T) { OptimisticModeFetcher: &mock.ChainService{Optimistic: true}, TimeFetcher: &mock.ChainService{Genesis: time.Now()}, CoreService: &core.Service{ - GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now()}, - HeadFetcher: &mock.ChainService{}, - AttestationCache: cache.NewAttestationCache(), + GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now()}, + HeadFetcher: &mock.ChainService{}, + AttestationCache: cache.NewAttestationCache(), + OptimisticModeFetcher: &mock.ChainService{Optimistic: true}, }, } _, err := as.GetAttestationData(context.Background(), ðpb.AttestationDataRequest{}) @@ -240,10 +243,11 @@ func TestGetAttestationData_Optimistic(t *testing.T) { OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, TimeFetcher: &mock.ChainService{Genesis: time.Now()}, CoreService: &core.Service{ - AttestationCache: cache.NewAttestationCache(), - GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now()}, - HeadFetcher: &mock.ChainService{Optimistic: false, State: beaconState}, - FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: ðpb.Checkpoint{}}, + AttestationCache: cache.NewAttestationCache(), + GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now()}, + HeadFetcher: &mock.ChainService{Optimistic: false, State: beaconState}, + FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: ðpb.Checkpoint{}}, + OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, }, } _, err = as.GetAttestationData(context.Background(), ðpb.AttestationDataRequest{}) @@ -260,7 +264,8 @@ func TestServer_GetAttestationData_InvalidRequestSlot(t *testing.T) { OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, TimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, CoreService: &core.Service{ - GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, + GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, + OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, }, } @@ -301,10 +306,11 @@ func TestServer_GetAttestationData_RequestSlotIsDifferentThanCurrentSlot(t *test OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, TimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, CoreService: &core.Service{ - HeadFetcher: &mock.ChainService{TargetRoot: blockRoot2, Root: blockRoot[:]}, - GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, - StateGen: stategen.New(db, doublylinkedtree.New()), - FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + HeadFetcher: &mock.ChainService{TargetRoot: blockRoot2, Root: blockRoot[:]}, + GenesisTimeFetcher: &mock.ChainService{Genesis: time.Now().Add(time.Duration(-1*offset) * time.Second)}, + StateGen: stategen.New(db, doublylinkedtree.New()), + FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, }, } util.SaveBlock(t, ctx, db, block) @@ -346,8 +352,9 @@ func TestGetAttestationData_SucceedsInFirstEpoch(t *testing.T) { HeadFetcher: &mock.ChainService{ TargetRoot: targetRoot, Root: blockRoot[:], }, - GenesisTimeFetcher: &mock.ChainService{Genesis: prysmTime.Now().Add(time.Duration(-1*offset) * time.Second)}, - FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + GenesisTimeFetcher: &mock.ChainService{Genesis: prysmTime.Now().Add(time.Duration(-1*offset) * time.Second)}, + FinalizedFetcher: &mock.ChainService{CurrentJustifiedCheckPoint: justifiedCheckpoint}, + OptimisticModeFetcher: &mock.ChainService{Optimistic: false}, }, } diff --git a/beacon-chain/rpc/service.go b/beacon-chain/rpc/service.go index 524e29780..1a7baeefc 100644 --- a/beacon-chain/rpc/service.go +++ b/beacon-chain/rpc/service.go @@ -359,16 +359,17 @@ func (s *Service) Start() { }) coreService := &core.Service{ - HeadFetcher: s.cfg.HeadFetcher, - GenesisTimeFetcher: s.cfg.GenesisTimeFetcher, - SyncChecker: s.cfg.SyncService, - Broadcaster: s.cfg.Broadcaster, - SyncCommitteePool: s.cfg.SyncCommitteeObjectPool, - OperationNotifier: s.cfg.OperationNotifier, - AttestationCache: cache.NewAttestationCache(), - StateGen: s.cfg.StateGen, - P2P: s.cfg.Broadcaster, - FinalizedFetcher: s.cfg.FinalizationFetcher, + HeadFetcher: s.cfg.HeadFetcher, + GenesisTimeFetcher: s.cfg.GenesisTimeFetcher, + SyncChecker: s.cfg.SyncService, + Broadcaster: s.cfg.Broadcaster, + SyncCommitteePool: s.cfg.SyncCommitteeObjectPool, + OperationNotifier: s.cfg.OperationNotifier, + AttestationCache: cache.NewAttestationCache(), + StateGen: s.cfg.StateGen, + P2P: s.cfg.Broadcaster, + FinalizedFetcher: s.cfg.FinalizationFetcher, + OptimisticModeFetcher: s.cfg.OptimisticModeFetcher, } validatorServer := &validatorv1alpha1.Server{