Mostly remove skipMerkleValidation by fixing Merkle proof construction/usage (#879)

* refactor and fix merkle proof construction in test suite and thereby remove most remaining skipMerkleValidation flags, now unnecessary

* a few non-semantic comment update/removals

beacon_chain/attestation_aggregation.nim

@@ -5,13 +5,6 @@
 #   * Apache v2 license (license terms in the root directory or at http://www.apache.org/licenses/LICENSE-2.0).
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
-# The other part is arguably part of attestation pool -- the validation's
-# something that should be happing on receipt, not aggregation per se. In
-# that part, check that messages conform -- so, check for each type
-# https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/specs/phase0/p2p-interface.md#topics-and-messages
-# specifies. So by the time this calls attestation pool, all validation's
-# already done.
-
 import
   options,
   ./spec/[beaconstate, datatypes, crypto, digest, helpers, validator,

beacon_chain/spec/presets/mainnet.nim

@@ -80,7 +80,7 @@ const
 
   # Time parameters
   # ---------------------------------------------------------------
-  # https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/configs/mainnet.yaml#L71
+  # https://github.com/ethereum/eth2.0-specs/blob/v0.11.1/configs/mainnet.yaml#L77
   MIN_GENESIS_DELAY* = 86400 # 86400 seconds (1 day)
 
   SECONDS_PER_SLOT*{.intdefine.} = 12'u64 # Compile with -d:SECONDS_PER_SLOT=1 for 12x faster slots

beacon_chain/spec/presets/minimal.nim

@@ -62,7 +62,7 @@ const
 
   # Initial values
   # ---------------------------------------------------------------
-  # https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/configs/minimal.yaml#L64
+  # https://github.com/ethereum/eth2.0-specs/blob/v0.11.1/configs/minimal.yaml#L70
 
   # Unchanged
   GENESIS_SLOT* = 0.Slot

beacon_chain/state_transition.nim

@@ -31,7 +31,7 @@
 # now.
 
 import
-  collections/sets, chronicles, sets, options,
+  collections/sets, chronicles, sets,
   ./extras, ./ssz, metrics,
   ./spec/[datatypes, crypto, digest, helpers, validator],
   ./spec/[state_transition_block, state_transition_epoch],

research/state_sim.nim

@@ -79,8 +79,7 @@ cli do(slots = SLOTS_PER_EPOCH * 6,
 
   let
     genesisState =
-      initialize_beacon_state_from_eth1(
+      initialize_beacon_state_from_eth1(Eth2Digest(), 0, deposits, flags)
-        Eth2Digest(), 0, deposits, flags + {skipMerkleValidation})
     genesisBlock = get_initial_beacon_block(genesisState)
 
   echo "Starting simulation..."

tests/mocking/mock_deposits.nim

@@ -1,5 +1,5 @@
 # beacon_chain
-# Copyright (c) 2018-2019 Status Research & Development GmbH
+# Copyright (c) 2018-2020 Status Research & Development GmbH
 # Licensed and distributed under either of
 #   * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
 #   * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
@@ -12,11 +12,13 @@ import
   # Standard library
   math, random,
   # Specs
-  ../../beacon_chain/spec/[datatypes, crypto, helpers, digest, beaconstate],
+  ../../beacon_chain/spec/[datatypes, crypto, helpers, digest],
   # Internals
   ../../beacon_chain/[ssz, extras],
   # Mocking procs
-  ./merkle_minimal, ./mock_validator_keys
+  ./mock_validator_keys,
+  # Other test utilities, for attachMerkleProofs()
+  ../testblockutil
 
 func signMockDepositData(
         deposit_data: var DepositData,
@@ -143,26 +145,6 @@ template mockGenesisDepositsImpl(
       depositsData.add result[valIdx].data
       depositsDataHash.add hash_tree_root(result[valIdx].data)
 
-    # 2nd & 3rd loops - build hashes and proofs
-    let root = hash_tree_root(depositsData)
-    let tree = merkleTreeFromLeaves(depositsDataHash)
-
-    # 4th loop - append proof
-    for valIdx in 0 ..< validatorCount.int:
-      # TODO ensure genesis & deposit process tests suffice to catch whether
-      # changes here break things; ensure that this matches the merkle proof
-      # sequence is_valid_merkle_branch(...) now looks for
-      result[valIdx].proof[0..31] = tree.getMerkleProof(valIdx)
-      result[valIdx].proof[32] =
-        Eth2Digest(data: int_to_bytes32((valIdx + 1).uint64))
-      doAssert is_valid_merkle_branch(
-          depositsDataHash[valIdx],
-          result[valIdx].proof,
-          DEPOSIT_CONTRACT_TREE_DEPTH,
-          valIdx.uint64,
-          root
-        )
-
 proc mockGenesisBalancedDeposits*(
         validatorCount: uint64,
         amountInEth: Positive,
@@ -179,6 +161,7 @@ proc mockGenesisBalancedDeposits*(
   let amount = amountInEth.uint64 * 10'u64^9
   mockGenesisDepositsImpl(result, validatorCount,amount,flags):
     discard
+  attachMerkleProofs(result)
 
 proc mockGenesisUnBalancedDeposits*(
         validatorCount: uint64,
@@ -199,6 +182,7 @@ proc mockGenesisUnBalancedDeposits*(
 
   mockGenesisDepositsImpl(result, validatorCount, amount, flags):
     amount = rng.rand(amountRangeInEth).uint64 * 10'u64^9
+  attachMerkleProofs(result)
 
 proc mockUpdateStateForNewDeposit*(
        state: var BeaconState,
@@ -220,14 +204,13 @@ proc mockUpdateStateForNewDeposit*(
     flags
   )
 
-  when false: # TODO
+  var result_seq = @[result]
-    let tree = merkleTreeFromLeaves([hash_tree_root(result.data)])
+  attachMerkleProofs(result_seq)
-    result[valIdx].proof[0..31] = tree.getMerkleProof(0)
+  result.proof = result_seq[0].proof
-    result[valIdx].proof[32] = int_to_bytes32(0 + 1)
-    # doAssert is_valid_merkle_branch(...)
 
   # TODO: this logic from the eth2.0-specs test suite seems strange
   #       but confirmed by running it
   state.eth1_deposit_index = 0
-  state.eth1_data.deposit_root = hash_tree_root(result.data)
+  state.eth1_data.deposit_root =
+     hash_tree_root(sszList(@[result.data], 2'i64^DEPOSIT_CONTRACT_TREE_DEPTH))
   state.eth1_data.deposit_count = 1

tests/mocking/mock_genesis.nim

@@ -1,5 +1,5 @@
 # beacon_chain
-# Copyright (c) 2018-2019 Status Research & Development GmbH
+# Copyright (c) 2018-2020 Status Research & Development GmbH
 # Licensed and distributed under either of
 #   * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
 #   * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
@@ -25,7 +25,7 @@ proc initGenesisState*(num_validators: uint64, genesis_time: uint64 = 0): Beacon
     )
 
   initialize_beacon_state_from_eth1(
-    eth1BlockHash, 0, deposits, {skipBlsValidation, skipMerkleValidation})
+    eth1BlockHash, 0, deposits, {skipBlsValidation})
 
 when isMainModule:
   # Smoke test

tests/spec_block_processing/test_process_deposits.nim

@@ -1,5 +1,5 @@
 # beacon_chain
-# Copyright (c) 2018 Status Research & Development GmbH
+# Copyright (c) 2018-2020 Status Research & Development GmbH
 # Licensed and distributed under either of
 #   * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
 #   * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
@@ -56,8 +56,7 @@ suiteReport "[Unit - Spec - Block processing] Deposits " & preset():
 
       # State transition
       # ----------------------------------------
-      check: state.process_deposit(deposit,
+      check: state.process_deposit(deposit, {skipBlsValidation})
-        {skipBlsValidation, skipMerkleValidation})
 
       # Check invariants
       # ----------------------------------------
@@ -101,8 +100,7 @@ suiteReport "[Unit - Spec - Block processing] Deposits " & preset():
 
     # State transition
     # ----------------------------------------
-    check: state.process_deposit(deposit,
+    check: state.process_deposit(deposit, {skipBlsValidation})
-      {skipBlsValidation, skipMerkleValidation})
 
     # Check invariants
     # ----------------------------------------

tests/test_beacon_chain_db.nim

@@ -99,7 +99,7 @@ suiteReport "Beacon chain DB" & preset():
 
     let
       state = initialize_beacon_state_from_eth1(
-        eth1BlockHash, 0, makeInitialDeposits(SLOTS_PER_EPOCH), {skipBlsValidation, skipMerkleValidation})
+        eth1BlockHash, 0, makeInitialDeposits(SLOTS_PER_EPOCH), {skipBlsValidation})
       root = hash_tree_root(state)
 
     db.putState(state)

tests/test_beaconstate.nim

@@ -16,6 +16,5 @@ import
 suiteReport "Beacon state" & preset():
   timedTest "Smoke test initialize_beacon_state_from_eth1" & preset():
     let state = initialize_beacon_state_from_eth1(
-      Eth2Digest(), 0,
+      Eth2Digest(), 0, makeInitialDeposits(SLOTS_PER_EPOCH, {}), {})
-      makeInitialDeposits(SLOTS_PER_EPOCH, {}), {skipMerkleValidation})
     check: state.validators.len == SLOTS_PER_EPOCH

tests/test_state_transition.nim

@@ -21,8 +21,7 @@ suiteReport "Block processing" & preset():
     # Genesis state with minimal number of deposits
     # TODO bls verification is a bit of a bottleneck here
     genesisState = initialize_beacon_state_from_eth1(
-      Eth2Digest(), 0,
+      Eth2Digest(), 0, makeInitialDeposits(), {})
-      makeInitialDeposits(), {skipMerkleValidation})
     genesisBlock = get_initial_beacon_block(genesisState)
     genesisRoot = hash_tree_root(genesisBlock.message)
 

tests/testblockutil.nim

@@ -6,8 +6,9 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  options, stew/endians2,
+  options, sequtils, stew/endians2,
   chronicles, eth/trie/[db],
+  ./mocking/merkle_minimal,
   ../beacon_chain/[beacon_chain_db, block_pool, extras, ssz, state_transition,
     validator_pool],
   ../beacon_chain/spec/[beaconstate, crypto, datatypes, digest,
@@ -57,11 +58,38 @@ func makeDeposit(i: int, flags: UpdateFlags): Deposit =
     let signing_root = compute_signing_root(result.getDepositMessage, domain)
     result.data.signature = bls_sign(privkey, signing_root.data)
 
-func makeInitialDeposits*(
+proc attachMerkleProofs*(deposits: var seq[Deposit]) =
+  let deposit_data_roots = mapIt(deposits, it.data.hash_tree_root)
+  var
+    deposit_data_sums: seq[Eth2Digest]
+  for prefix_root in hash_tree_roots_prefix(
+      deposit_data_roots, 1'i64 shl DEPOSIT_CONTRACT_TREE_DEPTH):
+    deposit_data_sums.add prefix_root
+
+  for val_idx in 0 ..< deposits.len:
+    let merkle_tree = merkleTreeFromLeaves(deposit_data_roots[0..val_idx])
+    deposits[val_idx].proof[0..31] = merkle_tree.getMerkleProof(val_idx)
+    deposits[val_idx].proof[32].data[0..7] = int_to_bytes8((val_idx + 1).uint64)
+
+    doAssert is_valid_merkle_branch(
+      deposit_data_roots[val_idx], deposits[val_idx].proof,
+      DEPOSIT_CONTRACT_TREE_DEPTH + 1, val_idx.uint64,
+      deposit_data_sums[val_idx])
+
+proc makeInitialDeposits*(
     n = SLOTS_PER_EPOCH, flags: UpdateFlags = {}): seq[Deposit] =
   for i in 0..<n.int:
     result.add makeDeposit(i, flags)
 
+  # This needs to be done as a batch, since the Merkle proof of the i'th
+  # deposit depends on the deposit (data) of the 0th through (i-1)st, of
+  # deposits. Computing partial hash_tree_root sequences of DepositData,
+  # and ideally (but not yet) efficiently only once calculating a Merkle
+  # tree utilizing as much of the shared substructure as feasible, means
+  # attaching proofs all together, as a separate step.
+  if skipMerkleValidation notin flags:
+    attachMerkleProofs(result)
+
 func signBlock*(
     fork: Fork, genesis_validators_root: Eth2Digest, blck: BeaconBlock,
     privKey: ValidatorPrivKey, flags: UpdateFlags = {}): SignedBeaconBlock =

tests/testutil.nim

@@ -101,7 +101,7 @@ proc makeTestDB*(validators: int): BeaconChainDB =
     genState = initialize_beacon_state_from_eth1(
       Eth2Digest(), 0,
       makeInitialDeposits(validators, flags = {skipBlsValidation}),
-        {skipBlsValidation, skipMerkleValidation})
+        {skipBlsValidation})
     genBlock = get_initial_beacon_block(genState)
   makeTestDB(genState, genBlock)