Use a cache of one entry to build attestation #13300

terencechain · 2023-12-08T01:11:15Z

Alternate Implementation to #13286 and #13256

This proposal introduces a modified caching strategy for single attestation entries, implementing stricter controls on the RPC behavior.

Check if the current slot matches the request slot. If not, the service is not provided (New behavior).
If a cached entry exists for the same slot, the committee index is appended to it.
If no relevant cached entry is present, retrieve the necessary data from the fork choice once, and cache it, replacing the existing cached attestation.

potuz · 2023-12-08T17:11:11Z

beacon-chain/cache/attestation_data.go

-	minDelay    = float64(10)        // 10 nanoseconds
-	maxDelay    = float64(100000000) // 0.1 second
-	delayFactor = 1.1
+const maxSize = 4


This was from the previous cache I think

potuz · 2023-12-08T17:18:05Z

beacon-chain/cache/attestation_data.go

-	cache      *cache.FIFO
-	lock       sync.RWMutex
-	inProgress map[string]bool
+	a    *ethpb.AttestationData


why would you keep the full attestation data with the given committee index ?

potuz · 2023-12-08T17:20:53Z

beacon-chain/cache/attestation_data.go

 		attestationCacheHit.Inc()
-		return ethpb.CopyAttestationData(item.(*attestationReqResWrapper).res), nil
+		return ethpb.CopyAttestationData(c.a), nil


This will return the wrong committee index for some validators.

potuz · 2023-12-08T17:45:35Z

beacon-chain/rpc/core/validator.go

