Redesign of the attestation pool #14324

rkapka · 2024-08-08T15:29:22Z

Idea

The new attestation pool is called an AttestationCache. The cache holds a map[attestation.Id]*attGroup where an attGroup is defined as:

type attGroup struct {
	slot primitives.Slot
	atts []ethpb.Att
}

It groups together all attestations for a single slot. When we add an attestation to the cache by calling Add(), we either create a new group with this attestation (if this is the first attestation for some slot) or two things can happen:

If the attestation is unaggregated, we add its attestation bit to attestation bits of the first attestation in the group.
If the attestation is aggregated, we append it to the group. There should be no redundancy in the list because we ignore redundant aggregates in gossip.

The first bullet point above means that we keep one aggregate attestation to which we keep appending bits as new single-bit attestations arrive. This means that at any point during seconds 0-4 of a slot we will have only one attestation for this slot in the cache. This results in much much less bookeeping and hashing of attestations for comparisons etc.

Impact

I took profiles of the total allocated space over a span of 30 minutes. The first image is from the old design, the second image is from the new one. There's a 87.5% reduction from 20% to 2.5% in the NewId function which hashes attestations.

Open questions

Should we aggregate all aggregates at some point? In the old design we do it every time we save an aggregate.
We can add incoming aggregated atts without checking for redundancy because we will later run MaxCover on all of them (there won't be too many).

rkapka · 2024-08-09T10:42:09Z

proto/prysm/v1alpha1/attestation.go

+// IsNil --
+func (a *Attestation) IsNil() bool {
+	return a == nil ||
+		a.Data == nil ||
+		a.Data.Source == nil ||
+		a.Data.Target == nil ||
+		a.AggregationBits == nil
+}
+
+// IsAggregated --
+func (a *Attestation) IsAggregated() bool {
+	return a.AggregationBits.Count() > 1
+}


I created these two methods because I ran into import cycles when trying to use existing helpers. That aside, I think having such methods is nicer.

rkapka · 2024-08-09T10:51:19Z

beacon-chain/cache/attestation_data.go

-type AttestationCache struct {
-	a *AttestationConsensusData
+// AttestationDataCache stores cached results of AttestationData requests.
+type AttestationDataCache struct {


I renamed this because I wanted the new pool to be called AttestationCache

rkapka · 2024-08-09T10:51:44Z

beacon-chain/core/helpers/attestation.go

@@ -70,12 +70,6 @@ func IsAggregator(committeeCount uint64, slotSig []byte) (bool, error) {
 	return binary.LittleEndian.Uint64(b[:8])%modulo == 0, nil
 }

-// IsAggregated returns true if the attestation is an aggregated attestation,
-// false otherwise.
-func IsAggregated(attestation ethpb.Att) bool {


Moved to a method defined on the attestation interface

rkapka · 2024-08-09T10:52:24Z

beacon-chain/operations/attestations/forkchoice/forkchoice.go

@@ -0,0 +1,87 @@
+package forkchoice


Moved to a separate package to avoid dependency cyles

rkapka · 2024-08-09T10:52:51Z

beacon-chain/operations/attestations/forkchoice/forkchoice.go

+)
+
+// Attestations --
+type Attestations struct {


I created a new type to hold forkchoice attestations to be able to reuse it between the old and new pools

rkapka · 2024-08-09T10:54:19Z

beacon-chain/operations/attestations/metrics.go

+	attCount = promauto.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "attestations_in_pool_total",
+			Help: "The number of attestations in the pool.",
+		},
+	)
+	expiredAtts = promauto.NewCounter(prometheus.CounterOpts{
+		Name: "expired_atts_total",
+		Help: "The number of expired and deleted attestations in the pool.",
+	})


There is no longer a need to have separate metrics for unaggregated and aggregated atts, so I created new metrics for the new design

rkapka · 2024-08-09T11:05:58Z

beacon-chain/operations/attestations/prune_expired.go

+				log.WithError(err).Error("Could not get expiry slot")
+				continue
+			}
+			numExpired := s.cfg.Cache.PruneBefore(expirySlot)


Instead of checking each attestation for expiry, we pass the expiry slot to the cache. That way we don't need to hash anything.

rkapka · 2024-08-09T11:08:05Z

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

-		if err := vs.AttPool.SaveUnaggregatedAttestation(attCopy); err != nil {
-			log.WithError(err).Error("Could not save unaggregated attestation")
-			return
+	if features.Get().EnableExperimentalAttestationPool {


I don't think we should spin up a goroutine here as is the case with the old design. Adding an attestation is very fast.

rkapka · 2024-08-09T11:08:56Z

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

@@ -409,15 +411,15 @@ func (a proposerAtts) dedup() (proposerAtts, error) {
 }

 // This filters the input attestations to return a list of valid attestations to be packaged inside a beacon block.
-func (vs *Server) validateAndDeleteAttsInPool(ctx context.Context, st state.BeaconState, atts []ethpb.Att) ([]ethpb.Att, error) {
+func (vs *Server) validateAndDeleteAttsInPool(ctx context.Context, st state.BeaconState, atts []ethpb.Att) []ethpb.Att {


I changed the signature of this function because not being able to delete an attestation from the pool should not stop proposing

rkapka · 2024-08-09T11:13:23Z

beacon-chain/sync/subscriber_beacon_attestation.go

-	if err != nil {
-		return errors.Wrap(err, "could not determine if attestation pool has this attestation")
+	if features.Get().EnableExperimentalAttestationPool {
+		return s.cfg.attestationCache.Add(a)


It doesn't make sense to perform a redundancy check here (similarly to how the old design calls HasAggregateAttestation) because this function deals with unaggregated atts.

rkapka · 2024-08-09T11:20:49Z

beacon-chain/cache/attestation.go

+	return count
+}
+
+func (c *AttestationCache) DeleteCovered(att ethpb.Att) error {


DeleteCovered is much more accurate than the current DeleteAggregateAttestation. The current name is misleading because we do not delete the passed attestation, but all attestations in the pool whose bits are covered by the incoming attestation.

rkapka · 2024-08-09T11:22:28Z

beacon-chain/cache/attestation.go

+	return pruneCount
+}
+
+func (c *AttestationCache) AggregateIsRedundant(att ethpb.Att) (bool, error) {


AggregateIsRedundant is much more accurate than the current HasAggregateAttestation. The current name is misleading because we check if there's an attestation in the pool whose bits cover all bits of the incoming attestation.

potuz · 2024-08-14T13:09:36Z

It doesn't make sense to keep a separate list of block atts.

Are you sure about this, at least on forkchoice, attestations that come from a block are treated differently than those that don't .

    # If the given attestation is not from a beacon block message, we have to check the target epoch scope.
    if not is_from_block:
        validate_target_epoch_against_current_time(store, attestation)

potuz · 2024-09-10T12:52:16Z

beacon-chain/blockchain/process_block.go

-				return err
+		if features.Get().EnableExperimentalAttestationPool {
+			if err := s.cfg.AttestationCache.DeleteCovered(att); err != nil {
+				return errors.Wrap(err, "could not delete attestation")
 			}
 		} else {


Suggested change

} else {

potuz · 2024-09-10T12:53:55Z

beacon-chain/blockchain/process_block.go

+			if att.IsAggregated() {
+				if err := s.cfg.AttPool.DeleteAggregatedAttestation(att); err != nil {
+					return err
+				}
+			} else {
+				if err := s.cfg.AttPool.DeleteUnaggregatedAttestation(att); err != nil {
+					return err
+				}


Suggested change

if att.IsAggregated() {

if err := s.cfg.AttPool.DeleteAggregatedAttestation(att); err != nil {

return err

}

} else {

if err := s.cfg.AttPool.DeleteUnaggregatedAttestation(att); err != nil {

return err

}

else if att.IsAggregated() {

if err := s.cfg.AttPool.DeleteAggregatedAttestation(att); err != nil {

return err

}

} else {

if err := s.cfg.AttPool.DeleteUnaggregatedAttestation(att); err != nil {

return err

}

}

potuz · 2024-09-10T12:55:49Z

beacon-chain/cache/attestation.go

+
+type attGroup struct {
+	slot     primitives.Slot
+	local    ethpb.Att


Just to document somewhere that this design in principle can result in worse aggregates since we lose the ability to aggregate some single bit attestations in case of overlaps with incoming aggregates.

potuz · 2024-09-10T12:58:14Z

beacon-chain/cache/attestation.go

+}
+
+type AttestationCache struct {
+	atts map[attestation.Id]*attGroup


Need to monitor for memory leaks as this will be a high impact path.

potuz · 2024-09-10T13:05:23Z

proto/prysm/v1alpha1/attestation/id.go


 // NewId --
 func NewId(att ethpb.Att, source IdSource) (Id, error) {
-	if err := helpers.ValidateNilAttestation(att); err != nil {
-		return Id{}, err
+	if att.IsNil() {


Just to document somewhere that I dislike this function, it uses a string method separating with commas for no reason that I can see, on a hot path.

I tried using bytes instead of strings and came up with this code (because the size of int is machine-dependent, I have to convert it to a fixed-size integer - binary.Write works only with fixed-size data):

indicesUint := make([]uint16, len(committeeIndices)) for i := range committeeIndices { indicesUint[i] = uint16(committeeIndices[i]) } buf := new(bytes.Buffer) if err = binary.Write(buf, binary.LittleEndian, indicesUint); err != nil { return Id{}, err } h = hash.Hash(append(dataHash[:], buf.Bytes()...))

Running NewId with 16 bits set

BenchmarkNewId-12 249559 5036 ns/op

Running the string version

BenchmarkNewId-12 275055 4297 ns/op

Do you have an idea how to make this faster?

potuz · 2024-09-10T13:28:12Z

beacon-chain/cache/attestation.go

+	}
+}
+
+func (c *AttestationCache) Add(att ethpb.Att) error {


Godoc missing

potuz · 2024-09-10T13:28:38Z

beacon-chain/cache/attestation.go

+	return pruneCount
+}
+
+func (c *AttestationCache) AggregateIsRedundant(att ethpb.Att) (bool, error) {


Godoc missing

potuz · 2024-09-10T13:30:45Z

beacon-chain/cache/attestation.go

+		} else if redundant {
+			return true, nil
+		}
+	}


Why don't we check against our local version?

not sure, I will add this

potuz · 2024-09-10T13:31:52Z

beacon-chain/cache/attestation.go

+	return c.forkchoiceAtts.DeleteForkchoiceAttestation(att)
+}
+
+func GetBySlotAndCommitteeIndex[T ethpb.Att](c *AttestationCache, slot primitives.Slot, committeeIndex primitives.CommitteeIndex) []T {


Godoc missing

potuz · 2024-09-10T13:49:12Z

beacon-chain/cache/attestation.go

+		local, ok := group.local.(T)
+		if ok {
+			if local.GetData().Slot == slot && local.CommitteeBitsVal().BitAt(uint64(committeeIndex)) {
+				result = append(result, local)
+				for _, a := range group.external {
+					a, ok := a.(T)
+					if ok {
+						result = append(result, a)
+					}
+				}
+			}
+		} else if len(group.external) > 0 {
+			// We can safely compare the first attestation because all attestations in a group
+			// must have the same slot and committee index, since they are under the same key.
+			a, ok := group.external[0].(T)
+			if ok && a.GetData().Slot == slot && a.CommitteeBitsVal().BitAt(uint64(committeeIndex)) {
+				for _, a := range group.external {
+					a, ok := a.(T)
+					if ok {
+						result = append(result, a)
+					}
+				}
+			}
+		}


This seems better since it has less nested components and no repeated snippets, in addition it avoids extra checks when the slot or committee indices are just wrong.

Suggested change

local, ok := group.local.(T)

if ok {

if local.GetData().Slot == slot && local.CommitteeBitsVal().BitAt(uint64(committeeIndex)) {

result = append(result, local)

for _, a := range group.external {

a, ok := a.(T)

if ok {

result = append(result, a)

}

}

}

} else if len(group.external) > 0 {

// We can safely compare the first attestation because all attestations in a group

// must have the same slot and committee index, since they are under the same key.

a, ok := group.external[0].(T)

if ok && a.GetData().Slot == slot && a.CommitteeBitsVal().BitAt(uint64(committeeIndex)) {

for _, a := range group.external {

a, ok := a.(T)

if ok {

result = append(result, a)

}

}

}

}

addExternal := false

local, ok := group.local.(T)

if ok {

if local.GetData().Slot != slot || !local.CommitteeBitsVal().BitAt(uint64(committeeIndex)) {

continue

}

result = append(result, local)

addExternal = true

} else if len(group.external) > 0 {

// We can safely compare the first attestation because all attestations in a group

// must have the same slot and committee index, since they are under the same key.

a, ok := group.external[0].(T)

if ok && a.GetData().Slot == slot && a.CommitteeBitsVal().BitAt(uint64(committeeIndex)) {

addExternal = true

}

}

if !addExternal {

continue

}

for _, a := range group.external {

a, ok := a.(T)

if ok {

result = append(result, a)

}

}

# Conflicts: # CHANGELOG.md # beacon-chain/rpc/eth/beacon/handlers_pool.go # beacon-chain/rpc/eth/validator/handlers.go

james-prysm · 2024-11-18T17:06:58Z

beacon-chain/operations/attestations/forkchoice/forkchoice.go

+
+// SaveForkchoiceAttestation saves a forkchoice attestation.
+func (a *Attestations) SaveForkchoiceAttestation(att ethpb.Att) error {
+	if att == nil {


you will need to integrate that nil check that will be merged in

# Conflicts: # CHANGELOG.md # beacon-chain/operations/attestations/kv/forkchoice.go # beacon-chain/operations/attestations/kv/unaggregated.go # proto/prysm/v1alpha1/attestation.go

nisdas · 2024-11-20T04:31:45Z

beacon-chain/cache/attestation.go

+	}
+
+	a := group.atts[0]
+	bit := att.GetAggregationBits().BitIndices()[0]


Is it guaranteed the bitfield will have a non-zero amount of bits ?

I added a guard at the beginning of the function

nisdas · 2024-11-20T04:33:50Z

beacon-chain/cache/attestation.go

+
+	for _, a := range group.atts {
+		if redundant, err := a.GetAggregationBits().Contains(att.GetAggregationBits()); err != nil {
+			return true, err


why are we returning true here ? shouldn't it be false because we get an error checking it

I am not sure which one is better. I chose to return true in error paths because the aggregate being redundant is less of a happy path, in opposition to an aggregate being not redundant (hence useful).

prestonvanloon · 2024-11-20T13:45:24Z

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

@@ -2656,7 +2656,7 @@ func TestProposer_FilterAttestation(t *testing.T) {
 				HeadFetcher: &mock.ChainService{State: st, Root: genesisRoot[:]},
 			}
 			atts := tt.inputAtts()
-			received, err := proposerServer.validateAndDeleteAttsInPool(context.Background(), st, atts)
+			received := proposerServer.validateAndDeleteAttsInPool(context.Background(), st, atts)
 			if tt.wantedErr != "" {


The error is removed from this signature, so should you delete the error handling things below?

thanks, fixed

prestonvanloon

Partial review, will resume later

potuz

LGTM

# Conflicts: # CHANGELOG.md # beacon-chain/rpc/eth/beacon/handlers_pool.go # beacon-chain/sync/validate_aggregate_proof.go

prestonvanloon · 2024-12-12T19:42:35Z

beacon-chain/operations/attestations/forkchoice/forkchoice.go

+)
+
+// Attestations --
+type Attestations struct {


I don't get this. What makes it "fork choice"? Seems like generic attestation cache

I renamed the package to attmap. See 2610005

prestonvanloon · 2024-12-12T19:46:49Z

proto/prysm/v1alpha1/attestation/id.go

@@ -22,12 +21,12 @@ const (
 )

 // Id represents an attestation ID. Its uniqueness depends on the IdSource provided when constructing the Id.
-type Id [33]byte


Why change this?

This is what Potuz told me:

I will strongly recommend that you just replkace this with [32]bytes and do the first byte to be the version, and the remaining 31 to be the hash[1:32]

having slices of unaligned data will either double the memory footprint of all these instances or add CPU latency to them

unnecessarily since you don't really care about the full hash anyway it seems

# Conflicts: # CHANGELOG.md

rkapka added 10 commits August 8, 2024 19:05

Rename existing AttestationCache

614ba32

Cache with simple Add

44b21bd

fix import cycle

37ac302

logic for unaggregated, aggregated and block attestations

0d796fa

some small fixes

32614d3

remove Seen

09f9447

finishing touches

1bbcb57

feature flag

ed3e42e

extract forkchoice atts to separate type

bb13a87

gate new functionality behind feature flag

5fdc569

rkapka force-pushed the att-pool-revamp branch from 4a805e2 to 5fdc569 Compare August 8, 2024 17:06

revert test files

1a888f8

rkapka commented Aug 9, 2024

View reviewed changes

preparing for review

965f4a6

rkapka changed the title ~~Att pool revamp~~ Redesign of the attestation pool Aug 9, 2024

change Id to [32]byte

53b83cf

potuz reviewed Sep 10, 2024

View reviewed changes

Potuz's review

36a30fb

rkapka added 2 commits October 30, 2024 17:32

fix tests

39963ef

documentation

dd2305f

rkapka force-pushed the att-pool-revamp branch from 21c8a27 to dd2305f Compare October 30, 2024 10:32

rkapka added 3 commits October 30, 2024 17:33

changelog <3

9777272

Merge branch '__develop' into att-pool-revamp

2a613fb

# Conflicts: # CHANGELOG.md # beacon-chain/rpc/eth/beacon/handlers_pool.go # beacon-chain/rpc/eth/validator/handlers.go

v2 handlers

e14f6e2

james-prysm reviewed Nov 18, 2024

View reviewed changes

rkapka added 2 commits November 19, 2024 12:59

nil check for forkchoice atts

d41f802

Merge branch '__develop' into att-pool-revamp

e2f372d

# Conflicts: # CHANGELOG.md # beacon-chain/operations/attestations/kv/forkchoice.go # beacon-chain/operations/attestations/kv/unaggregated.go # proto/prysm/v1alpha1/attestation.go

nisdas reviewed Nov 20, 2024

View reviewed changes

rkapka and others added 2 commits November 20, 2024 11:38

guard against 0 bits set

1651263

Merge branch 'develop' into att-pool-revamp

33a640f

prestonvanloon reviewed Nov 20, 2024

View reviewed changes

potuz previously approved these changes Nov 28, 2024

View reviewed changes

rkapka added 2 commits December 11, 2024 17:22

fix failing test

a3c0a02

Merge branch '__develop' into att-pool-revamp

f4cb240

# Conflicts: # CHANGELOG.md # beacon-chain/rpc/eth/beacon/handlers_pool.go # beacon-chain/sync/validate_aggregate_proof.go

rkapka dismissed potuz’s stale review via f4cb240 December 11, 2024 22:25

prestonvanloon reviewed Dec 12, 2024

View reviewed changes

Preston's review

0aebdf4

rkapka force-pushed the att-pool-revamp branch from 2610005 to 0aebdf4 Compare December 18, 2024 05:57

rkapka and others added 3 commits December 18, 2024 00:58

Merge branch 'develop' into att-pool-revamp

66235ee

better godocs

4575947

Merge branch '__develop' into att-pool-revamp

09330e7

# Conflicts: # CHANGELOG.md

prestonvanloon approved these changes Dec 20, 2024

View reviewed changes

rkapka added this pull request to the merge queue Dec 23, 2024

Merged via the queue into develop with commit d04b361 Dec 23, 2024
15 checks passed

rkapka deleted the att-pool-revamp branch December 23, 2024 10:06

Redesign of the attestation pool #14324

Redesign of the attestation pool #14324

Conversation

rkapka commented Aug 8, 2024 • edited Loading

Idea

Impact

Open questions

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

potuz commented Aug 14, 2024

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

james-prysm Nov 18, 2024 • 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

prestonvanloon left a comment

Choose a reason for hiding this comment

potuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka commented Aug 8, 2024 •

edited

Loading

james-prysm Nov 18, 2024 •

edited

Loading

rkapka Dec 14, 2024 •

edited

Loading