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

Attestation committee refactor #1009

Merged
merged 21 commits into from
May 1, 2019
Merged

Attestation committee refactor #1009

merged 21 commits into from
May 1, 2019

Conversation

vbuterin
Copy link
Contributor

@vbuterin vbuterin commented Apr 29, 2019

Mostly cosmetic and enabling some functionality needed in phase 1. A few substantive changes to Attestation data structure.

  • Remove get_crosslink_committees_at_slot (that function's ugly man...)
  • Make the "base" that everything works off instead be get_crosslink_committee (simpler and more generic)
  • Attestations store epoch, start shard and shard, no longer slot (slot can be calculated from the other three)
  • store inclusion_distance in PendingAttestation rather than inclusion_slot for more direct use in subsequent calculations.
  • Retaining start shard in attestations allows get_attesting_indices to peek much further back into the past, making it useful for slashings (Phase 1)
  • Some two-layer-deep nested loops become one-layer-deep loops
  • alter get_winning_crosslink_and_attesting_indices function signature to mirror that of get_crosslink_committee
  • is_double_vote and is_surround_vote replaced with single is_slashable_attestation_data

* Remove `get_crosslink_committees_at_slot` (that function's ugly man...)
* Make the "base" that everything works off instead be `get_crosslink_committee`
* Attestations store epoch, start shard and shard, no longer slot (slot can be calculated from the other three)
* Retaining start shard in attestations allows `get_attesting_indices` to peek much further back into the past, making it useful for slashings (Phase 1)
* Some two-layer-deep nested loops become one-layer-deep loops
@ethereum ethereum deleted a comment from djrtwo Apr 30, 2019
specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
@djrtwo
Copy link
Contributor

djrtwo commented Apr 30, 2019

I'm working on some edits based on the above feedback

@djrtwo
Copy link
Contributor

djrtwo commented Apr 30, 2019

and now working on some bugs :)

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.

@JustinDrake if we remove get_state_root helper, then I'd like to do it in a separate PR with an explanation rather than it just disappearing at some point

@vbuterin
Copy link
Contributor Author

vbuterin commented May 1, 2019

I still am not fully comfortable with compute_committee being removed. It's definitely the right choice if you look at phase 0 in a vacuum, but the reason I separated out that function initially was that get_persistent_committee in phase 1 also uses it, so we're making both kinds of committees from the same basic building block.

Here is the usage in phase 1: https://github.com/ethereum/eth2.0-specs/blob/dev/specs/core/1_shard-data-chains.md#get_period_committee

One possible compromise I see would be to define a function:

def get_indices_of_chunk(list_size: int, chunks: int, chunk: int) -> List[int]:
    start = (len(list_size) * chunk) // chunks
    end = (len(list_size) * (chunk + 1)) // chunks
    return list(range(start, end))

Then the PR would be, in get_crosslink_committee:

    return [
        active_indices[get_shuffled_index(i, len(active_indices), seed)]
        for i in get_indices_of_chunk(len(active_indices), committee_count, committee_index)
    ]

And the same in get_period_committee.

@JustinDrake
Copy link
Collaborator

@vbuterin: I've exposed get_committee which abstracts away common logic for both get_crosslink_committee (now a 2-liner) and get_period_committee (now a 1-liner).

JustinDrake added a commit that referenced this pull request May 1, 2019
Somewhat cleaner, both conceptually and in terms of presentation.

(This is a draft for early feedback—to be fully implemented after #1009 is merged.)
@djrtwo
Copy link
Contributor

djrtwo commented May 1, 2019

I think the previous function signature of compute_committee is more straight forward and readable and also is much clearer to the client what the explicit inputs are which could be used to cache results.

I vote going back to that function signature or something similar

@vbuterin
Copy link
Contributor Author

vbuterin commented May 1, 2019

I think the previous function signature of compute_committee is more straight forward and readable and also is much clearer to the client what the explicit inputs are which could be used to cache results.

Agreed.

@JustinDrake JustinDrake changed the title Attestation committee refactor attempt Attestation committee refactor May 1, 2019
@JustinDrake
Copy link
Collaborator

compute_committee restored! :)

@JustinDrake JustinDrake self-requested a review May 1, 2019 16:27
@djrtwo djrtwo changed the base branch from dev to master May 1, 2019 22:11
@djrtwo djrtwo merged commit 2ed8d99 into master May 1, 2019
@djrtwo djrtwo deleted the vbuterin-patch-2 branch May 1, 2019 22:25
@vbuterin vbuterin mentioned this pull request May 3, 2019
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.

4 participants