Potuz feedback

prysmaticlabs · Dec 15, 2023 · 575aacf · 575aacf
1 parent f134fec
commit 575aacf
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 50 deletions.
diff --git a/beacon-chain/cache/attestation_data.go b/beacon-chain/cache/attestation_data.go
@@ -1,7 +1,6 @@
 package cache
 
 import (
-	"context"
 	"errors"
 	"sync"
 
@@ -18,8 +17,8 @@ type AttestationConsensusData struct {
 
 // AttestationCache stores cached results of AttestationData requests.
 type AttestationCache struct {
-	a    *AttestationConsensusData
-	lock sync.RWMutex
+	a *AttestationConsensusData
+	sync.RWMutex
 }
 
 // NewAttestationCache creates a new instance of AttestationCache.
@@ -28,35 +27,15 @@ func NewAttestationCache() *AttestationCache {
 }
 
 // Get retrieves cached attestation data, recording a cache hit or miss. This method is lock free.
-func (c *AttestationCache) Get(ctx context.Context) (*AttestationConsensusData, error) {
-	return c.a, nil
+func (c *AttestationCache) Get() *AttestationConsensusData {
+	return c.a
 }
 
 // Put adds a response to the cache. This method is lock free.
-func (c *AttestationCache) Put(ctx context.Context, a *AttestationConsensusData) error {
+func (c *AttestationCache) Put(a *AttestationConsensusData) error {
 	if a == nil {
 		return errors.New("attestation cannot be nil")
 	}
 	c.a = a
 	return nil
 }
-
-// Lock locks the cache for writing.
-func (c *AttestationCache) Lock() {
-	c.lock.Lock()
-}
-
-// Unlock unlocks the cache for writing.
-func (c *AttestationCache) Unlock() {
-	c.lock.Unlock()
-}
-
-// RLock locks the cache for reading.
-func (c *AttestationCache) RLock() {
-	c.lock.RLock()
-}
-
-// RUnlock unlocks the cache for reading.
-func (c *AttestationCache) RUnlock() {
-	c.lock.RUnlock()
-}
diff --git a/beacon-chain/cache/attestation_data_test.go b/beacon-chain/cache/attestation_data_test.go
@@ -1,7 +1,6 @@
 package cache_test
 
 import (
-	"context"
 	"testing"
 
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/cache"
@@ -10,11 +9,9 @@ import (
 )
 
 func TestAttestationCache_RoundTrip(t *testing.T) {
-	ctx := context.Background()
 	c := cache.NewAttestationCache()
 
-	a, err := c.Get(ctx)
-	require.NoError(t, err)
+	a := c.Get()
 	require.Nil(t, a)
 
 	insert := &cache.AttestationConsensusData{
@@ -29,11 +26,10 @@ func TestAttestationCache_RoundTrip(t *testing.T) {
 			Root:  [32]byte{5},
 		},
 	}
-	err = c.Put(ctx, insert)
+	err := c.Put(insert)
 	require.NoError(t, err)
 
-	a, err = c.Get(ctx)
-	require.NoError(t, err)
+	a = c.Get()
 	require.Equal(t, insert, a)
 
 	insert = &cache.AttestationConsensusData{
@@ -49,10 +45,9 @@ func TestAttestationCache_RoundTrip(t *testing.T) {
 		},
 	}
 
-	err = c.Put(ctx, insert)
+	err = c.Put(insert)
 	require.NoError(t, err)
 
-	a, err = c.Get(ctx)
-	require.NoError(t, err)
+	a = c.Get()
 	require.Equal(t, insert, a)
 }
diff --git a/beacon-chain/rpc/core/validator.go b/beacon-chain/rpc/core/validator.go
@@ -315,6 +315,9 @@ func (s *Service) AggregatedSigAndAggregationBits(
 func (s *Service) GetAttestationData(
 	ctx context.Context, req *ethpb.AttestationDataRequest,
 ) (*ethpb.AttestationData, *RpcError) {
+	if req.Slot != s.GenesisTimeFetcher.CurrentSlot() {
+		return nil, &RpcError{Reason: BadRequest, Err: errors.Errorf("invalid request: slot %d is not the current slot %d", req.Slot, s.GenesisTimeFetcher.CurrentSlot())}
+	}
 	if err := helpers.ValidateAttestationTime(
 		req.Slot,
 		s.GenesisTimeFetcher.GenesisTime(),
@@ -323,16 +326,8 @@ func (s *Service) GetAttestationData(
 		return nil, &RpcError{Reason: BadRequest, Err: errors.Errorf("invalid request: %v", err)}
 	}
 
-	if req.Slot != s.GenesisTimeFetcher.CurrentSlot() {
-		return nil, &RpcError{Reason: BadRequest, Err: errors.Errorf("invalid request: slot %d is not the current slot %d", req.Slot, s.GenesisTimeFetcher.CurrentSlot())}
-	}
-
 	s.AttestationCache.RLock()
-	res, err := s.AttestationCache.Get(ctx)
-	if err != nil {
-		s.AttestationCache.RUnlock()
-		return nil, &RpcError{Reason: Internal, Err: errors.Errorf("could not retrieve data from attestation cache: %v", err)}
-	}
+	res := s.AttestationCache.Get()
 	if res != nil && res.Slot == req.Slot {
 		s.AttestationCache.RUnlock()
 		return &ethpb.AttestationData{
@@ -357,10 +352,7 @@ func (s *Service) GetAttestationData(
 	// We check the cache again as in the event there are multiple inflight requests for
 	// the same attestation data, the cache might have been filled while we were waiting
 	// to acquire the lock.
-	res, err = s.AttestationCache.Get(ctx)
-	if err != nil {
-		return nil, &RpcError{Reason: Internal, Err: errors.Errorf("could not retrieve data from attestation cache: %v", err)}
-	}
+	res = s.AttestationCache.Get()
 	if res != nil && res.Slot == req.Slot {
 		return &ethpb.AttestationData{
 			Slot:            res.Slot,
@@ -387,7 +379,7 @@ func (s *Service) GetAttestationData(
 		return nil, &RpcError{Reason: Internal, Err: errors.Wrap(err, "could not get target root")}
 	}
 	justifiedCheckpoint := s.FinalizedFetcher.CurrentJustifiedCheckpt()
-	if err = s.AttestationCache.Put(ctx, &cache.AttestationConsensusData{
+	if err = s.AttestationCache.Put(&cache.AttestationConsensusData{
 		Slot:     req.Slot,
 		HeadRoot: headRoot,
 		Target: forkchoicetypes.Checkpoint{