render get_beacon_proposer_index(...) return value an option and remove overly aggressive assertion formerly causing crashes when missing validators (#617)

* render get_beacon_proposer_index(...) return value an option and remove overly aggressive assertion formerly causing crashes when missing validators

* follow addBlock(...) after refactoring

* [skip ci] fix typo

beacon_chain/beacon_node.nim

@@ -645,9 +645,12 @@ proc handleProposal(node: BeaconNode, head: BlockRef, slot: Slot):
   #      revisit this - we should be able to advance behind
   var cache = get_empty_per_epoch_cache()
   node.blockPool.withState(node.stateCache, BlockSlot(blck: head, slot: slot)):
-    let
+    let proposerIdx = get_beacon_proposer_index(state, cache)
-      proposerIdx = get_beacon_proposer_index(state, cache)
+    if proposerIdx.isNone:
-      validator = node.getAttachedValidator(state, proposerIdx)
+      debug "Missing proposer index"
+      return head
+
+    let validator = node.getAttachedValidator(state, proposerIdx.get)
 
     if validator != nil:
       return await proposeBlock(node, validator, head, slot)
@@ -655,7 +658,7 @@ proc handleProposal(node: BeaconNode, head: BlockRef, slot: Slot):
     trace "Expecting block proposal",
       headRoot = shortLog(head.root),
       slot = shortLog(slot),
-      proposer = shortLog(state.validators[proposerIdx].pubKey),
+      proposer = shortLog(state.validators[proposerIdx.get].pubKey),
       cat = "consensus",
       pcs = "wait_for_proposal"
 

beacon_chain/spec/beaconstate.nim

@@ -6,7 +6,7 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  tables, algorithm, math, sequtils,
+  tables, algorithm, math, sequtils, options,
   json_serialization/std/sets, chronicles, stew/bitseqs,
   ../extras, ../ssz,
   ./crypto, ./datatypes, ./digest, ./helpers, ./validator
@@ -171,14 +171,19 @@ proc slash_validator*(state: var BeaconState, slashed_index: ValidatorIndex,
   decrease_balance(state, slashed_index,
     validator.effective_balance div MIN_SLASHING_PENALTY_QUOTIENT)
 
+  # The rest doesn't make sense without there being any proposer index, so skip
+  let proposer_index = get_beacon_proposer_index(state, stateCache)
+  if proposer_index.isNone:
+    debug "No beacon proposer index and probably no active validators"
+    return
+
   let
-    proposer_index = get_beacon_proposer_index(state, stateCache)
     # Spec has whistleblower_index as optional param, but it's never used.
-    whistleblower_index = proposer_index
+    whistleblower_index = proposer_index.get
     whistleblowing_reward =
       (validator.effective_balance div WHISTLEBLOWER_REWARD_QUOTIENT).Gwei
     proposer_reward = whistleblowing_reward div PROPOSER_REWARD_QUOTIENT
-  increase_balance(state, proposer_index, proposer_reward)
+  increase_balance(state, proposer_index.get, proposer_reward)
   # TODO: evaluate if spec bug / underflow can be triggered
   doAssert(whistleblowing_reward >= proposer_reward, "Spec bug: underflow in slash_validator")
   increase_balance(
@@ -492,6 +497,12 @@ proc process_attestation*(
   # reused when looking for suitable blocks to include in attestations.
   # TODO don't log warnings when looking for attestations (return
   #      Result[void, cstring] instead of logging in check_attestation?)
+
+  let proposer_index = get_beacon_proposer_index(state, stateCache)
+  if proposer_index.isNone:
+    debug "No beacon proposer index and probably no active validators"
+    return false
+
   if check_attestation(state, attestation, flags, stateCache):
     let
       attestation_slot = attestation.data.slot
@@ -499,7 +510,7 @@ proc process_attestation*(
         data: attestation.data,
         aggregation_bits: attestation.aggregation_bits,
         inclusion_delay: state.slot - attestation_slot,
-        proposer_index: get_beacon_proposer_index(state, stateCache).uint64,
+        proposer_index: proposer_index.get.uint64,
       )
 
     if attestation.data.target.epoch == get_current_epoch(state):

beacon_chain/spec/state_transition_block.nim

@@ -32,8 +32,8 @@
 # improvements to be made - other than that, keep things similar to spec for
 # now.
 
-import # TODO - cleanup imports
+import
-  algorithm, collections/sets, chronicles, sequtils, sets, tables,
+  algorithm, collections/sets, chronicles, options, sequtils, sets, tables,
   ../extras, ../ssz, metrics,
   beaconstate, crypto, datatypes, digest, helpers, validator
 
@@ -76,10 +76,13 @@ proc process_block_header*(
     signature: BlsValue[Signature](kind: OpaqueBlob)
   )
 
-
   # Verify proposer is not slashed
-  let proposer =
+  let proposer_index = get_beacon_proposer_index(state, stateCache)
-    state.validators[get_beacon_proposer_index(state, stateCache)]
+  if proposer_index.isNone:
+    debug "Block header: proposer missing"
+    return false
+
+  let proposer = state.validators[proposer_index.get]
   if proposer.slashed:
     notice "Block header: proposer slashed"
     return false
@@ -105,7 +108,12 @@ proc process_randao(
   let
     epoch = state.get_current_epoch()
     proposer_index = get_beacon_proposer_index(state, stateCache)
-    proposer = addr state.validators[proposer_index]
+
+  if proposer_index.isNone:
+    debug "Proposer index missing, probably along with any active validators"
+    return false
+
+  let proposer = addr state.validators[proposer_index.get]
 
   # Verify that the provided randao value is valid
   if skipValidation notin flags:

beacon_chain/spec/validator.nim

@@ -148,11 +148,12 @@ func get_empty_per_epoch_cache*(): StateCache =
 
 # https://github.com/ethereum/eth2.0-specs/blob/v0.9.2/specs/core/0_beacon-chain.md#compute_proposer_index
 func compute_proposer_index(state: BeaconState, indices: seq[ValidatorIndex],
-    seed: Eth2Digest, stateCache: var StateCache): ValidatorIndex =
+    seed: Eth2Digest, stateCache: var StateCache): Option[ValidatorIndex] =
   # Return from ``indices`` a random index sampled by effective balance.
   const MAX_RANDOM_BYTE = 255
 
-  doAssert len(indices) > 0
+  if len(indices) == 0:
+    return none(ValidatorIndex)
 
   # TODO fixme; should only be run once per slot and cached
   # There's exactly one beacon proposer per slot.
@@ -175,12 +176,12 @@ func compute_proposer_index(state: BeaconState, indices: seq[ValidatorIndex],
         state.validators[candidate_index].effective_balance
     if effective_balance * MAX_RANDOM_BYTE >=
         MAX_EFFECTIVE_BALANCE * random_byte:
-      return candidate_index
+      return some(candidate_index)
     i += 1
 
 # https://github.com/ethereum/eth2.0-specs/blob/v0.9.2/specs/core/0_beacon-chain.md#get_beacon_proposer_index
 func get_beacon_proposer_index*(state: BeaconState, stateCache: var StateCache):
-    ValidatorIndex =
+    Option[ValidatorIndex] =
   # Return the beacon proposer index at the current slot.
   let epoch = get_current_epoch(state)
 

tests/mocking/mock_blocks.nim

@@ -6,6 +6,7 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
+  options,
   # Specs
   ../../beacon_chain/spec/[datatypes, crypto, helpers, validator],
   # Internals
@@ -60,10 +61,10 @@ proc signMockBlock*(
   blck: var BeaconBlock
   ) =
 
-  var proposer_index: ValidatorIndex
   var emptyCache = get_empty_per_epoch_cache()
+  let proposer_index =
     if blck.slot == state.slot:
-    proposer_index = get_beacon_proposer_index(state, emptyCache)
+      get_beacon_proposer_index(state, emptyCache)
     else:
       # Stub to get proposer index of future slot
       # Note: this relies on ``let`` deep-copying the state
@@ -71,9 +72,10 @@ proc signMockBlock*(
       #            and not contain ref objects or pointers
       var stubState = state
       process_slots(stub_state, blck.slot)
-    proposer_index = get_beacon_proposer_index(stub_state, emptyCache)
+      get_beacon_proposer_index(stub_state, emptyCache)
 
-  signMockBlockImpl(state, blck, proposer_index)
+  # In tests, just let this throw if appropriate
+  signMockBlockImpl(state, blck, proposer_index.get)
 
 proc mockBlock*(
     state: BeaconState,

tests/testblockutil.nim

@@ -6,7 +6,7 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  stew/endians2,
+  options, stew/endians2,
   chronicles, eth/trie/[db],
   ../beacon_chain/[beacon_chain_db, block_pool, extras, ssz, state_transition,
     validator_pool],
@@ -87,7 +87,8 @@ proc addBlock*(
 
   let
     # Index from the new state, but registry from the old state.. hmm...
-    proposer = state.validators[proposer_index]
+    # In tests, let this throw
+    proposer = state.validators[proposer_index.get]
     privKey = hackPrivKey(proposer)
 
   # TODO ugly hack; API needs rethinking