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

Add DepositParametersRecord to protobuf types #1042

Merged
merged 3 commits into from
Dec 5, 2018

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Dec 4, 2018

Matching spec at https://github.com/ethereum/eth2.0-specs/blob/c2227a59de89a8b233974f58d55e8a4401568bce/specs/core/0_beacon-chain.md#depositparametersrecord

{
    # BLS pubkey
    'pubkey': 'uint384',
    # BLS proof of possession (a BLS signature)
    'proof_of_possession': ['uint384'],
    # Withdrawal credentials (TODO: define the format)
    'withdrawal_credentials': 'hash32',
    # The initial RANDAO commitment
    'randao_commitment': 'hash32',
}

Part of #781

@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #1042 into master will increase coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1042      +/-   ##
==========================================
+ Coverage   71.87%   72.08%   +0.21%     
==========================================
  Files          75       75              
  Lines        4579     4579              
==========================================
+ Hits         3291     3301      +10     
+ Misses        969      961       -8     
+ Partials      319      317       -2

@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #1042 into master will increase coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1042      +/-   ##
==========================================
+ Coverage   71.87%   72.08%   +0.21%     
==========================================
  Files          75       75              
  Lines        4579     4579              
==========================================
+ Hits         3291     3301      +10     
+ Misses        969      961       -8     
+ Partials      319      317       -2

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

I don't think DepositParametersRecord should to be part of proto.
Looking into the spec, DepositParametersRecord does not get passed within beacon services. What's going to happen is node serializes DepositParametersRecord into bytes, which gets included in the beacon block, then later other nodes deserializes ssz'ed bytes to verify special.data as part of block validation.

Can we think of any reason why this needs to be in proto when it's only gonna get ssz'ed and de-ssz'ed?

@prestonvanloon
Copy link
Member Author

Hmm. I don't have a specific use case for this as a proto. It might make sense for some query RPC data extraction service, but I'd rather not build for something we don't know we need yet.

I'll leave this open for a bit for other comments/thoughts before closing.

@nisdas
Copy link
Member

nisdas commented Dec 4, 2018

Maybe we could leave it here until the spec stabilizes ? imo all the data structures stated in the spec should be part of proto. If later on we do not find ourselves using the proto object anywhere we could remove it

@prestonvanloon
Copy link
Member Author

@nisdas that is what I was thinking. We could have all of the spec data structures as proto. it would make sense if we wanted to also represent these data in a UI. This definition is reusable in typescript or other languages

@terencechain
Copy link
Member

I don't feel strongly against putting the unused spec data structs in proto although we might not need this for querying RPC data far down the line.

I think in general we categorize spec data structs into 2 types:

Type 1

  • Gets passed between inter-services
  • Gets passed between beacon node and validator client
  • Gets passed between nodes in p2p network
  • Examples: Block, state and object within block like attestation or object within state like cross link
  • These should be in proto

Type 2

  • Gets serialized and deserialized
  • Only the bytecode gets included in block or state
  • Examples: Special objects, proposer attestations
  • These don't have to be in proto

I'm ok with either decision : )

@rauljordan
Copy link
Contributor

Given how rapidly spec is evolving I'm ok with having the types all defined in proto as that would make it easier for us to change fields if they keep changing

@prestonvanloon
Copy link
Member Author

I think we have achieved "rough consensus". Let's merge this for now and revisit later.

I'm also exploring proto's as schema definitions for relational database storage, so this might be useful in the future for a federated deployment / shared database access.

@prestonvanloon prestonvanloon merged commit 7b6c646 into master Dec 5, 2018
KimiWu123 pushed a commit to KimiWu123/prysm that referenced this pull request Dec 15, 2018
@nisdas nisdas deleted the Add-DepositParametersRecord branch January 28, 2019 12:10
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