Add back State To ComputeProposerIndex #5674
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5674 +/- ##
===========================================
+ Coverage 1.46% 53.19% +51.72%
===========================================
Files 69 308 +239
Lines 6548 25512 +18964
===========================================
+ Hits 96 13571 +13475
- Misses 6443 9911 +3468
- Partials 9 2030 +2021 |
|
nice improvement. thanks |
| @@ -179,11 +179,60 @@ 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. | |||
There was a problem hiding this comment.
Somewhere in the comment, can you please mention:
1.) This is faster than ComputeProposerIndexWithValidators
2.) It uses read only validator set from beacon state
There was a problem hiding this comment.
Alright done @terencechain
| @@ -202,7 +251,7 @@ 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) { | |||
| func ComputeProposerIndexWithValidators(validators []*ethpb.Validator, activeIndices []uint64, seed [32]byte) (uint64, error) { | |||
There was a problem hiding this comment.
Do we still need this? Can it be marked as deprecated?
| 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(), ...)
There was a problem hiding this comment.
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
| @@ -202,7 +251,7 @@ 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) { | |||
| func ComputeProposerIndexWithValidators(validators []*ethpb.Validator, activeIndices []uint64, seed [32]byte) (uint64, error) { | |||
There was a problem hiding this comment.
Can you make sure this method is tested as well?
Previously we passed in validators as an argument to
ComputeProposerIndexhowever with how our new state is structured, this is an expensive operation as we copy the whole validator set. It would instead be better to pass in our state object directly and use read only instances of validators to perform any operation in the function.For support of our archival methods, I did keep the old method as
ComputeProposerIndexWithValidators.