Attestations EF tests + Proper Real/Opaque BlsValue deserialization (#410)

* Add attestation unit test

* process_attestation doesn't throw exceptions

* Allow SSZ deserialization of both real and invalid signatures

* Add new process_attestation checks - pass all process_attestation tests

* Add sanity check for #361

* Fix SSZ testing after fromBytes/fromSSZBytes changes

beacon_chain/spec/beaconstate.nim

@@ -521,6 +521,20 @@ proc check_attestation*(
       state_slot = shortLog(stateSlot))
     return
 
+  let committee = get_crosslink_committee(state, data.target.epoch, data.crosslink.shard, stateCache)
+  if attestation.aggregation_bits.len != attestation.custody_bits.len:
+    warn("Inconsistent aggregation and custody bits",
+      aggregation_bits_len = attestation.aggregation_bits.len,
+      custody_bits_len = attestation.custody_bits.len
+    )
+    return
+  if attestation.aggregation_bits.len != committee.len:
+    warn("Inconsistent aggregation and committee length",
+      aggregation_bits_len = attestation.aggregation_bits.len,
+      committee_len = committee.len
+    )
+    return
+
   # Check FFG data, crosslink data, and signature
   let ffg_check_data = (data.source.epoch, data.source.root, data.target.epoch)
 

beacon_chain/spec/crypto.nim

@@ -183,7 +183,9 @@ func bls_verify_multiple*(
     sig: ValidatorSig, domain: uint64): bool =
   let L = len(pubkeys)
   doAssert L == len(message_hashes)
-  doAssert sig.kind == Real
+  if sig.kind != Real:
+    # TODO: chronicles warning
+    return false
 
   # TODO optimize using multiPairing
   for pubkey_message_hash in zip(pubkeys, message_hashes):
@@ -214,15 +216,17 @@ else:
 proc fromBytes*[T](R: type BlsValue[T], bytes: openarray[byte]): R =
   # This is a workaround, so that we can deserialize the serialization of a
   # default-initialized BlsValue without raising an exception
-  # TODO don't use exceptions for parsing, ever. Handle deserialization issues
-  #      sanely, always.
   when defined(ssz_testing):
+    # Only for SSZ parsing tests, everything is an opaque blob
     R(kind: OpaqueBlob, blob: toArray(result.blob.len, bytes))
   else:
-    if bytes.allIt(it == 0):
-      R(kind: OpaqueBlob)
-    else:
-      R(kind: Real, blsValue: init(T, bytes))
+    # Try if valid BLS value
+    let success = init(result.blsValue, bytes)
+    if not success:
+      # TODO: chronicles trace
+      result = R(kind: OpaqueBlob)
+      assert result.blob.len == bytes.len
+      result.blob[result.blob.low .. result.blob.high] = bytes
 
 proc initFromBytes*[T](val: var BlsValue[T], bytes: openarray[byte]) =
   val = fromBytes(BlsValue[T], bytes)

tests/official/all_fixtures_require_ssz.nim

@@ -12,4 +12,5 @@
 import
   ./test_fixture_sanity_slots,
   ./test_fixture_sanity_blocks,
-  ./test_fixture_state_transition_epoch
+  ./test_fixture_state_transition_epoch,
+  ./test_fixture_operations_attestations

tests/official/all_fixtures_require_ssz.nim.cfg

@@ -1 +0,0 @@
--d:"ssz_testing"

tests/official/test_fixture_operations_attestations.nim

@@ -0,0 +1,95 @@
+# beacon_chain
+# Copyright (c) 2018-Present Status Research & Development GmbH
+# Licensed and distributed under either of
+#   * MIT license (license terms in the root directory or at http://opensource.org/licenses/MIT).
+#   * 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.
+
+import
+  # Standard library
+  os, unittest, strutils,
+  # Beacon chain internals
+  ../../beacon_chain/spec/[datatypes, beaconstate, validator],
+  ../../beacon_chain/[ssz, extras],
+  # Test utilities
+  ../testutil,
+  ./fixtures_utils,
+  ../helpers/debug_state,
+  ../mocking/mock_blocks
+
+const OperationsAttestationsDir = SszTestsDir/const_preset/"phase0"/"operations"/"attestation"/"pyspec_tests"
+
+template runTest(testName: string, identifier: untyped) =
+  # We wrap the tests in a proc to avoid running out of globals
+  # in the future: Nim supports up to 3500 globals
+  # but unittest with the macro/templates put everything as globals
+  # https://github.com/nim-lang/Nim/issues/12084#issue-486866402
+
+  const testDir = OperationsAttestationsDir / astToStr(identifier)
+
+  proc `testImpl _ operations_attestations _ identifier`() =
+
+    var flags: UpdateFlags
+    var prefix: string
+    if not existsFile(testDir/"meta.yaml"):
+      flags.incl skipValidation
+    if existsFile(testDir/"post.ssz"):
+      prefix = "[Valid]   "
+    else:
+      prefix = "[Invalid] "
+
+    test prefix & testName & " (" & astToStr(identifier) & ")":
+      var stateRef, postRef: ref BeaconState
+      var attestationRef: ref Attestation
+      new attestationRef
+      new stateRef
+
+      var cache = get_empty_per_epoch_cache()
+
+      attestationRef[] = parseTest(testDir/"attestation.ssz", SSZ, Attestation)
+      stateRef[] = parseTest(testDir/"pre.ssz", SSZ, BeaconState)
+
+      if existsFile(testDir/"post.ssz"):
+        new postRef
+        postRef[] = parseTest(testDir/"post.ssz", SSZ, BeaconState)
+
+      if postRef.isNil:
+        let done = process_attestation(stateRef[], attestationRef[], flags, cache)
+        doAssert done == false, "We didn't expect this invalid attestation to be processed."
+      else:
+        let done = process_attestation(stateRef[], attestationRef[], flags, cache)
+        doAssert done, "Valid attestation not processed"
+        check: stateRef.hash_tree_root() == postRef.hash_tree_root()
+        reportDiff(stateRef, postRef)
+
+  `testImpl _ operations_attestations _ identifier`()
+
+suite "Official - Operations - Attestations " & preset():
+  runTest("success", success)
+  runTest("success previous epoch", success_previous_epoch)
+  when const_preset == "minimal":
+    runTest("success since max epochs per crosslink", success_since_max_epochs_per_crosslink)
+    runTest("wrong end epoch with max epochs per crosslink", wrong_end_epoch_with_max_epochs_per_crosslink)
+  runTest("invalid attestation signature", invalid_attestation_signature)
+  runTest("before inclusion delay", before_inclusion_delay)
+  runTest("after_epoch_slots", after_epoch_slots)
+  runTest("old source epoch", old_source_epoch)
+  runTest("wrong shard", wrong_shard)
+  runTest("invalid shard", invalid_shard)
+  runTest("old target epoch", old_target_epoch)
+  runTest("future target epoch", future_target_epoch)
+  runTest("new source epoch", new_source_epoch)
+  runTest("source root is target root", source_root_is_target_root)
+  runTest("invalid current source root", invalid_current_source_root)
+  runTest("bad source root", bad_source_root)
+  runTest("non-zero crosslink data root", non_zero_crosslink_data_root)
+  runTest("bad parent crosslink", bad_parent_crosslink)
+  runTest("bad crosslink start epoch", bad_crosslink_start_epoch)
+  runTest("bad crosslink end epoch", bad_crosslink_end_epoch)
+  runTest("inconsistent bits", inconsistent_bits)
+  runTest("non-empty custody bits", non_empty_custody_bits)
+  runTest("empty aggregation bits", empty_aggregation_bits)
+  runTest("too many aggregation bits", too_many_aggregation_bits)
+  runTest("too few aggregation bits", too_few_aggregation_bits)
+  runTest("too many custody bits", too_many_custody_bits)
+  runTest("too few custody bits", too_few_custody_bits)

tests/official/test_fixture_sanity_blocks.nim.cfg

@@ -1 +0,0 @@
--d:"ssz_testing"

tests/official/test_fixture_sanity_slots.nim.cfg

@@ -1 +0,0 @@
--d:"ssz_testing"

tests/official/test_fixture_state_transition_epoch.nim.cfg

@@ -1 +0,0 @@
--d:"ssz_testing"