address some TODOs (#2005)

beacon_chain/spec/beaconstate.nim

@@ -90,7 +90,7 @@ proc process_deposit*(preset: RuntimePreset,
 
   let
     pubkey = deposit.data.pubkey
-    pubkey_inited = pubkey.initPubKey # TODO replicate previous PR semantics, check later
+    pubkey_inited = pubkey.initPubKey
     amount = deposit.data.amount
   var index = -1
 
@@ -445,7 +445,6 @@ proc is_valid_indexed_attestation*(
 
   # Verify aggregate signature
   if skipBLSValidation notin flags:
-     # TODO: fuse loops with blsFastAggregateVerify
     let pubkeys = mapIt(
       indexed_attestation.attesting_indices, state.validators[it].pubkey)
     if not verify_attestation_signature(

beacon_chain/spec/crypto.nim

@@ -151,15 +151,6 @@ proc blsVerify*(
     # TODO: chronicles warning
     return false
 
-  # TODO: remove fully if the comment below is not true anymore and
-  #       and we don't need this workaround
-  # # TODO bls_verify_multiple(...) used to have this workaround, and now it
-  # # lives here. No matter the signature, there's also no meaningful way to
-  # # verify it -- it's a kind of vacuous truth. No pubkey/sig pairs. Sans a
-  # # getBytes() or similar mechanism, pubKey == default(ValidatorPubKey) is
-  # # a way to create many false positive matches. This seems odd.
-  # if pubkey.getBytes() == default(ValidatorPubKey).getBytes():
-  #   return true
   realkey.get.blsValue.verify(message, signature.blsValue)
 
 func blsSign*(privkey: ValidatorPrivKey, message: openArray[byte]): ValidatorSig =

beacon_chain/spec/datatypes.nim

@@ -444,7 +444,6 @@ type
     message*: AggregateAndProof
     signature*: ValidatorSig
 
-  # TODO to be replaced with some magic hash caching
   HashedBeaconState* = object
     data*: BeaconState
     root*: Eth2Digest # hash_tree_root(data)

beacon_chain/spec/state_transition.nim

@@ -225,16 +225,6 @@ proc state_transition*(
 
   false
 
-proc state_transition*(
-    preset: RuntimePreset,
-    state: var HashedBeaconState, signedBlock: SomeSignedBeaconBlock,
-    flags: UpdateFlags, rollback: RollbackHashedProc): bool {.nbench.} =
-  # TODO consider moving this to testutils or similar, since non-testing
-  # and fuzzing code should always be coming from block pool which should
-  # always be providing cache or equivalent
-  var cache = StateCache()
-  state_transition(preset, state, signedBlock, cache, flags, rollback)
-
 # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0/specs/phase0/validator.md#preparing-for-a-beaconblock
 proc makeBeaconBlock*(
     preset: RuntimePreset,

beacon_chain/spec/state_transition_epoch.nim

@@ -285,9 +285,10 @@ proc process_justification_and_finalization*(state: var BeaconState,
   ## Spec:
   ## state.justification_bits[1:] = state.justification_bits[:-1]
   ## state.justification_bits[0] = 0b0
-  # TODO JUSTIFICATION_BITS_LENGTH is a constant in spec, move there or fix
-  # BitVector serialization in SSZ layer
+
+  # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0/specs/phase0/beacon-chain.md#constants
   const JUSTIFICATION_BITS_LENGTH = 4
+
   state.justification_bits = (state.justification_bits shl 1) and
     cast[uint8]((2^JUSTIFICATION_BITS_LENGTH) - 1)
 

tests/mocking/mock_attestations.nim

@@ -140,7 +140,7 @@ proc add*(state: var HashedBeaconState, attestation: Attestation, slot: Slot) =
   signMockBlock(state.data, signedBlock)
 
   let success = state_transition(
-    defaultRuntimePreset, state, signedBlock,
+    defaultRuntimePreset, state, signedBlock, cache,
     flags = {skipStateRootValidation}, noRollback)
 
   doAssert success

tests/official/test_fixture_sanity_blocks.nim

@@ -39,6 +39,7 @@ proc runTest(testName, testDir, unitTestName: string) =
         preState = newClone(parseTest(testPath/"pre.ssz", SSZ, BeaconState))
         hashedPreState = (ref HashedBeaconState)(
           data: preState[], root: hash_tree_root(preState[]))
+        cache = StateCache()
 
       # In test cases with more than 10 blocks the first 10 aren't 0-prefixed,
       # so purely lexicographic sorting wouldn't sort properly.
@@ -48,11 +49,13 @@ proc runTest(testName, testDir, unitTestName: string) =
 
         if hasPostState:
           let success = state_transition(
-            defaultRuntimePreset, hashedPreState[], blck, flags = {}, noRollback)
+            defaultRuntimePreset, hashedPreState[], blck, cache, flags = {},
+            noRollback)
           doAssert success, "Failure when applying block " & $i
         else:
           let success = state_transition(
-            defaultRuntimePreset, hashedPreState[], blck, flags = {}, noRollback)
+            defaultRuntimePreset, hashedPreState[], blck, cache, flags = {},
+            noRollback)
           doAssert (i + 1 < numBlocks) or not success,
             "We didn't expect these invalid blocks to be processed"
 

tests/test_state_transition.nim

@@ -41,7 +41,8 @@ suiteReport "Block processing" & preset():
       previous_block_root = genesisBlock.root
       new_block = makeTestBlock(state[], previous_block_root, cache)
 
-    let block_ok = state_transition(defaultRuntimePreset, state[], new_block, {}, noRollback)
+    let block_ok = state_transition(
+      defaultRuntimePreset, state[], new_block, cache, {}, noRollback)
 
     check:
       block_ok
@@ -61,7 +62,8 @@ suiteReport "Block processing" & preset():
     for i in 1..SLOTS_PER_EPOCH:
       let new_block = makeTestBlock(state[], previous_block_root, cache)
 
-      let block_ok = state_transition(defaultRuntimePreset, state[], new_block, {}, noRollback)
+      let block_ok = state_transition(
+        defaultRuntimePreset, state[], new_block, cache, {}, noRollback)
 
       check:
         block_ok
@@ -97,7 +99,8 @@ suiteReport "Block processing" & preset():
       new_block = makeTestBlock(state[], previous_block_root, cache,
         attestations = @[attestation]
       )
-    check state_transition(defaultRuntimePreset, state[], new_block, {}, noRollback)
+    check state_transition(
+      defaultRuntimePreset, state[], new_block, cache, {}, noRollback)
 
     check:
       # TODO epoch attestations can get multiplied now; clean up paths to