Deposit contract fixes (#1362)

deposit_contract/contracts/validator_registration.v.py

@@ -1,10 +1,11 @@
+# Vyper target 0.1.0b12
 MIN_DEPOSIT_AMOUNT: constant(uint256) = 1000000000  # Gwei
 DEPOSIT_CONTRACT_TREE_DEPTH: constant(uint256) = 32
 MAX_DEPOSIT_COUNT: constant(uint256) = 4294967295 # 2**DEPOSIT_CONTRACT_TREE_DEPTH - 1
 PUBKEY_LENGTH: constant(uint256) = 48  # bytes
 WITHDRAWAL_CREDENTIALS_LENGTH: constant(uint256) = 32  # bytes
-AMOUNT_LENGTH: constant(uint256) = 8  # bytes
 SIGNATURE_LENGTH: constant(uint256) = 96  # bytes
+AMOUNT_LENGTH: constant(uint256) = 8  # bytes
 
 DepositEvent: event({
     pubkey: bytes[48],
@@ -42,7 +43,7 @@ def to_little_endian_64(value: uint256) -> bytes[8]:
 
 @public
 @constant
-def get_hash_tree_root() -> bytes32:
+def get_deposit_root() -> bytes32:
     zero_bytes32: bytes32 = 0x0000000000000000000000000000000000000000000000000000000000000000
     node: bytes32 = zero_bytes32
     size: uint256 = self.deposit_count
@@ -65,13 +66,16 @@ def get_deposit_count() -> bytes[8]:
 @public
 def deposit(pubkey: bytes[PUBKEY_LENGTH],
             withdrawal_credentials: bytes[WITHDRAWAL_CREDENTIALS_LENGTH],
-            signature: bytes[SIGNATURE_LENGTH]):
+            signature: bytes[SIGNATURE_LENGTH],
+            deposit_data_root: bytes32):
     # Avoid overflowing the Merkle tree (and prevent edge case in computing `self.branch`)
     assert self.deposit_count < MAX_DEPOSIT_COUNT
 
-    # Validate deposit data
+    # Check deposit amount
     deposit_amount: uint256 = msg.value / as_wei_value(1, "gwei")
     assert deposit_amount >= MIN_DEPOSIT_AMOUNT
+
+    # Length checks to facilitate formal verification (see https://github.com/ethereum/eth2.0-specs/pull/1362/files#r320361859)
     assert len(pubkey) == PUBKEY_LENGTH
     assert len(withdrawal_credentials) == WITHDRAWAL_CREDENTIALS_LENGTH
     assert len(signature) == SIGNATURE_LENGTH
@@ -80,7 +84,7 @@ def deposit(pubkey: bytes[PUBKEY_LENGTH],
     amount: bytes[8] = self.to_little_endian_64(deposit_amount)
     log.DepositEvent(pubkey, withdrawal_credentials, amount, signature, self.to_little_endian_64(self.deposit_count))
 
-    # Compute `DepositData` hash tree root
+    # Compute deposit data root (`DepositData` hash tree root)
     zero_bytes32: bytes32 = 0x0000000000000000000000000000000000000000000000000000000000000000
     pubkey_root: bytes32 = sha256(concat(pubkey, slice(zero_bytes32, start=0, len=64 - PUBKEY_LENGTH)))
     signature_root: bytes32 = sha256(concat(
@@ -91,8 +95,10 @@ def deposit(pubkey: bytes[PUBKEY_LENGTH],
         sha256(concat(pubkey_root, withdrawal_credentials)),
         sha256(concat(amount, slice(zero_bytes32, start=0, len=32 - AMOUNT_LENGTH), signature_root)),
     ))
+    # Verify computed and expected deposit data roots match
+    assert node == deposit_data_root
 
-    # Add `DepositData` hash tree root to Merkle tree (update a single `branch` node)
+    # Add deposit data root to Merkle tree (update a single `branch` node)
     self.deposit_count += 1
     size: uint256 = self.deposit_count
     for height in range(DEPOSIT_CONTRACT_TREE_DEPTH):

deposit_contract/requirements-testing.txt

@@ -1,5 +1,5 @@
 eth-tester[py-evm]==0.1.0b39
-vyper==0.1.0b10
+vyper==0.1.0b12
 web3==5.0.0b2
 pytest==3.6.1
 ../test_libs/pyspec

deposit_contract/tests/contracts/test_deposit.py

@@ -6,37 +6,49 @@
 
 import eth_utils
 from tests.contracts.conftest import (
-    DEPOSIT_CONTRACT_TREE_DEPTH,
     FULL_DEPOSIT_AMOUNT,
     MIN_DEPOSIT_AMOUNT,
 )
 
 from eth2spec.phase0.spec import (
     DepositData,
 )
-from eth2spec.utils.hash_function import hash
 from eth2spec.utils.ssz.ssz_typing import List
 from eth2spec.utils.ssz.ssz_impl import (
     hash_tree_root,
 )
 
 
+SAMPLE_PUBKEY = b'\x11' * 48
+SAMPLE_WITHDRAWAL_CREDENTIALS = b'\x22' * 32
+SAMPLE_VALID_SIGNATURE = b'\x33' * 96
+
+
 @pytest.fixture
-def deposit_input():
+def deposit_input(amount):
     """
     pubkey: bytes[48]
     withdrawal_credentials: bytes[32]
     signature: bytes[96]
+    deposit_data_root: bytes[32]
     """
     return (
-        b'\x11' * 48,
-        b'\x22' * 32,
-        b'\x33' * 96,
+        SAMPLE_PUBKEY,
+        SAMPLE_WITHDRAWAL_CREDENTIALS,
+        SAMPLE_VALID_SIGNATURE,
+        hash_tree_root(
+            DepositData(
+                pubkey=SAMPLE_PUBKEY,
+                withdrawal_credentials=SAMPLE_WITHDRAWAL_CREDENTIALS,
+                amount=amount,
+                signature=SAMPLE_VALID_SIGNATURE,
+            ),
+        )
     )
 
 
 @pytest.mark.parametrize(
-    'success,deposit_amount',
+    ('success', 'amount'),
     [
         (True, FULL_DEPOSIT_AMOUNT),
         (True, MIN_DEPOSIT_AMOUNT),
@@ -47,18 +59,24 @@ def deposit_input():
 def test_deposit_amount(registration_contract,
                         w3,
                         success,
-                        deposit_amount,
+                        amount,
                         assert_tx_failed,
                         deposit_input):
     call = registration_contract.functions.deposit(*deposit_input)
     if success:
-        assert call.transact({"value": deposit_amount * eth_utils.denoms.gwei})
+        assert call.transact({"value": amount * eth_utils.denoms.gwei})
     else:
         assert_tx_failed(
-            lambda: call.transact({"value": deposit_amount * eth_utils.denoms.gwei})
+            lambda: call.transact({"value": amount * eth_utils.denoms.gwei})
         )
 
 
+@pytest.mark.parametrize(
+    'amount',
+    [
+        (FULL_DEPOSIT_AMOUNT)
+    ]
+)
 @pytest.mark.parametrize(
     'invalid_pubkey,invalid_withdrawal_credentials,invalid_signature,success',
     [
@@ -71,38 +89,62 @@ def test_deposit_amount(registration_contract,
 def test_deposit_inputs(registration_contract,
                         w3,
                         assert_tx_failed,
-                        deposit_input,
+                        amount,
                         invalid_pubkey,
                         invalid_withdrawal_credentials,
                         invalid_signature,
                         success):
-    pubkey = deposit_input[0][2:] if invalid_pubkey else deposit_input[0]
-    if invalid_withdrawal_credentials:  # this one is different to satisfy linter
-        withdrawal_credentials = deposit_input[1][2:]
-    else:
-        withdrawal_credentials = deposit_input[1]
-    signature = deposit_input[2][2:] if invalid_signature else deposit_input[2]
+    pubkey = SAMPLE_PUBKEY[2:] if invalid_pubkey else SAMPLE_PUBKEY
+    withdrawal_credentials = (
+        SAMPLE_WITHDRAWAL_CREDENTIALS[2:] if invalid_withdrawal_credentials
+        else SAMPLE_WITHDRAWAL_CREDENTIALS
+    )
+    signature = SAMPLE_VALID_SIGNATURE[2:] if invalid_signature else SAMPLE_VALID_SIGNATURE
 
     call = registration_contract.functions.deposit(
         pubkey,
         withdrawal_credentials,
         signature,
+        hash_tree_root(
+            DepositData(
+                pubkey=SAMPLE_PUBKEY if invalid_pubkey else pubkey,
+                withdrawal_credentials=(
+                    SAMPLE_WITHDRAWAL_CREDENTIALS if invalid_withdrawal_credentials
+                    else withdrawal_credentials
+                ),
+                amount=amount,
+                signature=SAMPLE_VALID_SIGNATURE if invalid_signature else signature,
+            ),
+        )
     )
     if success:
-        assert call.transact({"value": FULL_DEPOSIT_AMOUNT * eth_utils.denoms.gwei})
+        assert call.transact({"value": amount * eth_utils.denoms.gwei})
     else:
         assert_tx_failed(
-            lambda: call.transact({"value": FULL_DEPOSIT_AMOUNT * eth_utils.denoms.gwei})
+            lambda: call.transact({"value": amount * eth_utils.denoms.gwei})
         )
 
 
-def test_deposit_event_log(registration_contract, a0, w3, deposit_input):
+def test_deposit_event_log(registration_contract, a0, w3):
     log_filter = registration_contract.events.DepositEvent.createFilter(
         fromBlock='latest',
     )
-
     deposit_amount_list = [randint(MIN_DEPOSIT_AMOUNT, FULL_DEPOSIT_AMOUNT * 2) for _ in range(3)]
+
     for i in range(3):
+        deposit_input = (
+            SAMPLE_PUBKEY,
+            SAMPLE_WITHDRAWAL_CREDENTIALS,
+            SAMPLE_VALID_SIGNATURE,
+            hash_tree_root(
+                DepositData(
+                    pubkey=SAMPLE_PUBKEY,
+                    withdrawal_credentials=SAMPLE_WITHDRAWAL_CREDENTIALS,
+                    amount=deposit_amount_list[i],
+                    signature=SAMPLE_VALID_SIGNATURE,
+                ),
+            )
+        )
         registration_contract.functions.deposit(
             *deposit_input,
         ).transact({"value": deposit_amount_list[i] * eth_utils.denoms.gwei})
@@ -118,14 +160,28 @@ def test_deposit_event_log(registration_contract, a0, w3, deposit_input):
         assert log['index'] == i.to_bytes(8, 'little')
 
 
-def test_deposit_tree(registration_contract, w3, assert_tx_failed, deposit_input):
+def test_deposit_tree(registration_contract, w3, assert_tx_failed):
     log_filter = registration_contract.events.DepositEvent.createFilter(
         fromBlock='latest',
     )
 
     deposit_amount_list = [randint(MIN_DEPOSIT_AMOUNT, FULL_DEPOSIT_AMOUNT * 2) for _ in range(10)]
     deposit_data_list = []
     for i in range(0, 10):
+        deposit_data = DepositData(
+            pubkey=SAMPLE_PUBKEY,
+            withdrawal_credentials=SAMPLE_WITHDRAWAL_CREDENTIALS,
+            amount=deposit_amount_list[i],
+            signature=SAMPLE_VALID_SIGNATURE,
+        )
+        deposit_input = (
+            SAMPLE_PUBKEY,
+            SAMPLE_WITHDRAWAL_CREDENTIALS,
+            SAMPLE_VALID_SIGNATURE,
+            hash_tree_root(deposit_data),
+        )
+        deposit_data_list.append(deposit_data)
+
         tx_hash = registration_contract.functions.deposit(
             *deposit_input,
         ).transact({"value": deposit_amount_list[i] * eth_utils.denoms.gwei})
@@ -138,12 +194,8 @@ def test_deposit_tree(registration_contract, w3, assert_tx_failed, deposit_input
 
         assert log["index"] == i.to_bytes(8, 'little')
 
-        deposit_data_list.append(DepositData(
-            pubkey=deposit_input[0],
-            withdrawal_credentials=deposit_input[1],
-            amount=deposit_amount_list[i],
-            signature=deposit_input[2],
-        ))
-
+        # Check deposit count and root
+        count = len(deposit_data_list).to_bytes(8, 'little')
+        assert count == registration_contract.functions.get_deposit_count().call()
         root = hash_tree_root(List[DepositData, 2**32](*deposit_data_list))
-        assert root == registration_contract.functions.get_hash_tree_root().call()
+        assert root == registration_contract.functions.get_deposit_root().call()

specs/core/0_deposit-contract.md

@@ -34,11 +34,11 @@ This document represents the specification for the beacon chain deposit contract
 
 ## Ethereum 1.0 deposit contract
 
-The initial deployment phases of Ethereum 2.0 are implemented without consensus changes to Ethereum 1.0. A deposit contract at address `DEPOSIT_CONTRACT_ADDRESS` is added to Ethereum 1.0 for deposits of ETH to the beacon chain. Validator balances will be withdrawable to the shards in Phase 2 (i.e. when the EVM 2.0 is deployed and the shards have state).
+The initial deployment phases of Ethereum 2.0 are implemented without consensus changes to Ethereum 1.0. A deposit contract at address `DEPOSIT_CONTRACT_ADDRESS` is added to Ethereum 1.0 for deposits of ETH to the beacon chain. Validator balances will be withdrawable to the shards in Phase 2.
 
 ### `deposit` function
 
-The deposit contract has a public `deposit` function to make deposits. It takes as arguments `pubkey: bytes[48], withdrawal_credentials: bytes[32], signature: bytes[96]` corresponding to a [`DepositData`](./0_beacon-chain.md#depositdata) object.
+The deposit contract has a public `deposit` function to make deposits. It takes as arguments `pubkey: bytes[48], withdrawal_credentials: bytes[32], signature: bytes[96], deposit_data_root: bytes32`. The first three arguments populate a [`DepositData`](./0_beacon-chain.md#depositdata) object, and `deposit_data_root` is the expected `DepositData` root as a protection against malformatted calldata.
 
 #### Deposit amount
 

specs/validator/0_beacon-chain-validator.md

@@ -218,7 +218,7 @@ def get_epoch_signature(state: BeaconState, block: BeaconBlock, privkey: int) ->
 
 ##### Eth1 Data
 
-The `block.eth1_data` field is for block proposers to vote on recent Eth 1.0 data. This recent data contains an Eth 1.0 block hash as well as the associated deposit root (as calculated by the `get_hash_tree_root()` method of the deposit contract) and deposit count after execution of the corresponding Eth 1.0 block. If over half of the block proposers in the current Eth 1.0 voting period vote for the same `eth1_data` then `state.eth1_data` updates at the end of the voting period. Each deposit in `block.body.deposits` must verify against `state.eth1_data.eth1_deposit_root`.
+The `block.eth1_data` field is for block proposers to vote on recent Eth 1.0 data. This recent data contains an Eth 1.0 block hash as well as the associated deposit root (as calculated by the `get_deposit_root()` method of the deposit contract) and deposit count after execution of the corresponding Eth 1.0 block. If over half of the block proposers in the current Eth 1.0 voting period vote for the same `eth1_data` then `state.eth1_data` updates at the end of the voting period. Each deposit in `block.body.deposits` must verify against `state.eth1_data.eth1_deposit_root`.
 
 Let `get_eth1_data(distance: uint64) -> Eth1Data` be the (subjective) function that returns the Eth 1.0 data at distance `distance` relative to the Eth 1.0 head at the start of the current Eth 1.0 voting period. Let `previous_eth1_distance` be the distance relative to the Eth 1.0 block corresponding to `state.eth1_data.block_hash` at the start of the current Eth 1.0 voting period. An honest block proposer sets `block.eth1_data = get_eth1_vote(state, previous_eth1_distance)` where: