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

Fix out-of-bounds in get_shuffling #641

Merged
merged 3 commits into from
Feb 18, 2019
Merged

Conversation

paulhauner
Copy link
Contributor

What

Change get_shuffling(...) so it gives shuffles using an index of active_validator_indices instead of a value of active_validator_indices.

Why

The following fails with a IndexError: list index out of range:

validator_indices = [
    0,    # inactive
    1,    # active
    2,    # inactive
    3,    # active
]

# ... therefore ...

active_validator_indices = [
    1,    # active
    3,    # active
]

def get_permuted_index(i, length):
    return i

shuffled_active_validator_indices = [
        active_validator_indices[get_permuted_index(i, len(active_validator_indices))]
        for i in active_validator_indices
]

Whereas this does not:

shuffled_active_validator_indices = [
        active_validator_indices[get_permuted_index(i, len(active_validator_indices))]
        for i in range(len(active_validator_indices))
]

Ensures it does not try to shuffle out of range of the `active_validator_indices` list.
@JustinDrake JustinDrake self-requested a review February 16, 2019 20:59
@JustinDrake
Copy link
Collaborator

@djrtwo Are we no longer seeding with epoch?

@djrtwo
Copy link
Contributor

djrtwo commented Feb 18, 2019

We moved epoch into the mix in generate_seed rather than on the fly in shuffle. This was clearer and still accomplishes the goal of ensuring that the shuffling changes at points of reshuffling even in the case that no new randao reveals occur.

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants