From 6795aa42b233f10fae6d00b5e339fef509f5cee3 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Sat, 2 Mar 2019 15:52:33 +1100 Subject: [PATCH 1/3] Fix log_int implementation, removing floats The cast from f32::MAX to u32 was undefined behaviour, and the use of floating point logarithms would yield incorrect results due to rounding and truncation, e.g. for the integer 16777206 --- eth2/fork_choice/Cargo.toml | 1 - eth2/fork_choice/src/bitwise_lmd_ghost.rs | 29 ++++++++++++----------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/eth2/fork_choice/Cargo.toml b/eth2/fork_choice/Cargo.toml index 210f3c235..819b84055 100644 --- a/eth2/fork_choice/Cargo.toml +++ b/eth2/fork_choice/Cargo.toml @@ -8,7 +8,6 @@ edition = "2018" db = { path = "../../beacon_node/db" } ssz = { path = "../utils/ssz" } types = { path = "../types" } -fast-math = "0.1.1" log = "0.4.6" bit-vec = "0.5.0" diff --git a/eth2/fork_choice/src/bitwise_lmd_ghost.rs b/eth2/fork_choice/src/bitwise_lmd_ghost.rs index 1e66de079..58cd2b5a3 100644 --- a/eth2/fork_choice/src/bitwise_lmd_ghost.rs +++ b/eth2/fork_choice/src/bitwise_lmd_ghost.rs @@ -1,5 +1,5 @@ +//! The optimised bitwise LMD-GHOST fork choice rule. extern crate bit_vec; -extern crate fast_math; use crate::{ForkChoice, ForkChoiceError}; use bit_vec::BitVec; @@ -7,7 +7,6 @@ use db::{ stores::{BeaconBlockStore, BeaconStateStore}, ClientDB, }; -use fast_math::log2_raw; use log::{debug, trace}; use std::collections::HashMap; use std::sync::Arc; @@ -19,22 +18,17 @@ use types::{ //TODO: Pruning - Children //TODO: Handle Syncing -/// The optimised bitwise LMD-GHOST fork choice rule. -/// NOTE: This uses u32 to represent difference between block heights. Thus this is only -/// applicable for block height differences in the range of a u32. -/// This can potentially be parallelized in some parts. -// we use fast log2, a log2 lookup table is implemented in Vitaliks code, potentially do -// the comparison. Log2_raw takes 2ns according to the documentation. +// NOTE: This uses u32 to represent difference between block heights. Thus this is only +// applicable for block height differences in the range of a u32. +// This can potentially be parallelized in some parts. + +/// Compute the base-2 logarithm of an integer, floored (rounded down) #[inline] fn log2_int(x: u32) -> u32 { if x == 0 { return 0; } - assert!( - x <= std::f32::MAX as u32, - "Height too large for fast log in bitwise fork choice" - ); - log2_raw(x as f32) as u32 + 31 - x.leading_zeros() } fn power_of_2_below(x: u32) -> u32 { @@ -469,7 +463,6 @@ mod tests { #[test] pub fn test_power_of_2_below() { - println!("{:?}", std::f32::MAX); assert_eq!(power_of_2_below(4), 4); assert_eq!(power_of_2_below(5), 4); assert_eq!(power_of_2_below(7), 4); @@ -478,4 +471,12 @@ mod tests { assert_eq!(power_of_2_below(33), 32); assert_eq!(power_of_2_below(63), 32); } + + #[test] + pub fn test_power_of_2_below_large() { + let pow: u32 = 1 << 24; + for x in (pow - 20)..(pow + 20) { + assert!(power_of_2_below(x) <= x, "{}", x); + } + } } From 8a768819b0a832f323a6dc00c7e1f5149902aa14 Mon Sep 17 00:00:00 2001 From: mjkeating Date: Sat, 2 Mar 2019 09:59:01 -0800 Subject: [PATCH 2/3] brought algorithm in TreeHash macro to spec --- eth2/utils/ssz_derive/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/eth2/utils/ssz_derive/src/lib.rs b/eth2/utils/ssz_derive/src/lib.rs index ac66526fe..f71bff709 100644 --- a/eth2/utils/ssz_derive/src/lib.rs +++ b/eth2/utils/ssz_derive/src/lib.rs @@ -147,12 +147,12 @@ pub fn ssz_tree_hash_derive(input: TokenStream) -> TokenStream { let output = quote! { impl ssz::TreeHash for #name { fn hash_tree_root_internal(&self) -> Vec { - let mut result: Vec = vec![]; + let mut list: Vec> = Vec::new(); #( - result.append(&mut self.#field_idents.hash_tree_root_internal()); + list.push(self.#field_idents.hash_tree_root_internal()); )* - ssz::hash(&result) + ssz::merkle_hash(&mut list) } } }; From 60cfdf6e554ec59555518495f74d297eb221aef9 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Sun, 3 Mar 2019 13:35:15 +1100 Subject: [PATCH 3/3] Convert bitwise ghost to use u64 block heights. --- eth2/fork_choice/src/bitwise_lmd_ghost.rs | 18 +++++++++--------- eth2/types/src/slot_height.rs | 1 - 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/eth2/fork_choice/src/bitwise_lmd_ghost.rs b/eth2/fork_choice/src/bitwise_lmd_ghost.rs index 58cd2b5a3..60aa38fe7 100644 --- a/eth2/fork_choice/src/bitwise_lmd_ghost.rs +++ b/eth2/fork_choice/src/bitwise_lmd_ghost.rs @@ -24,22 +24,22 @@ use types::{ /// Compute the base-2 logarithm of an integer, floored (rounded down) #[inline] -fn log2_int(x: u32) -> u32 { +fn log2_int(x: u64) -> u32 { if x == 0 { return 0; } - 31 - x.leading_zeros() + 63 - x.leading_zeros() } -fn power_of_2_below(x: u32) -> u32 { - 2u32.pow(log2_int(x)) +fn power_of_2_below(x: u64) -> u64 { + 2u64.pow(log2_int(x)) } /// Stores the necessary data structures to run the optimised bitwise lmd ghost algorithm. pub struct BitwiseLMDGhost { /// A cache of known ancestors at given heights for a specific block. //TODO: Consider FnvHashMap - cache: HashMap, Hash256>, + cache: HashMap, Hash256>, /// Log lookup table for blocks to their ancestors. //TODO: Verify we only want/need a size 16 log lookup ancestors: Vec>, @@ -141,7 +141,7 @@ where } } // check if the result is stored in our cache - let cache_key = CacheKey::new(&block_hash, target_height.as_u32()); + let cache_key = CacheKey::new(&block_hash, target_height.as_u64()); if let Some(ancestor) = self.cache.get(&cache_key) { return Some(*ancestor); } @@ -149,7 +149,7 @@ where // not in the cache recursively search for ancestors using a log-lookup if let Some(ancestor) = { let ancestor_lookup = self.ancestors - [log2_int((block_height - target_height - 1u64).as_u32()) as usize] + [log2_int((block_height - target_height - 1u64).as_u64()) as usize] .get(&block_hash) //TODO: Panic if we can't lookup and fork choice fails .expect("All blocks should be added to the ancestor log lookup table"); @@ -374,7 +374,7 @@ impl ForkChoice for BitwiseLMDGhost { // logarithmic lookup blocks to see if there are obvious winners, if so, // progress to the next iteration. let mut step = - power_of_2_below(self.max_known_height.saturating_sub(block_height).as_u32()) / 2; + power_of_2_below(self.max_known_height.saturating_sub(block_height).as_u64()) / 2; while step > 0 { trace!("Current Step: {}", step); if let Some(clear_winner) = self.get_clear_winner( @@ -474,7 +474,7 @@ mod tests { #[test] pub fn test_power_of_2_below_large() { - let pow: u32 = 1 << 24; + let pow: u64 = 1 << 24; for x in (pow - 20)..(pow + 20) { assert!(power_of_2_below(x) <= x, "{}", x); } diff --git a/eth2/types/src/slot_height.rs b/eth2/types/src/slot_height.rs index afa0ff775..f9370f485 100644 --- a/eth2/types/src/slot_height.rs +++ b/eth2/types/src/slot_height.rs @@ -13,7 +13,6 @@ use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Rem, Sub, SubAssi pub struct SlotHeight(u64); impl_common!(SlotHeight); -impl_into_u32!(SlotHeight); // SlotHeight can be converted to u32 impl SlotHeight { pub fn new(slot: u64) -> SlotHeight {