-
-
Notifications
You must be signed in to change notification settings - Fork 306
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
fix: cache attester index during attestation validation #7260
Conversation
@@ -11,8 +11,8 @@ type AttDataBase64 = string; | |||
export type AttestationDataCacheEntry = { | |||
// part of shuffling data, so this does not take memory | |||
committeeValidatorIndices: Uint32Array; | |||
// TODO: remove this? this is available in SingleAttestation |
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.
@twoeths I am not sure about this comment, seems simpler to keep the committee index in the cache as if in case of loading the attestation from cache we won't deserialize the SingleAttestation
to get the committe index from there, and for pre-electra we would still need it.
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.
yes let's keep it in the cache and remove "TODO"
@@ -498,6 +507,7 @@ async function validateAttestationNoSignatureCheck( | |||
chain.seenAttestationDatas.add(attSlot, committeeIndex, attDataKey, { | |||
committeeValidatorIndices, | |||
committeeIndex, | |||
attesterIndex: validatorIndex, |
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.
could consider renaming validatorIndex
to attesterIndex
(or vice versa)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devnet-5 #7260 +/- ##
============================================
- Coverage 48.44% 48.43% -0.01%
============================================
Files 602 602
Lines 40321 40328 +7
Branches 2057 2057
============================================
Hits 19532 19532
- Misses 20751 20758 +7
Partials 38 38 |
committeeIndex: CommitteeIndex; | ||
attesterIndex: ValidatorIndex; |
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.
it's not correct to have attesterIndex
here, this is the common cache for the whole committee while this is specific information of a validator
should also remove aggregationBits
below as it's specific to the validator
Closing as we need a different approach |
Motivation
Currently we always try to get the validator index from
attestationOrCache.attestation
but it might benull
if cached value is used leading to the following error:Caching the attester index also has some benefits pre-electra as we can safe a
getSingleTrueBit
call (see #7209) if we retrieve the attestation from the cache.Description
Cache attester index during attestation validation