-
Notifications
You must be signed in to change notification settings - Fork 248
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
Do not add Electra attestations if committees size is not retrievable #8323
Do not add Electra attestations if committees size is not retrievable #8323
Conversation
}; | ||
final Bytes32 targetRoot = attestationData.getTarget().getRoot(); | ||
LOG.debug( | ||
"Committees size was not readily available for attestation with target root {}. Will attempt to retrieve it using the relevant state.", |
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'd probably change this to just be something short like
Committee size was not found via target root {} for attestation at slot {}
if (attestation.requiresCommitteeBits()) { | ||
final AttestationData attestationData = attestation.getData(); | ||
LOG.debug( | ||
"Committees size was not found for target root {} for attestation at slot {}. Will attempt to retrieve it using the relevant state.", |
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 id still delete 'will attempt...'
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 deleted the whole log and moved it to getOrCreateAttestationGroup
. It is now:
LOG.debug("Committees size was not available for attestation at slot {} and block root {}. Will NOT add this attestation to the pool.",attestationData.getTarget().getRoot(), attestationData.getBeaconBlockRoot());
.../src/main/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPool.java
Outdated
Show resolved
Hide resolved
@@ -56,14 +55,14 @@ public class MatchingDataAttestationGroup implements Iterable<ValidatableAttesta | |||
private final Spec spec; | |||
private Optional<Bytes32> committeeShufflingSeed = Optional.empty(); | |||
private final AttestationData attestationData; | |||
private final Supplier<Int2IntMap> commiteesSizeSupplier; | |||
private final Optional<Int2IntMap> committeesSize; |
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 maybeCommitteeSize
would be cleaner
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.
Left it as it is because we use that name in other places (where it is still Optional
)
4f3da70
to
5c23687
Compare
PR Description
Filter out Electra attestations for which we can't retrieve the committees size from the relevant state
Fixed Issue(s)
N/A
Documentation
doc-change-required
label to this PR if updates are required.Changelog