-
Notifications
You must be signed in to change notification settings - Fork 918
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
Submit all slot attestations at once #13809
base: develop
Are you sure you want to change the base?
Conversation
v.SubmitAttestation(slotCtx, slot, pubKey) | ||
data := v.GetAttestationData(slotCtx, slot, pubKey) | ||
if data != nil { | ||
lock.Lock() |
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.
append
is not thread-safe when the size of the slice is predefined
@@ -235,6 +236,27 @@ func LogsContain(loggerFn assertionLoggerFn, hook *test.Hook, want string, flag | |||
} | |||
} | |||
|
|||
// LogsDontHaveErrors checks whether there are no error logs in the output. | |||
func LogsDontHaveErrors(loggerFn assertionLoggerFn, hook *test.Hook, msg ...interface{}) { |
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 much more robust than what we have now, which is checking that no Could not
logs exits. Not all error logs in the function have this format.
validator/client/attest.go
Outdated
log.WithError(err).Error("Failed attestation slashing protection check") | ||
log.WithFields( | ||
attestationLogFields(pubkeys[i], indexedAtt), | ||
).Debug("Attempted slashable attestation details") |
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.
What does this message mean? Is it details of an attempted slashable attestation?
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 improved the log
@@ -134,7 +134,7 @@ type ValidatorClient interface { | |||
PrepareBeaconProposer(ctx context.Context, in *ethpb.PrepareBeaconProposerRequest) (*empty.Empty, error) | |||
GetFeeRecipientByPubKey(ctx context.Context, in *ethpb.FeeRecipientByPubKeyRequest) (*ethpb.FeeRecipientByPubKeyResponse, error) | |||
GetAttestationData(ctx context.Context, in *ethpb.AttestationDataRequest) (*ethpb.AttestationData, error) | |||
ProposeAttestation(ctx context.Context, in *ethpb.Attestation) (*ethpb.AttestResponse, error) | |||
ProposeAttestations(ctx context.Context, in []*ethpb.Attestation) ([]*ethpb.AttestResponse, 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.
Since we call GetAttestationData
the same way here and in the validator, should we not keep the name of SubmitAttestations
the same?
ProposeAttestations(ctx context.Context, in []*ethpb.Attestation) ([]*ethpb.AttestResponse, error) | |
SubmitAttestations(ctx context.Context, in []*ethpb.Attestation) ([]*ethpb.AttestResponse, error) |
6a7c5c5
to
fa9a82e
Compare
What type of PR is this?
Other
What does this PR do? Why is it needed?
https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/submitPoolAttestations Beacon API endpoint allows submitting multiple attestations in a single request. We currently do not take advantage of this and submit one attestation at a time. To allow this the PR extracts a
GetAttestationData
function fromSubmitAttestation
so that we can build up a slice of attestation data values and pass it all at once for submission.