More state_transition unification (#953)

* remove incorrect/obsolete comment; deprecate BeaconState state transition functions

* remove deprecated state_transition(state: var BeaconState)

* add specific workarounds for state_transition() and process_slots() to nfuzz_block() and addTestBlock()

beacon_chain/state_transition.nim

@@ -157,7 +157,7 @@ proc process_slots*(state: var HashedBeaconState, slot: Slot) {.nbench.} =
     state.root = hash_tree_root(state.data)
 
 # TODO remove this once callers gone
-proc process_slots*(state: var BeaconState, slot: Slot) =
+proc process_slots*(state: var BeaconState, slot: Slot) {.deprecated: "Use HashedBeaconState version".} =
   var hashedState = HashedBeaconState(data: state, root: hash_tree_root(state))
   process_slots(hashedState, slot)
   state = hashedState.data
@@ -167,15 +167,12 @@ proc noRollback*(state: var HashedBeaconState) =
 
 proc state_transition*(
     state: var HashedBeaconState, signedBlock: SignedBeaconBlock,
-    flags: UpdateFlags, rollback: RollbackHashedProc): bool =
+    flags: UpdateFlags, rollback: RollbackHashedProc): bool {.nbench.} =
   ## Time in the beacon chain moves by slots. Every time (haha.) that happens,
   ## we will update the beacon state. Normally, the state updates will be driven
   ## by the contents of a new block, but it may happen that the block goes
   ## missing - the state updates happen regardless.
   ##
-  ## Each call to this function will advance the state by one slot - new_block,
-  ## must match that slot. If the update fails, the state will remain unchanged.
-  ##
   ## The flags are used to specify that certain validations should be skipped
   ## for the new block. This is done during block proposal, to create a state
   ## whose hash can be included in the new block.
@@ -232,11 +229,3 @@ proc state_transition*(
   rollback(state)
 
   false
-
-# TODO remove this once callers gone
-proc state_transition*(
-    state: var BeaconState, signedBlock: SignedBeaconBlock, flags: UpdateFlags,
-    rollback: RollbackHashedProc): bool {.nbench.} =
-  var hashedState = HashedBeaconState(data: state, root: hash_tree_root(state))
-  result = state_transition(hashedState, signedBlock, flags, rollback)
-  state = hashedState.data

nfuzz/libnfuzz.nim

@@ -106,8 +106,19 @@ proc nfuzz_attester_slashing(input: openArray[byte], output: ptr byte,
 
 proc nfuzz_block(input: openArray[byte], output: ptr byte,
     output_size: ptr uint, disable_bls: bool): bool {.exportc, raises: [FuzzCrashError, Defect].} =
+  # There's not a perfect approach here, but it's not worth switching the rest
+  # and requiring HashedBeaconState (yet). So to keep consistent, puts wrapper
+  # only in one function.
+  proc state_transition(
+      data: auto, blck: auto, flags: auto, rollback: RollbackHashedProc):
+      auto =
+    var hashedState =
+      HashedBeaconState(data: data.state, root: hash_tree_root(data.state))
+    result = state_transition(hashedState, blck, flags, rollback)
+    data.state = hashedState.data
+
   decodeAndProcess(BlockInput):
-    state_transition(data.state, data.beaconBlock, flags, noRollback)
+    state_transition(data, data.beaconBlock, flags, noRollback)
 
 proc nfuzz_block_header(input: openArray[byte], output: ptr byte,
     output_size: ptr uint, disable_bls: bool): bool {.exportc, raises: [FuzzCrashError, Defect].} =

tests/official/test_fixture_sanity_blocks.nim

@@ -34,8 +34,11 @@ proc runTest(identifier: string) =
       "[Invalid] "
 
     timedTest prefix & identifier:
-      var preState = newClone(parseTest(testDir/"pre.ssz", SSZ, BeaconState))
+      var
-      var hasPostState = existsFile(testDir/"post.ssz")
+        preState = newClone(parseTest(testDir/"pre.ssz", SSZ, BeaconState))
+        hasPostState = existsFile(testDir/"post.ssz")
+        hashedPreState = HashedBeaconState(
+          data: preState[], root: hash_tree_root(preState[]))
 
       # In test cases with more than 10 blocks the first 10 aren't 0-prefixed,
       # so purely lexicographic sorting wouldn't sort properly.
@@ -45,19 +48,18 @@ proc runTest(identifier: string) =
         if hasPostState:
           # TODO: The EF is using invalid BLS keys so we can't verify them
           let success = state_transition(
-            preState[], blck, flags = {skipBlsValidation}, noRollback)
+            hashedPreState, blck, flags = {skipBlsValidation}, noRollback)
           doAssert success, "Failure when applying block " & $i
         else:
           let success = state_transition(
-            preState[], blck, flags = {}, noRollback)
+            hashedPreState, blck, flags = {}, noRollback)
           doAssert not success, "We didn't expect this invalid block to be processed"
 
-      # check: preState.hash_tree_root() == postState.hash_tree_root()
       if hasPostState:
         let postState = newClone(parseTest(testDir/"post.ssz", SSZ, BeaconState))
         when false:
-          reportDiff(preState, postState)
+          reportDiff(hashedPreState.data, postState)
-        doAssert preState[].hash_tree_root() == postState[].hash_tree_root()
+        doAssert hashedPreState.root == postState[].hash_tree_root()
 
   `testImpl _ blck _ identifier`()
 

tests/official/test_fixture_sanity_slots.nim

@@ -31,13 +31,17 @@ proc runTest(identifier: string) =
 
   proc `testImpl _ slots _ identifier`() =
     timedTest "Slots - " & identifier:
-      var preState = newClone(parseTest(testDir/"pre.ssz", SSZ, BeaconState))
+      var
+        preState = newClone(parseTest(testDir/"pre.ssz", SSZ, BeaconState))
+        hashedPreState = HashedBeaconState(
+          data: preState[], root: hash_tree_root(preState[]))
       let postState = newClone(parseTest(testDir/"post.ssz", SSZ, BeaconState))
 
-      process_slots(preState[], preState.slot + num_slots)
+      process_slots(hashedPreState, hashedPreState.data.slot + num_slots)
 
-      check: preState[].hash_tree_root() == postState[].hash_tree_root()
+      check: hashedPreState.root == postState[].hash_tree_root()
-      reportDiff(preState, postState)
+      let newPreState = newClone(hashedPreState.data)
+      reportDiff(newPreState, postState)
 
   `testImpl _ slots _ identifier`()
 

tests/testblockutil.nim

@@ -95,7 +95,10 @@ proc addTestBlock*(
     flags: set[UpdateFlag] = {}): SignedBeaconBlock =
   # Create and add a block to state - state will advance by one slot!
 
-  process_slots(state, state.slot + 1)
+  # TODO workaround, disable when this works directly
+  var hashedState = HashedBeaconState(data: state, root: hash_tree_root(state))
+  process_slots(hashedState, hashedState.data.slot + 1)
+  state = hashedState.data
 
   var cache = get_empty_per_epoch_cache()
   let proposer_index = get_beacon_proposer_index(state, cache)

tests/testutil.nim

@@ -8,7 +8,7 @@
 import
   algorithm, strformat, stats, times, tables, std/monotimes, stew/endians2,
   testutils/markdown_reports, chronicles,
-  ../beacon_chain/[beacon_chain_db, block_pool, extras, ssz,  beacon_node_types],
+  ../beacon_chain/[beacon_chain_db, block_pool, extras, ssz, beacon_node_types],
   ../beacon_chain/spec/[digest, beaconstate, datatypes],
   eth/db/kvstore,
   testblockutil