From 558edabc78877531c8fe351f6f5de37515e2d62d Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Wed, 9 Dec 2020 14:09:28 -0600 Subject: [PATCH 01/10] flexible signing root --- .../standard-protection-format/import.go | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/validator/slashing-protection/local/standard-protection-format/import.go b/validator/slashing-protection/local/standard-protection-format/import.go index c45226f41656..4ff20962d371 100644 --- a/validator/slashing-protection/local/standard-protection-format/import.go +++ b/validator/slashing-protection/local/standard-protection-format/import.go @@ -207,17 +207,23 @@ func parseUniqueSignedAttestationsByPubKey(data []*ProtectionData) (map[[48]byte } func filterSlashablePubKeysFromBlocks(ctx context.Context, historyByPubKey map[[48]byte]kv.ProposalHistoryForPubkey) [][48]byte { - // We behave as strictly as possible and consider blocks with the same - // slot as slashable, as signing roots are optional in the EIP standard JSON file. + // Given signing roots are optional in the EIP standard, we behave as follows: + // For a given block: + // If we have a previous block with the same slot in our history: + // If signing root is nil, we consider that proposer public key as slashable + // If signing root is not nil , then we compare signing roots. If they are different, + // then we consider that proposer public key as slashable. slashablePubKeys := make([][48]byte, 0) for pubKey, proposals := range historyByPubKey { - seenSlots := make(map[uint64]bool) + seenSigningRootsBySlot := make(map[uint64][]byte) for _, blk := range proposals.Proposals { - if ok := seenSlots[blk.Slot]; ok { - slashablePubKeys = append(slashablePubKeys, pubKey) - break + if signingRoot, ok := seenSigningRootsBySlot[blk.Slot]; ok { + if signingRoot == nil || !bytes.Equal(signingRoot, blk.SigningRoot) { + slashablePubKeys = append(slashablePubKeys, pubKey) + break + } } - seenSlots[blk.Slot] = true + seenSigningRootsBySlot[blk.Slot] = blk.SigningRoot } } return slashablePubKeys From 7fdeb787c1bfad04bb72f7922d2773c61a585782 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Wed, 9 Dec 2020 14:19:10 -0600 Subject: [PATCH 02/10] add test --- .../standard-protection-format/import_test.go | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/validator/slashing-protection/local/standard-protection-format/import_test.go b/validator/slashing-protection/local/standard-protection-format/import_test.go index 990133feee06..0491d0c1a188 100644 --- a/validator/slashing-protection/local/standard-protection-format/import_test.go +++ b/validator/slashing-protection/local/standard-protection-format/import_test.go @@ -741,6 +741,44 @@ func Test_filterSlashablePubKeysFromBlocks(t *testing.T) { }, }, }, + { + name: "Considers nil signing roots and mismatched signing roots when determining slashable keys", + expected: [][48]byte{ + {2}, {3}, + }, + given: map[[48]byte][]*SignedBlock{ + // Different signing roots and same slot should not be slashable. + {1}: { + { + Slot: "1", + SigningRoot: fmt.Sprintf("%#x", [32]byte{1}), + }, + { + Slot: "1", + SigningRoot: fmt.Sprintf("%#x", [32]byte{1}), + }, + }, + // No signing root specified but same slot should be slashable. + {2}: { + { + Slot: "2", + }, + { + Slot: "2", + }, + }, + // No signing root in one slot, and same slot with signing root should be slashable. + {3}: { + { + Slot: "3", + }, + { + Slot: "3", + SigningRoot: fmt.Sprintf("%#x", [32]byte{3}), + }, + }, + }, + }, } for _, tt := range tests { tt := tt From ae8f83b77d3c22f16e4aa5d21bca45ea0050da22 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Wed, 9 Dec 2020 14:57:09 -0600 Subject: [PATCH 03/10] add tests --- validator/client/propose.go | 25 ++++++++++++------ validator/client/propose_protect.go | 32 ++++++++++++----------- validator/client/propose_protect_test.go | 33 ++++++++++++++---------- validator/client/validator.go | 2 +- 4 files changed, 54 insertions(+), 38 deletions(-) diff --git a/validator/client/propose.go b/validator/client/propose.go index 9c2187a14acf..a96728e0bad1 100644 --- a/validator/client/propose.go +++ b/validator/client/propose.go @@ -76,13 +76,6 @@ func (v *validator) ProposeBlock(ctx context.Context, slot uint64, pubKey [48]by return } - if err := v.preBlockSignValidations(ctx, pubKey, b); err != nil { - log.WithFields( - blockLogFields(pubKey, b, nil), - ).WithError(err).Error("Failed block slashing protection check") - return - } - // Sign returned block from beacon node sig, domain, err := v.signBlock(ctx, pubKey, epoch, b) if err != nil { @@ -97,7 +90,23 @@ func (v *validator) ProposeBlock(ctx context.Context, slot uint64, pubKey [48]by Signature: sig, } - if err := v.postBlockSignUpdate(ctx, pubKey, blk, domain); err != nil { + signingRoot, err := helpers.ComputeSigningRoot(b, domain.SignatureDomain) + if err != nil { + if v.emitAccountMetrics { + ValidatorProposeFailVec.WithLabelValues(fmtKey).Inc() + } + log.WithError(err).Error("Failed to compute signing root for block") + return + } + + if err := v.preBlockSignValidations(ctx, pubKey, b, signingRoot); err != nil { + log.WithFields( + blockLogFields(pubKey, b, nil), + ).WithError(err).Error("Failed block slashing protection check") + return + } + + if err := v.postBlockSignUpdate(ctx, pubKey, blk, signingRoot); err != nil { log.WithFields( blockLogFields(pubKey, b, sig), ).WithError(err).Error("Failed block slashing protection check") diff --git a/validator/client/propose_protect.go b/validator/client/propose_protect.go index ae3527f78a99..17c606d11cd8 100644 --- a/validator/client/propose_protect.go +++ b/validator/client/propose_protect.go @@ -6,9 +6,9 @@ import ( "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" - "github.com/prysmaticlabs/prysm/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/shared/blockutil" "github.com/prysmaticlabs/prysm/shared/featureconfig" + "github.com/prysmaticlabs/prysm/shared/params" "github.com/sirupsen/logrus" ) @@ -16,7 +16,9 @@ var failedPreBlockSignLocalErr = "attempted to sign a double proposal, block rej var failedPreBlockSignExternalErr = "attempted a double proposal, block rejected by remote slashing protection" var failedPostBlockSignErr = "made a double proposal, considered slashable by remote slashing protection" -func (v *validator) preBlockSignValidations(ctx context.Context, pubKey [48]byte, block *ethpb.BeaconBlock) error { +func (v *validator) preBlockSignValidations( + ctx context.Context, pubKey [48]byte, block *ethpb.BeaconBlock, signingRoot [32]byte, +) error { fmtKey := fmt.Sprintf("%#x", pubKey[:]) // Based on EIP3076, validator should refuse to sign any proposal with slot less @@ -33,17 +35,19 @@ func (v *validator) preBlockSignValidations(ctx context.Context, pubKey [48]byte ) } - _, exists, err = v.db.ProposalHistoryForSlot(ctx, pubKey, block.Slot) + prevSigningRoot, exists, err := v.db.ProposalHistoryForSlot(ctx, pubKey, block.Slot) if err != nil { if v.emitAccountMetrics { ValidatorProposeFailVec.WithLabelValues(fmtKey).Inc() } return errors.Wrap(err, "failed to get proposal history") } - // If a proposal exists in our history for the slot, we assume it is slashable. - // TODO(#7848): Add a more sophisticated strategy where if we indeed have the signing root, - // only blocks that have a conflicting signing root with a historical proposal are slashable. - if exists { + + // If a proposal exists in our history for the slot, we check the following: + // If the signing root is nil, then we consider it slashable. If signing root is not nil, + // we check if it is different than the incoming block's signing root. If that is the case, + // we consider that proposal slashable. + if exists && (prevSigningRoot == params.BeaconConfig().ZeroHash || prevSigningRoot != signingRoot) { if v.emitAccountMetrics { ValidatorProposeFailVec.WithLabelValues(fmtKey).Inc() } @@ -66,7 +70,12 @@ func (v *validator) preBlockSignValidations(ctx context.Context, pubKey [48]byte return nil } -func (v *validator) postBlockSignUpdate(ctx context.Context, pubKey [48]byte, block *ethpb.SignedBeaconBlock, domain *ethpb.DomainResponse) error { +func (v *validator) postBlockSignUpdate( + ctx context.Context, + pubKey [48]byte, + block *ethpb.SignedBeaconBlock, + signingRoot [32]byte, +) error { fmtKey := fmt.Sprintf("%#x", pubKey[:]) if featureconfig.Get().SlasherProtection && v.protector != nil { sbh, err := blockutil.SignedBeaconBlockHeaderFromBlock(block) @@ -84,13 +93,6 @@ func (v *validator) postBlockSignUpdate(ctx context.Context, pubKey [48]byte, bl return fmt.Errorf(failedPostBlockSignErr) } } - signingRoot, err := helpers.ComputeSigningRoot(block.Block, domain.SignatureDomain) - if err != nil { - if v.emitAccountMetrics { - ValidatorProposeFailVec.WithLabelValues(fmtKey).Inc() - } - return errors.Wrap(err, "failed to compute signing root for block") - } if err := v.db.SaveProposalHistoryForSlot(ctx, pubKey, block.Block.Slot, signingRoot[:]); err != nil { if v.emitAccountMetrics { ValidatorProposeFailVec.WithLabelValues(fmtKey).Inc() diff --git a/validator/client/propose_protect_test.go b/validator/client/propose_protect_test.go index 53991b3b50c3..fb1dc1c3f71d 100644 --- a/validator/client/propose_protect_test.go +++ b/validator/client/propose_protect_test.go @@ -30,7 +30,7 @@ func TestPreBlockSignLocalValidation_PreventsLowerThanMinProposal(t *testing.T) Slot: lowestSignedSlot - 1, ProposerIndex: 0, } - err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block) + err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{1}) require.ErrorContains(t, "could not sign block with slot <= lowest signed", err) // We expect the same block with a slot equal to the lowest @@ -39,7 +39,7 @@ func TestPreBlockSignLocalValidation_PreventsLowerThanMinProposal(t *testing.T) Slot: lowestSignedSlot, ProposerIndex: 0, } - err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block) + err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{2}) require.ErrorContains(t, "could not sign block with slot <= lowest signed", err) // We expect the same block with a slot > than the lowest @@ -48,7 +48,7 @@ func TestPreBlockSignLocalValidation_PreventsLowerThanMinProposal(t *testing.T) Slot: lowestSignedSlot + 1, ProposerIndex: 0, } - err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block) + err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{3}) require.NoError(t, err) } @@ -74,13 +74,18 @@ func TestPreBlockSignLocalValidation(t *testing.T) { require.NoError(t, err) // We save a proposal at slot 10 with a dummy signing root. - err = validator.db.SaveProposalHistoryForSlot(ctx, pubKeyBytes, 10, []byte{1}) + dummySigningRoot := [32]byte{1} + err = validator.db.SaveProposalHistoryForSlot(ctx, pubKeyBytes, 10, dummySigningRoot[:]) require.NoError(t, err) pubKey := [48]byte{} copy(pubKey[:], validatorKey.PublicKey().Marshal()) - // We expect the same block sent out should return slashable error. - err = validator.preBlockSignValidations(context.Background(), pubKey, block) + // We expect the same block sent out with the same root should not be slasahble. + err = validator.preBlockSignValidations(context.Background(), pubKey, block, dummySigningRoot) + require.NoError(t, err) + + // We expect the same block sent out with a different signing root should be slasahble. + err = validator.preBlockSignValidations(context.Background(), pubKey, block, [32]byte{2}) require.ErrorContains(t, failedPreBlockSignLocalErr, err) // We save a proposal at slot 11 with a nil signing root. @@ -90,13 +95,13 @@ func TestPreBlockSignLocalValidation(t *testing.T) { // We expect the same block sent out should return slashable error even // if we had a nil signing root stored in the database. - err = validator.preBlockSignValidations(context.Background(), pubKey, block) + err = validator.preBlockSignValidations(context.Background(), pubKey, block, [32]byte{2}) require.ErrorContains(t, failedPreBlockSignLocalErr, err) // A block with a different slot for which we do not have a proposing history // should not be failing validation. block.Slot = 9 - err = validator.preBlockSignValidations(context.Background(), pubKey, block) + err = validator.preBlockSignValidations(context.Background(), pubKey, block, [32]byte{3}) require.NoError(t, err, "Expected allowed block not to throw error") } @@ -118,11 +123,11 @@ func TestPreBlockSignValidation(t *testing.T) { } mockProtector := &mockSlasher.MockProtector{AllowBlock: false} validator.protector = mockProtector - err := validator.preBlockSignValidations(context.Background(), pubKey, block) + err := validator.preBlockSignValidations(context.Background(), pubKey, block, [32]byte{2}) require.ErrorContains(t, failedPreBlockSignExternalErr, err) mockProtector.AllowBlock = true - err = validator.preBlockSignValidations(context.Background(), pubKey, block) - require.NoError(t, err, "Expected allowed attestation not to throw error") + err = validator.preBlockSignValidations(context.Background(), pubKey, block, [32]byte{2}) + require.NoError(t, err, "Expected allowed block not to throw error") } func TestPostBlockSignUpdate(t *testing.T) { @@ -140,9 +145,9 @@ func TestPostBlockSignUpdate(t *testing.T) { emptyBlock.Block.ProposerIndex = 0 mockProtector := &mockSlasher.MockProtector{AllowBlock: false} validator.protector = mockProtector - err := validator.postBlockSignUpdate(context.Background(), pubKey, emptyBlock, nil) + err := validator.postBlockSignUpdate(context.Background(), pubKey, emptyBlock, [32]byte{}) require.ErrorContains(t, failedPostBlockSignErr, err, "Expected error when post signature update is detected as slashable") mockProtector.AllowBlock = true - err = validator.postBlockSignUpdate(context.Background(), pubKey, emptyBlock, ðpb.DomainResponse{SignatureDomain: make([]byte, 32)}) - require.NoError(t, err, "Expected allowed attestation not to throw error") + err = validator.postBlockSignUpdate(context.Background(), pubKey, emptyBlock, [32]byte{}) + require.NoError(t, err, "Expected allowed block not to throw error") } diff --git a/validator/client/validator.go b/validator/client/validator.go index 0de88b08d5a6..4d84d4d53c67 100644 --- a/validator/client/validator.go +++ b/validator/client/validator.go @@ -507,7 +507,7 @@ func (v *validator) RolesAt(ctx context.Context, slot uint64) (map[[48]byte][]Va func (v *validator) UpdateProtections(ctx context.Context, slot uint64) error { attestingPubKeys := make([][48]byte, 0, len(v.duties.CurrentEpochDuties)) for _, duty := range v.duties.CurrentEpochDuties { - if duty == nil || duty.AttesterSlot != slot { + if duty == nil || duty.AttesterSlot != slot-1 { continue } attestingPubKeys = append(attestingPubKeys, bytesutil.ToBytes48(duty.PublicKey)) From 67eaa89c54861e0e456ecbdc70dbaa3c59d13f61 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Wed, 9 Dec 2020 16:03:40 -0600 Subject: [PATCH 04/10] fix test --- validator/client/propose_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/validator/client/propose_test.go b/validator/client/propose_test.go index 9273e2728272..7f378f86fc15 100644 --- a/validator/client/propose_test.go +++ b/validator/client/propose_test.go @@ -205,12 +205,18 @@ func TestProposeBlock_BlocksDoubleProposal(t *testing.T) { m.validatorClient.EXPECT().GetBlock( gomock.Any(), // ctx gomock.Any(), - ).Times(2).Return(testBlock.Block, nil /*err*/) + ).Return(testBlock.Block, nil /*err*/) + + testBlock.Block.Body.Graffiti = []byte("someothergraffiti") + m.validatorClient.EXPECT().GetBlock( + gomock.Any(), // ctx + gomock.Any(), + ).Return(testBlock.Block, nil /*err*/) m.validatorClient.EXPECT().DomainData( gomock.Any(), // ctx gomock.Any(), //epoch - ).Return(ðpb.DomainResponse{SignatureDomain: make([]byte, 32)}, nil /*err*/) + ).Times(2).Return(ðpb.DomainResponse{SignatureDomain: make([]byte, 32)}, nil /*err*/) m.validatorClient.EXPECT().ProposeBlock( gomock.Any(), // ctx From b46d746f9d4a0dd09e7b59ed1b705f62ee20b2b1 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 11 Dec 2020 10:12:31 -0600 Subject: [PATCH 05/10] Preston's comments --- validator/client/propose_protect.go | 2 +- validator/client/validator.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/validator/client/propose_protect.go b/validator/client/propose_protect.go index 17c606d11cd8..ed5669645b40 100644 --- a/validator/client/propose_protect.go +++ b/validator/client/propose_protect.go @@ -44,7 +44,7 @@ func (v *validator) preBlockSignValidations( } // If a proposal exists in our history for the slot, we check the following: - // If the signing root is nil, then we consider it slashable. If signing root is not nil, + // If the signing root is empty (zero hash), then we consider it slashable. If signing root is not empty, // we check if it is different than the incoming block's signing root. If that is the case, // we consider that proposal slashable. if exists && (prevSigningRoot == params.BeaconConfig().ZeroHash || prevSigningRoot != signingRoot) { diff --git a/validator/client/validator.go b/validator/client/validator.go index 4d84d4d53c67..0de88b08d5a6 100644 --- a/validator/client/validator.go +++ b/validator/client/validator.go @@ -507,7 +507,7 @@ func (v *validator) RolesAt(ctx context.Context, slot uint64) (map[[48]byte][]Va func (v *validator) UpdateProtections(ctx context.Context, slot uint64) error { attestingPubKeys := make([][48]byte, 0, len(v.duties.CurrentEpochDuties)) for _, duty := range v.duties.CurrentEpochDuties { - if duty == nil || duty.AttesterSlot != slot-1 { + if duty == nil || duty.AttesterSlot != slot { continue } attestingPubKeys = append(attestingPubKeys, bytesutil.ToBytes48(duty.PublicKey)) From 210d362ecce1509957472a60752c39e4f6149612 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 11 Dec 2020 11:12:27 -0600 Subject: [PATCH 06/10] res tests --- validator/client/propose_test.go | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/validator/client/propose_test.go b/validator/client/propose_test.go index 7f378f86fc15..88d7c40dad79 100644 --- a/validator/client/propose_test.go +++ b/validator/client/propose_test.go @@ -190,8 +190,9 @@ func TestProposeBlock_BlocksDoubleProposal(t *testing.T) { pubKey := [48]byte{} copy(pubKey[:], validatorKey.PublicKey().Marshal()) + dummyRoot := [32]byte{} // Save a dummy proposal history at slot 0. - err := validator.db.SaveProposalHistoryForSlot(context.Background(), pubKey, 0, []byte{}) + err := validator.db.SaveProposalHistoryForSlot(context.Background(), pubKey, 0, dummyRoot[:]) require.NoError(t, err) m.validatorClient.EXPECT().DomainData( @@ -207,11 +208,15 @@ func TestProposeBlock_BlocksDoubleProposal(t *testing.T) { gomock.Any(), ).Return(testBlock.Block, nil /*err*/) - testBlock.Block.Body.Graffiti = []byte("someothergraffiti") + secondTestBlock := testutil.NewBeaconBlock() + secondTestBlock.Block.Slot = slot + graffiti := [32]byte{} + copy(graffiti[:], "someothergraffiti") + secondTestBlock.Block.Body.Graffiti = graffiti[:] m.validatorClient.EXPECT().GetBlock( gomock.Any(), // ctx gomock.Any(), - ).Return(testBlock.Block, nil /*err*/) + ).Return(secondTestBlock.Block, nil /*err*/) m.validatorClient.EXPECT().DomainData( gomock.Any(), // ctx @@ -237,8 +242,9 @@ func TestProposeBlock_BlocksDoubleProposal_After54KEpochs(t *testing.T) { pubKey := [48]byte{} copy(pubKey[:], validatorKey.PublicKey().Marshal()) + dummyRoot := [32]byte{} // Save a dummy proposal history at slot 0. - err := validator.db.SaveProposalHistoryForSlot(context.Background(), pubKey, 0, []byte{}) + err := validator.db.SaveProposalHistoryForSlot(context.Background(), pubKey, 0, dummyRoot[:]) require.NoError(t, err) m.validatorClient.EXPECT().DomainData( @@ -252,12 +258,22 @@ func TestProposeBlock_BlocksDoubleProposal_After54KEpochs(t *testing.T) { m.validatorClient.EXPECT().GetBlock( gomock.Any(), // ctx gomock.Any(), - ).Times(2).Return(testBlock.Block, nil /*err*/) + ).Return(testBlock.Block, nil /*err*/) + + secondTestBlock := testutil.NewBeaconBlock() + secondTestBlock.Block.Slot = farFuture + graffiti := [32]byte{} + copy(graffiti[:], "someothergraffiti") + secondTestBlock.Block.Body.Graffiti = graffiti[:] + m.validatorClient.EXPECT().GetBlock( + gomock.Any(), // ctx + gomock.Any(), + ).Return(secondTestBlock.Block, nil /*err*/) m.validatorClient.EXPECT().DomainData( gomock.Any(), // ctx gomock.Any(), //epoch - ).Return(ðpb.DomainResponse{SignatureDomain: make([]byte, 32)}, nil /*err*/) + ).Times(2).Return(ðpb.DomainResponse{SignatureDomain: make([]byte, 32)}, nil /*err*/) m.validatorClient.EXPECT().ProposeBlock( gomock.Any(), // ctx From 0df245dc9e0012119c4a3dd2be882a48d29cb464 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 11 Dec 2020 11:51:39 -0600 Subject: [PATCH 07/10] ensure we consider the case for minimum proposals --- validator/client/propose_protect.go | 36 ++++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/validator/client/propose_protect.go b/validator/client/propose_protect.go index ed5669645b40..48767c643217 100644 --- a/validator/client/propose_protect.go +++ b/validator/client/propose_protect.go @@ -21,21 +21,7 @@ func (v *validator) preBlockSignValidations( ) error { fmtKey := fmt.Sprintf("%#x", pubKey[:]) - // Based on EIP3076, validator should refuse to sign any proposal with slot less - // than or equal to the minimum signed proposal present in the DB for that public key. - lowestSignedProposalSlot, exists, err := v.db.LowestSignedProposal(ctx, pubKey) - if err != nil { - return err - } - if exists && lowestSignedProposalSlot >= block.Slot { - return fmt.Errorf( - "could not sign block with slot <= lowest signed slot in db, lowest signed slot: %d >= block slot: %d", - lowestSignedProposalSlot, - block.Slot, - ) - } - - prevSigningRoot, exists, err := v.db.ProposalHistoryForSlot(ctx, pubKey, block.Slot) + prevSigningRoot, proposalAtSlotExists, err := v.db.ProposalHistoryForSlot(ctx, pubKey, block.Slot) if err != nil { if v.emitAccountMetrics { ValidatorProposeFailVec.WithLabelValues(fmtKey).Inc() @@ -43,17 +29,35 @@ func (v *validator) preBlockSignValidations( return errors.Wrap(err, "failed to get proposal history") } + lowestSignedProposalSlot, lowestProposalExists, err := v.db.LowestSignedProposal(ctx, pubKey) + if err != nil { + return err + } + // If a proposal exists in our history for the slot, we check the following: // If the signing root is empty (zero hash), then we consider it slashable. If signing root is not empty, // we check if it is different than the incoming block's signing root. If that is the case, // we consider that proposal slashable. - if exists && (prevSigningRoot == params.BeaconConfig().ZeroHash || prevSigningRoot != signingRoot) { + signingRootIsDifferent := prevSigningRoot == params.BeaconConfig().ZeroHash || prevSigningRoot != signingRoot + if proposalAtSlotExists && signingRootIsDifferent { if v.emitAccountMetrics { ValidatorProposeFailVec.WithLabelValues(fmtKey).Inc() } return errors.New(failedPreBlockSignLocalErr) } + // Based on EIP3076, validator should refuse to sign any proposal with slot less + // than or equal to the minimum signed proposal present in the DB for that public key. + // In the case the slot of the incoming block is equal to the minimum signed proposal, we + // then also check the signing root is different. + if lowestProposalExists && signingRootIsDifferent && lowestSignedProposalSlot >= block.Slot { + return fmt.Errorf( + "could not sign block with slot <= lowest signed slot in db, lowest signed slot: %d >= block slot: %d", + lowestSignedProposalSlot, + block.Slot, + ) + } + if featureconfig.Get().SlasherProtection && v.protector != nil { blockHdr, err := blockutil.BeaconBlockHeaderFromBlock(block) if err != nil { From ee3273d37024f90503dbef4e9a5fb331b9bf83ea Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 11 Dec 2020 12:17:33 -0600 Subject: [PATCH 08/10] pass test --- validator/client/propose_protect_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/validator/client/propose_protect_test.go b/validator/client/propose_protect_test.go index fb1dc1c3f71d..649e9e0c2cf0 100644 --- a/validator/client/propose_protect_test.go +++ b/validator/client/propose_protect_test.go @@ -30,17 +30,26 @@ func TestPreBlockSignLocalValidation_PreventsLowerThanMinProposal(t *testing.T) Slot: lowestSignedSlot - 1, ProposerIndex: 0, } - err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{1}) + err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{4}) require.ErrorContains(t, "could not sign block with slot <= lowest signed", err) // We expect the same block with a slot equal to the lowest - // signed slot to fail validation. + // signed slot to pass validation if signing roots are equal. block = ðpb.BeaconBlock{ Slot: lowestSignedSlot, ProposerIndex: 0, } - err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{2}) - require.ErrorContains(t, "could not sign block with slot <= lowest signed", err) + err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{1}) + require.NoError(t, err) + + // We expect the same block with a slot equal to the lowest + // signed slot to fail validation if signing roots are different. + block = ðpb.BeaconBlock{ + Slot: lowestSignedSlot, + ProposerIndex: 0, + } + err = validator.preBlockSignValidations(context.Background(), pubKeyBytes, block, [32]byte{4}) + require.ErrorContains(t, failedPreBlockSignLocalErr, err) // We expect the same block with a slot > than the lowest // signed slot to pass validation. From 8cd31ebd6e4dfdd2c0c680f276d971fe25174b8e Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 11 Dec 2020 12:31:55 -0600 Subject: [PATCH 09/10] tests passing --- .../local/standard-protection-format/round_trip_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/validator/slashing-protection/local/standard-protection-format/round_trip_test.go b/validator/slashing-protection/local/standard-protection-format/round_trip_test.go index 5f4c823051d7..fba57eccdd1d 100644 --- a/validator/slashing-protection/local/standard-protection-format/round_trip_test.go +++ b/validator/slashing-protection/local/standard-protection-format/round_trip_test.go @@ -128,10 +128,12 @@ func TestImportInterchangeData_OK_SavesBlacklistedPublicKeys(t *testing.T) { standardProtectionFormat.Data[0].SignedBlocks = append( standardProtectionFormat.Data[0].SignedBlocks, &protectionFormat.SignedBlock{ - Slot: "700", + Slot: "700", + SigningRoot: fmt.Sprintf("%#x", [32]byte{1}), }, &protectionFormat.SignedBlock{ - Slot: "700", + Slot: "700", + SigningRoot: fmt.Sprintf("%#x", [32]byte{2}), }, ) @@ -143,10 +145,12 @@ func TestImportInterchangeData_OK_SavesBlacklistedPublicKeys(t *testing.T) { &protectionFormat.SignedAttestation{ TargetEpoch: "700", SourceEpoch: "699", + SigningRoot: fmt.Sprintf("%#x", [32]byte{1}), }, &protectionFormat.SignedAttestation{ TargetEpoch: "700", SourceEpoch: "699", + SigningRoot: fmt.Sprintf("%#x", [32]byte{2}), }, ) From 1449a0a7cad23db5f1e8e7f951718248c1f231e1 Mon Sep 17 00:00:00 2001 From: Raul Jordan Date: Fri, 11 Dec 2020 15:02:24 -0600 Subject: [PATCH 10/10] rem unused code --- beacon-chain/state/stateutil/attestations.go | 60 -------------------- validator/client/validator.go | 1 - 2 files changed, 61 deletions(-) diff --git a/beacon-chain/state/stateutil/attestations.go b/beacon-chain/state/stateutil/attestations.go index c63f23e8e329..9eddcd09b362 100644 --- a/beacon-chain/state/stateutil/attestations.go +++ b/beacon-chain/state/stateutil/attestations.go @@ -84,66 +84,6 @@ func marshalAttestationData(data *ethpb.AttestationData) []byte { return enc } -func attestationRoot(hasher htrutils.HashFn, att *ethpb.Attestation) ([32]byte, error) { - fieldRoots := make([][32]byte, 3) - - // Bitfield. - aggregationRoot, err := htrutils.BitlistRoot(hasher, att.AggregationBits, params.BeaconConfig().MaxValidatorsPerCommittee) - if err != nil { - return [32]byte{}, err - } - fieldRoots[0] = aggregationRoot - - dataRoot, err := attestationDataRoot(hasher, att.Data) - if err != nil { - return [32]byte{}, err - } - fieldRoots[1] = dataRoot - - signatureBuf := bytesutil.ToBytes96(att.Signature) - packedSig, err := htrutils.Pack([][]byte{signatureBuf[:]}) - if err != nil { - return [32]byte{}, err - } - sigRoot, err := htrutils.BitwiseMerkleize(hasher, packedSig, uint64(len(packedSig)), uint64(len(packedSig))) - if err != nil { - return [32]byte{}, err - } - fieldRoots[2] = sigRoot - return htrutils.BitwiseMerkleizeArrays(hasher, fieldRoots, uint64(len(fieldRoots)), uint64(len(fieldRoots))) -} - -func blockAttestationRoot(atts []*ethpb.Attestation) ([32]byte, error) { - hasher := hashutil.CustomSHA256Hasher() - roots := make([][]byte, len(atts)) - for i := 0; i < len(atts); i++ { - pendingRoot, err := attestationRoot(hasher, atts[i]) - if err != nil { - return [32]byte{}, errors.Wrap(err, "could not attestation merkleization") - } - roots[i] = pendingRoot[:] - } - - attsRootsRoot, err := htrutils.BitwiseMerkleize( - hasher, - roots, - uint64(len(roots)), - params.BeaconConfig().MaxAttestations, - ) - if err != nil { - return [32]byte{}, errors.Wrap(err, "could not compute block attestations merkleization") - } - attsLenBuf := new(bytes.Buffer) - if err := binary.Write(attsLenBuf, binary.LittleEndian, uint64(len(atts))); err != nil { - return [32]byte{}, errors.Wrap(err, "could not marshal epoch attestations length") - } - // We need to mix in the length of the slice. - attsLenRoot := make([]byte, 32) - copy(attsLenRoot, attsLenBuf.Bytes()) - res := htrutils.MixInLength(attsRootsRoot, attsLenRoot) - return res, nil -} - func attestationDataRoot(hasher htrutils.HashFn, data *ethpb.AttestationData) ([32]byte, error) { fieldRoots := make([][]byte, 5) diff --git a/validator/client/validator.go b/validator/client/validator.go index c3a84b007d57..bc4918617e3e 100644 --- a/validator/client/validator.go +++ b/validator/client/validator.go @@ -58,7 +58,6 @@ type validator struct { attLogsLock sync.Mutex aggregatedSlotCommitteeIDCacheLock sync.Mutex prevBalanceLock sync.RWMutex - attesterHistoryByPubKeyLock sync.RWMutex slashableKeysLock sync.RWMutex walletInitializedFeed *event.Feed genesisTime uint64