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.

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

Add back State To ComputeProposerIndex #5674

Merged
merged 9 commits into from
Apr 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions beacon-chain/core/helpers/committee.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,11 +372,10 @@ func precomputeProposerIndices(state *stateTrie.BeaconState, activeIndices []uin
return nil, errors.Wrap(err, "could not generate seed")
}
slot := StartSlot(e)
vals := state.Validators()
for i := uint64(0); i < params.BeaconConfig().SlotsPerEpoch; i++ {
seedWithSlot := append(seed[:], bytesutil.Bytes8(slot+i)...)
seedWithSlotHash := hashFunc(seedWithSlot)
index, err := ComputeProposerIndex(vals, activeIndices, seedWithSlotHash)
index, err := ComputeProposerIndex(state, activeIndices, seedWithSlotHash)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion beacon-chain/core/helpers/committee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ func TestPrecomputeProposerIndices_Ok(t *testing.T) {
for i := uint64(0); i < params.BeaconConfig().SlotsPerEpoch; i++ {
seedWithSlot := append(seed[:], bytesutil.Bytes8(i)...)
seedWithSlotHash := hashutil.Hash(seedWithSlot)
index, err := ComputeProposerIndex(state.Validators(), indices, seedWithSlotHash)
index, err := ComputeProposerIndex(state, indices, seedWithSlotHash)
if err != nil {
t.Fatal(err)
}
Expand Down
57 changes: 55 additions & 2 deletions beacon-chain/core/helpers/validators.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,63 @@ func BeaconProposerIndex(state *stateTrie.BeaconState) (uint64, error) {
return 0, errors.Wrap(err, "could not update committee cache")
}

return ComputeProposerIndex(state.Validators(), indices, seedWithSlotHash)
return ComputeProposerIndex(state, indices, seedWithSlotHash)
}

// ComputeProposerIndex returns the index sampled by effective balance, which is used to calculate proposer.
Copy link
Member

Choose a reason for hiding this comment

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

Somewhere in the comment, can you please mention:
1.) This is faster than ComputeProposerIndexWithValidators
2.) It uses read only validator set from beacon state

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright done @terencechain

//
// This method is more efficient than ComputeProposerIndexWithValidators as it uses the read only validator
// abstraction to retrieve validator related data. Whereas the other method requires a whole copy of the validator
// set.
//
// Spec pseudocode definition:
// def compute_proposer_index(state: BeaconState, indices: Sequence[ValidatorIndex], seed: Hash) -> ValidatorIndex:
// """
// Return from ``indices`` a random index sampled by effective balance.
// """
// assert len(indices) > 0
// MAX_RANDOM_BYTE = 2**8 - 1
// i = 0
// while True:
// candidate_index = indices[compute_shuffled_index(ValidatorIndex(i % len(indices)), len(indices), seed)]
// random_byte = hash(seed + int_to_bytes(i // 32, length=8))[i % 32]
// effective_balance = state.validators[candidate_index].effective_balance
// if effective_balance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * random_byte:
// return ValidatorIndex(candidate_index)
// i += 1
func ComputeProposerIndex(bState *stateTrie.BeaconState, activeIndices []uint64, seed [32]byte) (uint64, error) {
length := uint64(len(activeIndices))
if length == 0 {
return 0, errors.New("empty active indices list")
}
maxRandomByte := uint64(1<<8 - 1)
hashFunc := hashutil.CustomSHA256Hasher()

for i := uint64(0); ; i++ {
candidateIndex, err := ComputeShuffledIndex(i%length, length, seed, true /* shuffle */)
if err != nil {
return 0, err
}
candidateIndex = activeIndices[candidateIndex]
if int(candidateIndex) >= bState.NumValidators() {
return 0, errors.New("active index out of range")
}
b := append(seed[:], bytesutil.Bytes8(i/32)...)
randomByte := hashFunc(b)[i%32]
v, err := bState.ValidatorAtIndexReadOnly(candidateIndex)
if err != nil {
return 0, nil
}
effectiveBal := v.EffectiveBalance()

if effectiveBal*maxRandomByte >= params.BeaconConfig().MaxEffectiveBalance*uint64(randomByte) {
return candidateIndex, nil
}
}
}

