Batch verify aggregated attestation signatures #7744
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7744 +/- ##
==========================================
- Coverage 62.15% 62.14% -0.02%
==========================================
Files 429 429
Lines 30248 30292 +44
==========================================
+ Hits 18802 18824 +22
- Misses 8521 8539 +18
- Partials 2925 2929 +4 |
| return helpers.ComputeDomainVerifySigningRoot(s, a.Message.AggregatorIndex, | ||
| helpers.SlotToEpoch(a.Message.Aggregate.Data.Slot), a.Message, params.BeaconConfig().DomainAggregateAndProof, a.Signature) | ||
| // This returns aggregator signature set which can be used to batch verify. | ||
| func aggSigSet(s *stateTrie.BeaconState, a *ethpb.SignedAggregateAttestationAndProof) (*bls.SignatureSet, error) { |
There was a problem hiding this comment.
can we use our core methods here if possible, the logic here seems duplicated.
There was a problem hiding this comment.
which core method? this is for DomainAggregateAndProof over the entire message
There was a problem hiding this comment.
wouldn't we just be using this ?
func validateAggregatorSignature(s *stateTrie.BeaconState, a *ethpb.SignedAggregateAttestationAndProof) error {
return helpers.ComputeDomainVerifySigningRoot(s, a.Message.AggregatorIndex,
helpers.SlotToEpoch(a.Message.Aggregate.Data.Slot), a.Message, params.BeaconConfig().DomainAggregateAndProof, a.Signature)
}any way to refactor that method to return a signature set instead ? Goal would be to avoid having 2 different methods
for signature verification.
There was a problem hiding this comment.
I'll open a subsequent PR to address this. We should just refactor for all. There's some duplications from BLS helpers to signing helpers to block processing and to this. Better to improve all at once
| traceutil.AnnotateError(span, errors.Wrapf(err, "Could not validate selection for validator %d", signed.Message.AggregatorIndex)) | ||
| return pubsub.ValidationReject | ||
| return pubsub.ValidationIgnore |
| traceutil.AnnotateError(span, errors.Wrapf(err, "Could not verify aggregator signature %d", signed.Message.AggregatorIndex)) | ||
| return pubsub.ValidationReject | ||
| return pubsub.ValidationIgnore |
There was a problem hiding this comment.
Failing to construct attestation signature set is a self-error and we shouldn't stop broadcasting it to our peers
There was a problem hiding this comment.
That depends on what kind of error, ex:
https://github.com/prysmaticlabs/prysm/blob/86e4c90fd999d9045e60cb76d2284d86ba0081da/beacon-chain/core/blocks/signature.go#L127:6
Most of the errors there would only be possible because of an invalid attestation aggregate. Ignoring rather than rejecting something like this seems risky.
There was a problem hiding this comment.
Not necessarily. These errors could come from:
- Bad implementation of caching (e.g. BeaconCommitteeFromState)
- Bad implementation of hash tree root (e.g. ComputeSigningRoot)
- Bad implementation of helper functions (e.g. ConvertToIndexed, IsValidAttestationIndices)
These are likely client independent bug that varies from releases and languages. Example, if prysm has a bug on a specific version, it'll kill liveness for lh and nimbus as well. We don't want to stop broadcast and descore peers due to reasons above.
Likewise, if the aggregation is really bad, the signature validation will fail which will cause a reject. It feels safer on the liveness to be lenient on the first check
This PR batches verify all 3 signatures within aggregated attestation, the 3 signatures in aggregated attestation: selection signature, aggregator signature, and attestation signature.
It combines the following 3 verifications into 1:
With the following benchmark: https://gist.github.com/terencechain/412ef767ff37562a2735237c7ffc8b71
We gathered the following results:
The result showed 40% improvements, roughly 1200000ns of saving on per attestation. There's currently 260 aggregated attestation per slot in medalla. That’s about 300ms we could save on per slot.