Strict verify attestations in pubsub (#4782)

* Verify attestations before putting them into the pool
* use existing method
* Validate aggregated ones too
* Revert "Validate aggregated ones too"

This reverts commit a55646d131.
* Merge branch 'master' of github.com:prysmaticlabs/prysm into verify-all-atts
* Add feature flag
* The remaining shared reference fields with conditional copy on write
* Merge branch 'master' into better-copy-2
* Merge branch 'better-copy-2' of github.com:prysmaticlabs/prysm into verify-all-atts
* gaz
* fix build, put into validate
* lint
* Merge branch 'master' of github.com:prysmaticlabs/prysm into verify-all-atts
* why does goland do this to me
* revert unrelated change
* fix tests
* Update shared/featureconfig/config.go

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>
* Merge refs/heads/master into verify-all-atts
* Update beacon-chain/blockchain/testing/mock.go

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>
* gofmt
This commit is contained in:
Preston Van Loon
2020-02-06 19:21:55 -08:00
committed by GitHub
parent 4df74a3b9d
commit 34178aff2a
13 changed files with 101 additions and 55 deletions

View File

@@ -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.

View File

@@ -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
}

View File

@@ -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()

View File

@@ -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(),

View File

@@ -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",

View File

@@ -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)
}

View File

@@ -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,

View File

@@ -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

View File

@@ -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: &ethpb.Checkpoint{
Epoch: 0,
}},

View File

@@ -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
}

View File

@@ -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: &ethpb.Attestation{
AggregationBits: bitfield.Bitlist{0b1010},
Data: &ethpb.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: &ethpb.Attestation{
AggregationBits: bitfield.Bitlist{0b1010},
Data: &ethpb.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)
}

View File

@@ -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)
}

View File

@@ -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.