// ComputeProposerIndexWithValidators returns the index sampled by effective balance, which is used to calculate proposer.
//
// Note: This method signature deviates slightly from the spec recommended definition. The full
// state object is not required to compute the proposer index.
//
Expand All @@ -202,7 +254,8 @@ func BeaconProposerIndex(state *stateTrie.BeaconState) (uint64, error) {
// if effective_balance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * random_byte:
// return ValidatorIndex(candidate_index)
// i += 1
func ComputeProposerIndex(validators []*ethpb.Validator, activeIndices []uint64, seed [32]byte) (uint64, error) {
// Deprecated: Prefer using the beacon state with ComputeProposerIndex to avoid an unnecessary copy of the validator set.
func ComputeProposerIndexWithValidators(validators []*ethpb.Validator, activeIndices []uint64, seed [32]byte) (uint64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this? Can it be marked as deprecated?

Suggested change
func ComputeProposerIndexWithValidators(validators []*ethpb.Validator, activeIndices []uint64, seed [32]byte) (uint64, error) {
//
// Deprecated: Prefer using the beacon state with ComputeProposerIndex to avoid an unnecessary copy of the validator set.
func ComputeProposerIndexWithValidators(validators []*ethpb.Validator, activeIndices []uint64, seed [32]byte) (uint64, error) {

If deprecated is not appropriate, please still put a message somewhere that users should prefer the other method whenever possible.Otherwise we could still see misuse with ComputeProposerIndexWithValidators(state.Validators(), ...)

Copy link
Member Author

@nisdas nisdas Apr 29, 2020

Choose a reason for hiding this comment

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

I think deprecated would be appropriate here. This is only still there because an archival method requires this, but that archival method is going to be deprecated anyway

Copy link
Member

Choose a reason for hiding this comment

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

Can you make sure this method is tested as well?

length := uint64(len(activeIndices))
if length == 0 {
return 0, errors.New("empty active indices list")
Expand Down
66 changes: 65 additions & 1 deletion beacon-chain/core/helpers/validators_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"reflect"
"testing"

"github.com/prysmaticlabs/prysm/shared/hashutil"

ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
beaconstate "github.com/prysmaticlabs/prysm/beacon-chain/state"
pb "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1"
Expand Down Expand Up @@ -184,6 +186,62 @@ func TestBeaconProposerIndex_OK(t *testing.T) {
}
}

func TestComputeProposerIndex_Compatibility(t *testing.T) {
validators := make([]*ethpb.Validator, params.BeaconConfig().MinGenesisActiveValidatorCount)
for i := 0; i < len(validators); i++ {
validators[i] = &ethpb.Validator{
ExitEpoch: params.BeaconConfig().FarFutureEpoch,
}
}

state, err := beaconstate.InitializeFromProto(&pb.BeaconState{
Validators: validators,
RandaoMixes: make([][]byte, params.BeaconConfig().EpochsPerHistoricalVector),
})
if err != nil {
t.Fatal(err)
}

indices, err := ActiveValidatorIndices(state, 0)
if err != nil {
t.Fatal(err)
}

var proposerIndices []uint64
seed, err := Seed(state, 0, params.BeaconConfig().DomainBeaconProposer)
if err != nil {
t.Fatal(err)
}
for i := uint64(0); i < params.BeaconConfig().SlotsPerEpoch; i++ {
seedWithSlot := append(seed[:], bytesutil.Bytes8(i)...)
seedWithSlotHash := hashutil.Hash(seedWithSlot)
index, err := ComputeProposerIndex(state, indices, seedWithSlotHash)
if err != nil {
t.Fatal(err)
}
proposerIndices = append(proposerIndices, index)
}

var wantedProposerIndices []uint64
seed, err = Seed(state, 0, params.BeaconConfig().DomainBeaconProposer)
if err != nil {
t.Fatal(err)
}
for i := uint64(0); i < params.BeaconConfig().SlotsPerEpoch; i++ {
seedWithSlot := append(seed[:], bytesutil.Bytes8(i)...)
seedWithSlotHash := hashutil.Hash(seedWithSlot)
index, err := ComputeProposerIndexWithValidators(state.Validators(), indices, seedWithSlotHash)
if err != nil {
t.Fatal(err)
}
wantedProposerIndices = append(wantedProposerIndices, index)
}

if !reflect.DeepEqual(wantedProposerIndices, proposerIndices) {
t.Error("Wanted proposer indices from ComputeProposerIndexWithValidators does not match")
}
}

func TestDelayedActivationExitEpoch_OK(t *testing.T) {
epoch := uint64(9999)
got := ActivationExitEpoch(epoch)
Expand Down Expand Up @@ -541,7 +599,13 @@ func TestComputeProposerIndex(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := ComputeProposerIndex(tt.args.validators, tt.args.indices, tt.args.seed)
shayzluf marked this conversation as resolved.
Show resolved Hide resolved
bState := &pb.BeaconState{Validators: tt.args.validators}
stTrie, err := beaconstate.InitializeFromProtoUnsafe(bState)
if err != nil {
t.Error(err)
return
}
got, err := ComputeProposerIndex(stTrie, tt.args.indices, tt.args.seed)
if (err != nil) != tt.wantErr {
t.Errorf("ComputeProposerIndex() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down
2 changes: 1 addition & 1 deletion beacon-chain/rpc/beacon/assignments.go
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ func archivedValidatorCommittee(
for slot := startSlot; slot < startSlot+params.BeaconConfig().SlotsPerEpoch; slot++ {
seedWithSlot := append(proposerSeed[:], bytesutil.Bytes8(slot)...)
seedWithSlotHash := hashutil.Hash(seedWithSlot)
i, err := helpers.ComputeProposerIndex(activeVals, activeIndices, seedWithSlotHash)
i, err := helpers.ComputeProposerIndexWithValidators(activeVals, activeIndices, seedWithSlotHash)
if err != nil {
return nil, errors.Wrapf(err, "could not check proposer at slot %d", slot)
}
Expand Down