+	isCurrentOrPastSlot := s.HeadFetcher.HeadSlot() == req.Slot || currentUnixTime >= timeThreshold
+
+	if isCurrentOrPastSlot {
+		err := s.AttestationCache.Put(ctx, res)


If someone requests an attestation for the previous slot you will overwrite the cache, why is this? Say you hold 1000 keys, and one of them for some reason (for example is run on a different VC on a different network and that one lags) requests for the previous slot. If you overwrite the cache then you'll fail on any of the other 1000 keys. I think we shouldn't even serve these attestations.

Also notice that there is another bug here: Suppose that the requested slot is from 5 slots ago, the first branch of || will be false but the second will be true. The attestation here will be for the wrong headBlock!

I've updated the design to ensure RPC only serves current slot attestation, it should address most of the concerns. Let's wait a few days and gather more feedback from the team

potuz · 2023-12-12T13:38:27Z

beacon-chain/cache/attestation_data.go

+	TargetCheckpoint *ethpb.Checkpoint
+	SourceCheckpoint *ethpb.Checkpoint


Why are these pointers?

I don't want to copy the whole protobuf object; there are many irrelevant fields.
Instead of Checkpoint, I'll switch to the root and epoch

You can use forkchoicetypes.Checkpoint for this

potuz · 2023-12-12T13:42:45Z

beacon-chain/cache/error.go

+	// ErrAlreadyInProgress appears when attempting to mark a cache as in progress while it is
+	// already in progress. The client should handle this error and wait for the in progress
+	// data to resolve via Get.
+	ErrAlreadyInProgress = errors.New("already in progress")


Is this still used? ah perhaps in another cache?

nisdas · 2023-12-13T08:12:05Z

beacon-chain/rpc/core/validator.go

-			targetRoot = headRoot
-		}
+	justifiedCheckpoint := s.FinalizedFetcher.CurrentJustifiedCheckpt()
+	if err = s.AttestationCache.Put(ctx, &cache.AttestationConsensusData{


Could possibly be contention here with 100+ routines trying to put this in the cache if this beacon node hosts many validators.

nisdas · 2023-12-14T04:26:59Z

beacon-chain/rpc/core/validator.go

 	if err != nil {
-		return nil, &RpcError{Reason: Internal, Err: errors.Errorf("could not retrieve head state: %v", err)}
+		return nil, &RpcError{Reason: Internal, Err: errors.Errorf("could not retrieve data from attestation cache: %v", err)}


If there is an error, we need to unlock the attestation cache, otherwise it remains permanently locked.

That function never errors, we should change the signature.

potuz · 2023-12-15T14:03:27Z

beacon-chain/cache/attestation_data.go

-	attestationCacheMiss.Inc()
-	return nil, nil
+// 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) {


Why the error signature here?

potuz · 2023-12-15T14:04:44Z

beacon-chain/cache/attestation_data.go

-	lock       sync.RWMutex
-	inProgress map[string]bool
+	a    *AttestationConsensusData
+	lock sync.RWMutex


no need to have this named, see also below.

potuz · 2023-12-15T14:05:01Z

beacon-chain/cache/attestation_data.go

-	}
-	return reqToKey(w.req)
+// Unlock unlocks the cache for writing.
+func (c *AttestationCache) Unlock() {


this is not needed if you not name the lock

potuz · 2023-12-15T14:05:16Z

beacon-chain/cache/attestation_data.go

-func reqToKey(req *ethpb.AttestationDataRequest) (string, error) {
-	return fmt.Sprintf("%d", req.Slot), nil
+// RLock locks the cache for reading.
+func (c *AttestationCache) RLock() {


this is not needed if you don' t name the lock.

potuz · 2023-12-15T14:08:37Z

beacon-chain/rpc/core/validator.go

-			return res, nil
-		}
-		return nil, &RpcError{Reason: Internal, Err: errors.Errorf("could not mark attestation as in-progress: %v", err)}
+	if req.Slot != s.GenesisTimeFetcher.CurrentSlot() {


Please move this before the call to ValidateAttestationTime that does a bunch of silly computations.

potuz · 2023-12-15T14:09:11Z

beacon-chain/rpc/core/validator.go

 	if err != nil {
-		return nil, &RpcError{Reason: Internal, Err: errors.Errorf("could not retrieve head state: %v", err)}
+		return nil, &RpcError{Reason: Internal, Err: errors.Errorf("could not retrieve data from attestation cache: %v", err)}


That function never errors, we should change the signature.

beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go

Co-authored-by: Potuz <potuz@prysmaticlabs.com>

terencechain requested a review from a team as a code owner December 8, 2023 01:11

terencechain requested review from kasey, nalepae and james-prysm December 8, 2023 01:11

potuz reviewed Dec 8, 2023

View reviewed changes

potuz requested changes Dec 8, 2023

View reviewed changes

potuz reviewed Dec 12, 2023

View reviewed changes

terencechain force-pushed the attester-use-fc-2 branch from 96549ca to 23fa758 Compare December 13, 2023 01:56

nisdas reviewed Dec 13, 2023

View reviewed changes

terencechain force-pushed the attester-use-fc-2 branch 2 times, most recently from 37518b2 to 30afc71 Compare December 13, 2023 16:55

nisdas reviewed Dec 14, 2023

View reviewed changes

nisdas previously approved these changes Dec 14, 2023

View reviewed changes

terencechain force-pushed the attester-use-fc-2 branch from a8a6ebb to 8512108 Compare December 14, 2023 22:40

potuz reviewed Dec 15, 2023

View reviewed changes

terencechain dismissed nisdas’s stale review via 4a1deb5 December 15, 2023 21:49

terencechain force-pushed the attester-use-fc-2 branch 4 times, most recently from 575aacf to e6e73fe Compare December 18, 2023 08:29

terencechain added 10 commits December 18, 2023 22:34

Use a cache of one entry to build attestation

2258bb8

Gazelle

dfa6365

Enforce on RPC side

d305ed1

Rm unused var

1e2428f

Potuz feedback, dont use pointer

c90ac15

Fix tests

a942eca

Init fetcher

ce68534

Add in-progress

a3eefce

Add back missing lock

59075b7

Potuz feedback

466ca55

terencechain force-pushed the attester-use-fc-2 branch from e6e73fe to 466ca55 Compare December 18, 2023 14:34

potuz previously approved these changes Dec 18, 2023

View reviewed changes

beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go Outdated Show resolved Hide resolved

Update beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go

f059cb7

Co-authored-by: Potuz <potuz@prysmaticlabs.com>

terencechain dismissed potuz’s stale review via f059cb7 December 18, 2023 15:57

terencechain enabled auto-merge December 18, 2023 15:58

potuz approved these changes Dec 18, 2023

View reviewed changes

terencechain added this pull request to the merge queue Dec 18, 2023

Merged via the queue into develop with commit 0eff83c Dec 18, 2023
17 checks passed

terencechain deleted the attester-use-fc-2 branch December 18, 2023 16:28

terencechain mentioned this pull request Jan 12, 2024

Revert 13300 and use fork choice store #13463

Closed

terencechain mentioned this pull request Mar 9, 2024

Use justified checkpoint from head state to build attestation #13703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a cache of one entry to build attestation #13300

Use a cache of one entry to build attestation #13300

terencechain commented Dec 8, 2023 •

edited

Loading

potuz Dec 8, 2023

potuz Dec 8, 2023

potuz Dec 8, 2023

potuz Dec 8, 2023

terencechain Dec 8, 2023

potuz Dec 12, 2023

terencechain Dec 13, 2023

potuz Dec 13, 2023

potuz Dec 12, 2023

nisdas Dec 13, 2023

nisdas Dec 14, 2023

potuz Dec 15, 2023

potuz Dec 15, 2023

potuz Dec 15, 2023

potuz Dec 15, 2023

potuz Dec 15, 2023

potuz Dec 15, 2023

potuz Dec 15, 2023

		TargetCheckpoint *ethpb.Checkpoint
		SourceCheckpoint *ethpb.Checkpoint

Use a cache of one entry to build attestation #13300

Use a cache of one entry to build attestation #13300

Conversation

terencechain commented Dec 8, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Dec 8, 2023 •

edited

Loading