Datatype cleanup (#1953)

* clear up spec todo

* test fix

* remove unnecessary toSszType

* type

* one more

beacon_chain/block_pools/spec_cache.nim

@@ -176,7 +176,7 @@ proc is_valid_indexed_attestation*(
 
 func makeAttestationData*(
     epochRef: EpochRef, bs: BlockSlot,
-    committee_index: uint64): AttestationData =
+    committee_index: CommitteeIndex): AttestationData =
   ## Create an attestation / vote for the block `bs` using the
   ## data in `epochRef` to fill in the rest of the fields.
   ## `epochRef` is the epoch information corresponding to the `bs` advanced to
@@ -193,7 +193,7 @@ func makeAttestationData*(
   # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/validator.md#attestation-data
   AttestationData(
     slot: slot,
-    index: committee_index,
+    index: committee_index.uint64,
     beacon_block_root: bs.blck.root,
     source: epochRef.current_justified_checkpoint,
     target: Checkpoint(

beacon_chain/rpc/config_api.nim

@@ -9,7 +9,7 @@ import
   chronicles,
   ../beacon_node_common,
 
-  ../spec/datatypes, ../eth2_json_rpc_serialization
+  ../spec/datatypes
 
 logScope: topics = "configapi"
 

beacon_chain/rpc/validator_api.nim

@@ -61,7 +61,7 @@ proc installValidatorApiHandlers*(rpcServer: RpcServer, node: BeaconNode) =
     let
       head = node.doChecksAndGetCurrentHead(slot)
       epochRef = node.chainDag.getEpochRef(head, slot.epoch)
-    return makeAttestationData(epochRef, head.atSlot(slot), committee_index.uint64)
+    return makeAttestationData(epochRef, head.atSlot(slot), committee_index)
 
   rpcServer.rpc("get_v1_validator_aggregate_attestation") do (
       slot: Slot, attestation_data_root: Eth2Digest)-> Attestation:

beacon_chain/spec/beaconstate.nim

@@ -105,9 +105,6 @@ proc process_deposit*(preset: RuntimePreset,
     if skipBLSValidation notin flags:
       if not verify_deposit_signature(preset, deposit.data):
         # It's ok that deposits fail - they get included in blocks regardless
-        # TODO spec test?
-        # TODO: This is temporary set to trace level in order to deal with the
-        #       large number of invalid deposits on Altona
         trace "Skipping deposit with invalid signature",
           deposit = shortLog(deposit.data)
         return ok()
@@ -316,8 +313,7 @@ func emptyBeaconBlockBody(): BeaconBlockBody =
   # TODO: This shouldn't be necessary if OpaqueBlob is the default
   BeaconBlockBody(randao_reveal: ValidatorSig(kind: OpaqueBlob))
 
-# TODO this is now a non-spec helper function, and it's not really accurate
+# https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/beacon-chain.md#genesis-block
-# so only usable/used in research/ and tests/
 func get_initial_beacon_block*(state: BeaconState): SignedBeaconBlock =
   let message = BeaconBlock(
     slot: state.slot,
@@ -659,7 +655,7 @@ proc process_attestation*(
   ok()
 
 func makeAttestationData*(
-    state: BeaconState, slot: Slot, committee_index: uint64,
+    state: BeaconState, slot: Slot, committee_index: CommitteeIndex,
     beacon_block_root: Eth2Digest): AttestationData =
   ## Create an attestation / vote for the block `beacon_block_root` using the
   ## data in `state` to fill in the rest of the fields.
@@ -680,7 +676,7 @@ func makeAttestationData*(
   # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/validator.md#attestation-data
   AttestationData(
     slot: slot,
-    index: committee_index,
+    index: committee_index.uint64,
     beacon_block_root: beacon_block_root,
     source: state.current_justified_checkpoint,
     target: Checkpoint(

beacon_chain/spec/crypto.nim

@@ -53,8 +53,8 @@ type
     OpaqueBlob
 
   BlsValue*[N: static int, T: blscurve.PublicKey or blscurve.Signature] = object
-    # TODO This is a temporary type needed until we sort out the
+    # Invalid BLS values may appear in SSZ blobs, their validity being checked
-    # issues with invalid BLS values appearing in the SSZ test suites.
+    # in consensus rather than SSZ decoding, thus we use a variant to hold either
     case kind*: BlsValueType
     of Real:
       blsValue*: T
@@ -86,9 +86,7 @@ export AggregateSignature
 
 func toPubKey*(privkey: ValidatorPrivKey): ValidatorPubKey =
   ## Create a private key from a public key
-  # Un-specced in either hash-to-curve or Eth2
+  # Un-specced in either hash-to-curve or Eth2  result.kind = Real
-  # TODO: Test suite should use `keyGen` instead
-  result.kind = Real
   let ok = result.blsValue.publicFromSecret(SecretKey privkey)
   doAssert ok, "The validator private key was a zero key. This should never happen."
 
@@ -111,10 +109,9 @@ proc toRealPubKey(pubkey: ValidatorPubKey): Option[ValidatorPubKey] =
           none ValidatorPubKey
       return validatorKeyCache.mGetOrPut(pubkey.blob, maybeRealKey)
 
-# TODO this needs a massive comment explaining the reasoning along with every
-# seemingly ad-hoc place where it's called - one shouldn't have to git-blame
-# commits and PRs for information which ought to be inplace here in the code
 proc initPubKey*(pubkey: ValidatorPubKey): ValidatorPubKey =
+  # Public keys are lazy-initialized, so this needs to be called before any
+  # other function using the public key is tried
   let key = toRealPubKey(pubkey)
   if key.isNone:
     return ValidatorPubKey()

beacon_chain/spec/datatypes.nim

@@ -33,7 +33,6 @@ import
 export
   sszTypes, presets
 
-# TODO Data types:
 # Presently, we're reusing the data types from the serialization (uint64) in the
 # objects we pass around to the beacon chain logic, thus keeping the two
 # similar. This is convenient for keeping up with the specification, but
@@ -65,12 +64,6 @@ const
   # Not part of spec. Still useful, pending removing usage if appropriate.
   ZERO_HASH* = Eth2Digest()
 
-  # Not part of spec
-  WEAK_SUBJECTVITY_PERIOD* =
-    Slot(uint64(4 * 30 * 24 * 60 * 60) div SECONDS_PER_SLOT)
-    # TODO: This needs revisiting.
-    # Why was the validator WITHDRAWAL_PERIOD altered in the spec?
-
   # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/p2p-interface.md#configuration
   ATTESTATION_PROPAGATION_SLOT_RANGE* = 32
   MAXIMUM_GOSSIP_CLOCK_DISPARITY* = 500.millis
@@ -101,15 +94,25 @@ type
 
   # https://github.com/nim-lang/Nim/issues/574 and be consistent across
   # 32-bit and 64-bit word platforms.
-  # TODO VALIDATOR_REGISTRY_LIMIT is 1 shl 40 in 0.12.1, and
+  # The distinct types here should only be used when data has been de-tainted
-  # proc newSeq(typ: PNimType, len: int): pointer {.compilerRtl.}
+  # following overflow checks - they cannot be used in SSZ objects as SSZ
-  # in Nim/lib/system/gc.nim quite tightly ties seq addressibility
+  # instances are not invalid _per se_ when they hold an out-of-bounds index -
-  # to the system wordsize. This lifts smaller, and now incorrect,
+  # that is part of consensus.
-  # range-limit.
+  # VALIDATOR_REGISTRY_LIMIT is 1^40 in spec 1.0, but if the number of
+  # validators ever grows near 1^32 that we support here, we'll have bigger
+  # issues than the size of this type to take care of. Until then, we'll use
+  # uint32 as it halves memory requirements for active validator sets,
+  # improves consistency on 32-vs-64-bit platforms and works better with
+  # Nim seq constraints.
   ValidatorIndex* = distinct uint32
-  Gwei* = uint64
+
+  # Though in theory the committee index would fit in a uint8, it is not used
+  # in a way that would significantly benefit from the smaller type, thus we
+  # leave it at spec size
   CommitteeIndex* = distinct uint64
 
+  Gwei* = uint64
+
   # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/beacon-chain.md#proposerslashing
   ProposerSlashing* = object
     signed_header_1*: SignedBeaconBlockHeader
@@ -122,13 +125,11 @@ type
 
   # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/beacon-chain.md#indexedattestation
   IndexedAttestation* = object
-    # TODO ValidatorIndex, but that doesn't serialize properly
     attesting_indices*: List[uint64, Limit MAX_VALIDATORS_PER_COMMITTEE]
     data*: AttestationData
     signature*: ValidatorSig
 
   TrustedIndexedAttestation* = object
-    # TODO ValidatorIndex, but that doesn't serialize properly
     attesting_indices*: List[uint64, Limit MAX_VALIDATORS_PER_COMMITTEE]
     data*: AttestationData
     signature*: TrustedSig
@@ -162,8 +163,6 @@ type
   AttestationData* = object
     slot*: Slot
 
-    # TODO this is actually a CommitteeIndex; remove some conversions by
-    # allowing SSZ to directly handle this
     index*: uint64
 
     # LMD GHOST vote
@@ -381,7 +380,6 @@ type
     aggregation_bits*: CommitteeValidatorsBits
     data*: AttestationData
 
-    # TODO this is a Slot
     inclusion_delay*: uint64
 
     proposer_index*: uint64
@@ -393,8 +391,6 @@ type
 
   # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/beacon-chain.md#fork
   Fork* = object
-    # TODO: Spec introduced an alias for Version = array[4, byte]
-    #       and a default parameter to compute_domain
     previous_version*: Version
     current_version*: Version
 
@@ -524,11 +520,19 @@ template ethTimeUnit(typ: type) {.dirty.} =
 
 proc writeValue*(writer: var JsonWriter, value: ValidatorIndex)
                 {.raises: [IOError, Defect].} =
-  writeValue(writer, uint32 value)
+  writeValue(writer, distinctBase value)
 
 proc readValue*(reader: var JsonReader, value: var ValidatorIndex)
                {.raises: [IOError, SerializationError, Defect].} =
-  value = ValidatorIndex reader.readValue(uint32)
+  value = ValidatorIndex reader.readValue(distinctBase ValidatorIndex)
+
+proc writeValue*(writer: var JsonWriter, value: CommitteeIndex)
+                {.raises: [IOError, Defect].} =
+  writeValue(writer, distinctBase value)
+
+proc readValue*(reader: var JsonReader, value: var CommitteeIndex)
+               {.raises: [IOError, SerializationError, Defect].} =
+  value = CommitteeIndex reader.readValue(distinctBase CommitteeIndex)
 
 template writeValue*(writer: var JsonWriter, value: Version | ForkDigest) =
   writeValue(writer, $value)
@@ -550,10 +554,6 @@ proc readValue*(reader: var JsonReader, value: var ForkDigest)
     raiseUnexpectedValue(reader, "Hex string of 4 bytes expected")
 
 # `ValidatorIndex` seq handling.
-# TODO harden these against uint32/uint64 to int type conversion risks
-func max*(a: ValidatorIndex, b: int) : auto =
-  max(a.int, b)
-
 func `[]`*[T](a: var seq[T], b: ValidatorIndex): var T =
   a[b.int]
 
@@ -567,7 +567,12 @@ func `[]=`*[T](a: var seq[T], b: ValidatorIndex, c: T) =
 proc `==`*(x, y: ValidatorIndex) : bool {.borrow, noSideEffect.}
 proc `<`*(x, y: ValidatorIndex) : bool {.borrow, noSideEffect.}
 proc hash*(x: ValidatorIndex): Hash {.borrow, noSideEffect.}
-func `$`*(x: ValidatorIndex): auto = $(x.int64)
+func `$`*(x: ValidatorIndex): auto = $(distinctBase(x))
+
+proc `==`*(x, y: CommitteeIndex) : bool {.borrow, noSideEffect.}
+proc `<`*(x, y: CommitteeIndex) : bool {.borrow, noSideEffect.}
+proc hash*(x: CommitteeIndex): Hash {.borrow, noSideEffect.}
+func `$`*(x: CommitteeIndex): auto = $(distinctBase(x))
 
 func `as`*(d: DepositData, T: type DepositMessage): T =
   T(pubkey: d.pubkey,

beacon_chain/spec/state_transition.nim

@@ -8,22 +8,14 @@
 # State transition, as described in
 # https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#beacon-chain-state-transition-function
 #
-# The purpose of this code right is primarily educational, to help piece
+# The entry point is `state_transition` which is at the bottom of the file!
-# together the mechanics of the beacon state and to discover potential problem
-# areas. The entry point is `state_transition` which is at the bottom of the file!
 #
-# General notes about the code (TODO):
+# General notes about the code:
 # * Weird styling - the sections taken from the spec use python styling while
 #   the others use NEP-1 - helps grepping identifiers in spec
-# * We mix procedural and functional styles for no good reason, except that the
+# * When updating the code, add TODO sections to mark where there are clear
-#   spec does so also.
+#   improvements to be made - other than that, keep things similar to spec unless
-# * For indices, we get a mix of uint64, ValidatorIndex and int - this is currently
+#   motivated by security or performance considerations
-#   swept under the rug with casts
-# * Sane error handling is missing in most cases (yay, we'll get the chance to
-#   debate exceptions again!)
-# When updating the code, add TODO sections to mark where there are clear
-# improvements to be made - other than that, keep things similar to spec for
-# now.
 
 {.push raises: [Defect].}
 
@@ -180,7 +172,6 @@ proc noRollback*(state: var HashedBeaconState) =
 proc state_transition*(
     preset: RuntimePreset,
     state: var HashedBeaconState, signedBlock: SomeSignedBeaconBlock,
-    # TODO this is ... okay, but not perfect; align with EpochRef
     stateCache: var StateCache,
     flags: UpdateFlags, rollback: RollbackHashedProc): bool {.nbench.} =
   ## Time in the beacon chain moves by slots. Every time (haha.) that happens,
@@ -212,7 +203,6 @@ proc state_transition*(
   if skipBLSValidation in flags or
       verify_block_signature(state.data, signedBlock):
 
-    # TODO after checking scaffolding, remove this
     trace "state_transition: processing block, signature passed",
       signature = shortLog(signedBlock.signature),
       blockRoot = shortLog(signedBlock.root)
@@ -246,8 +236,6 @@ proc state_transition*(
   state_transition(preset, state, signedBlock, cache, flags, rollback)
 
 # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/validator.md#preparing-for-a-beaconblock
-# TODO There's more to do here - the spec has helpers that deal set up some of
-#      the fields in here!
 proc makeBeaconBlock*(
     preset: RuntimePreset,
     state: var HashedBeaconState,

beacon_chain/spec/state_transition_block.nim

@@ -8,22 +8,14 @@
 # State transition - block processing, as described in
 # https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#beacon-chain-state-transition-function
 #
-# The purpose of this code right is primarily educational, to help piece
-# together the mechanics of the beacon state and to discover potential problem
-# areas.
-#
 # The entry point is `process_block` which is at the bottom of this file.
 #
-# General notes about the code (TODO):
+# General notes about the code:
 # * Weird styling - the sections taken from the spec use python styling while
 #   the others use NEP-1 - helps grepping identifiers in spec
-# * We mix procedural and functional styles for no good reason, except that the
+# * When updating the code, add TODO sections to mark where there are clear
-#   spec does so also.
+#   improvements to be made - other than that, keep things similar to spec unless
-# * For indices, we get a mix of uint64, ValidatorIndex and int - this is currently
+#   motivated by security or performance considerations
-#   swept under the rug with casts
-# When updating the code, add TODO sections to mark where there are clear
-# improvements to be made - other than that, keep things similar to spec for
-# now.
 
 {.push raises: [Defect].}
 

beacon_chain/spec/state_transition_epoch.nim

@@ -10,14 +10,12 @@
 #
 # The entry point is `process_epoch`, which is at the bottom of this file.
 #
-# General notes about the code (TODO):
+# General notes about the code:
 # * Weird styling - the sections taken from the spec use python styling while
 #   the others use NEP-1 - helps grepping identifiers in spec
-# * For indices, we get a mix of uint64, ValidatorIndex and int - this is currently
+# * When updating the code, add TODO sections to mark where there are clear
-#   swept under the rug with casts
+#   improvements to be made - other than that, keep things similar to spec unless
-# When updating the code, add TODO sections to mark where there are clear
+#   motivated by security or performance considerations
-# improvements to be made - other than that, keep things similar to spec for
-# now.
 
 {.push raises: [Defect].}
 

beacon_chain/spec/validator.nim

@@ -161,10 +161,6 @@ func get_committee_count_per_slot*(state: BeaconState,
                                    cache: var StateCache): uint64 =
   # Return the number of committees at ``slot``.
 
-  # TODO this is mostly used in for loops which have indexes which then need to
-  # be converted to CommitteeIndex types for get_beacon_committee(...); replace
-  # with better and more type-safe use pattern, probably beginning with using a
-  # CommitteeIndex return type here.
   let
     active_validator_count = count_active_validators(state, epoch, cache)
   result = get_committee_count_per_slot(active_validator_count)

beacon_chain/ssz/bytes_reader.nim

@@ -46,11 +46,11 @@ func fromSszBytes*(T: type GraffitiBytes, data: openArray[byte]): T {.raisesssz.
     raiseIncorrectSize T
   copyMem(result.addr, unsafeAddr data[0], sizeof(result))
 
-template fromSszBytes*(T: type Slot, bytes: openArray[byte]): Slot =
+template fromSszBytes*(T: type Slot, bytes: openArray[byte]): T =
-  Slot fromSszBytes(uint64, bytes)
+  T fromSszBytes(uint64, bytes)
 
-template fromSszBytes*(T: type Epoch, bytes: openArray[byte]): Epoch =
+template fromSszBytes*(T: type Epoch, bytes: openArray[byte]): T =
-  Epoch fromSszBytes(uint64, bytes)
+  T fromSszBytes(uint64, bytes)
 
 func fromSszBytes*(T: type ForkDigest, bytes: openArray[byte]): T {.raisesssz.} =
   if bytes.len != sizeof(result):

beacon_chain/validator_duties.nim

@@ -361,7 +361,8 @@ proc handleAttestations(node: BeaconNode, head: BlockRef, slot: Slot) =
     for index_in_committee, validatorIdx in committee:
       let validator = node.getAttachedValidator(epochRef, validatorIdx)
       if validator != nil:
-        let ad = makeAttestationData(epochRef, attestationHead, committee_index)
+        let ad = makeAttestationData(
+          epochRef, attestationHead, committee_index.CommitteeIndex)
         attestations.add((ad, committee.len, index_in_committee, validator))
 
   for a in attestations:

research/block_sim.nim

@@ -80,7 +80,8 @@ cli do(slots = SLOTS_PER_EPOCH * 6,
         for index_in_committee, validatorIdx in committee:
           if rand(r, 1.0) <= attesterRatio:
             let
-              data = makeAttestationData(state, slot, committee_index, blck.root)
+              data = makeAttestationData(
+                state, slot, committee_index.CommitteeIndex, blck.root)
               sig =
                 get_attestation_signature(state.fork,
                   state.genesis_validators_root,

research/state_sim.nim

@@ -130,13 +130,13 @@ cli do(slots = SLOTS_PER_EPOCH * 6,
               if first:
                 attestation =
                   makeAttestation(state[].data, latest_block_root, scas, target_slot,
-                    i.uint64, v, cache, flags)
+                    i.CommitteeIndex, v, cache, flags)
                 agg.init(attestation.signature)
                 first = false
               else:
                 let att2 =
                   makeAttestation(state[].data, latest_block_root, scas, target_slot,
-                    i.uint64, v, cache, flags)
+                    i.CommitteeIndex, v, cache, flags)
                 if not att2.aggregation_bits.overlaps(attestation.aggregation_bits):
                   attestation.aggregation_bits.combine(att2.aggregation_bits)
                   if skipBlsValidation notin flags:

tests/test_attestation_pool.nim

@@ -366,8 +366,7 @@ suiteReport "Attestation pool processing" & preset():
             aggregation_bits: aggregation_bits,
             data: makeAttestationData(
               state.data.data, state.data.data.slot,
-              index, blockroot
+              index.CommitteeIndex, blockroot)
-            )
             # signature: ValidatorSig()
           )
 

tests/testblockutil.nim

@@ -7,9 +7,7 @@
 
 import
   options, stew/endians2,
-  chronicles, eth/trie/[db],
+  ../beacon_chain/[extras, merkle_minimal, validator_pool],
-  ../beacon_chain/[beacon_chain_db, extras,
-                   merkle_minimal, validator_pool],
   ../beacon_chain/ssz/merkleization,
   ../beacon_chain/spec/[beaconstate, crypto, datatypes, digest, presets,
                         helpers, validator, signatures, state_transition]
@@ -157,8 +155,8 @@ proc makeTestBlock*(
 
 proc makeAttestation*(
     state: BeaconState, beacon_block_root: Eth2Digest,
-    committee: seq[ValidatorIndex], slot: Slot, index: uint64,
+    committee: seq[ValidatorIndex], slot: Slot, index: CommitteeIndex,
-    validator_index: auto, cache: var StateCache,
+    validator_index: ValidatorIndex, cache: var StateCache,
     flags: UpdateFlags = {}): Attestation =
   # Avoids state_sim silliness; as it's responsible for all validators,
   # transforming, from monotonic enumerable index -> committee index ->
@@ -197,8 +195,8 @@ proc find_beacon_committee(
     let
       slot = ((epoch_committee_index mod SLOTS_PER_EPOCH) +
         epoch.compute_start_slot_at_epoch.uint64).Slot
-      index = epoch_committee_index div SLOTS_PER_EPOCH
+      index = CommitteeIndex(epoch_committee_index div SLOTS_PER_EPOCH)
-      committee = get_beacon_committee(state, slot, index.CommitteeIndex, cache)
+      committee = get_beacon_committee(state, slot, index, cache)
     if validator_index in committee:
       return (committee, slot, index)
   doAssert false
@@ -225,7 +223,7 @@ proc makeFullAttestations*(
     let
       committee = get_beacon_committee(
         state, slot, index.CommitteeIndex, cache)
-      data = makeAttestationData(state, slot, index, beacon_block_root)
+      data = makeAttestationData(state, slot, index.CommitteeIndex, beacon_block_root)
 
     doAssert committee.len() >= 1
     # Initial attestation