diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 7f6c97a0f85..ec4549debb4 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2999,7 +2999,7 @@ pub fn serve( let fork_choice_nodes = proto_array .nodes - .iter() + .values() .map(|node| { let execution_status = if node.execution_status.is_execution_enabled() { Some(node.execution_status.to_string()) @@ -3010,10 +3010,7 @@ pub fn serve( ForkChoiceNode { slot: node.slot, block_root: node.root, - parent_root: node - .parent - .and_then(|index| proto_array.nodes.get(index)) - .map(|parent| parent.root), + parent_root: node.parent, justified_epoch: node.justified_checkpoint.epoch, finalized_epoch: node.finalized_checkpoint.epoch, weight: node.weight, diff --git a/consensus/fork_choice/src/metrics.rs b/consensus/fork_choice/src/metrics.rs index b5cda2f5871..b31b9ab833f 100644 --- a/consensus/fork_choice/src/metrics.rs +++ b/consensus/fork_choice/src/metrics.rs @@ -13,12 +13,6 @@ pub static FORK_CHOICE_QUEUED_ATTESTATIONS: LazyLock> = LazyLoc pub static FORK_CHOICE_NODES: LazyLock> = LazyLock::new(|| { try_create_int_gauge("fork_choice_nodes", "Current count of proto array nodes") }); -pub static FORK_CHOICE_INDICES: LazyLock> = LazyLock::new(|| { - try_create_int_gauge( - "fork_choice_indices", - "Current count of proto array indices", - ) -}); pub static FORK_CHOICE_DEQUEUED_ATTESTATIONS: LazyLock> = LazyLock::new(|| { try_create_int_counter( "fork_choice_dequeued_attestations_total", @@ -55,8 +49,4 @@ pub fn scrape_for_metrics, E: EthSpec>(fork_choice: &ForkC &FORK_CHOICE_NODES, fork_choice.proto_array().core_proto_array().nodes.len() as i64, ); - set_gauge( - &FORK_CHOICE_INDICES, - fork_choice.proto_array().core_proto_array().indices.len() as i64, - ); } diff --git a/consensus/proto_array/src/error.rs b/consensus/proto_array/src/error.rs index 35cb4007b78..80cc5b6facb 100644 --- a/consensus/proto_array/src/error.rs +++ b/consensus/proto_array/src/error.rs @@ -7,20 +7,20 @@ pub enum Error { JustifiedNodeUnknown(Hash256), NodeUnknown(Hash256), InvalidFinalizedRootChange, - InvalidNodeIndex(usize), + InvalidNodeIndex(Hash256), InvalidParentIndex(usize), InvalidBestChildIndex(usize), - InvalidJustifiedIndex(usize), - InvalidBestDescendant(usize), - InvalidParentDelta(usize), - InvalidNodeDelta(usize), + InvalidJustifiedIndex(Hash256), + InvalidBestDescendant(Hash256), + InvalidParentDelta(Hash256), + InvalidNodeDelta(Hash256), MissingJustifiedCheckpoint, MissingFinalizedCheckpoint, - DeltaOverflow(usize), - ProposerBoostOverflow(usize), + DeltaOverflow(Hash256), + ProposerBoostOverflow(Hash256), ReOrgThresholdOverflow, IndexOverflow(&'static str), - InvalidExecutionDeltaOverflow(usize), + InvalidExecutionDeltaOverflow(Hash256), InvalidDeltaLen { deltas: usize, indices: usize, diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 18af2dfc24c..2f2df1871a2 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -90,17 +90,14 @@ pub struct ProtoNode { pub current_epoch_shuffling_id: AttestationShufflingId, pub next_epoch_shuffling_id: AttestationShufflingId, pub root: Hash256, - #[ssz(with = "four_byte_option_usize")] - pub parent: Option, + pub parent: Option, #[superstruct(only(V17))] pub justified_checkpoint: Checkpoint, #[superstruct(only(V17))] pub finalized_checkpoint: Checkpoint, pub weight: u64, - #[ssz(with = "four_byte_option_usize")] - pub best_child: Option, - #[ssz(with = "four_byte_option_usize")] - pub best_descendant: Option, + pub best_child: Option, + pub best_descendant: Option, /// Indicates if an execution node has marked this block as valid. Also contains the execution /// block hash. pub execution_status: ExecutionStatus, @@ -132,8 +129,7 @@ pub struct ProtoArray { pub prune_threshold: usize, pub justified_checkpoint: Checkpoint, pub finalized_checkpoint: Checkpoint, - pub nodes: Vec, - pub indices: HashMap, + pub nodes: HashMap, pub previous_proposer_boost: ProposerBoost, } @@ -154,7 +150,7 @@ impl ProtoArray { #[allow(clippy::too_many_arguments)] pub fn apply_score_changes( &mut self, - mut deltas: Vec, + mut deltas: HashMap, justified_checkpoint: Checkpoint, finalized_checkpoint: Checkpoint, new_justified_balances: &JustifiedBalances, @@ -162,10 +158,10 @@ impl ProtoArray { current_slot: Slot, spec: &ChainSpec, ) -> Result<(), Error> { - if deltas.len() != self.indices.len() { + if deltas.len() != self.nodes.len() { return Err(Error::InvalidDeltaLen { deltas: deltas.len(), - indices: self.indices.len(), + indices: self.nodes.len(), }); } @@ -180,12 +176,7 @@ impl ProtoArray { let mut proposer_score = 0; // Iterate backwards through all indices in `self.nodes`. - for node_index in (0..self.nodes.len()).rev() { - let node = self - .nodes - .get_mut(node_index) - .ok_or(Error::InvalidNodeIndex(node_index))?; - + for node in iter_mut_all_backwards(&mut self.nodes) { // There is no need to adjust the balances or manage parent of the zero hash since it // is an alias to the genesis block. The weight applied to the genesis block is // irrelevant as we _always_ choose it and it's impossible for it to have a parent. @@ -199,12 +190,12 @@ impl ProtoArray { // If the node has an invalid execution payload, reduce its weight to zero. 0_i64 .checked_sub(node.weight as i64) - .ok_or(Error::InvalidExecutionDeltaOverflow(node_index))? + .ok_or(Error::InvalidExecutionDeltaOverflow(node.root))? } else { deltas - .get(node_index) + .get(&node.root) .copied() - .ok_or(Error::InvalidNodeDelta(node_index))? + .ok_or(Error::InvalidNodeDelta(node.root))? }; // If we find the node for which the proposer boost was previously applied, decrease @@ -217,7 +208,7 @@ impl ProtoArray { { node_delta = node_delta .checked_sub(self.previous_proposer_boost.score as i64) - .ok_or(Error::DeltaOverflow(node_index))?; + .ok_or(Error::DeltaOverflow(node.root))?; } // If we find the node matching the current proposer boost root, increase // the delta by the new score amount (unless the block has an invalid execution status). @@ -231,10 +222,10 @@ impl ProtoArray { { proposer_score = calculate_committee_fraction::(new_justified_balances, proposer_score_boost) - .ok_or(Error::ProposerBoostOverflow(node_index))?; + .ok_or(Error::ProposerBoostOverflow(node.root))?; node_delta = node_delta .checked_add(proposer_score as i64) - .ok_or(Error::DeltaOverflow(node_index))?; + .ok_or(Error::DeltaOverflow(node.root))?; } // Apply the delta to the node. @@ -254,19 +245,19 @@ impl ProtoArray { node.weight = node .weight .checked_sub(node_delta.unsigned_abs()) - .ok_or(Error::DeltaOverflow(node_index))?; + .ok_or(Error::DeltaOverflow(node.root))?; } else { node.weight = node .weight .checked_add(node_delta as u64) - .ok_or(Error::DeltaOverflow(node_index))?; + .ok_or(Error::DeltaOverflow(node.root))?; } // Update the parent delta (if any). - if let Some(parent_index) = node.parent { + if let Some(parent_root) = node.parent { let parent_delta = deltas - .get_mut(parent_index) - .ok_or(Error::InvalidParentDelta(parent_index))?; + .get_mut(&parent_root) + .ok_or(Error::InvalidParentDelta(parent_root))?; // Back-propagate the nodes delta to its parent. *parent_delta += node_delta; @@ -284,17 +275,15 @@ impl ProtoArray { // We _must_ perform these functions separate from the weight-updating loop above to ensure // that we have a fully coherent set of weights before updating parent // best-child/descendant. - for node_index in (0..self.nodes.len()).rev() { - let node = self - .nodes - .get_mut(node_index) - .ok_or(Error::InvalidNodeIndex(node_index))?; - + let nodes = iter_mut_all_forwards(&mut self.nodes) + .map(|node| (node.parent, node.root)) + .collect::>(); + for (parent_root, block_root) in nodes { // If the node has a parent, try to update its best-child and best-descendant. - if let Some(parent_index) = node.parent { + if let Some(parent_root) = parent_root { self.maybe_update_best_child_and_descendant::( - parent_index, - node_index, + parent_root, + block_root, current_slot, )?; } @@ -308,12 +297,10 @@ impl ProtoArray { /// It is only sane to supply a `None` parent for the genesis block. pub fn on_block(&mut self, block: Block, current_slot: Slot) -> Result<(), Error> { // If the block is already known, simply ignore it. - if self.indices.contains_key(&block.root) { + if self.nodes.contains_key(&block.root) { return Ok(()); } - let node_index = self.nodes.len(); - let node = ProtoNode { slot: block.slot, root: block.root, @@ -321,9 +308,7 @@ impl ProtoArray { current_epoch_shuffling_id: block.current_epoch_shuffling_id, next_epoch_shuffling_id: block.next_epoch_shuffling_id, state_root: block.state_root, - parent: block - .parent_root - .and_then(|parent| self.indices.get(&parent).copied()), + parent: block.parent_root, justified_checkpoint: block.justified_checkpoint, finalized_checkpoint: block.finalized_checkpoint, weight: 0, @@ -336,11 +321,11 @@ impl ProtoArray { // If the parent has an invalid execution status, return an error before adding the block to // `self`. - if let Some(parent_index) = node.parent { + if let Some(parent_root) = node.parent { let parent = self .nodes - .get(parent_index) - .ok_or(Error::InvalidNodeIndex(parent_index))?; + .get(&parent_root) + .ok_or(Error::InvalidNodeIndex(parent_root))?; if parent.execution_status.is_invalid() { return Err(Error::ParentExecutionStatusIsInvalid { block_root: block.root, @@ -349,18 +334,13 @@ impl ProtoArray { } } - self.indices.insert(node.root, node_index); - self.nodes.push(node.clone()); + self.nodes.insert(node.root, node.clone()); - if let Some(parent_index) = node.parent { - self.maybe_update_best_child_and_descendant::( - parent_index, - node_index, - current_slot, - )?; + if let Some(parent_root) = node.parent { + self.maybe_update_best_child_and_descendant::(parent_root, node.root, current_slot)?; if matches!(block.execution_status, ExecutionStatus::Valid(_)) { - self.propagate_execution_payload_validation_by_index(parent_index)?; + self.propagate_execution_payload_validation(parent_root)?; } } @@ -377,11 +357,7 @@ impl ProtoArray { &mut self, block_root: Hash256, ) -> Result<(), Error> { - let index = *self - .indices - .get(&block_root) - .ok_or(Error::NodeUnknown(block_root))?; - self.propagate_execution_payload_validation_by_index(index) + self.propagate_execution_payload_validation_by_index(block_root) } /// Updates the `verified_node_index` and all ancestors to have validated execution payloads. @@ -392,15 +368,15 @@ impl ProtoArray { /// - Any of the to-be-validated payloads are already invalid. fn propagate_execution_payload_validation_by_index( &mut self, - verified_node_index: usize, + verified_block_root: Hash256, ) -> Result<(), Error> { - let mut index = verified_node_index; + let mut root = verified_block_root; loop { let node = self .nodes - .get_mut(index) - .ok_or(Error::InvalidNodeIndex(index))?; - let parent_index = match node.execution_status { + .get_mut(&root) + .ok_or(Error::InvalidNodeIndex(root))?; + let parent_root = match node.execution_status { // We have reached a node that we already know is valid. No need to iterate further // since we assume an ancestors have already been set to valid. ExecutionStatus::Valid(_) => return Ok(()), @@ -412,8 +388,8 @@ impl ProtoArray { // payload can be considered valid. ExecutionStatus::Optimistic(payload_block_hash) => { node.execution_status = ExecutionStatus::Valid(payload_block_hash); - if let Some(parent_index) = node.parent { - parent_index + if let Some(parent_root) = node.parent { + parent_root } else { // We have reached the root block, iteration complete. return Ok(()); @@ -429,7 +405,7 @@ impl ProtoArray { } }; - index = parent_index; + root = parent_root; } } @@ -440,7 +416,7 @@ impl ProtoArray { &mut self, op: &InvalidationOperation, ) -> Result<(), Error> { - let mut invalidated_indices: HashSet = <_>::default(); + let mut invalidated_roots: HashSet = <_>::default(); let head_block_root = op.block_root(); /* @@ -450,10 +426,7 @@ impl ProtoArray { * all invalidated block indices in `invalidated_indices`. */ - let mut index = *self - .indices - .get(&head_block_root) - .ok_or(Error::NodeUnknown(head_block_root))?; + let mut root = head_block_root; // Try to map the ancestor payload *hash* to an ancestor beacon block *root*. let latest_valid_ancestor_root = op @@ -475,8 +448,8 @@ impl ProtoArray { loop { let node = self .nodes - .get_mut(index) - .ok_or(Error::InvalidNodeIndex(index))?; + .get_mut(&root) + .ok_or(Error::InvalidNodeIndex(root))?; match node.execution_status { ExecutionStatus::Valid(hash) @@ -502,13 +475,13 @@ impl ProtoArray { // head. if node .best_child - .is_some_and(|i| invalidated_indices.contains(&i)) + .is_some_and(|root| invalidated_roots.contains(&root)) { node.best_child = None } if node .best_descendant - .is_some_and(|i| invalidated_indices.contains(&i)) + .is_some_and(|root| invalidated_roots.contains(&root)) { node.best_descendant = None } @@ -537,7 +510,7 @@ impl ProtoArray { }); } ExecutionStatus::Optimistic(hash) => { - invalidated_indices.insert(index); + invalidated_roots.insert(node.root); node.execution_status = ExecutionStatus::Invalid(*hash); // It's impossible for an invalid block to lead to a "best" block, so set these @@ -557,8 +530,8 @@ impl ProtoArray { } } - if let Some(parent_index) = node.parent { - index = parent_index + if let Some(parent_root) = node.parent { + root = parent_root } else { // The root of the block tree has been reached (aka the finalized block), without // matching `latest_valid_ancestor_hash`. It's not possible or useful to go any @@ -574,25 +547,13 @@ impl ProtoArray { * *forwards* to invalidate all descendants of all blocks in `invalidated_indices`. */ - let starting_block_root = latest_valid_ancestor_root - .filter(|_| latest_valid_ancestor_is_descendant) - .unwrap_or(head_block_root); - let latest_valid_ancestor_index = *self - .indices - .get(&starting_block_root) - .ok_or(Error::NodeUnknown(starting_block_root))?; - let first_potential_descendant = latest_valid_ancestor_index + 1; - + // TODO: Skip blocks older than starting_block_root, latest_valid_ancestor_root + // // Collect all *descendants* which have been declared invalid since they're the descendant of a block // with an invalid execution payload. - for index in first_potential_descendant..self.nodes.len() { - let node = self - .nodes - .get_mut(index) - .ok_or(Error::InvalidNodeIndex(index))?; - + for node in iter_mut_all_forwards(&mut self.nodes) { if let Some(parent_index) = node.parent - && invalidated_indices.contains(&parent_index) + && invalidated_roots.contains(&parent_index) { match &node.execution_status { ExecutionStatus::Valid(hash) => { @@ -611,7 +572,7 @@ impl ProtoArray { } } - invalidated_indices.insert(index); + invalidated_roots.insert(node.root); } } @@ -631,16 +592,10 @@ impl ProtoArray { justified_root: &Hash256, current_slot: Slot, ) -> Result { - let justified_index = self - .indices - .get(justified_root) - .copied() - .ok_or(Error::JustifiedNodeUnknown(*justified_root))?; - let justified_node = self .nodes - .get(justified_index) - .ok_or(Error::InvalidJustifiedIndex(justified_index))?; + .get(justified_root) + .ok_or(Error::InvalidJustifiedIndex(*justified_root))?; // Since there are no valid descendants of a justified block with an invalid execution // payload, there would be no head to choose from. @@ -655,12 +610,12 @@ impl ProtoArray { }); } - let best_descendant_index = justified_node.best_descendant.unwrap_or(justified_index); + let best_descendant_root = justified_node.best_descendant.unwrap_or(*justified_root); let best_node = self .nodes - .get(best_descendant_index) - .ok_or(Error::InvalidBestDescendant(best_descendant_index))?; + .get(&best_descendant_root) + .ok_or(Error::InvalidBestDescendant(best_descendant_root))?; // Perform a sanity check that the node is indeed valid to be the head. if !self.node_is_viable_for_head::(best_node, current_slot) { @@ -692,58 +647,18 @@ impl ProtoArray { /// - The finalized epoch is equal to the current one, but the finalized root is different. /// - There is some internal error relating to invalid indices inside `self`. pub fn maybe_prune(&mut self, finalized_root: Hash256) -> Result<(), Error> { - let finalized_index = *self - .indices - .get(&finalized_root) - .ok_or(Error::FinalizedNodeUnknown(finalized_root))?; - - if finalized_index < self.prune_threshold { + if self.nodes.len() < self.prune_threshold { // Pruning at small numbers incurs more cost than benefit. return Ok(()); } - // Remove the `self.indices` key/values for all the to-be-deleted nodes. - for node_index in 0..finalized_index { - let root = &self - .nodes - .get(node_index) - .ok_or(Error::InvalidNodeIndex(node_index))? - .root; - self.indices.remove(root); - } - - // Drop all the nodes prior to finalization. - self.nodes = self.nodes.split_off(finalized_index); - - // Adjust the indices map. - for (_root, index) in self.indices.iter_mut() { - *index = index - .checked_sub(finalized_index) - .ok_or(Error::IndexOverflow("indices"))?; - } + let finalized_slot = self + .nodes + .get(&finalized_root) + .map(|node| node.slot) + .ok_or(Error::FinalizedNodeUnknown(finalized_root))?; - // Iterate through all the existing nodes and adjust their indices to match the new layout - // of `self.nodes`. - for node in self.nodes.iter_mut() { - if let Some(parent) = node.parent { - // If `node.parent` is less than `finalized_index`, set it to `None`. - node.parent = parent.checked_sub(finalized_index); - } - if let Some(best_child) = node.best_child { - node.best_child = Some( - best_child - .checked_sub(finalized_index) - .ok_or(Error::IndexOverflow("best_child"))?, - ); - } - if let Some(best_descendant) = node.best_descendant { - node.best_descendant = Some( - best_descendant - .checked_sub(finalized_index) - .ok_or(Error::IndexOverflow("best_descendant"))?, - ); - } - } + self.nodes.retain(|_, node| node.slot >= finalized_slot); Ok(()) } @@ -762,19 +677,19 @@ impl ProtoArray { /// - The child is not the best child and does not become the best child. fn maybe_update_best_child_and_descendant( &mut self, - parent_index: usize, - child_index: usize, + parent_root: Hash256, + child_root: Hash256, current_slot: Slot, ) -> Result<(), Error> { let child = self .nodes - .get(child_index) - .ok_or(Error::InvalidNodeIndex(child_index))?; + .get(&child_root) + .ok_or(Error::InvalidNodeIndex(child_root))?; let parent = self .nodes - .get(parent_index) - .ok_or(Error::InvalidNodeIndex(parent_index))?; + .get(&parent_root) + .ok_or(Error::InvalidNodeIndex(parent_root))?; let child_leads_to_viable_head = self.node_leads_to_viable_head::(child, current_slot)?; @@ -784,26 +699,23 @@ impl ProtoArray { // // I use the aliases to assist readability. let change_to_none = (None, None); - let change_to_child = ( - Some(child_index), - child.best_descendant.or(Some(child_index)), - ); + let change_to_child = (Some(child_root), child.best_descendant.or(Some(child_root))); let no_change = (parent.best_child, parent.best_descendant); let (new_best_child, new_best_descendant) = if let Some(best_child_index) = parent.best_child { - if best_child_index == child_index && !child_leads_to_viable_head { + if best_child_index == child_root && !child_leads_to_viable_head { // If the child is already the best-child of the parent but it's not viable for // the head, remove it. change_to_none - } else if best_child_index == child_index { + } else if best_child_index == child_root { // If the child is the best-child already, set it again to ensure that the // best-descendant of the parent is updated. change_to_child } else { let best_child = self .nodes - .get(best_child_index) + .get(&best_child_index) .ok_or(Error::InvalidBestDescendant(best_child_index))?; let best_child_leads_to_viable_head = @@ -841,8 +753,8 @@ impl ProtoArray { let parent = self .nodes - .get_mut(parent_index) - .ok_or(Error::InvalidNodeIndex(parent_index))?; + .get_mut(&parent_root) + .ok_or(Error::InvalidNodeIndex(parent_root))?; parent.best_child = new_best_child; parent.best_descendant = new_best_descendant; @@ -861,7 +773,7 @@ impl ProtoArray { if let Some(best_descendant_index) = node.best_descendant { let best_descendant = self .nodes - .get(best_descendant_index) + .get(&best_descendant_index) .ok_or(Error::InvalidBestDescendant(best_descendant_index))?; self.node_is_viable_for_head::(best_descendant, current_slot) @@ -913,9 +825,8 @@ impl ProtoArray { /// Return a reverse iterator over the nodes which comprise the chain ending at `block_root`. pub fn iter_nodes<'a>(&'a self, block_root: &Hash256) -> Iter<'a> { - let next_node_index = self.indices.get(block_root).copied(); Iter { - next_node_index, + next_block_root: Some(*block_root), proto_array: self, } } @@ -944,9 +855,8 @@ impl ProtoArray { /// finalized checkpoint. Use `Self::is_finalized_checkpoint_or_descendant` /// instead. pub fn is_descendant(&self, ancestor_root: Hash256, descendant_root: Hash256) -> bool { - self.indices + self.nodes .get(&ancestor_root) - .and_then(|ancestor_index| self.nodes.get(*ancestor_index)) .and_then(|ancestor| { self.iter_block_roots(&descendant_root) .take_while(|(_root, slot)| *slot >= ancestor.slot) @@ -968,11 +878,7 @@ impl ProtoArray { .epoch .start_slot(E::slots_per_epoch()); - let Some(mut node) = self - .indices - .get(&root) - .and_then(|index| self.nodes.get(*index)) - else { + let Some(mut node) = self.nodes.get(&root) else { // An unknown root is not a finalized descendant. This line can only // be reached if the user supplies a root that is not known to fork // choice. @@ -1010,7 +916,7 @@ impl ProtoArray { // Since `node` is from a higher slot that the finalized checkpoint, // replace `node` with the parent of `node`. - if let Some(parent) = node.parent.and_then(|index| self.nodes.get(index)) { + if let Some(parent) = node.parent.and_then(|root| self.nodes.get(&root)) { node = parent } else { // If `node` is not the finalized block and its parent does not @@ -1029,8 +935,7 @@ impl ProtoArray { block_hash: &ExecutionBlockHash, ) -> Option { self.nodes - .iter() - .rev() + .values() .find(|node| { node.execution_status .block_hash() @@ -1046,7 +951,7 @@ impl ProtoArray { /// definition of "head" used by pruning (which does not consider viability) and fork choice. pub fn heads_descended_from_finalization(&self) -> Vec<&ProtoNode> { self.nodes - .iter() + .values() .filter(|node| { node.best_child.is_none() && self.is_finalized_checkpoint_or_descendant::(node.root) @@ -1070,9 +975,27 @@ pub fn calculate_committee_fraction( .checked_div(100) } +pub(crate) fn iter_mut_all_backwards( + nodes: &mut HashMap, +) -> impl Iterator { + let mut nodes = nodes.values_mut().collect::>(); + // TODO: Test this is backwards + nodes.sort_by_key(|node| node.slot); + nodes.into_iter() +} + +pub(crate) fn iter_mut_all_forwards( + nodes: &mut HashMap, +) -> impl Iterator { + let mut nodes = nodes.values_mut().collect::>(); + // TODO: Test this is forwards + nodes.sort_by_key(|node| node.slot); + nodes.into_iter() +} + /// Reverse iterator over one path through a `ProtoArray`. pub struct Iter<'a> { - next_node_index: Option, + next_block_root: Option, proto_array: &'a ProtoArray, } @@ -1080,9 +1003,9 @@ impl<'a> Iterator for Iter<'a> { type Item = &'a ProtoNode; fn next(&mut self) -> Option { - let next_node_index = self.next_node_index?; - let node = self.proto_array.nodes.get(next_node_index)?; - self.next_node_index = node.parent; + let next_block_root = self.next_block_root?; + let node = self.proto_array.nodes.get(&next_block_root)?; + self.next_block_root = node.parent; Some(node) } } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index dea853d245d..954ff268c3c 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -3,7 +3,7 @@ use crate::{ error::Error, proto_array::{ InvalidationOperation, Iter, ProposerBoost, ProtoArray, ProtoNode, - calculate_committee_fraction, + calculate_committee_fraction, iter_mut_all_backwards, }, ssz_container::SszContainer, }; @@ -426,8 +426,7 @@ impl ProtoArrayForkChoice { prune_threshold: DEFAULT_PRUNE_THRESHOLD, justified_checkpoint, finalized_checkpoint, - nodes: Vec::with_capacity(1), - indices: HashMap::with_capacity(1), + nodes: <_>::default(), previous_proposer_boost: ProposerBoost::default(), }; @@ -524,7 +523,6 @@ impl ProtoArrayForkChoice { let new_balances = justified_state_balances; let deltas = compute_deltas( - &self.proto_array.indices, &mut self.votes, &old_balances.effective_balances, &new_balances.effective_balances, @@ -705,7 +703,7 @@ impl ProtoArrayForkChoice { pub fn contains_invalid_payloads(&mut self) -> bool { self.proto_array .nodes - .iter() + .values() .any(|node| node.execution_status.is_invalid()) } @@ -724,12 +722,15 @@ impl ProtoArrayForkChoice { // This function will touch all blocks, even those that do not descend from the finalized // block. Since this function is expected to run at start-up during very rare // circumstances we prefer simplicity over efficiency. - for node_index in (0..self.proto_array.nodes.len()).rev() { + let block_roots_backwards = iter_mut_all_backwards(&mut self.proto_array.nodes) + .map(|node| node.root) + .collect::>(); + for block_root in block_roots_backwards { let node = self .proto_array .nodes - .get_mut(node_index) - .ok_or("unreachable index out of bounds in proto_array nodes")?; + .get_mut(&block_root) + .ok_or("unreachable key is part proto_array nodes")?; match node.execution_status { ExecutionStatus::Invalid(block_hash) => { @@ -782,12 +783,12 @@ impl ProtoArrayForkChoice { .checked_add(restored_weight) .ok_or("Overflow when adding weight to ancestor")?; - if let Some(parent_index) = node_or_ancestor.parent { + if let Some(parent_root) = node_or_ancestor.parent { node_or_ancestor = self .proto_array .nodes - .get_mut(parent_index) - .ok_or(format!("Missing parent index: {}", parent_index))?; + .get_mut(&parent_root) + .ok_or(format!("Missing parent node: {}", parent_root))?; } else { // This is either the finalized block or a block that does not // descend from the finalized block. @@ -828,25 +829,19 @@ impl ProtoArrayForkChoice { } pub fn contains_block(&self, block_root: &Hash256) -> bool { - self.proto_array.indices.contains_key(block_root) + self.proto_array.nodes.contains_key(block_root) } fn get_proto_node(&self, block_root: &Hash256) -> Option<&ProtoNode> { - let block_index = self.proto_array.indices.get(block_root)?; - self.proto_array.nodes.get(*block_index) + self.proto_array.nodes.get(block_root) } pub fn get_block(&self, block_root: &Hash256) -> Option { let block = self.get_proto_node(block_root)?; - let parent_root = block - .parent - .and_then(|i| self.proto_array.nodes.get(i)) - .map(|parent| parent.root); - Some(Block { slot: block.slot, root: block.root, - parent_root, + parent_root: block.parent, state_root: block.state_root, target_root: block.target_root, current_epoch_shuffling_id: block.current_epoch_shuffling_id.clone(), @@ -867,10 +862,9 @@ impl ProtoArrayForkChoice { /// Returns the weight of a given block. pub fn get_weight(&self, block_root: &Hash256) -> Option { - let block_index = self.proto_array.indices.get(block_root)?; self.proto_array .nodes - .get(*block_index) + .get(block_root) .map(|node| node.weight) } @@ -970,13 +964,12 @@ impl ProtoArrayForkChoice { /// - If some `Hash256` in `votes` is not a key in `indices` (except for `Hash256::zero()`, this is /// always valid). fn compute_deltas( - indices: &HashMap, votes: &mut ElasticList, old_balances: &[u64], new_balances: &[u64], equivocating_indices: &BTreeSet, -) -> Result, Error> { - let mut deltas = vec![0_i64; indices.len()]; +) -> Result, Error> { + let mut deltas = HashMap::::new(); for (val_index, vote) in votes.iter_mut().enumerate() { // There is no need to create a score change if the validator has never voted or both their @@ -999,17 +992,10 @@ fn compute_deltas( // 2. Set their `current_root` (permanently) to zero. if !vote.current_root.is_zero() { let old_balance = old_balances.get(val_index).copied().unwrap_or(0); - - if let Some(current_delta_index) = indices.get(&vote.current_root).copied() { - let delta = deltas - .get(current_delta_index) - .ok_or(Error::InvalidNodeDelta(current_delta_index))? - .checked_sub(old_balance as i64) - .ok_or(Error::DeltaOverflow(current_delta_index))?; - - // Array access safe due to check on previous line. - deltas[current_delta_index] = delta; - } + let delta = deltas.entry(vote.current_root).or_default(); + *delta = delta + .checked_sub(old_balance as i64) + .ok_or(Error::DeltaOverflow(vote.current_root))?; vote.current_root = Hash256::zero(); } @@ -1031,29 +1017,15 @@ fn compute_deltas( if vote.current_root != vote.next_root || old_balance != new_balance { // We ignore the vote if it is not known in `indices`. We assume that it is outside // of our tree (i.e., pre-finalization) and therefore not interesting. - if let Some(current_delta_index) = indices.get(&vote.current_root).copied() { - let delta = deltas - .get(current_delta_index) - .ok_or(Error::InvalidNodeDelta(current_delta_index))? - .checked_sub(old_balance as i64) - .ok_or(Error::DeltaOverflow(current_delta_index))?; - - // Array access safe due to check on previous line. - deltas[current_delta_index] = delta; - } - - // We ignore the vote if it is not known in `indices`. We assume that it is outside - // of our tree (i.e., pre-finalization) and therefore not interesting. - if let Some(next_delta_index) = indices.get(&vote.next_root).copied() { - let delta = deltas - .get(next_delta_index) - .ok_or(Error::InvalidNodeDelta(next_delta_index))? - .checked_add(new_balance as i64) - .ok_or(Error::DeltaOverflow(next_delta_index))?; - - // Array access safe due to check on previous line. - deltas[next_delta_index] = delta; - } + let delta = deltas.entry(vote.current_root).or_default(); + *delta = delta + .checked_sub(old_balance as i64) + .ok_or(Error::DeltaOverflow(vote.current_root))?; + + let delta = deltas.entry(vote.next_root).or_default(); + *delta = delta + .checked_sub(new_balance as i64) + .ok_or(Error::DeltaOverflow(vote.next_root))?; vote.current_root = vote.next_root; } diff --git a/consensus/proto_array/src/ssz_container.rs b/consensus/proto_array/src/ssz_container.rs index 0bb3f2b35d8..6c336b50470 100644 --- a/consensus/proto_array/src/ssz_container.rs +++ b/consensus/proto_array/src/ssz_container.rs @@ -6,9 +6,8 @@ use crate::{ }; use ssz::{Encode, four_byte_option_impl}; use ssz_derive::{Decode, Encode}; -use std::collections::HashMap; use superstruct::superstruct; -use types::{Checkpoint, Hash256}; +use types::Checkpoint; // Define a "legacy" implementation of `Option` which uses four bytes for encoding the union // selector. @@ -29,7 +28,6 @@ pub struct SszContainer { pub justified_checkpoint: Checkpoint, pub finalized_checkpoint: Checkpoint, pub nodes: Vec, - pub indices: Vec<(Hash256, usize)>, pub previous_proposer_boost: ProposerBoost, } @@ -42,8 +40,7 @@ impl From<&ProtoArrayForkChoice> for SszContainer { prune_threshold: proto_array.prune_threshold, justified_checkpoint: proto_array.justified_checkpoint, finalized_checkpoint: proto_array.finalized_checkpoint, - nodes: proto_array.nodes.clone(), - indices: proto_array.indices.iter().map(|(k, v)| (*k, *v)).collect(), + nodes: proto_array.nodes.values().cloned().collect(), previous_proposer_boost: proto_array.previous_proposer_boost, } } @@ -57,8 +54,11 @@ impl TryFrom<(SszContainer, JustifiedBalances)> for ProtoArrayForkChoice { prune_threshold: from.prune_threshold, justified_checkpoint: from.justified_checkpoint, finalized_checkpoint: from.finalized_checkpoint, - nodes: from.nodes, - indices: from.indices.into_iter().collect::>(), + nodes: from + .nodes + .into_iter() + .map(|node| (node.root, node)) + .collect(), previous_proposer_boost: from.previous_proposer_boost, }; @@ -79,7 +79,6 @@ impl From for SszContainerV28 { justified_checkpoint: v17.justified_checkpoint, finalized_checkpoint: v17.finalized_checkpoint, nodes: v17.nodes, - indices: v17.indices, previous_proposer_boost: v17.previous_proposer_boost, } } @@ -95,7 +94,6 @@ impl From<(SszContainerV28, JustifiedBalances)> for SszContainerV17 { justified_checkpoint: v28.justified_checkpoint, finalized_checkpoint: v28.finalized_checkpoint, nodes: v28.nodes, - indices: v28.indices, previous_proposer_boost: v28.previous_proposer_boost, } }