From 7d56e74e2a55f56fb843cad4d29cccb5a32c9a68 Mon Sep 17 00:00:00 2001 From: nisdas Date: Thu, 13 Aug 2020 16:17:47 +0800 Subject: [PATCH 1/4] fix bad bug --- beacon-chain/core/blocks/attestation.go | 108 ++++++++++++------------ beacon-chain/core/blocks/signature.go | 3 + 2 files changed, 57 insertions(+), 54 deletions(-) diff --git a/beacon-chain/core/blocks/attestation.go b/beacon-chain/core/blocks/attestation.go index 574f270bf594..983d36625ad8 100644 --- a/beacon-chain/core/blocks/attestation.go +++ b/beacon-chain/core/blocks/attestation.go @@ -199,60 +199,6 @@ func ProcessAttestationNoVerify( return beaconState, nil } -// VerifyIndexedAttestation determines the validity of an indexed attestation. -// -// Spec pseudocode definition: -// def is_valid_indexed_attestation(state: BeaconState, indexed_attestation: IndexedAttestation) -> bool: -// """ -// Check if ``indexed_attestation`` is not empty, has sorted and unique indices and has a valid aggregate signature. -// """ -// # Verify indices are sorted and unique -// indices = indexed_attestation.attesting_indices -// if len(indices) == 0 or not indices == sorted(set(indices)): -// return False -// # Verify aggregate signature -// pubkeys = [state.validators[i].pubkey for i in indices] -// domain = get_domain(state, DOMAIN_BEACON_ATTESTER, indexed_attestation.data.target.epoch) -// signing_root = compute_signing_root(indexed_attestation.data, domain) -// return bls.FastAggregateVerify(pubkeys, signing_root, indexed_attestation.signature) -func VerifyIndexedAttestation(ctx context.Context, beaconState *stateTrie.BeaconState, indexedAtt *ethpb.IndexedAttestation) error { - ctx, span := trace.StartSpan(ctx, "core.VerifyIndexedAttestation") - defer span.End() - - if err := attestationutil.IsValidAttestationIndices(ctx, indexedAtt); err != nil { - return err - } - domain, err := helpers.Domain(beaconState.Fork(), indexedAtt.Data.Target.Epoch, params.BeaconConfig().DomainBeaconAttester, beaconState.GenesisValidatorRoot()) - if err != nil { - return err - } - indices := indexedAtt.AttestingIndices - pubkeys := []bls.PublicKey{} - for i := 0; i < len(indices); i++ { - pubkeyAtIdx := beaconState.PubkeyAtIndex(indices[i]) - pk, err := bls.PublicKeyFromBytes(pubkeyAtIdx[:]) - if err != nil { - return errors.Wrap(err, "could not deserialize validator public key") - } - pubkeys = append(pubkeys, pk) - } - return attestationutil.VerifyIndexedAttestationSig(ctx, indexedAtt, pubkeys, domain) -} - -// VerifyAttestation converts and attestation into an indexed attestation and verifies -// the signature in that attestation. -func VerifyAttestation(ctx context.Context, beaconState *stateTrie.BeaconState, att *ethpb.Attestation) error { - if att == nil || att.Data == nil { - return fmt.Errorf("nil or missing attestation data: %v", att) - } - committee, err := helpers.BeaconCommitteeFromState(beaconState, att.Data.Slot, att.Data.CommitteeIndex) - if err != nil { - return err - } - indexedAtt := attestationutil.ConvertToIndexed(ctx, att, committee) - return VerifyIndexedAttestation(ctx, beaconState, indexedAtt) -} - // VerifyAttestations will verify the signatures of the provided attestations. This method performs // a single BLS verification call to verify the signatures of all of the provided attestations. All // of the provided attestations must have valid signatures or this method will return an error. @@ -306,6 +252,60 @@ func VerifyAttestations(ctx context.Context, beaconState *stateTrie.BeaconState, return verifyAttestationsWithDomain(ctx, beaconState, postForkAtts, currDomain) } +// VerifyAttestation converts and attestation into an indexed attestation and verifies +// the signature in that attestation. +func VerifyAttestation(ctx context.Context, beaconState *stateTrie.BeaconState, att *ethpb.Attestation) error { + if att == nil || att.Data == nil { + return fmt.Errorf("nil or missing attestation data: %v", att) + } + committee, err := helpers.BeaconCommitteeFromState(beaconState, att.Data.Slot, att.Data.CommitteeIndex) + if err != nil { + return err + } + indexedAtt := attestationutil.ConvertToIndexed(ctx, att, committee) + return VerifyIndexedAttestation(ctx, beaconState, indexedAtt) +} + +// VerifyIndexedAttestation determines the validity of an indexed attestation. +// +// Spec pseudocode definition: +// def is_valid_indexed_attestation(state: BeaconState, indexed_attestation: IndexedAttestation) -> bool: +// """ +// Check if ``indexed_attestation`` is not empty, has sorted and unique indices and has a valid aggregate signature. +// """ +// # Verify indices are sorted and unique +// indices = indexed_attestation.attesting_indices +// if len(indices) == 0 or not indices == sorted(set(indices)): +// return False +// # Verify aggregate signature +// pubkeys = [state.validators[i].pubkey for i in indices] +// domain = get_domain(state, DOMAIN_BEACON_ATTESTER, indexed_attestation.data.target.epoch) +// signing_root = compute_signing_root(indexed_attestation.data, domain) +// return bls.FastAggregateVerify(pubkeys, signing_root, indexed_attestation.signature) +func VerifyIndexedAttestation(ctx context.Context, beaconState *stateTrie.BeaconState, indexedAtt *ethpb.IndexedAttestation) error { + ctx, span := trace.StartSpan(ctx, "core.VerifyIndexedAttestation") + defer span.End() + + if err := attestationutil.IsValidAttestationIndices(ctx, indexedAtt); err != nil { + return err + } + domain, err := helpers.Domain(beaconState.Fork(), indexedAtt.Data.Target.Epoch, params.BeaconConfig().DomainBeaconAttester, beaconState.GenesisValidatorRoot()) + if err != nil { + return err + } + indices := indexedAtt.AttestingIndices + pubkeys := []bls.PublicKey{} + for i := 0; i < len(indices); i++ { + pubkeyAtIdx := beaconState.PubkeyAtIndex(indices[i]) + pk, err := bls.PublicKeyFromBytes(pubkeyAtIdx[:]) + if err != nil { + return errors.Wrap(err, "could not deserialize validator public key") + } + pubkeys = append(pubkeys, pk) + } + return attestationutil.VerifyIndexedAttestationSig(ctx, indexedAtt, pubkeys, domain) +} + // Inner method to verify attestations. This abstraction allows for the domain to be provided as an // argument. func verifyAttestationsWithDomain(ctx context.Context, beaconState *stateTrie.BeaconState, atts []*ethpb.Attestation, domain []byte) error { diff --git a/beacon-chain/core/blocks/signature.go b/beacon-chain/core/blocks/signature.go index 33b569980084..8e48bbe9b517 100644 --- a/beacon-chain/core/blocks/signature.go +++ b/beacon-chain/core/blocks/signature.go @@ -144,6 +144,9 @@ func createAttestationSignatureSet(ctx context.Context, beaconState *stateTrie.B return nil, err } ia := attestationutil.ConvertToIndexed(ctx, a, c) + if err := attestationutil.IsValidAttestationIndices(ctx, ia); err != nil { + return nil, err + } indices := ia.AttestingIndices var pk bls.PublicKey for i := 0; i < len(indices); i++ { From 5843385e054d834626fbf2658b3c3780a85a91c0 Mon Sep 17 00:00:00 2001 From: nisdas Date: Thu, 13 Aug 2020 16:20:19 +0800 Subject: [PATCH 2/4] add test --- beacon-chain/core/blocks/attestation_test.go | 42 ++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/beacon-chain/core/blocks/attestation_test.go b/beacon-chain/core/blocks/attestation_test.go index 30035e2bf639..1ac8b55af6ea 100644 --- a/beacon-chain/core/blocks/attestation_test.go +++ b/beacon-chain/core/blocks/attestation_test.go @@ -603,6 +603,48 @@ func TestValidateIndexedAttestation_AboveMaxLength(t *testing.T) { assert.ErrorContains(t, want, err) } +func TestValidateIndexedAttestation_BadAttestationsSignatureSet(t *testing.T) { + beaconState, keys := testutil.DeterministicGenesisState(t, 1000) + + sig := keys[0].Sign([]byte{'t', 'e', 's', 't'}) + list := bitfield.Bitlist{0b11111111} + atts := []*ethpb.Attestation{} + for i := uint64(0); i < 1000; i++ { + atts = append(atts, ðpb.Attestation{ + Data: ðpb.AttestationData{ + CommitteeIndex: 1, + Slot: 1, + }, + Signature: sig.Marshal(), + AggregationBits: list, + }) + } + + want := "nil or missing indexed attestation data" + _, err := blocks.AttestationSignatureSet(context.Background(), beaconState, atts) + assert.ErrorContains(t, want, err) + + atts = []*ethpb.Attestation{} + list = bitfield.Bitlist{0b00000000} + for i := uint64(0); i < 1000; i++ { + atts = append(atts, ðpb.Attestation{ + Data: ðpb.AttestationData{ + CommitteeIndex: 1, + Slot: 1, + Target: ðpb.Checkpoint{ + Root: []byte{}, + }, + }, + Signature: sig.Marshal(), + AggregationBits: list, + }) + } + + want = "expected non-empty attesting indices" + _, err = blocks.AttestationSignatureSet(context.Background(), beaconState, atts) + assert.ErrorContains(t, want, err) +} + func TestVerifyAttestations_VerifiesMultipleAttestations(t *testing.T) { ctx := context.Background() numOfValidators := 4 * params.BeaconConfig().SlotsPerEpoch From 5cc4dbf8b2bb2bde2067cb8ceba4fae36ae5572c Mon Sep 17 00:00:00 2001 From: nisdas Date: Thu, 13 Aug 2020 19:17:32 +0800 Subject: [PATCH 3/4] target --- beacon-chain/core/blocks/attestation_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/beacon-chain/core/blocks/attestation_test.go b/beacon-chain/core/blocks/attestation_test.go index 1ac8b55af6ea..a537cb466007 100644 --- a/beacon-chain/core/blocks/attestation_test.go +++ b/beacon-chain/core/blocks/attestation_test.go @@ -676,6 +676,7 @@ func TestVerifyAttestations_VerifiesMultipleAttestations(t *testing.T) { Data: ðpb.AttestationData{ Slot: 1, CommitteeIndex: 0, + Target: new(ethpb.Checkpoint), }, Signature: nil, } @@ -697,6 +698,7 @@ func TestVerifyAttestations_VerifiesMultipleAttestations(t *testing.T) { Data: ðpb.AttestationData{ Slot: 1, CommitteeIndex: 1, + Target: new(ethpb.Checkpoint), }, Signature: nil, } @@ -744,6 +746,7 @@ func TestVerifyAttestations_HandlesPlannedFork(t *testing.T) { Data: ðpb.AttestationData{ Slot: 1, CommitteeIndex: 0, + Target: new(ethpb.Checkpoint), }, Signature: nil, } @@ -765,6 +768,7 @@ func TestVerifyAttestations_HandlesPlannedFork(t *testing.T) { Data: ðpb.AttestationData{ Slot: 1*params.BeaconConfig().SlotsPerEpoch + 1, CommitteeIndex: 1, + Target: new(ethpb.Checkpoint), }, Signature: nil, } @@ -812,6 +816,7 @@ func TestRetrieveAttestationSignatureSet_VerifiesMultipleAttestations(t *testing Data: ðpb.AttestationData{ Slot: 1, CommitteeIndex: 0, + Target: new(ethpb.Checkpoint), }, Signature: nil, } @@ -833,6 +838,7 @@ func TestRetrieveAttestationSignatureSet_VerifiesMultipleAttestations(t *testing Data: ðpb.AttestationData{ Slot: 1, CommitteeIndex: 1, + Target: new(ethpb.Checkpoint), }, Signature: nil, } From b4a21aed580e1c23e8e4417244cb0c20ab762d87 Mon Sep 17 00:00:00 2001 From: nisdas Date: Thu, 13 Aug 2020 21:26:22 +0800 Subject: [PATCH 4/4] add cheap check --- beacon-chain/core/blocks/attestation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon-chain/core/blocks/attestation.go b/beacon-chain/core/blocks/attestation.go index 983d36625ad8..d0c41940473c 100644 --- a/beacon-chain/core/blocks/attestation.go +++ b/beacon-chain/core/blocks/attestation.go @@ -255,7 +255,7 @@ func VerifyAttestations(ctx context.Context, beaconState *stateTrie.BeaconState, // VerifyAttestation converts and attestation into an indexed attestation and verifies // the signature in that attestation. func VerifyAttestation(ctx context.Context, beaconState *stateTrie.BeaconState, att *ethpb.Attestation) error { - if att == nil || att.Data == nil { + if att == nil || att.Data == nil || att.AggregationBits.Count() == 0 { return fmt.Errorf("nil or missing attestation data: %v", att) } committee, err := helpers.BeaconCommitteeFromState(beaconState, att.Data.Slot, att.Data.CommitteeIndex)