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

get_shuffling can't handle sample larger than validator_count #18

Closed
djrtwo opened this issue Jun 19, 2018 · 4 comments
Closed

get_shuffling can't handle sample larger than validator_count #18

djrtwo opened this issue Jun 19, 2018 · 4 comments
Labels
bug Something isn't working

Comments

@djrtwo
Copy link
Contributor

djrtwo commented Jun 19, 2018

Issue

get_shuffling can't handle when sample larger than validator_count. The algorithm does an infinite loop. This actually occurs in code when len(active_validators) is less than ATTESTER_COUNT and get_attesters_and_signer is called.

Proposed Implementation

stub: need to think more

probably do one of the following:

  • prevent this from happening with an assertion
  • wrap shuffling array around to be the length of sample
@djrtwo djrtwo added the bug Something isn't working label Jun 19, 2018
@hwwhww
Copy link
Contributor

hwwhww commented Jul 2, 2018

It seems good to set the MINIMUM_VALIDATOR_COUNT for safety reason: https://ethresear.ch/t/safe-notary-pool-size/1728 and https://ethresear.ch/t/registrations-shard-count-and-shuffling/2129

@paulhauner
Copy link

paulhauner commented Jul 11, 2018

Turns out I made a duplicate issue. I'll close it and just use this one.

FWIW, here's the suggestion I had in the duplicate issue:

if active_validators < 2:
    raise Exception()

ideal_val_count = config['attester_count'] + skip_count + 1

if ideal_val_count > active_validators:
     attesters = shuffled[:-1]
     proposer = shuffled[-1]
else:
     attesters = shuffled[:ideal_val_count - 1]
     proposer = shuffled[ideal_val_count - 1]

I agree with the MINIMUM_VALIDATOR_COUNT suggestion. I guess we'd face similar issues if skip_count gets too high, though.

EDIT: modified the code to be more sensible.

@paulhauner
Copy link

After playing around with this in Rust, my thoughts are that get_shuffling() shouldn't be concerned with "sampling" (i.e., getting some subset of shuffled active validators). That should be handled by whatever downstream function requires that sampling.

I'm only a little of the way into this state transition stuff so take that with a grain of salt. Just my thoughts so far.

@djrtwo
Copy link
Contributor Author

djrtwo commented Sep 2, 2018

get_new_shuffling in current spec does not handle sampling yay.

@djrtwo djrtwo closed this as completed Sep 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants