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

[WIP] Attestation committees #1185

Merged
merged 14 commits into from Sep 26, 2018
Merged

[WIP] Attestation committees #1185

merged 14 commits into from Sep 26, 2018

Conversation

mkalinin
Copy link
Contributor

There one thing worth to add before the merge. Proposer service schedules each slot in current implementation, it should be linked to committees transition to only produce blocks for slots it assigned to.

Resolves #1142

@mkalinin mkalinin added this to the Beacon milestone Sep 25, 2018
@mkalinin mkalinin added this to On Review in Beacon chain via automation Sep 25, 2018
@coveralls
Copy link

coveralls commented Sep 25, 2018

Coverage Status

Coverage increased (+0.2%) to 56.371% when pulling 05ecebb on sharding/committees into 89e417c on research/sharding.

Copy link
Collaborator

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

Look good!

return;

// get number of the next slot that validator is eligible to propose
long slotNumber = calcNextProposingSlot(proposer.getSlotNumber(System.currentTimeMillis()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move System.currentTimeMillis() somewhere to one source, as sharding logic is dependant of time and we may need some accuracy tweaks for it in future or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think adding getCurrentSlot() method to BeaconProposer should manage that

// each committee dedicated for own shard
int shard = startShard;
for (Committee[] slot : committees) {
for (Committee committee : slot) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

slot.length == SHARD_COUNT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, slot.length == CYCLE_LENGTH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, slot.length depends on the size of active validator set. It varies from 1 to (1 << 24) / CYCLE_LENGTH / (MIN_COMMITTEE_SIZE * 2) + 1, it's calculated with ShufflingCommitteeFactory#calcShardsPerSlot()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it

public void testShuffle() {
int[] in = IntStream.range(1, BeaconConstants.MIN_COMMITTEE_SIZE * 1024).toArray();
int[] out = new ShufflingCommitteeFactory().shuffle(HashUtil.randomHash(), in);
assertFalse(Arrays.equals(in, out));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will fail one day 😿

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, in theory it may be, but hardly the case in practice :)

@mkalinin mkalinin merged commit 82af2ac into research/sharding Sep 26, 2018
Beacon chain automation moved this from On Review to Done Sep 26, 2018
@mkalinin mkalinin deleted the sharding/committees branch December 26, 2018 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Beacon chain
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants