Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Batch verify aggregated attestation signatures #7744

Merged
merged 8 commits into from
Nov 10, 2020
Merged

Batch verify aggregated attestation signatures #7744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
131a413
First take. Got benchmark numbers
terencechain Nov 6, 2020
9317db8
Remove benchmark test
terencechain Nov 7, 2020
86e4c90
Final clean up
terencechain Nov 7, 2020
42cb5a9
Merge branch 'master' of github.com:prysmaticlabs/prysm into batch-ve…
terencechain Nov 9, 2020
628e145
Failing to verify aggregator index should be reject
terencechain Nov 9, 2020
9e1f9a1
Merge branch 'master' into batch-veriy-aggre
terencechain Nov 9, 2020
c230f96
Merge branch 'master' into batch-veriy-aggre
terencechain Nov 9, 2020
deb6d27
Merge branch 'master' into batch-veriy-aggre
terencechain Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 77 additions & 21 deletions beacon-chain/sync/validate_aggregate_proof.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
"github.com/prysmaticlabs/prysm/beacon-chain/core/state"
stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state"
"github.com/prysmaticlabs/prysm/shared/bls"
"github.com/prysmaticlabs/prysm/shared/bytesutil"
"github.com/prysmaticlabs/prysm/shared/params"
"github.com/prysmaticlabs/prysm/shared/traceutil"
Expand Down Expand Up @@ -131,21 +132,34 @@ func (s *Service) validateAggregatedAtt(ctx context.Context, signed *ethpb.Signe
return pubsub.ValidationReject
}

// Verify selection proof reflects to the right validator and signature is valid.
if err := validateSelection(ctx, bs, signed.Message.Aggregate.Data, signed.Message.AggregatorIndex, signed.Message.SelectionProof); err != nil {
// Verify selection proof reflects to the right validator.
selectionSigSet, err := validateSelectionIndex(ctx, bs, signed.Message.Aggregate.Data, signed.Message.AggregatorIndex, signed.Message.SelectionProof)
if err != nil {
traceutil.AnnotateError(span, errors.Wrapf(err, "Could not validate selection for validator %d", signed.Message.AggregatorIndex))
return pubsub.ValidationReject
}

// Verify the aggregator's signature is valid.
if err := validateAggregatorSignature(bs, signed); err != nil {
// Verify selection signature, aggregator signature and attestation signature are valid.
// We use batch verify here to save compute.
aggregatorSigSet, err := aggSigSet(bs, signed)
if err != nil {
traceutil.AnnotateError(span, errors.Wrapf(err, "Could not get aggregator sig set %d", signed.Message.AggregatorIndex))
return pubsub.ValidationIgnore
}
attSigSet, err := blocks.AttestationSignatureSet(ctx, bs, []*ethpb.Attestation{signed.Message.Aggregate})
if err != nil {
traceutil.AnnotateError(span, errors.Wrapf(err, "Could not verify aggregator signature %d", signed.Message.AggregatorIndex))
return pubsub.ValidationReject
return pubsub.ValidationIgnore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing to construct attestation signature set is a self-error and we shouldn't stop broadcasting it to our peers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// Verify aggregated attestation has a valid signature.
if err := blocks.VerifyAttestationSignature(ctx, bs, signed.Message.Aggregate); err != nil {
traceutil.AnnotateError(span, err)
set := bls.NewSet()
set.Join(selectionSigSet).Join(aggregatorSigSet).Join(attSigSet)
valid, err := set.Verify()
if err != nil {
traceutil.AnnotateError(span, errors.Errorf("Could not join signature set"))
return pubsub.ValidationIgnore
}
if !valid {
traceutil.AnnotateError(span, errors.Errorf("Could not verify selection or aggregator or attestation signature"))
return pubsub.ValidationReject
}

Expand Down Expand Up @@ -210,32 +224,74 @@ func validateIndexInCommittee(ctx context.Context, bs *stateTrie.BeaconState, a
return nil
}

// This validates selection proof by validating it's from the correct validator index of the slot and selection
// proof is a valid signature.
func validateSelection(ctx context.Context, bs *stateTrie.BeaconState, data *ethpb.AttestationData, validatorIndex uint64, proof []byte) error {
_, span := trace.StartSpan(ctx, "sync.validateSelection")
// This validates selection proof by validating it's from the correct validator index of the slot.
// It does not verify the selection proof, it returns the signature set of selection proof which can be used for batch verify.
func validateSelectionIndex(ctx context.Context, bs *stateTrie.BeaconState, data *ethpb.AttestationData, validatorIndex uint64, proof []byte) (*bls.SignatureSet, error) {
_, span := trace.StartSpan(ctx, "sync.validateSelectionIndex")
defer span.End()

committee, err := helpers.BeaconCommitteeFromState(bs, data.Slot, data.CommitteeIndex)
if err != nil {
return err
return nil, err
}
aggregator, err := helpers.IsAggregator(uint64(len(committee)), proof)
if err != nil {
return err
return nil, err
}
if !aggregator {
return fmt.Errorf("validator is not an aggregator for slot %d", data.Slot)
return nil, fmt.Errorf("validator is not an aggregator for slot %d", data.Slot)
}

domain := params.BeaconConfig().DomainSelectionProof
epoch := helpers.SlotToEpoch(data.Slot)
return helpers.ComputeDomainVerifySigningRoot(bs, validatorIndex, epoch, data.Slot, domain, proof)

v, err := bs.ValidatorAtIndex(validatorIndex)
if err != nil {
return nil, err
}
publicKey, err := bls.PublicKeyFromBytes(v.PublicKey)
if err != nil {
return nil, err
}

d, err := helpers.Domain(bs.Fork(), epoch, domain, bs.GenesisValidatorRoot())
if err != nil {
return nil, err
}
root, err := helpers.ComputeSigningRoot(data.Slot, d)
if err != nil {
return nil, err
}
return &bls.SignatureSet{
Signatures: [][]byte{proof},
PublicKeys: []bls.PublicKey{publicKey},
Messages: [][32]byte{root},
}, nil
}

// This verifies aggregator signature over the signed aggregate and proof object.
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)
// This returns aggregator signature set which can be used to batch verify.
func aggSigSet(s *stateTrie.BeaconState, a *ethpb.SignedAggregateAttestationAndProof) (*bls.SignatureSet, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use our core methods here if possible, the logic here seems duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which core method? this is for DomainAggregateAndProof over the entire message

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

v, err := s.ValidatorAtIndex(a.Message.AggregatorIndex)
if err != nil {
return nil, err
}
publicKey, err := bls.PublicKeyFromBytes(v.PublicKey)
if err != nil {
return nil, err
}

epoch := helpers.SlotToEpoch(a.Message.Aggregate.Data.Slot)
d, err := helpers.Domain(s.Fork(), epoch, params.BeaconConfig().DomainAggregateAndProof, s.GenesisValidatorRoot())
if err != nil {
return nil, err
}
root, err := helpers.ComputeSigningRoot(a.Message, d)
if err != nil {
return nil, err
}
return &bls.SignatureSet{
Signatures: [][]byte{a.Signature},
PublicKeys: []bls.PublicKey{publicKey},
Messages: [][32]byte{root},
}, nil
}
34 changes: 2 additions & 32 deletions beacon-chain/sync/validate_aggregate_proof_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,39 +90,9 @@ func TestVerifySelection_NotAnAggregator(t *testing.T) {
Source: &ethpb.Checkpoint{Root: make([]byte, 32)},
}

_, err := validateSelectionIndex(ctx, beaconState, data, 0, sig.Marshal())
wanted := "validator is not an aggregator for slot"
assert.ErrorContains(t, wanted, validateSelection(ctx, beaconState, data, 0, sig.Marshal()))
}

func TestVerifySelection_BadSignature(t *testing.T) {
ctx := context.Background()
validators := uint64(256)
beaconState, privKeys := testutil.DeterministicGenesisState(t, validators)

sig := privKeys[0].Sign([]byte{'A'})
data := &ethpb.AttestationData{
BeaconBlockRoot: make([]byte, 32),
Target: &ethpb.Checkpoint{Root: make([]byte, 32)},
Source: &ethpb.Checkpoint{Root: make([]byte, 32)},
}

wanted := "signature did not verify"
assert.ErrorContains(t, wanted, validateSelection(ctx, beaconState, data, 0, sig.Marshal()))
}

func TestVerifySelection_CanVerify(t *testing.T) {
ctx := context.Background()
validators := uint64(256)
beaconState, privKeys := testutil.DeterministicGenesisState(t, validators)

data := &ethpb.AttestationData{
BeaconBlockRoot: make([]byte, 32),
Target: &ethpb.Checkpoint{Root: make([]byte, 32)},
Source: &ethpb.Checkpoint{Root: make([]byte, 32)},
}
sig, err := helpers.ComputeDomainAndSign(beaconState, 0, data.Slot, params.BeaconConfig().DomainSelectionProof, privKeys[0])
require.NoError(t, err)
require.NoError(t, validateSelection(ctx, beaconState, data, 0, sig))
assert.ErrorContains(t, wanted, err)
}

func TestValidateAggregateAndProof_NoBlock(t *testing.T) {
Expand Down