Fixes a few bugs with deposit verification.

[ralexstokes]

1. The order of the deposit_data serialization does not match the current
   Vyper contract. The description now matches that serialization.

2. The deposit.merkle_tree_index was not being used (at least explicitly) so
   the text now reflects which inputs are to be used for which parameters in the
   pseudocode spec that follows.

3. There seems to be a bug where we want the initial leaf to be the hash of
   the DepositData, not the data itself. The text now reflects this requirement.

[JustinDrake]

Great stuff. I'm tempted to homogenise naming between the Vyper contract (which uses deposit_count) and the Deposit object (which uses merkle_tree_index).

[ralexstokes]

Yeah given the ambient complexity of this protocol I would prefer to be as simple and clear as we can in the spec when possible :) easier grokking => fewer bugs

[JustinDrake]

I suggest we review the Vyper contract naming and merge after #383 (which itself has be bunch of naming cleanups regarding deposits).

[hwwhww]

@ralexstokes @JustinDrake #383 is merged!

[djrtwo]

We should make the deposit_tree_index a uint64 to match the incoming type in Deposit on the beacon chain. We can either do a convert when called log.Eth1Deposit or we can change most of the uint256 types to uint64 as the extra 192 bits aren't needed in the tree calculations.

Suggested change

	Eth1Deposit: event({previous_receipt_root: bytes32, data: bytes[2064], deposit_tree_index: uint256})
	Eth1Deposit: event({previous_receipt_root: bytes32, data: bytes[2064], deposit_tree_index: uint64})

[ralexstokes]

looks like Vyper only has uint256 unsigned integer types

i would say we just push the conversion to the beacon chain client -- when it ingests a Deposit event it can convert the uint256 to a uint64. note that this functionality is not currently discussed in the spec right now. we could add a sentence to address it if you think it should be called out explicitly, or perhaps we go ahead and start defining the Eth1.0 to Eth2.0 client interface (RPC API style)

[djrtwo]

Ah, we should do a convert to bytes[8] like msg_gwei_bytes8 and timestamp_bytes8. We are avoiding introducing big-int support on the beacon chain and I would lean toward cleaning it up on the 1.0 side to this end.

(note: we have sigs and keys defined as uint384 but should probably just have them as bytes48 to explicitly avoid any big integer requirement confusion)

[ralexstokes]

addressed in the latest commit. shouldn't be any truncation issues as we are only really expecting 2**32 entries to the tree.

[ralexstokes]

The only discrepancy I see w/ master is that what is called deposit_count in the Eth1.0 event is called index in the Deposit type definition.

Do we want to match the names so it is super clear what goes where? As it stands, it is clear from the specification of how to use verify_merkle_branch so not critical.

[djrtwo]

Contract has a bug regardless of names at this point:

• deposit_tree_index is being incremented each time deposit is called and used to define index, but it is not defined anywhere in the contract.
• deposit_count is defined and broadcast in the log, but is never incremented.
• Even if we fix the above, we are just broadcasting the count and not the actual merkle tree index which is defined as index

I think we should remove deposit_tree_index, use and track deposit_count, and change the name of index to merkle_tree_index and broadcast this value in the log.

[ralexstokes]

@djrtwo whoops sorry that was a bad rebase. fixed on this branch

[djrtwo]

This looks good to me. I checked the contract and it builds, but we'll need to port the changes into the beacon_chain repo to update the testing.

a side note: it might make sense to move this contract to a dedicated repo ethereum/eth1-deposit-contract or something. Beyond that, I think we should strip the contract portion of the phase 0 spec to just defining the interface and linking to the contract in the dedicated repo. Managing this code in two places is unnecessary and a pain imo