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

Cleanup containers #917

Closed
wants to merge 2 commits into from
Closed

Cleanup containers #917

wants to merge 2 commits into from

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented Apr 14, 2019

  • Remove unnecessary per-field comments, focus on comments for field grouping
  • Group Validator fields relevant to light-clients together (small optimisation for light clients)
  • Rework grouping of BeaconState fields to "tell a story":
    • # Versioning (in space and time) specifies a blockchain
    • # History (block/state roots) is the "bones" of any blockchain
    • # Eth1 voting leads to deposits
    • # Registry emerges from deposits
    • # Shuffling the registry leads to committees
    • # Attestations are committee votes
    • # Crosslinks emerge from attestations
    • # Finality also emerges from attestations
  • deposit_index => eth1_deposit_index

Do not merge before:

* Remove unnecessary per-field comments, focus on field grouping
* Group `Validator` fields relevant to light-clients together (small optimisation for light clients)
* Rework grouping of `BeaconState` fields
* `deposit_index` => `eth1_deposit_index`

Do not merge before:

* #695 which specifies custom types for individual container fields, offsetting the removal of comments in some instances
* #896 and #843 so that we don't have to continuously maintain the genesis `BeaconState`
* #874 which introduces `current_crosslinks` and `previous_crosslinks`
@JustinDrake JustinDrake added general:presentation Presentation (as opposed to content) general:simplification labels Apr 17, 2019
@@ -411,36 +392,27 @@ The types are defined topologically to aid in facilitating an executable version

```python
{
# BLS public key
# Fields relevant to light clients
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I agree with the grouping here. pubkey and slashed are relevant to far more than light clients

@dankrad
Copy link
Contributor

dankrad commented Apr 17, 2019

So I kind of disagree with this. I believe it is good to have a human-readable interpretation of a field next to it, I just think the comments should be clearer than they are now. This goes exactly against what I propose in #942.

My suggestion is that in an ideal world, a field has a corresponding description that can be checked against the actual implementing algorithm. Let's give an example:

{
    # Validator indices
    'custody_bit_0_indices': ['uint64'],
    'custody_bit_1_indices': ['uint64'],
    # Attestation data
    'data': AttestationData,
    # Aggregate signature
    'aggregate_signature': 'bytes96',
}

I agree that the comment "Validator indices" is useless. However, if we change it to

{
    # Indices of all the validators that voted attested with custody bit = 0
    'custody_bit_0_indices': ['uint64'],
    # Indices of all the validators that voted attested with custody bit = 1
    'custody_bit_1_indices': ['uint64'],
    # Attestation data
    'data': AttestationData,
    # Aggregate signature
    'aggregate_signature': 'bytes96',
}

then it actually becomes more readable and we can use this information to check whether the algorithm computing this actually does what it claims.

@protolambda
Copy link
Collaborator

@JustinDrake
Now that #695 and #1077 are complete, can we get this PR ready again, if you still like to merge it? It needs an update to account for the new fields, and the syntax is effectively just name: type now.

@JustinDrake
Copy link
Collaborator Author

Close in favour of #1156

@JustinDrake JustinDrake closed this Jun 9, 2019
@djrtwo djrtwo deleted the JustinDrake-patch-8 branch September 7, 2019 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content) general:simplification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants