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

Use custom types in data structures #667

Closed
Nashatyrev opened this issue Feb 20, 2019 · 5 comments
Closed

Use custom types in data structures #667

Nashatyrev opened this issue Feb 20, 2019 · 5 comments
Labels
general:presentation Presentation (as opposed to content)

Comments

@Nashatyrev
Copy link
Member

As soon as there are strict custom types for helper functions may be it makes sense to use them appropriately for data structures members like below.

BeaconState
{
    # Misc
    'slot': Slot,
    'genesis_time': 'uint64',
    'fork': Fork,  # For versioning hard forks

    # Validator registry
    'validator_registry': [Validator],
    'validator_balances': [Gwei],
    'validator_registry_update_epoch': Epoch,

    # Randomness and committees
    'latest_randao_mixes': [Bytes32],
    'previous_shuffling_start_shard': Shard,
    'current_shuffling_start_shard': Shard,
    'previous_shuffling_epoch': Epoch,
    'current_shuffling_epoch': Epoch,
    'previous_shuffling_seed': Bytes32,
    'current_shuffling_seed': Bytes32,
...

In our Java implementation we already converted the structures members (like this) and constants (like this) to custom types (where applicable) which significantly improved readability and type safety.

@JustinDrake
Copy link
Collaborator

Agreed this improves readability! We can even define a Hash custom type.

@Nashatyrev
Copy link
Member Author

Nashatyrev commented Feb 22, 2019

Cool! I can take care of PR then.
We also added Time type (aka number of seconds), though it's not too widespread.
Another question: is there any means to specify the array index type, like below?

  'validator_balances': [ValidatorIndex => Gwei],
...
  'latest_block_roots': [Slot => Hash],
  'latest_index_roots': [Epoch => Hash],

@hwwhww
Copy link
Contributor

hwwhww commented Feb 22, 2019

I'm not sure if I like it or not. Some notes:

  1. The current data structure section is defined with SSZ serialization types. I’d say let's not mix custom type and SSZ in the same section; so if we want to use custom types to describe data structure, do it fully. That said, some other types like Bitfield: bytes, Bool: bool (Turned slashed and initiated_exit into booleans #658) would need to be defined as well.
  2. However, declaimed, in Python, the typing custom types are not strict. So it won’t improve type safety in programming-level (helper functions), only in readability-level.

@Nashatyrev
Copy link
Member Author

  1. I would consider custom types as alias types for SSZ serialization types so nothing should change from SSZ point of view.
    We currently have the following additional types:
  • Hash
  • Time
  • Bitfield for Attesstaion
  • Bitfield64 for justification_bitfield

What's left 'raw' UInt64:

  • Eth1DataVote.vote_count
  • Deposit.index
  • ForkData.previous/current_version
  • different flags

(some may be outdated or missing since we didn't yet migrate from Spec v0.1)

  1. Sure I suppose the type-safety is about implementation not the spec.

@JustinDrake JustinDrake added the general:presentation Presentation (as opposed to content) label Feb 22, 2019
JustinDrake added a commit that referenced this issue May 1, 2019
This draft PR try to fix a longstanding wart (see #196, #371, #667) regarding bitfields. We define bitfields as lists or vectors of `bool`s, as opposed to `bytes`. Benefits:

* Remove ugly-as-hell helper function `verify_bitfield`
* No more bit manipulation with `justification_bitfield`
* Merkleisation-friendly `justification_bitfield` (i.e. `justification_bitfield` does not change unless a new epoch is justified).
* Easy parametrisation of the number of bits in `justification_bitfield` (can be more or fewer than 64—more may be useful for dApps and light clients)
* Semantically cleaner typing

This requires a minor SSZ change (Merkleisation not affected) where lists and vectors of `bool`s should be packed, similar to packing of `uintN`. We could alias `bool` to `uint1` so that the packing logic only needs to be defined once for `uintN`.
@JustinDrake
Copy link
Collaborator

Closing in favour of #1156 :)

Sorry it too this long @Nashatyrev. Feel free to review that PR.

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)
Projects
None yet
Development

No branches or pull requests

3 participants