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

Change BLS Public Key Length Check in Beacon RPC Server #1571

Merged
merged 11 commits into from
Feb 13, 2019
Merged

Change BLS Public Key Length Check in Beacon RPC Server #1571

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5843c3a
change to empty check
Feb 12, 2019
641cf22
Merge branch 'less-strict-length' of github.com:prysmaticlabs/prysm i…
Feb 12, 2019
683b51f
resolve comments
Feb 12, 2019
6715c5e
formatting
rauljordan Feb 12, 2019
6481d37
fix length tests
Feb 12, 2019
656b6b4
error in array init
Feb 12, 2019
5cabdc4
nonconstant array error fix
rauljordan Feb 12, 2019
b1beec9
Merge branch 'master' into less-strict-length
Feb 13, 2019
da8369b
Merge branch 'master' into less-strict-length
rauljordan Feb 13, 2019
f75e453
Merge branch 'master' into less-strict-length
rauljordan Feb 13, 2019
81ac9df
Merge branch 'master' into less-strict-length
rauljordan Feb 13, 2019
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
8 changes: 6 additions & 2 deletions beacon-chain/rpc/validator_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,12 @@ func (vs *ValidatorServer) ValidatorEpochAssignments(
ctx context.Context,
req *pb.ValidatorEpochAssignmentsRequest,
) (*pb.ValidatorEpochAssignmentsResponse, error) {
if len(req.PublicKey) != 48 {
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? Didn't we have panics where invalid public keys were sent?

Copy link
Member

Choose a reason for hiding this comment

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

IMO, this needs to be fixed to the expectation size. If it's 96 now then it should be 96.

Copy link
Member

Choose a reason for hiding this comment

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

pub key size should really become its own variable if it's referenced in multiple places

Copy link
Member

Choose a reason for hiding this comment

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

yes, it can be a params constant

return nil, fmt.Errorf("expected 48 byte public key, received %d", len(req.PublicKey))
if len(req.PublicKey) != params.BeaconConfig().BLSPubkeyLength {
return nil, fmt.Errorf(
"expected public key to have length %d, received %d",
params.BeaconConfig().BLSPubkeyLength,
len(req.PublicKey),
)
}
beaconState, err := vs.beaconDB.State()
if err != nil {
Expand Down
26 changes: 22 additions & 4 deletions beacon-chain/rpc/validator_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package rpc

import (
"context"
"fmt"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -69,7 +70,7 @@ func TestValidatorEpochAssignments_Ok(t *testing.T) {
genesisTime := params.BeaconConfig().GenesisTime.Unix()
deposits := make([]*pbp2p.Deposit, params.BeaconConfig().DepositsForChainStart)
for i := 0; i < len(deposits); i++ {
var pubKey [48]byte
var pubKey [96]byte
copy(pubKey[:], []byte(strconv.Itoa(i)))
depositInput := &pbp2p.DepositInput{
Pubkey: pubKey[:],
Expand All @@ -96,7 +97,7 @@ func TestValidatorEpochAssignments_Ok(t *testing.T) {
validatorServer := &ValidatorServer{
beaconDB: db,
}
var pubKey [48]byte
var pubKey [96]byte
copy(pubKey[:], []byte("0"))
req := &pb.ValidatorEpochAssignmentsRequest{
EpochStart: params.BeaconConfig().GenesisSlot,
Expand Down Expand Up @@ -125,6 +126,23 @@ func TestValidatorEpochAssignments_Ok(t *testing.T) {
}
}

func TestValidatorEpochAssignments_WrongPubkeyLength(t *testing.T) {
db := internal.SetupDB(t)
defer internal.TeardownDB(t, db)

validatorServer := &ValidatorServer{
beaconDB: db,
}
req := &pb.ValidatorEpochAssignmentsRequest{
EpochStart: params.BeaconConfig().GenesisSlot,
PublicKey: []byte{},
}
want := fmt.Sprintf("expected public key to have length %d", params.BeaconConfig().BLSPubkeyLength)
if _, err := validatorServer.ValidatorEpochAssignments(context.Background(), req); !strings.Contains(err.Error(), want) {
t.Errorf("Expected %v, received %v", want, err)
}
}

func TestValidatorCommitteeAtSlot_CrosslinkCommitteesFailure(t *testing.T) {
db := internal.SetupDB(t)
defer internal.TeardownDB(t, db)
Expand All @@ -136,7 +154,7 @@ func TestValidatorCommitteeAtSlot_CrosslinkCommitteesFailure(t *testing.T) {
genesisTime := params.BeaconConfig().GenesisTime.Unix()
deposits := make([]*pbp2p.Deposit, params.BeaconConfig().DepositsForChainStart)
for i := 0; i < len(deposits); i++ {
var pubKey [48]byte
var pubKey [96]byte
copy(pubKey[:], []byte(strconv.Itoa(i)))
depositInput := &pbp2p.DepositInput{
Pubkey: pubKey[:],
Expand Down Expand Up @@ -182,7 +200,7 @@ func TestValidatorCommitteeAtSlot_Ok(t *testing.T) {
genesisTime := params.BeaconConfig().GenesisTime.Unix()
deposits := make([]*pbp2p.Deposit, params.BeaconConfig().DepositsForChainStart)
for i := 0; i < len(deposits); i++ {
var pubKey [48]byte
var pubKey [96]byte
copy(pubKey[:], []byte(strconv.Itoa(i)))
depositInput := &pbp2p.DepositInput{
Pubkey: pubKey[:],
Expand Down
3 changes: 3 additions & 0 deletions shared/params/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type BeaconChainConfig struct {
LatestPenalizedExitLength uint64 // LatestPenalizedExitLength is used to track penalized exit balances per time interval.
LatestIndexRootsLength uint64 // LatestIndexRootsLength is the number of index roots kept in beacon state, used by light client.
MaxWithdrawalsPerEpoch uint64 // MaxWithdrawalsPerEpoch is the max withdrawals can happen for a single epoch.
BLSPubkeyLength int // BLSPubkeyLength defines the expected length of BLS public keys in bytes.

// Deposit contract constants.
DepositContractAddress []byte // DepositContractAddress is the address of the deposit contract in PoW chain.
Expand Down Expand Up @@ -104,6 +105,7 @@ var defaultBeaconConfig = &BeaconChainConfig{
LatestPenalizedExitLength: 8192,
LatestIndexRootsLength: 8192,
MaxWithdrawalsPerEpoch: 4,
BLSPubkeyLength: 96,

// Deposit contract constants.
DepositContractTreeDepth: 32,
Expand Down Expand Up @@ -165,6 +167,7 @@ var demoBeaconConfig = &BeaconChainConfig{
LatestPenalizedExitLength: defaultBeaconConfig.LatestPenalizedExitLength,
LatestIndexRootsLength: defaultBeaconConfig.LatestIndexRootsLength,
MaxWithdrawalsPerEpoch: defaultBeaconConfig.MaxWithdrawalsPerEpoch,
BLSPubkeyLength: defaultBeaconConfig.BLSPubkeyLength,

// Deposit contract constants.
DepositContractTreeDepth: defaultBeaconConfig.DepositContractTreeDepth,
Expand Down