From 32547373e528e8ba584f349f99f7523e8d14a175 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 8 Apr 2019 14:37:01 +1000 Subject: [PATCH] spec: simplify `cache_state` The `latest_block_root` input argument was unnecessary as we were always setting it to something almost equivalent to `state.latest_block_root` anyway, and more importantly, it was messing up the caching of the state root. Previously it was possible for the function to update the state's latest block root, and then hash the outdated block root that was passed in as an argument. --- beacon_node/beacon_chain/src/beacon_chain.rs | 11 +++-------- eth2/state_processing/src/per_slot_processing.rs | 16 ++++------------ eth2/state_processing/tests/tests.rs | 3 +-- 3 files changed, 8 insertions(+), 22 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index a22f4179e..41a718655 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -303,8 +303,6 @@ where /// then having it iteratively updated -- in such a case it's possible for another thread to /// find the state at an old slot. pub fn update_state(&self, mut state: BeaconState) -> Result<(), Error> { - let latest_block_header = self.head().beacon_block.block_header(); - let present_slot = match self.slot_clock.present_slot() { Ok(Some(slot)) => slot, _ => return Err(Error::UnableToReadSlot), @@ -312,7 +310,7 @@ where // If required, transition the new state to the present slot. for _ in state.slot.as_u64()..present_slot.as_u64() { - per_slot_processing(&mut state, &latest_block_header, &self.spec)?; + per_slot_processing(&mut state, &self.spec)?; } state.build_all_caches(&self.spec)?; @@ -324,8 +322,6 @@ where /// Ensures the current canonical `BeaconState` has been transitioned to match the `slot_clock`. pub fn catchup_state(&self) -> Result<(), Error> { - let latest_block_header = self.head().beacon_block.block_header(); - let present_slot = match self.slot_clock.present_slot() { Ok(Some(slot)) => slot, _ => return Err(Error::UnableToReadSlot), @@ -339,7 +335,7 @@ where state.build_epoch_cache(RelativeEpoch::NextWithoutRegistryChange, &self.spec)?; state.build_epoch_cache(RelativeEpoch::NextWithRegistryChange, &self.spec)?; - per_slot_processing(&mut *state, &latest_block_header, &self.spec)?; + per_slot_processing(&mut *state, &self.spec)?; } state.build_all_caches(&self.spec)?; @@ -617,9 +613,8 @@ where // Transition the parent state to the block slot. let mut state = parent_state; - let previous_block_header = parent_block.block_header(); for _ in state.slot.as_u64()..block.slot.as_u64() { - if let Err(e) = per_slot_processing(&mut state, &previous_block_header, &self.spec) { + if let Err(e) = per_slot_processing(&mut state, &self.spec) { return Ok(BlockProcessingOutcome::InvalidBlock( InvalidBlock::SlotProcessingError(e), )); diff --git a/eth2/state_processing/src/per_slot_processing.rs b/eth2/state_processing/src/per_slot_processing.rs index cd129a5f1..7d2bb468f 100644 --- a/eth2/state_processing/src/per_slot_processing.rs +++ b/eth2/state_processing/src/per_slot_processing.rs @@ -11,12 +11,8 @@ pub enum Error { /// Advances a state forward by one slot, performing per-epoch processing if required. /// /// Spec v0.5.0 -pub fn per_slot_processing( - state: &mut BeaconState, - latest_block_header: &BeaconBlockHeader, - spec: &ChainSpec, -) -> Result<(), Error> { - cache_state(state, latest_block_header, spec)?; +pub fn per_slot_processing(state: &mut BeaconState, spec: &ChainSpec) -> Result<(), Error> { + cache_state(state, spec)?; if (state.slot + 1) % spec.slots_per_epoch == 0 { per_epoch_processing(state, spec)?; @@ -27,11 +23,7 @@ pub fn per_slot_processing( Ok(()) } -fn cache_state( - state: &mut BeaconState, - latest_block_header: &BeaconBlockHeader, - spec: &ChainSpec, -) -> Result<(), Error> { +fn cache_state(state: &mut BeaconState, spec: &ChainSpec) -> Result<(), Error> { let previous_slot_state_root = Hash256::from_slice(&state.tree_hash_root()[..]); // Note: increment the state slot here to allow use of our `state_root` and `block_root` @@ -46,7 +38,7 @@ fn cache_state( state.latest_block_header.state_root = previous_slot_state_root } - let latest_block_root = Hash256::from_slice(&latest_block_header.tree_hash_root()[..]); + let latest_block_root = Hash256::from_slice(&state.latest_block_header.tree_hash_root()[..]); state.set_block_root(previous_slot, latest_block_root, spec)?; // Set the state slot back to what it should be. diff --git a/eth2/state_processing/tests/tests.rs b/eth2/state_processing/tests/tests.rs index 54fd6bf8d..d305b2d3c 100644 --- a/eth2/state_processing/tests/tests.rs +++ b/eth2/state_processing/tests/tests.rs @@ -105,8 +105,7 @@ fn run_state_transition_test(test_name: &str) { let mut state = test_case.initial_state.clone(); for (j, block) in test_case.blocks.iter().enumerate() { while block.slot > state.slot { - let latest_block_header = state.latest_block_header.clone(); - per_slot_processing(&mut state, &latest_block_header, &test_case.config).unwrap(); + per_slot_processing(&mut state, &test_case.config).unwrap(); } let res = per_block_processing(&mut state, &block, &test_case.config); if res.is_err() {