From c101c12e5fa5a2a87c7c7ecda3fdf887cb5d3b5a Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Wed, 30 Jun 2021 00:52:31 -0300 Subject: [PATCH] return errors in state methods (#2417) --- zebra-state/src/service.rs | 4 +- .../src/service/non_finalized_state.rs | 48 ++++++++++----- .../src/service/non_finalized_state/chain.rs | 58 +++++++++++++------ .../service/non_finalized_state/tests/prop.rs | 10 ++-- .../non_finalized_state/tests/vectors.rs | 52 ++++++++--------- 5 files changed, 106 insertions(+), 66 deletions(-) diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index 362b04a0..024f3add 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -178,9 +178,9 @@ impl StateService { let parent_hash = prepared.block.header.previous_block_hash; if self.disk.finalized_tip_hash() == parent_hash { - self.mem.commit_new_chain(prepared); + self.mem.commit_new_chain(prepared)?; } else { - self.mem.commit_block(prepared); + self.mem.commit_block(prepared)?; } Ok(()) diff --git a/zebra-state/src/service/non_finalized_state.rs b/zebra-state/src/service/non_finalized_state.rs index 2ab479c9..52296268 100644 --- a/zebra-state/src/service/non_finalized_state.rs +++ b/zebra-state/src/service/non_finalized_state.rs @@ -19,7 +19,7 @@ use zebra_chain::{ transparent, }; -use crate::{FinalizedBlock, HashOrHeight, PreparedBlock, Utxo}; +use crate::{FinalizedBlock, HashOrHeight, PreparedBlock, Utxo, ValidateContextError}; use self::chain::Chain; @@ -74,7 +74,7 @@ impl NonFinalizedState { } /// Commit block to the non-finalized state. - pub fn commit_block(&mut self, prepared: PreparedBlock) { + pub fn commit_block(&mut self, prepared: PreparedBlock) -> Result<(), ValidateContextError> { let parent_hash = prepared.block.header.previous_block_hash; let (height, hash) = (prepared.height, prepared.hash); @@ -85,29 +85,26 @@ impl NonFinalizedState { ); } - let mut parent_chain = self - .take_chain_if(|chain| chain.non_finalized_tip_hash() == parent_hash) - .or_else(|| { - self.chain_set - .iter() - .find_map(|chain| chain.fork(parent_hash)) - .map(Box::new) - }) - .expect("commit_block is only called with blocks that are ready to be commited"); + let mut parent_chain = self.parent_chain(parent_hash)?; - parent_chain.push(prepared); + parent_chain.push(prepared)?; self.chain_set.insert(parent_chain); self.update_metrics_for_committed_block(height, hash); + Ok(()) } /// Commit block to the non-finalized state as a new chain where its parent /// is the finalized tip. - pub fn commit_new_chain(&mut self, prepared: PreparedBlock) { + pub fn commit_new_chain( + &mut self, + prepared: PreparedBlock, + ) -> Result<(), ValidateContextError> { let mut chain = Chain::default(); let (height, hash) = (prepared.height, prepared.hash); - chain.push(prepared); + chain.push(prepared)?; self.chain_set.insert(Box::new(chain)); self.update_metrics_for_committed_block(height, hash); + Ok(()) } /// Returns the length of the non-finalized portion of the current best chain. @@ -245,6 +242,29 @@ impl NonFinalizedState { .map(|box_chain| box_chain.deref()) } + /// Return the chain whose tip block hash is `parent_hash`. + /// + /// The chain can be an existing chain in the non-finalized state or a freshly + /// created fork, if needed. + fn parent_chain( + &mut self, + parent_hash: block::Hash, + ) -> Result, ValidateContextError> { + match self.take_chain_if(|chain| chain.non_finalized_tip_hash() == parent_hash) { + // An existing chain in the non-finalized state + Some(chain) => Ok(chain), + // Create a new fork + None => Ok(Box::new( + self.chain_set + .iter() + .find_map(|chain| chain.fork(parent_hash).transpose()) + .expect( + "commit_block is only called with blocks that are ready to be commited", + )?, + )), + } + } + /// Update the metrics after `block` is committed fn update_metrics_for_committed_block(&self, height: block::Height, hash: block::Hash) { metrics::counter!("state.memory.committed.block.count", 1); diff --git a/zebra-state/src/service/non_finalized_state/chain.rs b/zebra-state/src/service/non_finalized_state/chain.rs index abe1f637..5315ad1e 100644 --- a/zebra-state/src/service/non_finalized_state/chain.rs +++ b/zebra-state/src/service/non_finalized_state/chain.rs @@ -10,7 +10,7 @@ use zebra_chain::{ transaction::Transaction::*, transparent, work::difficulty::PartialCumulativeWork, }; -use crate::{PreparedBlock, Utxo}; +use crate::{PreparedBlock, Utxo, ValidateContextError}; #[derive(Default, Clone)] pub struct Chain { @@ -32,11 +32,12 @@ pub struct Chain { impl Chain { /// Push a contextually valid non-finalized block into a chain as the new tip. #[instrument(level = "debug", skip(self, block), fields(block = %block.block))] - pub fn push(&mut self, block: PreparedBlock) { + pub fn push(&mut self, block: PreparedBlock) -> Result<(), ValidateContextError> { // update cumulative data members - self.update_chain_state_with(&block); + self.update_chain_state_with(&block)?; tracing::debug!(block = %block.block, "adding block to chain"); self.blocks.insert(block.height, block); + Ok(()) } /// Remove the lowest height block of the non-finalized portion of a chain. @@ -67,9 +68,9 @@ impl Chain { /// Fork a chain at the block with the given hash, if it is part of this /// chain. - pub fn fork(&self, fork_tip: block::Hash) -> Option { + pub fn fork(&self, fork_tip: block::Hash) -> Result, ValidateContextError> { if !self.height_by_hash.contains_key(&fork_tip) { - return None; + return Ok(None); } let mut forked = self.clone(); @@ -78,7 +79,7 @@ impl Chain { forked.pop_tip(); } - Some(forked) + Ok(Some(forked)) } pub fn non_finalized_tip_hash(&self) -> block::Hash { @@ -129,7 +130,7 @@ impl Chain { trait UpdateWith { /// Update `Chain` cumulative data members to add data that are derived from /// `T` - fn update_chain_state_with(&mut self, _: &T); + fn update_chain_state_with(&mut self, _: &T) -> Result<(), ValidateContextError>; /// Update `Chain` cumulative data members to remove data that are derived /// from `T` @@ -137,7 +138,10 @@ trait UpdateWith { } impl UpdateWith for Chain { - fn update_chain_state_with(&mut self, prepared: &PreparedBlock) { + fn update_chain_state_with( + &mut self, + prepared: &PreparedBlock, + ) -> Result<(), ValidateContextError> { let (block, hash, height, transaction_hashes) = ( prepared.block.as_ref(), prepared.hash, @@ -207,16 +211,18 @@ impl UpdateWith for Chain { ); // add the utxos this produced - self.update_chain_state_with(&prepared.new_outputs); + self.update_chain_state_with(&prepared.new_outputs)?; // add the utxos this consumed - self.update_chain_state_with(inputs); + self.update_chain_state_with(inputs)?; // add the shielded data - self.update_chain_state_with(joinsplit_data); - self.update_chain_state_with(sapling_shielded_data_per_spend_anchor); - self.update_chain_state_with(sapling_shielded_data_shared_anchor); - self.update_chain_state_with(orchard_shielded_data); + self.update_chain_state_with(joinsplit_data)?; + self.update_chain_state_with(sapling_shielded_data_per_spend_anchor)?; + self.update_chain_state_with(sapling_shielded_data_shared_anchor)?; + self.update_chain_state_with(orchard_shielded_data)?; } + + Ok(()) } #[instrument(skip(self, prepared), fields(block = %prepared.block))] @@ -296,9 +302,13 @@ impl UpdateWith for Chain { } impl UpdateWith> for Chain { - fn update_chain_state_with(&mut self, utxos: &HashMap) { + fn update_chain_state_with( + &mut self, + utxos: &HashMap, + ) -> Result<(), ValidateContextError> { self.created_utxos .extend(utxos.iter().map(|(k, v)| (*k, v.clone()))); + Ok(()) } fn revert_chain_state_with(&mut self, utxos: &HashMap) { @@ -308,7 +318,10 @@ impl UpdateWith> for Chain { } impl UpdateWith> for Chain { - fn update_chain_state_with(&mut self, inputs: &Vec) { + fn update_chain_state_with( + &mut self, + inputs: &Vec, + ) -> Result<(), ValidateContextError> { for consumed_utxo in inputs { match consumed_utxo { transparent::Input::PrevOut { outpoint, .. } => { @@ -317,6 +330,7 @@ impl UpdateWith> for Chain { transparent::Input::Coinbase { .. } => {} } } + Ok(()) } fn revert_chain_state_with(&mut self, inputs: &Vec) { @@ -339,7 +353,7 @@ impl UpdateWith>> for Chain { fn update_chain_state_with( &mut self, joinsplit_data: &Option>, - ) { + ) -> Result<(), ValidateContextError> { if let Some(joinsplit_data) = joinsplit_data { for sprout::JoinSplit { nullifiers, .. } in joinsplit_data.joinsplits() { let span = debug_span!("revert_chain_state_with", ?nullifiers); @@ -349,6 +363,7 @@ impl UpdateWith>> for Chain { self.sprout_nullifiers.insert(nullifiers[1]); } } + Ok(()) } #[instrument(skip(self, joinsplit_data))] @@ -381,12 +396,13 @@ where fn update_chain_state_with( &mut self, sapling_shielded_data: &Option>, - ) { + ) -> Result<(), ValidateContextError> { if let Some(sapling_shielded_data) = sapling_shielded_data { for nullifier in sapling_shielded_data.nullifiers() { self.sapling_nullifiers.insert(*nullifier); } } + Ok(()) } fn revert_chain_state_with( @@ -405,12 +421,16 @@ where } impl UpdateWith> for Chain { - fn update_chain_state_with(&mut self, orchard_shielded_data: &Option) { + fn update_chain_state_with( + &mut self, + orchard_shielded_data: &Option, + ) -> Result<(), ValidateContextError> { if let Some(orchard_shielded_data) = orchard_shielded_data { for nullifier in orchard_shielded_data.nullifiers() { self.orchard_nullifiers.insert(*nullifier); } } + Ok(()) } fn revert_chain_state_with(&mut self, orchard_shielded_data: &Option) { diff --git a/zebra-state/src/service/non_finalized_state/tests/prop.rs b/zebra-state/src/service/non_finalized_state/tests/prop.rs index d5143f6d..94625e5b 100644 --- a/zebra-state/src/service/non_finalized_state/tests/prop.rs +++ b/zebra-state/src/service/non_finalized_state/tests/prop.rs @@ -20,13 +20,13 @@ fn forked_equals_pushed() -> Result<()> { let mut partial_chain = Chain::default(); for block in chain.iter().take(count) { - partial_chain.push(block.clone()); + partial_chain.push(block.clone())?; } for block in chain.iter() { - full_chain.push(block.clone()); + full_chain.push(block.clone())?; } - let forked = full_chain.fork(fork_tip_hash).expect("hash is present"); + let forked = full_chain.fork(fork_tip_hash).expect("fork works").expect("hash is present"); prop_assert_eq!(forked.blocks.len(), partial_chain.blocks.len()); }); @@ -48,10 +48,10 @@ fn finalized_equals_pushed() -> Result<()> { let mut partial_chain = Chain::default(); for block in chain.iter().skip(finalized_count) { - partial_chain.push(block.clone()); + partial_chain.push(block.clone())?; } for block in chain.iter() { - full_chain.push(block.clone()); + full_chain.push(block.clone())?; } for _ in 0..finalized_count { diff --git a/zebra-state/src/service/non_finalized_state/tests/vectors.rs b/zebra-state/src/service/non_finalized_state/tests/vectors.rs index c6adb460..04260d97 100644 --- a/zebra-state/src/service/non_finalized_state/tests/vectors.rs +++ b/zebra-state/src/service/non_finalized_state/tests/vectors.rs @@ -23,7 +23,7 @@ fn construct_single() -> Result<()> { zebra_test::vectors::BLOCK_MAINNET_434873_BYTES.zcash_deserialize_into()?; let mut chain = Chain::default(); - chain.push(block.prepare()); + chain.push(block.prepare())?; assert_eq!(1, chain.blocks.len()); @@ -47,7 +47,7 @@ fn construct_many() -> Result<()> { let mut chain = Chain::default(); for block in blocks { - chain.push(block.prepare()); + chain.push(block.prepare())?; } assert_eq!(100, chain.blocks.len()); @@ -64,10 +64,10 @@ fn ord_matches_work() -> Result<()> { let more_block = less_block.clone().set_work(10); let mut lesser_chain = Chain::default(); - lesser_chain.push(less_block.prepare()); + lesser_chain.push(less_block.prepare())?; let mut bigger_chain = Chain::default(); - bigger_chain.push(more_block.prepare()); + bigger_chain.push(more_block.prepare())?; assert!(bigger_chain > lesser_chain); @@ -100,8 +100,8 @@ fn best_chain_wins_for_network(network: Network) -> Result<()> { let expected_hash = block2.hash(); let mut state = NonFinalizedState::default(); - state.commit_new_chain(block2.prepare()); - state.commit_new_chain(child.prepare()); + state.commit_new_chain(block2.prepare())?; + state.commit_new_chain(child.prepare())?; let best_chain = state.best_chain().unwrap(); assert!(best_chain.height_by_hash.contains_key(&expected_hash)); @@ -133,9 +133,9 @@ fn finalize_pops_from_best_chain_for_network(network: Network) -> Result<()> { let child = block1.make_fake_child().set_work(1); let mut state = NonFinalizedState::default(); - state.commit_new_chain(block1.clone().prepare()); - state.commit_block(block2.clone().prepare()); - state.commit_block(child.prepare()); + state.commit_new_chain(block1.clone().prepare())?; + state.commit_block(block2.clone().prepare())?; + state.commit_block(child.prepare())?; let finalized = state.finalize(); assert_eq!(block1, finalized.block); @@ -177,13 +177,13 @@ fn commit_block_extending_best_chain_doesnt_drop_worst_chains_for_network( let mut state = NonFinalizedState::default(); assert_eq!(0, state.chain_set.len()); - state.commit_new_chain(block1.prepare()); + state.commit_new_chain(block1.prepare())?; assert_eq!(1, state.chain_set.len()); - state.commit_block(block2.prepare()); + state.commit_block(block2.prepare())?; assert_eq!(1, state.chain_set.len()); - state.commit_block(child1.prepare()); + state.commit_block(child1.prepare())?; assert_eq!(2, state.chain_set.len()); - state.commit_block(child2.prepare()); + state.commit_block(child2.prepare())?; assert_eq!(2, state.chain_set.len()); Ok(()) @@ -215,10 +215,10 @@ fn shorter_chain_can_be_best_chain_for_network(network: Network) -> Result<()> { let short_chain_block = block1.make_fake_child().set_work(3); let mut state = NonFinalizedState::default(); - state.commit_new_chain(block1.prepare()); - state.commit_block(long_chain_block1.prepare()); - state.commit_block(long_chain_block2.prepare()); - state.commit_block(short_chain_block.prepare()); + state.commit_new_chain(block1.prepare())?; + state.commit_block(long_chain_block1.prepare())?; + state.commit_block(long_chain_block2.prepare())?; + state.commit_block(short_chain_block.prepare())?; assert_eq!(2, state.chain_set.len()); assert_eq!(2, state.best_chain_len()); @@ -254,12 +254,12 @@ fn longer_chain_with_more_work_wins_for_network(network: Network) -> Result<()> let short_chain_block = block1.make_fake_child().set_work(3); let mut state = NonFinalizedState::default(); - state.commit_new_chain(block1.prepare()); - state.commit_block(long_chain_block1.prepare()); - state.commit_block(long_chain_block2.prepare()); - state.commit_block(long_chain_block3.prepare()); - state.commit_block(long_chain_block4.prepare()); - state.commit_block(short_chain_block.prepare()); + state.commit_new_chain(block1.prepare())?; + state.commit_block(long_chain_block1.prepare())?; + state.commit_block(long_chain_block2.prepare())?; + state.commit_block(long_chain_block3.prepare())?; + state.commit_block(long_chain_block4.prepare())?; + state.commit_block(short_chain_block.prepare())?; assert_eq!(2, state.chain_set.len()); assert_eq!(5, state.best_chain_len()); @@ -291,9 +291,9 @@ fn equal_length_goes_to_more_work_for_network(network: Network) -> Result<()> { let expected_hash = more_work_child.hash(); let mut state = NonFinalizedState::default(); - state.commit_new_chain(block1.prepare()); - state.commit_block(less_work_child.prepare()); - state.commit_block(more_work_child.prepare()); + state.commit_new_chain(block1.prepare())?; + state.commit_block(less_work_child.prepare())?; + state.commit_block(more_work_child.prepare())?; assert_eq!(2, state.chain_set.len()); let tip_hash = state.best_tip().unwrap().1;