-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ignore subset aggregates #10674
Ignore subset aggregates #10674
Conversation
…ysm into ignore-subset-aggregate
c1a22ac
to
e1368e9
Compare
…ysm into ignore-subset-aggregate
@@ -221,6 +223,7 @@ func (s *Service) initCaches() { | |||
s.seenUnAggregatedAttestationCache = lruwrpr.New(seenUnaggregatedAttSize) | |||
s.seenSyncMessageCache = lruwrpr.New(seenSyncMsgSize) | |||
s.seenSyncContributionCache = lruwrpr.New(seenSyncContributionSize) | |||
s.syncContributionBitsOverlapCache = lruwrpr.New(seenSyncContributionSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simplicity, I opted in same cache size as seenSyncContributionSize
, open for other suggestions
871d8f5
to
d7c3fd3
Compare
d7c3fd3
to
b1f7eeb
Compare
44511ef
to
417187f
Compare
if seen { | ||
return pubsub.ValidationIgnore, nil | ||
} | ||
seen = s.hasSeenSyncContributionIndexSlot(c.Slot, m.Message.AggregatorIndex, types.CommitteeIndex(c.SubcommitteeIndex)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is copying ignoreCached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I removed ignoreCached
. I could also use ignoreCached
here too if you prefer that
if !ok { | ||
return errors.New("could not covert cached value to []bitfield.Bitvector") | ||
} | ||
s.syncContributionBitsOverlapCache.Add(string(b), append(bitsList, c.AggregationBits.Bytes())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to check whether it contains a bitlist that already contains over what the contribution has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks!
func (s *Service) setSyncContributionBits(c *ethpb.SyncCommitteeContribution) error { | ||
s.syncContributionBitsOverlapLock.Lock() | ||
defer s.syncContributionBitsOverlapLock.Unlock() | ||
b := append(c.BlockRoot, bytesutil.Bytes32(uint64(c.Slot))...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should copy c.BlockRoot
first before appending it here. Due to how protobuf unmarshalling is carried out, you could unintentionally mutate this sync contribution.
} | ||
|
||
// BitListOverlaps returns true if there's an overlap between two bitlists. | ||
func BitListOverlaps(bitLists [][]byte, b []byte) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to export it here, as no consumer is going to use this from the sync package currently.
@@ -10,3 +10,7 @@ import ( | |||
func NewSyncCommitteeAggregationBits() bitfield.Bitvector128 { | |||
return bitfield.NewBitvector128() | |||
} | |||
|
|||
func ConvertSyncContributionBitVector(b []byte) bitfield.Bitvector128 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func ConvertSyncContributionBitVector(b []byte) bitfield.Bitvector128 { | |
func ConvertToSyncContributionBitVector(b []byte) bitfield.Bitvector128 { |
@@ -10,3 +10,7 @@ import ( | |||
func NewSyncCommitteeAggregationBits() bitfield.Bitvector8 { | |||
return bitfield.NewBitvector8() | |||
} | |||
|
|||
func ConvertSyncContributionBitVector(b []byte) bitfield.Bitvector8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func ConvertSyncContributionBitVector(b []byte) bitfield.Bitvector8 { | |
func ConvertToSyncContributionBitVector(b []byte) bitfield.Bitvector8 { |
This reverts commit 61cbe37.
Implements and rationale: ethereum/consensus-specs#2847
This PR drops aggregated objects if a better aggregate has been seen. (i.e. bitfield overlaps). As a result, we should see a reduction in CPU usage and incoming and outgoing bandwidth.
Note: we did not have to do anything with aggregated attestation,
HasAggregatedAttestation
already checks the subset 🙌🏼