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

Remove serialization from consensus #924

Merged
merged 7 commits into from
Apr 22, 2019
Merged

Remove serialization from consensus #924

merged 7 commits into from
Apr 22, 2019

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented Apr 14, 2019

Consensus now only cares about Merkleisation (i.e. hash_tree_root), not about serialization (i.e. serialize). This simplifies consensus code by a few tens of lines, is conceptually cleaner, and is more future proof.

A corresponding change in the deposit contract.

Consensus now only cares about Merkleisation (i.e. `hash_tree_root`), not about serialization (i.e. `serialize`). This simplifies consensus code by a few tens of lines, is conceptually cleaner, and is more future proof.

A corresponding change is required in the deposit contract.
@JustinDrake JustinDrake requested a review from hwwhww April 14, 2019 22:12
@hwwhww
Copy link
Contributor

hwwhww commented Apr 14, 2019

@JustinDrake
In the current deposit contract, the API is def deposit(deposit_data: bytes[184]), where the input is the serialized DepositData and calculate the leaf value by sha3 in the contract.

So regarding the deposit contract side, my understanding is:

  1. Keep having serialization input -> build tree on chain
  2. Need to implement tree hash on contract (only for deposit_data case)

Is it what you think?

@JustinDrake
Copy link
Collaborator Author

JustinDrake commented Apr 14, 2019

My suggestion would be to change the deposit contract API to def deposit(pubkey: bytes[48], withdrawal_credentials: bytes[32], amount: bytes[8], signature: bytes[96]) and then manually compute hash_tree_root (which uses sha2) inside the deposit contract.

@hwwhww
Copy link
Contributor

hwwhww commented Apr 14, 2019

@JustinDrake ahh right that would be more reasonable. 😅

Note that, having the fields in API would limit our options to update eth2 fields without forking eth1 or creating a new deposit contract. Although I remember the last time we discuss the upgradability of the deposit contract, the solution would just be just moving to a new contract, right?

@@ -2230,7 +2230,7 @@ def process_deposit(state: BeaconState, deposit: Deposit) -> None:

# Verify the Merkle branch
merkle_branch_is_valid = verify_merkle_branch(
leaf=hash(serialize(deposit.data)), # 48 + 32 + 8 + 96 = 184 bytes serialization
leaf=hash_tree_root(deposit.data),
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this mean that we also need to calculate the SSZ root of the deposit data in the Vyper contract?

Copy link
Collaborator Author

@JustinDrake JustinDrake Apr 14, 2019

Choose a reason for hiding this comment

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

Yes, we need to calculate hash_tree_root in the contract. Assuming def deposit(pubkey: bytes[48], withdrawal_credentials: bytes[32], amount: bytes[8], signature: bytes[96]) it's something like:

pubkey_root = hash(pubkey + zero_hash[0:16])
signature_root = hash(
    hash(signature[0:64]) +
    hash(signature[64:96] + zero_hash)
)
hash_tree_root = hash(
    hash(pubkey_root + withdrawal_credentials) +
    hash(amount + zero_hash[0:24] + signature_root)
)

@JustinDrake
Copy link
Collaborator Author

the solution would just be just moving to a new contract, right?

Right :)

@hwwhww
Copy link
Contributor

hwwhww commented Apr 18, 2019

Opened ethereum/deposit_contract#29.
I'll take it on the deposit contract side and then come back to clear and merge this PR.

@hwwhww
Copy link
Contributor

hwwhww commented Apr 19, 2019

link the contract side: ethereum/deposit_contract#31

@hwwhww
Copy link
Contributor

hwwhww commented Apr 22, 2019

@djrtwo ethereum/deposit_contract#31 is ready. :)

@djrtwo djrtwo merged commit 0079c63 into dev Apr 22, 2019
@djrtwo djrtwo deleted the JustinDrake-patch-11 branch April 22, 2019 15:41
@JustinDrake JustinDrake mentioned this pull request May 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants