diff --git a/beacon-chain/blockchain/receive_attestation.go b/beacon-chain/blockchain/receive_attestation.go index 704a320a25..04ea6dd26a 100644 --- a/beacon-chain/blockchain/receive_attestation.go +++ b/beacon-chain/blockchain/receive_attestation.go @@ -7,6 +7,7 @@ import ( "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" + "github.com/prysmaticlabs/prysm/beacon-chain/core/blocks" "github.com/prysmaticlabs/prysm/beacon-chain/core/feed" "github.com/prysmaticlabs/prysm/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/shared/bytesutil" @@ -19,6 +20,7 @@ import ( // AttestationReceiver interface defines the methods of chain service receive and processing new attestations. type AttestationReceiver interface { ReceiveAttestationNoPubsub(ctx context.Context, att *ethpb.Attestation) error + IsValidAttestation(ctx context.Context, att *ethpb.Attestation) bool } // ReceiveAttestationNoPubsub is a function that defines the operations that are preformed on @@ -41,6 +43,22 @@ func (s *Service) ReceiveAttestationNoPubsub(ctx context.Context, att *ethpb.Att return nil } +// IsValidAttestation returns true if the attestation can be verified against its pre-state. +func (s *Service) IsValidAttestation(ctx context.Context, att *ethpb.Attestation) bool { + baseState, err := s.getAttPreState(ctx, att.Data.Target) + if err != nil { + log.WithError(err).Error("Failed to validate attestation") + return false + } + + if err := blocks.VerifyAttestation(ctx, baseState, att); err != nil { + log.WithError(err).Error("Failed to validate attestation") + return false + } + + return true +} + // This processes attestations from the attestation pool to account for validator votes and fork choice. func (s *Service) processAttestation() { // Wait for state to be initialized. diff --git a/beacon-chain/blockchain/testing/mock.go b/beacon-chain/blockchain/testing/mock.go index 4b5b86f77b..0ce7f9c44f 100644 --- a/beacon-chain/blockchain/testing/mock.go +++ b/beacon-chain/blockchain/testing/mock.go @@ -37,6 +37,7 @@ type ChainService struct { stateNotifier statefeed.Notifier blockNotifier blockfeed.Notifier opNotifier opfeed.Notifier + ValidAttestation bool } // StateNotifier mocks the same method in the chain service. @@ -225,3 +226,8 @@ func (ms *ChainService) CurrentSlot() uint64 { func (ms *ChainService) Participation(epoch uint64) *precompute.Balance { return ms.Balance } + +// IsValidAttestation always returns true. +func (ms *ChainService) IsValidAttestation(ctx context.Context, att *ethpb.Attestation) bool { + return ms.ValidAttestation +} diff --git a/beacon-chain/state/setters.go b/beacon-chain/state/setters.go index 4c2fdd8f32..c0b9bada84 100644 --- a/beacon-chain/state/setters.go +++ b/beacon-chain/state/setters.go @@ -325,7 +325,7 @@ func (b *BeaconState) SetBalances(val []uint64) error { defer b.lock.Unlock() b.sharedFieldReferences[balances].refs-- - b.sharedFieldReferences[balances] = &reference{refs:1} + b.sharedFieldReferences[balances] = &reference{refs: 1} b.state.Balances = val b.markFieldAsDirty(balances) @@ -457,7 +457,6 @@ func (b *BeaconState) SetCurrentEpochAttestations(val []*pbp2p.PendingAttestatio b.lock.Lock() defer b.lock.Unlock() - b.sharedFieldReferences[currentEpochAttestations].refs-- b.sharedFieldReferences[currentEpochAttestations] = &reference{refs: 1} @@ -515,7 +514,7 @@ func (b *BeaconState) AppendPreviousEpochAttestations(val *pbp2p.PendingAttestat if b.sharedFieldReferences[previousEpochAttestations].refs > 1 { atts = b.PreviousEpochAttestations() b.sharedFieldReferences[previousEpochAttestations].refs-- - b.sharedFieldReferences[previousEpochAttestations] = &reference{refs:1} + b.sharedFieldReferences[previousEpochAttestations] = &reference{refs: 1} } b.lock.RUnlock() @@ -535,7 +534,7 @@ func (b *BeaconState) AppendValidator(val *ethpb.Validator) error { if b.sharedFieldReferences[validators].refs > 1 { vals = b.Validators() b.sharedFieldReferences[validators].refs-- - b.sharedFieldReferences[validators] = &reference{refs:1} + b.sharedFieldReferences[validators] = &reference{refs: 1} } b.lock.RUnlock() @@ -556,7 +555,7 @@ func (b *BeaconState) AppendBalance(bal uint64) error { if b.sharedFieldReferences[balances].refs > 1 { bals = b.Balances() b.sharedFieldReferences[balances].refs-- - b.sharedFieldReferences[balances] = &reference{refs:1} + b.sharedFieldReferences[balances] = &reference{refs: 1} } b.lock.RUnlock() diff --git a/beacon-chain/state/types.go b/beacon-chain/state/types.go index 4e8e3ac622..67a41d3bc5 100644 --- a/beacon-chain/state/types.go +++ b/beacon-chain/state/types.go @@ -103,7 +103,6 @@ func (b *BeaconState) Copy() *BeaconState { Balances: b.state.Balances, HistoricalRoots: b.state.HistoricalRoots, - // Everything else, too small to be concerned about, constant size. Fork: b.Fork(), LatestBlockHeader: b.LatestBlockHeader(), diff --git a/beacon-chain/sync/BUILD.bazel b/beacon-chain/sync/BUILD.bazel index 84d11975b7..475aca2f17 100644 --- a/beacon-chain/sync/BUILD.bazel +++ b/beacon-chain/sync/BUILD.bazel @@ -53,6 +53,7 @@ go_library( "//shared/attestationutil:go_default_library", "//shared/bls:go_default_library", "//shared/bytesutil:go_default_library", + "//shared/featureconfig:go_default_library", "//shared/messagehandler:go_default_library", "//shared/params:go_default_library", "//shared/roughtime:go_default_library", diff --git a/beacon-chain/sync/subscriber_committee_index_beacon_attestation.go b/beacon-chain/sync/subscriber_committee_index_beacon_attestation.go index 12ead0f03b..aa8f3518ac 100644 --- a/beacon-chain/sync/subscriber_committee_index_beacon_attestation.go +++ b/beacon-chain/sync/subscriber_committee_index_beacon_attestation.go @@ -14,6 +14,11 @@ func (r *Service) committeeIndexBeaconAttestationSubscriber(ctx context.Context, if !ok { return fmt.Errorf("message was not type *eth.Attestation, type=%T", msg) } + + if exists, _ := r.attPool.HasAggregatedAttestation(a); exists { + return nil + } + return r.attPool.SaveUnaggregatedAttestation(a) } diff --git a/beacon-chain/sync/subscriber_committee_index_beacon_attestation_test.go b/beacon-chain/sync/subscriber_committee_index_beacon_attestation_test.go index 8e52ffca02..135e78ecac 100644 --- a/beacon-chain/sync/subscriber_committee_index_beacon_attestation_test.go +++ b/beacon-chain/sync/subscriber_committee_index_beacon_attestation_test.go @@ -42,8 +42,9 @@ func TestService_committeeIndexBeaconAttestationSubscriber_ValidMessage(t *testi r := &Service{ attPool: attestations.NewPool(), chain: &mock.ChainService{ - State: s, - Genesis: time.Now(), + State: s, + Genesis: time.Now(), + ValidAttestation: true, }, chainStarted: true, p2p: p, diff --git a/beacon-chain/sync/validate_aggregate_proof.go b/beacon-chain/sync/validate_aggregate_proof.go index fdcd326a72..67403d1faf 100644 --- a/beacon-chain/sync/validate_aggregate_proof.go +++ b/beacon-chain/sync/validate_aggregate_proof.go @@ -16,6 +16,7 @@ import ( "github.com/prysmaticlabs/prysm/shared/attestationutil" "github.com/prysmaticlabs/prysm/shared/bls" "github.com/prysmaticlabs/prysm/shared/bytesutil" + "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/params" "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/prysmaticlabs/prysm/shared/traceutil" @@ -63,6 +64,10 @@ func (r *Service) validateAggregateAndProof(ctx context.Context, pid peer.ID, ms return false } + if !featureconfig.Get().DisableStrictAttestationPubsubVerification && !r.chain.IsValidAttestation(ctx, m.Aggregate) { + return false + } + msg.ValidatorData = m return true diff --git a/beacon-chain/sync/validate_aggregate_proof_test.go b/beacon-chain/sync/validate_aggregate_proof_test.go index a6e40369c7..c4884d2d9b 100644 --- a/beacon-chain/sync/validate_aggregate_proof_test.go +++ b/beacon-chain/sync/validate_aggregate_proof_test.go @@ -357,7 +357,8 @@ func TestValidateAggregateAndProof_CanValidate(t *testing.T) { db: db, initialSync: &mockSync.Sync{IsSyncing: false}, chain: &mock.ChainService{Genesis: time.Now(), - State: beaconState, + State: beaconState, + ValidAttestation: true, FinalizedCheckPoint: ðpb.Checkpoint{ Epoch: 0, }}, diff --git a/beacon-chain/sync/validate_committee_index_beacon_attestation.go b/beacon-chain/sync/validate_committee_index_beacon_attestation.go index ff93fb1cbc..ecd98426b8 100644 --- a/beacon-chain/sync/validate_committee_index_beacon_attestation.go +++ b/beacon-chain/sync/validate_committee_index_beacon_attestation.go @@ -11,8 +11,8 @@ import ( eth "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" "github.com/prysmaticlabs/prysm/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/beacon-chain/p2p" - "github.com/prysmaticlabs/prysm/shared/bls" "github.com/prysmaticlabs/prysm/shared/bytesutil" + "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/params" "github.com/prysmaticlabs/prysm/shared/traceutil" "go.opencensus.io/trace" @@ -80,8 +80,8 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p return false } - // Attestation's signature is a valid BLS signature. - if _, err := bls.SignatureFromBytes(att.Signature); err != nil { + // Attestation's signature is a valid BLS signature and belongs to correct public key.. + if !featureconfig.Get().DisableStrictAttestationPubsubVerification && !s.chain.IsValidAttestation(ctx, att) { return false } diff --git a/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go b/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go index b948876780..34e610db57 100644 --- a/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go +++ b/beacon-chain/sync/validate_committee_index_beacon_attestation_test.go @@ -15,7 +15,6 @@ import ( dbtest "github.com/prysmaticlabs/prysm/beacon-chain/db/testing" p2ptest "github.com/prysmaticlabs/prysm/beacon-chain/p2p/testing" mockSync "github.com/prysmaticlabs/prysm/beacon-chain/sync/initial-sync/testing" - "github.com/prysmaticlabs/prysm/shared/bls" "github.com/prysmaticlabs/prysm/shared/params" ) @@ -24,13 +23,15 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { p := p2ptest.NewTestP2P(t) db := dbtest.SetupDB(t) defer dbtest.TeardownDB(t, db) + chain := &mockChain.ChainService{ + Genesis: time.Now().Add(time.Duration(-64*int64(params.BeaconConfig().SecondsPerSlot)) * time.Second), // 64 slots ago + ValidAttestation: true, + } s := &Service{ - initialSync: &mockSync.Sync{IsSyncing: false}, - p2p: p, - db: db, - chain: &mockChain.ChainService{ - Genesis: time.Now().Add(time.Duration(-64*int64(params.BeaconConfig().SecondsPerSlot)) * time.Second), // 64 slots ago - }, + initialSync: &mockSync.Sync{IsSyncing: false}, + p2p: p, + db: db, + chain: chain, blkRootToPendingAtts: make(map[[32]byte][]*ethpb.AggregateAttestationAndProof), } @@ -48,16 +49,15 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { t.Fatal(err) } - validSig := bls.RandKey().Sign([]byte("foo"), 0).Marshal() - tests := []struct { - name string - msg *ethpb.Attestation - topic string - want bool + name string + msg *ethpb.Attestation + topic string + validAttestationSignature bool + want bool }{ { - name: "valid", + name: "validAttestationSignature", msg: ðpb.Attestation{ AggregationBits: bitfield.Bitlist{0b1010}, Data: ðpb.AttestationData{ @@ -65,10 +65,10 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { CommitteeIndex: 1, Slot: 63, }, - Signature: validSig, }, - topic: "/eth2/committee_index1_beacon_attestation", - want: true, + topic: "/eth2/committee_index1_beacon_attestation", + validAttestationSignature: true, + want: true, }, { name: "wrong committee index", @@ -79,10 +79,10 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { CommitteeIndex: 2, Slot: 63, }, - Signature: validSig, }, - topic: "/eth2/committee_index3_beacon_attestation", - want: false, + topic: "/eth2/committee_index3_beacon_attestation", + validAttestationSignature: true, + want: false, }, { name: "already aggregated", @@ -93,10 +93,10 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { CommitteeIndex: 1, Slot: 63, }, - Signature: validSig, }, - topic: "/eth2/committee_index1_beacon_attestation", - want: false, + topic: "/eth2/committee_index1_beacon_attestation", + validAttestationSignature: true, + want: false, }, { name: "missing block", @@ -107,13 +107,13 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { CommitteeIndex: 1, Slot: 63, }, - Signature: validSig, }, - topic: "/eth2/committee_index1_beacon_attestation", - want: false, + topic: "/eth2/committee_index1_beacon_attestation", + validAttestationSignature: true, + want: false, }, { - name: "invalid sig", + name: "invalid attestation", msg: ðpb.Attestation{ AggregationBits: bitfield.Bitlist{0b1010}, Data: ðpb.AttestationData{ @@ -121,10 +121,10 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { CommitteeIndex: 1, Slot: 63, }, - Signature: []byte("bad"), }, - topic: "/eth2/committee_index1_beacon_attestation", - want: false, + topic: "/eth2/committee_index1_beacon_attestation", + validAttestationSignature: false, + want: false, }, } @@ -141,6 +141,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { TopicIDs: []string{tt.topic}, }, } + chain.ValidAttestation = tt.validAttestationSignature if s.validateCommitteeIndexBeaconAttestation(ctx, "" /*peerID*/, m) != tt.want { t.Errorf("Did not received wanted validation. Got %v, wanted %v", !tt.want, tt.want) } diff --git a/shared/featureconfig/config.go b/shared/featureconfig/config.go index 6f03233d98..5f049ab99f 100644 --- a/shared/featureconfig/config.go +++ b/shared/featureconfig/config.go @@ -28,19 +28,20 @@ var log = logrus.WithField("prefix", "flags") // Flags is a struct to represent which features the client will perform on runtime. type Flags struct { - CustomGenesisDelay uint64 // CustomGenesisDelay signals how long of a delay to set to start the chain. - MinimalConfig bool // MinimalConfig as defined in the spec. - WriteSSZStateTransitions bool // WriteSSZStateTransitions to tmp directory. - InitSyncNoVerify bool // InitSyncNoVerify when initial syncing w/o verifying block's contents. - SkipBLSVerify bool // Skips BLS verification across the runtime. - EnableBackupWebhook bool // EnableBackupWebhook to allow database backups to trigger from monitoring port /db/backup. - PruneEpochBoundaryStates bool // PruneEpochBoundaryStates prunes the epoch boundary state before last finalized check point. - EnableSnappyDBCompression bool // EnableSnappyDBCompression in the database. - InitSyncCacheState bool // InitSyncCacheState caches state during initial sync. - KafkaBootstrapServers string // KafkaBootstrapServers to find kafka servers to stream blocks, attestations, etc. - ProtectProposer bool // ProtectProposer prevents the validator client from signing any proposals that would be considered a slashable offense. - ProtectAttester bool // ProtectAttester prevents the validator client from signing any attestations that would be considered a slashable offense. - ForkchoiceAggregateAttestations bool // ForkchoiceAggregateAttestations attempts to aggregate attestations before processing in fork choice. + CustomGenesisDelay uint64 // CustomGenesisDelay signals how long of a delay to set to start the chain. + MinimalConfig bool // MinimalConfig as defined in the spec. + WriteSSZStateTransitions bool // WriteSSZStateTransitions to tmp directory. + InitSyncNoVerify bool // InitSyncNoVerify when initial syncing w/o verifying block's contents. + SkipBLSVerify bool // Skips BLS verification across the runtime. + EnableBackupWebhook bool // EnableBackupWebhook to allow database backups to trigger from monitoring port /db/backup. + PruneEpochBoundaryStates bool // PruneEpochBoundaryStates prunes the epoch boundary state before last finalized check point. + EnableSnappyDBCompression bool // EnableSnappyDBCompression in the database. + InitSyncCacheState bool // InitSyncCacheState caches state during initial sync. + KafkaBootstrapServers string // KafkaBootstrapServers to find kafka servers to stream blocks, attestations, etc. + ProtectProposer bool // ProtectProposer prevents the validator client from signing any proposals that would be considered a slashable offense. + ProtectAttester bool // ProtectAttester prevents the validator client from signing any attestations that would be considered a slashable offense. + ForkchoiceAggregateAttestations bool // ForkchoiceAggregateAttestations attempts to aggregate attestations before processing in fork choice. + DisableStrictAttestationPubsubVerification bool // DisableStrictAttestationPubsubVerification will disabling strict signature verification in pubsub. // DisableForkChoice disables using LMD-GHOST fork choice to update // the head of the chain based on attestations and instead accepts any valid received block @@ -143,6 +144,10 @@ func ConfigureBeaconChain(ctx *cli.Context) { log.Warn("Enabled fork choice aggregation pre-processing of attestations") cfg.ForkchoiceAggregateAttestations = true } + if ctx.GlobalBool(disableStrictAttestationPubsubVerificationFlag.Name) { + log.Warn("Disabled strict attestation signature verification in pubsub") + cfg.DisableStrictAttestationPubsubVerification = true + } Init(cfg) } diff --git a/shared/featureconfig/flags.go b/shared/featureconfig/flags.go index 88653127f0..548508b2a0 100644 --- a/shared/featureconfig/flags.go +++ b/shared/featureconfig/flags.go @@ -94,6 +94,10 @@ var ( Name: "forkchoice-aggregate-attestations", Usage: "Preprocess attestations by aggregation before running fork choice.", } + disableStrictAttestationPubsubVerificationFlag = cli.BoolFlag{ + Name: "disable-strict-attestation-pubsub-verification", + Usage: "Disable strict signature verification of attestations in pubsub. See PR 4782 for details.", + } ) // Deprecated flags list. @@ -244,6 +248,7 @@ var BeaconChainFlags = append(deprecatedFlags, []cli.Flag{ enableSlasherFlag, cacheFilteredBlockTreeFlag, forkchoiceAggregateAttestations, + disableStrictAttestationPubsubVerificationFlag, }...) // E2EBeaconChainFlags contains a list of the beacon chain feature flags to be tested in E2E.