state processing fixes (#177)

* remove some redundant state updates
* when attesting late, use correct state / head
* don't send out obsolete attestations
* don't propose obsolete blocks
* remove some more resundant state updates :)
* simplify block logging (experimental)
* document fork choice division
* fix some Slot / Epoch conversion warnings

beacon_chain/beacon_node.nim

@@ -71,7 +71,6 @@ proc init*(T: type BeaconNode, conf: BeaconNodeConf): Future[BeaconNode] {.async
   let head = result.blockPool.get(result.db.getHeadBlock().get())
 
   result.state = result.blockPool.loadTailState()
-  result.blockPool.updateState(result.state, head.get().refs)
 
   let addressFile = string(conf.dataDir) / "beacon_node.address"
   result.network.saveConnectionAddressFile(addressFile)
@@ -172,7 +171,10 @@ proc updateHead(node: BeaconNode): BlockRef =
   let
     justifiedHead = node.blockPool.latestJustifiedBlock()
 
-  node.blockPool.updateState(node.state, justifiedHead)
+  # TODO slot number is wrong here, it should be the start of the epoch that
+  #      got finalized:
+  #      https://github.com/ethereum/eth2.0-specs/issues/768
+  node.blockPool.updateState(node.state, justifiedHead, justifiedHead.slot)
 
   let newHead = lmdGhost(node.attestationPool, node.state.data, justifiedHead)
   node.blockPool.updateHead(node.state, newHead)
@@ -192,24 +194,42 @@ proc makeAttestation(node: BeaconNode,
   # TODO this lazy update of the head is good because it delays head resolution
   #      until the very latest moment - on the other hand, if it takes long, the
   #      attestation might be late!
-  let head = node.updateHead()
+  let
+    head = node.updateHead()
 
-  node.blockPool.updateState(node.state, head)
+  if slot + MIN_ATTESTATION_INCLUSION_DELAY < head.slot:
+    # What happened here is that we're being really slow or there's something
+    # really fishy going on with the slot - let's not send out any attestations
+    # just in case...
+    # TODO is this the right cutoff?
+    notice "Skipping attestation, head is too recent",
+      headSlot = humaneSlotNum(head.slot),
+      slot = humaneSlotNum(slot)
+    return
+
+  let attestationHead = head.findAncestorBySlot(slot)
+  if head != attestationHead:
+    # In rare cases, such as when we're busy syncing or just slow, we'll be
+    # attesting to a past state - we must then recreate the world as it looked
+    # like back then
+    notice "Attesting to a state in the past, falling behind?",
+      headSlot = humaneSlotNum(head.slot),
+      attestationHeadSlot = humaneSlotNum(attestationHead.slot),
+      attestationSlot = humaneSlotNum(slot)
+
+  # We need to run attestations exactly for the slot that we're attesting to.
+  # In case blocks went missing, this means advancing past the latest block
+  # using empty slots as fillers.
+  node.blockPool.updateState(node.state, attestationHead, slot)
 
   # Check pending attestations - maybe we found some blocks for them
   node.attestationPool.resolve(node.state.data)
 
-  # It might be that the latest block we found is an old one - if this is the
-  # case, we need to fast-forward the state
-  skipSlots(node.state.data, node.state.blck.root, slot)
-
-  # If we call makeAttestation too late, we must advance head only to `slot`
-  doAssert node.state.data.slot == slot,
-    "Corner case: head advanced beyond sheduled attestation slot"
-
   let
     attestationData =
       makeAttestationData(node.state.data, shard, node.state.blck.root)
+
+    # Careful - after await. node.state (etc) might have changed in async race
     validatorSignature = await validator.signAttestation(attestationData)
 
   var aggregationBitfield = repeat(0'u8, ceil_div8(committeeLen))
@@ -245,12 +265,30 @@ proc proposeBlock(node: BeaconNode,
   # we'll be building the next block upon..
   let head = node.updateHead()
 
-  node.blockPool.updateState(node.state, head)
+  if head.slot > slot:
+    notice "Skipping proposal, we've already selected a newer head",
+      headSlot = humaneSlotNum(head.slot),
+      headBlockRoot = shortLog(head.root),
+      slot = humaneSlotNum(slot)
+
+  if head.slot == slot:
+    # Weird, we should never see as head the same slot as we're proposing a
+    # block for - did someone else steal our slot? why didn't we discard it?
+    warn "Found head at same slot as we're supposed to propose for!",
+      headSlot = humaneSlotNum(head.slot),
+      headBlockRoot = shortLog(head.root)
+    # TODO investigate how and when this happens.. maybe it shouldn't be an
+    #      assert?
+    doAssert false, "head slot matches proposal slot (!)"
+    # return
+
+  # There might be gaps between our proposal and what we think is the head -
+  # make sure the state we get takes that into account: we want it to point
+  # to the slot just before our proposal.
+  node.blockPool.updateState(node.state, head, slot - 1)
 
   # To create a block, we'll first apply a partial block to the state, skipping
   # some validations.
-  skipSlots(node.state.data, node.state.blck.root, slot - 1)
-
   var blockBody = BeaconBlockBody(
     attestations: node.attestationPool.getAttestationsForBlock(slot))
 
@@ -356,16 +394,12 @@ proc scheduleEpochActions(node: BeaconNode, epoch: Epoch) =
   doAssert epoch >= GENESIS_EPOCH,
     "Epoch: " & $epoch & ", humane epoch: " & $humaneEpochNum(epoch)
 
-  debug "Scheduling epoch actions",
+  # In case some late blocks dropped in..
-    epoch = humaneEpochNum(epoch),
-    stateEpoch = humaneEpochNum(node.state.data.slot.slot_to_epoch())
-
-  # In case some late blocks dropped in
   let head = node.updateHead()
-  node.blockPool.updateState(node.state, head)
 
   # Sanity check - verify that the current head block is not too far behind
-  if node.state.data.slot.slot_to_epoch() + 1 < epoch:
+  # TODO what if the head block is too far ahead? that would be.. weird.
+  if head.slot.slot_to_epoch() + 1 < epoch:
     # We're hopelessly behind!
     #
     # There's a few ways this can happen:
@@ -387,7 +421,7 @@ proc scheduleEpochActions(node: BeaconNode, epoch: Epoch) =
       at = node.slotStart(nextSlot)
 
     notice "Delaying epoch scheduling, head too old - scheduling new attempt",
-      stateSlot = humaneSlotNum(node.state.data.slot),
+      headSlot = humaneSlotNum(head.slot),
       expectedEpoch = humaneEpochNum(epoch),
       expectedSlot = humaneSlotNum(expectedSlot),
       fromNow = (at - fastEpochTime()) div 1000
@@ -396,12 +430,13 @@ proc scheduleEpochActions(node: BeaconNode, epoch: Epoch) =
       node.scheduleEpochActions(nextSlot.slot_to_epoch())
     return
 
+
+  updateState(node.blockPool, node.state, head, epoch.get_epoch_start_slot())
+
   # TODO: is this necessary with the new shuffling?
   #       see get_beacon_proposer_index
   var nextState = node.state.data
 
-  skipSlots(nextState, node.state.blck.root, epoch.get_epoch_start_slot())
-
   # TODO we don't need to do anything at slot 0 - what about slots we missed
   #      if we got delayed above?
   let start = if epoch == GENESIS_EPOCH: 1.uint64 else: 0.uint64
@@ -431,6 +466,10 @@ proc scheduleEpochActions(node: BeaconNode, epoch: Epoch) =
               crosslink_committee.committee.len, i)
 
   let
+    # TODO we need to readjust here for wall clock time, in case computer
+    #      goes to sleep for example, so that we don't walk epochs one by one
+    #      to catch up.. we should also check the current head most likely to
+    #      see if we're suspiciously off, in terms of wall clock vs head time.
     nextEpoch = epoch + 1
     at = node.slotStart(nextEpoch.get_epoch_start_slot())
 

beacon_chain/block_pool.nim

@@ -21,7 +21,8 @@ proc init*(T: type BlockRef, root: Eth2Digest, slot: Slot): BlockRef =
 proc init*(T: type BlockRef, root: Eth2Digest, blck: BeaconBlock): BlockRef =
   BlockRef.init(root, blck.slot)
 
-proc findAncestorBySlot(blck: BlockRef, slot: Slot): BlockRef =
+proc findAncestorBySlot*(blck: BlockRef, slot: Slot): BlockRef =
+  ## Find the first ancestor that has a slot number less than or equal to `slot`
   result = blck
 
   while result != nil and result.slot > slot:
@@ -128,7 +129,7 @@ proc addSlotMapping(pool: BlockPool, slot: uint64, br: BlockRef) =
   pool.blocksBySlot.mgetOrPut(slot, @[]).addIfMissing(br)
 
 proc updateState*(
-  pool: BlockPool, state: var StateData, blck: BlockRef) {.gcsafe.}
+  pool: BlockPool, state: var StateData, blck: BlockRef, slot: Slot) {.gcsafe.}
 
 proc add*(
     pool: var BlockPool, state: var StateData, blockRoot: Eth2Digest,
@@ -144,9 +145,7 @@ proc add*(
   # Already seen this block??
   if blockRoot in pool.blocks:
     debug "Block already exists",
-      slot = humaneSlotNum(blck.slot),
+      blck = shortLog(blck),
-      stateRoot = shortLog(blck.state_root),
-      parentRoot = shortLog(blck.parent_root),
       blockRoot = shortLog(blockRoot)
 
     return true
@@ -157,10 +156,8 @@ proc add*(
   # by the time it gets here.
   if blck.slot <= pool.finalizedHead.slot:
     debug "Old block, dropping",
-      slot = humaneSlotNum(blck.slot),
+      blck = shortLog(blck),
       tailSlot = humaneSlotNum(pool.tail.slot),
-      stateRoot = shortLog(blck.state_root),
-      parentRoot = shortLog(blck.parent_root),
       blockRoot = shortLog(blockRoot)
 
     return true
@@ -175,23 +172,13 @@ proc add*(
 
     # The block is resolved, now it's time to validate it to ensure that the
     # blocks we add to the database are clean for the given state
-    updateState(pool, state, parent)
+    updateState(pool, state, parent, blck.slot - 1)
-    skipSlots(state.data, parent.root, blck.slot - 1)
 
     if not updateState(state.data, parent.root, blck, {}):
       # TODO find a better way to log all this block data
       notice "Invalid block",
-        blockRoot = shortLog(blockRoot),
+        blck = shortLog(blck),
-        slot = humaneSlotNum(blck.slot),
+        blockRoot = shortLog(blockRoot)
-        stateRoot = shortLog(blck.state_root),
-        parentRoot = shortLog(blck.parent_root),
-        signature = shortLog(blck.signature),
-        proposer_slashings = blck.body.proposer_slashings.len,
-        attester_slashings = blck.body.attester_slashings.len,
-        attestations = blck.body.attestations.len,
-        deposits = blck.body.deposits.len,
-        voluntary_exits = blck.body.voluntary_exits.len,
-        transfers = blck.body.transfers.len
 
       return
 
@@ -222,17 +209,8 @@ proc add*(
       justifiedBlock.justified = true
 
     info "Block resolved",
-      blockRoot = shortLog(blockRoot),
+      blck = shortLog(blck),
-      slot = humaneSlotNum(blck.slot),
+      blockRoot = shortLog(blockRoot)
-      stateRoot = shortLog(blck.state_root),
-      parentRoot = shortLog(blck.parent_root),
-      signature = shortLog(blck.signature),
-      proposer_slashings = blck.body.proposer_slashings.len,
-      attester_slashings = blck.body.attester_slashings.len,
-      attestations = blck.body.attestations.len,
-      deposits = blck.body.deposits.len,
-      voluntary_exits = blck.body.voluntary_exits.len,
-      transfers = blck.body.transfers.len
 
     # Now that we have the new block, we should see if any of the previously
     # unresolved blocks magically become resolved
@@ -263,9 +241,7 @@ proc add*(
   #      a risk of being slashed, making attestations a more valuable spam
   #      filter.
   debug "Unresolved block",
-    slot = humaneSlotNum(blck.slot),
+    blck = shortLog(blck),
-    stateRoot = shortLog(blck.state_root),
-    parentRoot = shortLog(blck.parent_root),
     blockRoot = shortLog(blockRoot)
 
   pool.unresolved[blck.parent_root] = UnresolvedBlock()
@@ -345,20 +321,24 @@ proc maybePutState(pool: BlockPool, state: BeaconState) =
     pool.db.putState(state)
 
 proc updateState*(
-    pool: BlockPool, state: var StateData, blck: BlockRef) =
+    pool: BlockPool, state: var StateData, blck: BlockRef, slot: Slot) =
-  # Rewind or advance state such that it matches the given block - this may
+  ## Rewind or advance state such that it matches the given block and slot -
-  # include replaying from an earlier snapshot if blck is on a different branch
+  ## this may include replaying from an earlier snapshot if blck is on a
-  # or has advanced to a higher slot number than blck
+  ## different branch or has advanced to a higher slot number than slot
-  var ancestors = @[pool.get(blck)]
+  ## If slot is higher than blck.slot, replay will fill in with empty/non-block
+  ## slots, else it is ignored
 
   # We need to check the slot because the state might have moved forwards
   # without blocks
-  if state.blck.root == blck.root and state.data.slot == ancestors[0].data.slot:
+  if state.blck.root == blck.root and state.data.slot == slot:
     return # State already at the right spot
 
-  # Common case: blck points to a block that is one step ahead of state
+  var ancestors = @[pool.get(blck)]
+
+  # Common case: the last thing that was applied to the state was the parent
+  # of blck
   if state.blck.root == ancestors[0].data.parent_root and
-      state.data.slot + 1 == ancestors[0].data.slot:
+      state.data.slot < blck.slot:
     let ok = skipAndUpdateState(
         state.data, ancestors[0].data, {skipValidation}) do (state: BeaconState):
       pool.maybePutState(state)
@@ -366,6 +346,9 @@ proc updateState*(
     state.blck = blck
     state.root = ancestors[0].data.state_root
 
+    skipSlots(state.data, state.blck.root, slot) do (state: BeaconState):
+      pool.maybePutState(state)
+
     return
 
   # It appears that the parent root of the proposed new block is different from
@@ -426,6 +409,9 @@ proc updateState*(
 
   pool.maybePutState(state.data)
 
+  skipSlots(state.data, state.blck.root, slot) do (state: BeaconState):
+    pool.maybePutState(state)
+
 proc loadTailState*(pool: BlockPool): StateData =
   ## Load the state associated with the current tail in the pool
   let stateRoot = pool.db.getBlock(pool.tail.root).get().state_root
@@ -451,12 +437,14 @@ proc updateHead*(pool: BlockPool, state: var StateData, blck: BlockRef) =
   pool.head = blck
 
   # Start off by making sure we have the right state
-  updateState(pool, state, blck)
+  updateState(pool, state, blck, blck.slot)
 
   info "Updated head",
     stateRoot = shortLog(state.root),
     headBlockRoot = shortLog(state.blck.root),
-    stateSlot = humaneSlotNum(state.data.slot)
+    stateSlot = humaneSlotNum(state.data.slot),
+    justifiedEpoch = humaneEpochNum(state.data.justified_epoch),
+    finalizedEpoch = humaneEpochNum(state.data.finalized_epoch)
 
   let
     # TODO there might not be a block at the epoch boundary - what then?

beacon_chain/fork_choice.nim

@@ -37,6 +37,8 @@ proc lmdGhost*(
     var res: uint64
     for validator_index, target in attestation_targets.items():
       if get_ancestor(target, blck.slot) == blck:
+        # The div on the balance is to chop off the insignification bits that
+        # fluctuate a lot epoch to epoch to have a more stable fork choice
         res += get_effective_balance(start_state, validator_index) div
             FORK_CHOICE_BALANCE_INCREMENT
     res

beacon_chain/spec/beaconstate.nim

@@ -280,7 +280,7 @@ func get_attestation_participants*(state: BeaconState,
   ## Return the participant indices at for the ``attestation_data`` and
   ## ``bitfield``.
   let crosslink_committees = get_crosslink_committees_at_slot(
-    state, attestation_data.slot.Slot)
+    state, attestation_data.slot)
   doAssert anyIt(
     crosslink_committees,
     it[1] == attestation_data.shard)
@@ -367,7 +367,7 @@ proc checkAttestation*(
   ## at the current slot. When acting as a proposer, the same rules need to
   ## be followed!
 
-  let attestation_data_slot = attestation.data.slot.Slot
+  let attestation_data_slot = attestation.data.slot
 
   if not (attestation.data.slot >= GENESIS_SLOT):
     warn("Attestation predates genesis slot",
@@ -388,7 +388,7 @@ proc checkAttestation*(
     return
 
   let expected_justified_epoch =
-    if slot_to_epoch(attestation.data.slot.Slot + 1) >= get_current_epoch(state):
+    if slot_to_epoch(attestation.data.slot + 1) >= get_current_epoch(state):
       state.justified_epoch
     else:
       state.previous_justified_epoch
@@ -477,7 +477,7 @@ proc checkAttestation*(
           data: attestation.data, custody_bit: true)),
       ],
       attestation.aggregate_signature,
-      get_domain(state.fork, slot_to_epoch(attestation.data.slot.Slot),
+      get_domain(state.fork, slot_to_epoch(attestation.data.slot),
                  DOMAIN_ATTESTATION),
     )
 

beacon_chain/spec/datatypes.nim

@@ -572,10 +572,27 @@ ethTimeUnit Slot
 ethTimeUnit Epoch
 
 func humaneSlotNum*(s: Slot): uint64 =
-  s.Slot - GENESIS_SLOT
+  s - GENESIS_SLOT
 
 func humaneEpochNum*(e: Epoch): uint64 =
-  e.Epoch - GENESIS_EPOCH
+  e - GENESIS_EPOCH
+
+func shortLog*(v: BeaconBlock): tuple[
+    slot: uint64, parent_root: string, state_root: string,
+    randao_reveal: string, #[ eth1_data ]#
+    proposer_slashings_len: int, attester_slashings_len: int,
+    attestations_len: int,
+    deposits_len: int,
+    voluntary_exits_len: int,
+    transfers_len: int,
+    signature: string
+  ] = (
+    humaneSlotNum(v.slot), shortLog(v.parent_root), shortLog(v.state_root),
+    shortLog(v.randao_reveal), v.body.proposer_slashings.len(),
+    v.body.attester_slashings.len(), v.body.attestations.len(),
+    v.body.deposits.len(), v.body.voluntary_exits.len(), v.body.transfers.len(),
+    shortLog(v.signature)
+  )
 
 import nimcrypto, json_serialization
 export json_serialization

beacon_chain/spec/helpers.nim

@@ -135,8 +135,8 @@ func is_double_vote*(attestation_data_1: AttestationData,
   ## target.
   let
     # RLP artifact
-    target_epoch_1 = slot_to_epoch(attestation_data_1.slot.Slot)
+    target_epoch_1 = slot_to_epoch(attestation_data_1.slot)
-    target_epoch_2 = slot_to_epoch(attestation_data_2.slot.Slot)
+    target_epoch_2 = slot_to_epoch(attestation_data_2.slot)
   target_epoch_1 == target_epoch_2
 
 # https://github.com/ethereum/eth2.0-specs/blob/0.4.0/specs/core/0_beacon-chain.md#is_surround_vote
@@ -147,8 +147,8 @@ func is_surround_vote*(attestation_data_1: AttestationData,
     source_epoch_1 = attestation_data_1.justified_epoch
     source_epoch_2 = attestation_data_2.justified_epoch
     # RLP artifact
-    target_epoch_1 = slot_to_epoch(attestation_data_1.slot.Slot)
+    target_epoch_1 = slot_to_epoch(attestation_data_1.slot)
-    target_epoch_2 = slot_to_epoch(attestation_data_2.slot.Slot)
+    target_epoch_2 = slot_to_epoch(attestation_data_2.slot)
 
   source_epoch_1 < source_epoch_2 and target_epoch_2 < target_epoch_1
 

beacon_chain/state_transition.nim

@@ -547,7 +547,7 @@ func processEpoch(state: var BeaconState) =
   let
     current_epoch = get_current_epoch(state)
     previous_epoch = get_previous_epoch(state)
-    next_epoch = (current_epoch + 1).Epoch
+    next_epoch = (current_epoch + 1)
 
     # Spec grabs this later, but it's part of current_total_balance
     active_validator_indices =
@@ -611,7 +611,7 @@ func processEpoch(state: var BeaconState) =
   let
     previous_epoch_head_attestations =
       previous_epoch_attestations.filterIt(
-        it.data.beacon_block_root == get_block_root(state, it.data.slot.Slot))
+        it.data.beacon_block_root == get_block_root(state, it.data.slot))
 
     previous_epoch_head_attester_indices =
       toSet(get_attester_indices(state, previous_epoch_head_attestations))
@@ -1033,7 +1033,7 @@ proc advanceState*(
 proc skipSlots*(state: var BeaconState, parentRoot: Eth2Digest, slot: Slot,
     afterSlot: proc (state: BeaconState) = nil) =
   if state.slot < slot:
-    debug "Advancing state past slot gap",
+    debug "Advancing state with empty slots",
       targetSlot = humaneSlotNum(slot),
       stateSlot = humaneSlotNum(state.slot)