small spec cleanups (#1501)

* clean up logging a bit
* return error on indexed attestation check

beacon_chain/attestation_aggregation.nim

@@ -82,18 +82,18 @@ proc isValidAttestationSlot(
     attestationBlck = shortLog(attestationBlck)
 
   if not (attestationBlck.slot > pool.chainDag.finalizedHead.slot):
-    debug "voting for already-finalized block"
+    debug "Voting for already-finalized block"
     return false
 
   # we'll also cap it at 4 epochs which is somewhat arbitrary, but puts an
   # upper bound on the processing done to validate the attestation
   # TODO revisit with less arbitrary approach
   if not (attestationSlot >= attestationBlck.slot):
-    debug "voting for block that didn't exist at the time"
+    debug "Voting for block that didn't exist at the time"
     return false
 
   if not ((attestationSlot - attestationBlck.slot) <= uint64(4 * SLOTS_PER_EPOCH)):
-    debug "voting for very old block"
+    debug "Voting for very old block"
     return false
 
   true
@@ -121,7 +121,7 @@ proc isValidAttestation*(
     return false
 
   if not checkPropagationSlotRange(attestation.data, current_slot):
-    debug "attestation.data.slot not within ATTESTATION_PROPAGATION_SLOT_RANGE",
+    debug "Attestation slot not in propagation range",
       current_slot
     return false
 
@@ -137,10 +137,10 @@ proc isValidAttestation*(
       continue
     onesCount += 1
     if onesCount > 1:
-      debug "attestation has too many aggregation bits"
+      debug "Attestation has too many aggregation bits"
       return false
   if onesCount != 1:
-    debug "attestation has too few aggregation bits"
+    debug "Attestation has too few aggregation bits"
     return false
 
   # The attestation is the first valid attestation received for the
@@ -153,7 +153,7 @@ proc isValidAttestation*(
       # Attestations might be aggregated eagerly or lazily; allow for both.
       for validation in attestationEntry.validations:
         if attestation.aggregation_bits.isSubsetOf(validation.aggregation_bits):
-          debug "attestation already exists at slot",
+          debug "Attestation already exists at slot",
             attestation_pool_validation = validation.aggregation_bits
           return false
 
@@ -163,7 +163,7 @@ proc isValidAttestation*(
   # of the block in the pool.
   let attestationBlck = pool.chainDag.getRef(attestation.data.beacon_block_root)
   if attestationBlck.isNil:
-    debug "Block not found"
+    debug "Attestation block unknown"
     pool.addUnresolved(attestation)
     pool.quarantine.addMissing(attestation.data.beacon_block_root)
     return false
@@ -174,7 +174,7 @@ proc isValidAttestation*(
 
   let tgtBlck = pool.chainDag.getRef(attestation.data.target.root)
   if tgtBlck.isNil:
-    debug "Target block not found"
+    debug "Attestation target block unknown"
     pool.addUnresolved(attestation)
     pool.quarantine.addMissing(attestation.data.target.root)
     return false
@@ -205,7 +205,7 @@ proc isValidAttestation*(
         attestation.data.slot, attestation.data.index.CommitteeIndex)
 
   if requiredSubnetIndex != topicCommitteeIndex:
-    debug "attestation's committee index not for the correct subnet",
+    debug "Attestation's committee index not for the correct subnet",
       topicCommitteeIndex = topicCommitteeIndex,
       attestation_data_index = attestation.data.index,
       requiredSubnetIndex = requiredSubnetIndex
@@ -217,10 +217,10 @@ proc isValidAttestation*(
       pool.chainDag.headState.data.data.genesis_validators_root
 
     # The signature of attestation is valid.
-  if not is_valid_indexed_attestation(
+  if (let v = is_valid_indexed_attestation(
       fork, genesis_validators_root,
-      epochRef, get_indexed_attestation(epochRef, attestation), {}):
+      epochRef, get_indexed_attestation(epochRef, attestation), {}); v.isErr):
-    debug "signature verification failed"
+    debug "Attestation verification failed", err = v.error()
     return false
 
   true
@@ -251,7 +251,7 @@ proc isValidAggregatedAttestation*(
   # MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. aggregate.data.slot +
   # ATTESTATION_PROPAGATION_SLOT_RANGE >= current_slot >= aggregate.data.slot
   if not checkPropagationSlotRange(aggregate.data, current_slot):
-    debug "aggregation.data.slot not within ATTESTATION_PROPAGATION_SLOT_RANGE"
+    debug "Aggregate slot not in propoagation range"
     return false
 
   # [IGNORE] The valid aggregate attestation defined by
@@ -273,7 +273,7 @@ proc isValidAggregatedAttestation*(
   # passes validation.
   let attestationBlck = pool.chainDag.getRef(aggregate.data.beacon_block_root)
   if attestationBlck.isNil:
-    debug "Block not found"
+    debug "Aggregate block unknown"
     pool.quarantine.addMissing(aggregate.data.beacon_block_root)
     return false
 
@@ -291,7 +291,7 @@ proc isValidAggregatedAttestation*(
   # But (2) would reflect an invalid aggregation in other ways, so reject it
   # either way.
   if isZeros(aggregate.aggregation_bits):
-    debug "Attestation has no or invalid aggregation bits"
+    debug "Aggregate has no or invalid aggregation bits"
     return false
 
   if not isValidAttestationSlot(pool, aggregate.data.slot, attestationBlck):
@@ -303,7 +303,7 @@ proc isValidAggregatedAttestation*(
   # aggregate.data.index, aggregate_and_proof.selection_proof) returns True.
   let tgtBlck = pool.chainDag.getRef(aggregate.data.target.root)
   if tgtBlck.isNil:
-    debug "Target block not found"
+    debug "Aggregate target block unknown"
     pool.quarantine.addMissing(aggregate.data.target.root)
     return
 
@@ -348,14 +348,14 @@ proc isValidAggregatedAttestation*(
       fork, genesis_validators_root, aggregate_and_proof,
       epochRef.validator_keys[aggregate_and_proof.aggregator_index],
       signed_aggregate_and_proof.signature):
-    debug "Signed_aggregate_and_proof signature verification failed"
+    debug "signed_aggregate_and_proof signature verification failed"
     return false
 
   # [REJECT] The signature of aggregate is valid.
-  if not is_valid_indexed_attestation(
+  if (let v = is_valid_indexed_attestation(
       fork, genesis_validators_root,
-      epochRef, get_indexed_attestation(epochRef, aggregate), {}):
+      epochRef, get_indexed_attestation(epochRef, aggregate), {}); v.isErr):
-    debug "Aggregate signature verification failed"
+    debug "Aggregate verification failed", err = v.error()
     return false
 
   # The following rule follows implicitly from that we clear out any

beacon_chain/block_pools/spec_cache.nim

@@ -7,7 +7,6 @@
 
 import
   std/[algorithm, sequtils, sets],
-  chronicles,
   ../spec/[
     beaconstate, crypto, datatypes, digest, helpers, presets, signatures,
     validator],
@@ -65,7 +64,6 @@ func get_beacon_committee_len*(
     epochRef: EpochRef, slot: Slot, index: CommitteeIndex): uint64 =
   # Return the number of members in the beacon committee at ``slot`` for ``index``.
   let
-    epoch = compute_epoch_at_slot(slot)
     committees_per_slot = get_committee_count_per_slot(epochRef)
 
   compute_committee_len(
@@ -87,41 +85,41 @@ func is_aggregator*(epochRef: EpochRef, slot: Slot, index: CommitteeIndex,
 proc is_valid_indexed_attestation*(
     fork: Fork, genesis_validators_root: Eth2Digest,
     epochRef: EpochRef, indexed_attestation: SomeIndexedAttestation,
-    flags: UpdateFlags): bool =
+    flags: UpdateFlags): Result[void, cstring] =
   # Check if ``indexed_attestation`` is not empty, has sorted and unique
   # indices and has a valid aggregate signature.
 
   template is_sorted_and_unique(s: untyped): bool =
+    var res = true
     for i in 1 ..< s.len:
       if s[i - 1].uint64 >= s[i].uint64:
-        return false
+        res = false
+        break
+    res
 
-    true
+  if len(indexed_attestation.attesting_indices) == 0:
+    return err("indexed_attestation: no attesting indices")
 
   # Not from spec, but this function gets used in front-line roles, not just
   # behind firewall.
   let num_validators = epochRef.validator_keys.lenu64
   if anyIt(indexed_attestation.attesting_indices, it >= num_validators):
-    trace "indexed attestation: not all indices valid validators"
+    return err("indexed attestation: not all indices valid validators")
-    return false
 
-  # Verify indices are sorted and unique
+  if not is_sorted_and_unique(indexed_attestation.attesting_indices):
-  let indices = indexed_attestation.attesting_indices.asSeq
+    return err("indexed attestation: indices not sorted and unique")
-  if len(indices) == 0 or not is_sorted_and_unique(indices):
-    trace "indexed attestation: indices not sorted and unique"
-    return false
 
   # Verify aggregate signature
   if skipBLSValidation notin flags:
      # TODO: fuse loops with blsFastAggregateVerify
-    let pubkeys = mapIt(indices, epochRef.validator_keys[it])
+    let pubkeys = mapIt(
+      indexed_attestation.attesting_indices, epochRef.validator_keys[it])
     if not verify_attestation_signature(
         fork, genesis_validators_root, indexed_attestation.data,
         pubkeys, indexed_attestation.signature):
-      trace "indexed attestation: signature verification failure"
+      return err("indexed attestation: signature verification failure")
-      return false
 
-  true
+  ok()
 
 func makeAttestationData*(
     epochRef: EpochRef, bs: BlockSlot,

beacon_chain/spec/beaconstate.nim

@@ -15,7 +15,9 @@ import
   ../../nbench/bench_lab
 
 # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md#is_valid_merkle_branch
-func is_valid_merkle_branch*(leaf: Eth2Digest, branch: openarray[Eth2Digest], depth: int, index: uint64, root: Eth2Digest): bool {.nbench.}=
+func is_valid_merkle_branch*(leaf: Eth2Digest, branch: openarray[Eth2Digest],
+                             depth: int, index: uint64,
+                             root: Eth2Digest): bool {.nbench.}=
   ## Check if ``leaf`` at ``index`` verifies against the Merkle ``root`` and
   ## ``branch``.
   var
@@ -416,41 +418,41 @@ proc process_registry_updates*(state: var BeaconState,
 # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md#is_valid_indexed_attestation
 proc is_valid_indexed_attestation*(
     state: BeaconState, indexed_attestation: SomeIndexedAttestation,
-    flags: UpdateFlags): bool =
+    flags: UpdateFlags): Result[void, cstring] =
   # Check if ``indexed_attestation`` is not empty, has sorted and unique
   # indices and has a valid aggregate signature.
 
   template is_sorted_and_unique(s: untyped): bool =
+    var res = true
     for i in 1 ..< s.len:
       if s[i - 1].uint64 >= s[i].uint64:
-        return false
+        res = false
+        break
+    res
 
-    true
+  if len(indexed_attestation.attesting_indices) == 0:
+    return err("indexed_attestation: no attesting indices")
 
   # Not from spec, but this function gets used in front-line roles, not just
   # behind firewall.
   let num_validators = state.validators.lenu64
   if anyIt(indexed_attestation.attesting_indices, it >= num_validators):
-    trace "indexed attestation: not all indices valid validators"
+    return err("indexed attestation: not all indices valid validators")
-    return false
 
-  # Verify indices are sorted and unique
+  if not is_sorted_and_unique(indexed_attestation.attesting_indices):
-  let indices = indexed_attestation.attesting_indices.asSeq
+    return err("indexed attestation: indices not sorted and unique")
-  if len(indices) == 0 or not is_sorted_and_unique(indices):
-    trace "indexed attestation: indices not sorted and unique"
-    return false
 
   # Verify aggregate signature
   if skipBLSValidation notin flags:
      # TODO: fuse loops with blsFastAggregateVerify
-    let pubkeys = mapIt(indices, state.validators[it].pubkey)
+    let pubkeys = mapIt(
+      indexed_attestation.attesting_indices, state.validators[it].pubkey)
     if not verify_attestation_signature(
         state.fork, state.genesis_validators_root, indexed_attestation.data,
         pubkeys, indexed_attestation.signature):
-      trace "indexed attestation: signature verification failure"
+      return err("indexed attestation: signature verification failure")
-      return false
 
-  true
+  ok()
 
 # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md#get_attesting_indices
 func get_attesting_indices*(bits: CommitteeValidatorsBits,
@@ -582,9 +584,8 @@ proc check_attestation*(
     if not (data.source == state.previous_justified_checkpoint):
       return err("FFG data not matching previous justified epoch")
 
-  if not is_valid_indexed_attestation(
+  ? is_valid_indexed_attestation(
-      state, get_indexed_attestation(state, attestation, cache), flags):
+      state, get_indexed_attestation(state, attestation, cache), flags)
-    return err("signature or bitfields incorrect")
 
   ok()
 

beacon_chain/spec/state_transition_block.nim

@@ -207,10 +207,10 @@ proc process_attester_slashing*(
       attestation_1.data, attestation_2.data):
     return err("Attester slashing: surround or double vote check failed")
 
-  if not is_valid_indexed_attestation(state, attestation_1, flags):
+  if not is_valid_indexed_attestation(state, attestation_1, flags).isOk():
     return err("Attester slashing: invalid attestation 1")
 
-  if not is_valid_indexed_attestation(state, attestation_2, flags):
+  if not is_valid_indexed_attestation(state, attestation_2, flags).isOk():
     return err("Attester slashing: invalid attestation 2")
 
   var slashed_any = false

beacon_chain/spec/validator.nim

@@ -167,10 +167,7 @@ func compute_committee*(shuffled_indices: seq[ValidatorIndex],
 
   # In spec, this calls get_shuffled_index() every time, but that's wasteful
   # Here, get_beacon_committee() gets the shuffled version.
-  try:
+  shuffled_indices[start.int .. (endIdx.int-1)]
-    shuffled_indices[start.int .. (endIdx.int-1)]
-  except KeyError:
-    raiseAssert("Cached entries are added before use")
 
 func compute_committee_len*(active_validators: uint64,
     index: uint64, count: uint64): uint64 =