From 98629ce74184ed09855d1658d459f4d0ce0ddfdb Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 1 Mar 2022 15:49:40 +1100 Subject: [PATCH] Several changes * Fix state cache pruning of finalized state from block map * Update to latest `milhouse` * Check beacon state diffs in EF tests --- Cargo.lock | 45 ++++++++++++++++++- .../beacon_chain/src/block_verification.rs | 7 ++- beacon_node/beacon_chain/tests/store_tests.rs | 1 - beacon_node/store/Cargo.toml | 3 +- beacon_node/store/src/chunked_vector.rs | 2 +- beacon_node/store/src/errors.rs | 9 +--- beacon_node/store/src/state_cache.rs | 5 +-- beacon_node/store/src/state_diff.rs | 32 ++++++++----- consensus/state_processing/src/genesis.rs | 2 +- .../src/per_epoch_processing/resets.rs | 2 +- consensus/types/src/beacon_state.rs | 13 +++--- testing/ef_tests/Cargo.toml | 1 + testing/ef_tests/src/case_result.rs | 19 +++++++- .../ef_tests/src/cases/epoch_processing.rs | 7 +-- testing/ef_tests/src/cases/operations.rs | 5 ++- testing/ef_tests/src/cases/sanity_blocks.rs | 7 ++- testing/ef_tests/src/cases/sanity_slots.rs | 5 ++- 17 files changed, 115 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d922a0d7a5..714d72ce43 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -638,6 +638,9 @@ name = "cc" version = "1.0.72" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "22a9137b95ea06864e018375b72adfb7db6e6f68cfc8df5a04d00288050485ee" +dependencies = [ + "jobserver", +] [[package]] name = "cexpr" @@ -1339,6 +1342,7 @@ dependencies = [ "fork_choice", "fs2", "hex", + "logging", "malloc_utils", "rayon", "serde", @@ -2753,6 +2757,15 @@ dependencies = [ "libc", ] +[[package]] +name = "jobserver" +version = "0.1.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af25a77299a7f711a01975c35a6a424eb6862092cc2d6c72c4ed6cbc56dfc1fa" +dependencies = [ + "libc", +] + [[package]] name = "js-sys" version = "0.3.56" @@ -5817,12 +5830,10 @@ name = "store" version = "0.2.0" dependencies = [ "beacon_chain", - "bincode", "db-key", "directory", "eth2_ssz", "eth2_ssz_derive 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", - "flate2", "itertools", "lazy_static", "leveldb", @@ -5839,6 +5850,7 @@ dependencies = [ "tempfile", "tree_hash", "types", + "zstd", ] [[package]] @@ -7286,3 +7298,32 @@ dependencies = [ "thiserror", "time 0.1.43", ] + +[[package]] +name = "zstd" +version = "0.10.0+zstd.1.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b1365becbe415f3f0fcd024e2f7b45bacfb5bdd055f0dc113571394114e7bdd" +dependencies = [ + "zstd-safe", +] + +[[package]] +name = "zstd-safe" +version = "4.1.4+zstd.1.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f7cd17c9af1a4d6c24beb1cc54b17e2ef7b593dc92f19e9d9acad8b182bbaee" +dependencies = [ + "libc", + "zstd-sys", +] + +[[package]] +name = "zstd-sys" +version = "1.6.3+zstd.1.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc49afa5c8d634e75761feda8c592051e7eeb4683ba827211eb0d731d3402ea8" +dependencies = [ + "cc", + "libc", +] diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index fdd347f144..fc80b5b3ec 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1077,10 +1077,9 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> { // Store the state immediately, marking it as temporary, and staging the deletion // of its temporary status as part of the larger atomic operation. let txn_lock = chain.store.hot_db.begin_rw_transaction(); - chain.store.do_atomically(vec![ - StoreOp::PutState(state_root, &state), - StoreOp::PutStateTemporaryFlag(state_root), - ])?; + chain + .store + .do_atomically(vec![StoreOp::PutState(state_root, &state)])?; drop(txn_lock); confirmation_db_batch.push(StoreOp::DeleteStateTemporaryFlag(state_root)); diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 3b37d95bef..f96b102a17 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -25,7 +25,6 @@ use store::{ HotColdDB, LevelDB, StoreConfig, }; use tempfile::{tempdir, TempDir}; -use tree_hash::TreeHash; use types::test_utils::{SeedableRng, XorShiftRng}; use types::*; diff --git a/beacon_node/store/Cargo.toml b/beacon_node/store/Cargo.toml index cf120a782c..adb42fc2a4 100644 --- a/beacon_node/store/Cargo.toml +++ b/beacon_node/store/Cargo.toml @@ -28,8 +28,7 @@ sloggers = { version = "2.1.1", features = ["json"] } directory = { path = "../../common/directory" } tree_hash = "0.4.0" take-until = "0.1.0" -flate2 = { version = "1.0.22", features = ["zlib"], default-features = false } -bincode = "1.3.3" +zstd = "0.10.0" [features] milhouse = ["state_processing/milhouse"] diff --git a/beacon_node/store/src/chunked_vector.rs b/beacon_node/store/src/chunked_vector.rs index c0b8572d43..b222d68532 100644 --- a/beacon_node/store/src/chunked_vector.rs +++ b/beacon_node/store/src/chunked_vector.rs @@ -543,7 +543,7 @@ pub fn load_variable_list_from_db, E: EthSpec, S: KeyV let chunks: Vec> = range_query(store, F::column(), start_cindex, end_cindex)?; - let mut result = VList::empty()?; + let mut result = VList::empty(); for (chunk_index, chunk) in chunks.into_iter().enumerate() { for (i, value) in chunk.values.into_iter().enumerate() { diff --git a/beacon_node/store/src/errors.rs b/beacon_node/store/src/errors.rs index 41dc0c0546..66c877b5c3 100644 --- a/beacon_node/store/src/errors.rs +++ b/beacon_node/store/src/errors.rs @@ -47,8 +47,7 @@ pub enum Error { BlockReplayError(BlockReplayError), #[cfg(feature = "milhouse")] MilhouseError(milhouse::Error), - Bincode(Box), - FlateCompression(std::io::Error), + Compression(std::io::Error), } pub trait HandleUnavailable { @@ -114,12 +113,6 @@ impl From for Error { } } -impl From> for Error { - fn from(e: Box) -> Self { - Self::Bincode(e) - } -} - #[derive(Debug)] pub struct DBError { pub message: String, diff --git a/beacon_node/store/src/state_cache.rs b/beacon_node/store/src/state_cache.rs index 223df9f0ba..fbdf83196c 100644 --- a/beacon_node/store/src/state_cache.rs +++ b/beacon_node/store/src/state_cache.rs @@ -95,8 +95,7 @@ impl StateCache { finalized_state.state_root == state_root }) { - // FIXME(sproul): this should technically be true - return Ok(false); + return Ok(true); } if self.states.peek(&state_root).is_some() { return Ok(true); @@ -167,7 +166,7 @@ impl BlockMap { self.blocks.retain(|_, slot_map| { slot_map.slots.retain(|slot, state_root| { - let keep = *slot > finalized_slot; + let keep = *slot >= finalized_slot; if !keep { pruned_states.insert(*state_root); } diff --git a/beacon_node/store/src/state_diff.rs b/beacon_node/store/src/state_diff.rs index 11d4b0262b..c98a567792 100644 --- a/beacon_node/store/src/state_diff.rs +++ b/beacon_node/store/src/state_diff.rs @@ -1,8 +1,18 @@ use crate::{metrics, DBColumn, Error, StoreItem}; -use flate2::bufread::{ZlibDecoder, ZlibEncoder}; use ssz::{Decode, Encode}; -use std::io::Read; +use std::io::{Read, Write}; use types::{beacon_state::BeaconStateDiff, EthSpec}; +use zstd::{Decoder, Encoder}; + +const EST_COMPRESSION_FACTOR: usize = 2; + +fn estimate_compressed_size(len: usize, compression_level: i32) -> usize { + if compression_level == 0 { + len + } else { + len / EST_COMPRESSION_FACTOR + } +} impl StoreItem for BeaconStateDiff { fn db_column() -> DBColumn { @@ -14,13 +24,13 @@ impl StoreItem for BeaconStateDiff { let value = self.as_ssz_bytes(); drop(encode_timer); - // FIXME(sproul): try vec with capacity let compression_timer = metrics::start_timer(&metrics::BEACON_STATE_DIFF_COMPRESSION_TIME); - let mut encoder = ZlibEncoder::new(&value[..], flate2::Compression::fast()); - let mut compressed_value = vec![]; - encoder - .read_to_end(&mut compressed_value) - .map_err(Error::FlateCompression)?; + + let level = 1; + let mut compressed_value = Vec::with_capacity(estimate_compressed_size(value.len(), level)); + let mut encoder = Encoder::new(&mut compressed_value, level).map_err(Error::Compression)?; + encoder.write_all(&value).map_err(Error::Compression)?; + encoder.finish().map_err(Error::Compression)?; drop(compression_timer); let compression_ratio = value.len() as f64 / compressed_value.len() as f64; @@ -39,11 +49,11 @@ impl StoreItem for BeaconStateDiff { } fn from_store_bytes(bytes: &[u8]) -> Result { - let mut ssz_bytes = vec![]; - let mut decoder = ZlibDecoder::new(bytes); + let mut ssz_bytes = Vec::with_capacity(EST_COMPRESSION_FACTOR * bytes.len()); + let mut decoder = Decoder::new(bytes).map_err(Error::Compression)?; decoder .read_to_end(&mut ssz_bytes) - .map_err(Error::FlateCompression)?; + .map_err(Error::Compression)?; Ok(Self::from_ssz_bytes(&ssz_bytes)?) } } diff --git a/consensus/state_processing/src/genesis.rs b/consensus/state_processing/src/genesis.rs index 6680c615db..274664816e 100644 --- a/consensus/state_processing/src/genesis.rs +++ b/consensus/state_processing/src/genesis.rs @@ -26,7 +26,7 @@ pub fn initialize_beacon_state_from_eth1( let mut state = BeaconState::new(genesis_time, eth1_data, spec); // Seed RANDAO with Eth1 entropy - state.fill_randao_mixes_with(eth1_block_hash); + state.fill_randao_mixes_with(eth1_block_hash)?; let mut deposit_tree = DepositDataTree::create(&[], 0, DEPOSIT_TREE_DEPTH); diff --git a/consensus/state_processing/src/per_epoch_processing/resets.rs b/consensus/state_processing/src/per_epoch_processing/resets.rs index 4a24a0c19e..8664bd98aa 100644 --- a/consensus/state_processing/src/per_epoch_processing/resets.rs +++ b/consensus/state_processing/src/per_epoch_processing/resets.rs @@ -13,7 +13,7 @@ pub fn process_eth1_data_reset( .safe_rem(T::SlotsPerEth1VotingPeriod::to_u64())? == 0 { - *state.eth1_data_votes_mut() = VList::empty()?; + *state.eth1_data_votes_mut() = VList::empty(); } Ok(()) } diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 4b298d1c5d..3f240c29b8 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -363,8 +363,8 @@ impl BeaconState { // History latest_block_header: BeaconBlock::::empty(spec).temporary_block_header(), - block_roots: FixedVector::from_elem(Hash256::zero()), - state_roots: FixedVector::from_elem(Hash256::zero()), + block_roots: FixedVector::default(), + state_roots: FixedVector::default(), historical_roots: VList::default(), // Eth1 @@ -377,10 +377,10 @@ impl BeaconState { balances: VList::default(), // Set later. // Randomness - randao_mixes: FixedVector::from_elem(Hash256::zero()), + randao_mixes: FixedVector::default(), // Slashings - slashings: FixedVector::from_elem(0), + slashings: FixedVector::default(), // Attestations previous_epoch_attestations: VList::default(), @@ -970,8 +970,9 @@ impl BeaconState { } /// Fill `randao_mixes` with - pub fn fill_randao_mixes_with(&mut self, index_root: Hash256) { - *self.randao_mixes_mut() = FixedVector::from_elem(index_root); + pub fn fill_randao_mixes_with(&mut self, index_root: Hash256) -> Result<(), Error> { + *self.randao_mixes_mut() = FixedVector::from_elem(index_root)?; + Ok(()) } /// Safely obtains the index for `randao_mixes` diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index 6407b8b54f..a9c87ed34d 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -36,3 +36,4 @@ beacon_chain = { path = "../../beacon_node/beacon_chain" } store = { path = "../../beacon_node/store" } fork_choice = { path = "../../consensus/fork_choice" } malloc_utils = { path = "../../common/malloc_utils" } +logging = { path = "../../common/logging" } diff --git a/testing/ef_tests/src/case_result.rs b/testing/ef_tests/src/case_result.rs index 935cb74215..e0ab308418 100644 --- a/testing/ef_tests/src/case_result.rs +++ b/testing/ef_tests/src/case_result.rs @@ -2,7 +2,7 @@ use super::*; use compare_fields::{CompareFields, Comparison, FieldComparison}; use std::fmt::Debug; use std::path::{Path, PathBuf}; -use types::BeaconState; +use types::{beacon_state::BeaconStateDiff, milhouse::diff::Diff, BeaconState}; pub const MAX_VALUE_STRING_LEN: usize = 500; @@ -118,6 +118,23 @@ where } } +pub fn check_state_diff( + pre_state: &BeaconState, + opt_post_state: &Option>, +) -> Result<(), Error> { + if let Some(post_state) = opt_post_state { + let diff = BeaconStateDiff::compute_diff(pre_state, post_state) + .expect("BeaconStateDiff should compute"); + let mut diffed_state = pre_state.clone(); + diff.apply_diff(&mut diffed_state) + .expect("BeaconStateDiff should apply"); + + compare_result_detailed::<_, ()>(&Ok(diffed_state), opt_post_state) + } else { + Ok(()) + } +} + fn fmt_val(val: T) -> String { let mut string = format!("{:?}", val); string.truncate(MAX_VALUE_STRING_LEN); diff --git a/testing/ef_tests/src/cases/epoch_processing.rs b/testing/ef_tests/src/cases/epoch_processing.rs index c52089a039..b64176cef0 100644 --- a/testing/ef_tests/src/cases/epoch_processing.rs +++ b/testing/ef_tests/src/cases/epoch_processing.rs @@ -1,6 +1,6 @@ use super::*; use crate::bls_setting::BlsSetting; -use crate::case_result::compare_beacon_state_results_without_caches; +use crate::case_result::{check_state_diff, compare_beacon_state_results_without_caches}; use crate::decode::{ssz_decode_state, yaml_decode_file}; use crate::type_name; use crate::type_name::TypeName; @@ -274,7 +274,7 @@ impl> Case for EpochProcessing { && T::name() != "inactivity_updates" && T::name() != "participation_flag_updates" } - ForkName::Altair | ForkName::Merge => true, // TODO: revisit when tests are out + ForkName::Altair | ForkName::Merge => true, } } @@ -293,6 +293,7 @@ impl> Case for EpochProcessing { T::run(&mut state, spec).map(|_| state) })(); - compare_beacon_state_results_without_caches(&mut result, &mut expected) + compare_beacon_state_results_without_caches(&mut result, &mut expected)?; + check_state_diff(&self.pre, &self.post) } } diff --git a/testing/ef_tests/src/cases/operations.rs b/testing/ef_tests/src/cases/operations.rs index d940c985d3..17ccf471f3 100644 --- a/testing/ef_tests/src/cases/operations.rs +++ b/testing/ef_tests/src/cases/operations.rs @@ -1,6 +1,6 @@ use super::*; use crate::bls_setting::BlsSetting; -use crate::case_result::compare_beacon_state_results_without_caches; +use crate::case_result::{check_state_diff, compare_beacon_state_results_without_caches}; use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; use crate::testing_spec; use crate::type_name::TypeName; @@ -335,6 +335,7 @@ impl> Case for Operations { .apply_to(&mut state, spec, self) .map(|()| state); - compare_beacon_state_results_without_caches(&mut result, &mut expected) + compare_beacon_state_results_without_caches(&mut result, &mut expected)?; + check_state_diff(&self.pre, &self.post) } } diff --git a/testing/ef_tests/src/cases/sanity_blocks.rs b/testing/ef_tests/src/cases/sanity_blocks.rs index 6c2890cd15..bc9439f64e 100644 --- a/testing/ef_tests/src/cases/sanity_blocks.rs +++ b/testing/ef_tests/src/cases/sanity_blocks.rs @@ -1,6 +1,6 @@ use super::*; use crate::bls_setting::BlsSetting; -use crate::case_result::compare_beacon_state_results_without_caches; +use crate::case_result::{check_state_diff, compare_beacon_state_results_without_caches}; use crate::decode::{ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; use serde_derive::Deserialize; use state_processing::{ @@ -129,6 +129,9 @@ impl Case for SanityBlocks { Ok(res) => (Ok(res.0), Ok(res.1)), }; compare_beacon_state_results_without_caches(&mut indiv_result, &mut expected)?; - compare_beacon_state_results_without_caches(&mut bulk_result, &mut expected) + compare_beacon_state_results_without_caches(&mut bulk_result, &mut expected)?; + check_state_diff(&self.pre, &self.post)?; + + Ok(()) } } diff --git a/testing/ef_tests/src/cases/sanity_slots.rs b/testing/ef_tests/src/cases/sanity_slots.rs index 93a05b3641..fade93d3af 100644 --- a/testing/ef_tests/src/cases/sanity_slots.rs +++ b/testing/ef_tests/src/cases/sanity_slots.rs @@ -1,6 +1,6 @@ use super::*; use crate::bls_setting::BlsSetting; -use crate::case_result::compare_beacon_state_results_without_caches; +use crate::case_result::{check_state_diff, compare_beacon_state_results_without_caches}; use crate::decode::{ssz_decode_state, yaml_decode_file}; use serde_derive::Deserialize; use state_processing::per_slot_processing; @@ -70,6 +70,7 @@ impl Case for SanitySlots { .try_for_each(|_| per_slot_processing(&mut state, None, spec).map(|_| ())) .map(|_| state); - compare_beacon_state_results_without_caches(&mut result, &mut expected) + compare_beacon_state_results_without_caches(&mut result, &mut expected)?; + check_state_diff(&self.pre, &self.post) } }