Skip to content

Commit

Permalink
Address clippy lints in tree-states (sigp#4414)
Browse files Browse the repository at this point in the history
* Address some clippy lints

* Box errors to fix error size lint

* Add Default impl for Validator

* Address more clippy lints

* Re-implement `check_state_diff`

* Fix misc test compile errors
  • Loading branch information
paulhauner authored Jun 20, 2023
1 parent 23db089 commit d56cec8
Show file tree
Hide file tree
Showing 26 changed files with 94 additions and 73 deletions.
2 changes: 1 addition & 1 deletion beacon_node/beacon_chain/src/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
.max_by_key(|f| f.finalized_checkpoint.epoch);

// Do a bit of state reconstruction first if required.
if let Some(_) = reconstruction_notif {
if reconstruction_notif.is_some() {
let timer = std::time::Instant::now();

match db.reconstruct_historic_states(Some(BLOCKS_PER_RECONSTRUCTION)) {
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/beacon_chain/src/sync_committee_rewards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
.zip(sync_aggregate.sync_committee_bits.iter())
{
let participant_balance = balances
.get_mut(&validator_index)
.get_mut(validator_index)
.ok_or(BeaconChainError::SyncCommitteeRewardsSyncError)?;

if participant_bit {
Expand Down
2 changes: 0 additions & 2 deletions beacon_node/beacon_chain/tests/store_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ use store::{
HotColdDB, LevelDB, StoreConfig,
};
use tempfile::{tempdir, TempDir};
use tokio::time::sleep;
use tree_hash::TreeHash;
use types::test_utils::{SeedableRng, XorShiftRng};
use types::*;

Expand Down
8 changes: 4 additions & 4 deletions beacon_node/execution_layer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ use tokio::{
};
use tokio_stream::wrappers::WatchStream;
use tree_hash::TreeHash;
use types::{AbstractExecPayload, BeaconStateError, ExecPayload, Withdrawals};
use types::{AbstractExecPayload, BeaconStateError, ExecPayload};
use types::{
BlindedPayload, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ExecutionPayload,
ExecutionPayloadCapella, ExecutionPayloadMerge, ForkName, ForkVersionedResponse,
ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock, Slot, Uint256,
BlindedPayload, BlockType, ChainSpec, Epoch, ExecutionPayloadCapella, ExecutionPayloadMerge,
ForkVersionedResponse, ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock,
Slot,
};

mod block_hash;
Expand Down
3 changes: 2 additions & 1 deletion beacon_node/genesis/src/interop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,8 @@ mod test {
}

for (index, v) in state.validators().iter().enumerate() {
let creds = v.withdrawal_credentials.as_bytes();
let withdrawal_credientials = v.withdrawal_credentials();
let creds = withdrawal_credientials.as_bytes();
if index % 2 == 0 {
assert_eq!(
creds[0], spec.bls_withdrawal_prefix_byte,
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/operation_pool/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1249,7 +1249,7 @@ mod release_tests {
// Each validator will have a multiple of 1_000_000_000 wei.
// Safe from overflow unless there are about 18B validators (2^64 / 1_000_000_000).
for i in 0..state.validators().len() {
state.validators_mut()[i].effective_balance = 1_000_000_000 * i as u64;
state.validators_mut().get_mut(i).unwrap().effective_balance = 1_000_000_000 * i as u64;
}

let num_validators = num_committees
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/store/src/forwards_iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ impl<'a, E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>>
// of the pre iterator.
None => {
let continuation_data = continuation_data.take();
let start_slot = Slot::from(iter.limit);
let start_slot = iter.limit;

*self = PostFinalizationLazy {
continuation_data,
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/store/src/hdiff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ mod tests {

let xor_diff = XorDiff::compute(&x_values, &y_values).unwrap();

let mut y_from_xor = x_values.clone();
let mut y_from_xor = x_values;
xor_diff.apply(&mut y_from_xor).unwrap();

assert_eq!(y_values, y_from_xor);
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/store/src/hot_cold_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1573,7 +1573,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> HotColdDB<E, Hot, Cold>
(start_slot.as_u64()..=end_slot.as_u64())
.map(Slot::new)
.map(|slot| self.get_cold_blinded_block_by_slot(slot)),
|iter| iter.filter_map(|x| x).collect(),
|iter| iter.flatten().collect(),
)
}

Expand Down
2 changes: 1 addition & 1 deletion beacon_node/store/src/leveldb_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ impl<E: EthSpec> KeyValueStore<E> for LevelDB<E> {
}

fn iter_column_from<K: Key>(&self, column: DBColumn, from: &[u8]) -> ColumnIter<K> {
let start_key = BytesKey::from_vec(get_key_for_col(column.into(), &from));
let start_key = BytesKey::from_vec(get_key_for_col(column.into(), from));

let iter = self.db.iter(self.read_options());
iter.seek(&start_key);
Expand Down
4 changes: 2 additions & 2 deletions consensus/fork_choice/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ impl ForkChoiceTest {
let state_root = harness
.chain
.store
.get_blinded_block(&fc.fc_store().justified_checkpoint().root)
.get_blinded_block(&fc.fc_store().justified_checkpoint().root, None)
.unwrap()
.unwrap()
.message()
Expand All @@ -361,7 +361,7 @@ impl ForkChoiceTest {
.into_iter()
.map(|v| {
if v.is_active_at(state.current_epoch()) {
v.effective_balance
v.effective_balance()
} else {
0
}
Expand Down
6 changes: 4 additions & 2 deletions consensus/state_processing/src/per_block_processing/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,11 @@ pub enum AttestationInvalid {
///
/// `is_current` is `true` if the attestation was compared to the
/// `state.current_justified_checkpoint`, `false` if compared to `state.previous_justified_checkpoint`.
///
/// Checkpoints have been boxed to keep the error size down and prevent clippy failures.
WrongJustifiedCheckpoint {
state: Checkpoint,
attestation: Checkpoint,
state: Box<Checkpoint>,
attestation: Box<Checkpoint>,
is_current: bool,
},
/// The aggregation bitfield length is not the smallest possible size to represent the committee.
Expand Down
4 changes: 2 additions & 2 deletions consensus/state_processing/src/per_block_processing/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,8 +451,8 @@ async fn invalid_attestation_wrong_justified_checkpoint() {
Err(BlockProcessingError::AttestationInvalid {
index: 0,
reason: AttestationInvalid::WrongJustifiedCheckpoint {
state: old_justified_checkpoint,
attestation: new_justified_checkpoint,
state: Box::new(old_justified_checkpoint),
attestation: Box::new(new_justified_checkpoint),
is_current: true,
}
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ fn verify_casper_ffg_vote<T: EthSpec>(
verify!(
data.source == state.current_justified_checkpoint(),
Invalid::WrongJustifiedCheckpoint {
state: state.current_justified_checkpoint(),
attestation: data.source,
state: Box::new(state.current_justified_checkpoint()),
attestation: Box::new(data.source),
is_current: true,
}
);
Expand All @@ -103,8 +103,8 @@ fn verify_casper_ffg_vote<T: EthSpec>(
verify!(
data.source == state.previous_justified_checkpoint(),
Invalid::WrongJustifiedCheckpoint {
state: state.previous_justified_checkpoint(),
attestation: data.source,
state: Box::new(state.previous_justified_checkpoint()),
attestation: Box::new(data.source),
is_current: false,
}
);
Expand Down
12 changes: 5 additions & 7 deletions consensus/types/src/beacon_state/tests.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
#![cfg(test)]
use crate::{test_utils::*, ForkName};
use beacon_chain::test_utils::{
interop_genesis_state_with_eth1, test_spec, BeaconChainHarness, EphemeralHarnessType,
DEFAULT_ETH1_BLOCK_HASH,
};
use beacon_chain::test_utils::{BeaconChainHarness, EphemeralHarnessType};
use beacon_chain::types::{
test_utils::TestRandom, BeaconState, BeaconStateAltair, BeaconStateBase, BeaconStateError,
BeaconStateMerge, ChainSpec, Domain, Epoch, EthSpec, FixedVector, Hash256, Keypair,
MainnetEthSpec, MinimalEthSpec, RelativeEpoch, Slot,
test_utils::TestRandom, BeaconState, BeaconStateAltair, BeaconStateBase, BeaconStateCapella,
BeaconStateError, BeaconStateMerge, ChainSpec, Domain, Epoch, EthSpec, FixedVector, Hash256,
Keypair, MainnetEthSpec, MinimalEthSpec, RelativeEpoch, Slot,
};
use ssz::Encode;
use std::ops::Mul;
Expand Down Expand Up @@ -418,6 +415,7 @@ fn check_num_fields_pow2() {
ForkName::Base => BeaconStateBase::<E>::NUM_FIELDS,
ForkName::Altair => BeaconStateAltair::<E>::NUM_FIELDS,
ForkName::Merge => BeaconStateMerge::<E>::NUM_FIELDS,
ForkName::Capella => BeaconStateCapella::<E>::NUM_FIELDS,
};
assert_eq!(
num_fields.next_power_of_two(),
Expand Down
11 changes: 9 additions & 2 deletions consensus/types/src/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,15 @@ impl Validator {
}
}

/*
impl Default for Validator {
fn default() -> Self {
Validator {
pubkey: Arc::new(PublicKeyBytes::empty()),
mutable: <_>::default(),
}
}
}

impl Default for ValidatorMutable {
fn default() -> Self {
ValidatorMutable {
Expand All @@ -244,7 +252,6 @@ impl Default for ValidatorMutable {
}
}
}
*/

impl TreeHash for Validator {
fn tree_hash_type() -> tree_hash::TreeHashType {
Expand Down
29 changes: 16 additions & 13 deletions lcli/src/new_testnet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@ use std::fs::File;
use std::io::Read;
use std::path::PathBuf;
use std::str::FromStr;
use std::sync::Arc;
use std::time::{SystemTime, UNIX_EPOCH};
use types::ExecutionBlockHash;
use types::{
test_utils::generate_deterministic_keypairs, Address, BeaconState, ChainSpec, Config, Epoch,
Eth1Data, EthSpec, ExecutionPayloadHeader, ExecutionPayloadHeaderCapella,
ExecutionPayloadHeaderMerge, ExecutionPayloadHeaderRefMut, ForkName, Hash256, Keypair,
PublicKey, Validator,
PublicKey, Validator, ValidatorMutable,
};

pub fn run<T: EthSpec>(testnet_dir_path: PathBuf, matches: &ArgMatches) -> Result<(), String> {
Expand Down Expand Up @@ -233,7 +234,7 @@ fn initialize_state_with_validators<T: EthSpec>(
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).unwrap();

for keypair in keypairs.iter() {
let withdrawal_credentials = |pubkey: &PublicKey| {
Expand All @@ -244,17 +245,19 @@ fn initialize_state_with_validators<T: EthSpec>(
let amount = spec.max_effective_balance;
// Create a new validator.
let validator = Validator {
pubkey: keypair.0.pk.clone().into(),
withdrawal_credentials: withdrawal_credentials(&keypair.1.pk),
activation_eligibility_epoch: spec.far_future_epoch,
activation_epoch: spec.far_future_epoch,
exit_epoch: spec.far_future_epoch,
withdrawable_epoch: spec.far_future_epoch,
effective_balance: std::cmp::min(
amount - amount % (spec.effective_balance_increment),
spec.max_effective_balance,
),
slashed: false,
pubkey: Arc::new(keypair.0.pk.clone().into()),
mutable: ValidatorMutable {
withdrawal_credentials: withdrawal_credentials(&keypair.1.pk),
activation_eligibility_epoch: spec.far_future_epoch,
activation_epoch: spec.far_future_epoch,
exit_epoch: spec.far_future_epoch,
withdrawable_epoch: spec.far_future_epoch,
effective_balance: std::cmp::min(
amount - amount % (spec.effective_balance_increment),
spec.max_effective_balance,
),
slashed: false,
},
};
state.validators_mut().push(validator).unwrap();
state.balances_mut().push(amount).unwrap();
Expand Down
8 changes: 3 additions & 5 deletions lcli/src/state_diff.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
use crate::transition_blocks::load_from_ssz_with;
use clap::ArgMatches;
use clap_utils::{parse_optional, parse_required};
use clap_utils::parse_required;
use environment::Environment;
use eth2::{types::BlockId, BeaconNodeHttpClient, SensitiveUrl, Timeouts};
use std::path::PathBuf;
use std::time::{Duration, Instant};
use store::hdiff::{HDiff, HDiffBuffer};
use types::{BeaconState, EthSpec, FullPayload, SignedBeaconBlock};
use types::{BeaconState, EthSpec};

pub fn run<T: EthSpec>(env: Environment<T>, matches: &ArgMatches) -> Result<(), String> {
pub fn run<T: EthSpec>(_env: Environment<T>, matches: &ArgMatches) -> Result<(), String> {
let state1_path: PathBuf = parse_required(matches, "state1")?;
let state2_path: PathBuf = parse_required(matches, "state2")?;
let spec = &T::default_spec();
Expand Down
2 changes: 1 addition & 1 deletion lcli/src/transition_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ use eth2::{
use ssz::Encode;
use state_processing::{
block_signature_verifier::BlockSignatureVerifier, per_block_processing, per_slot_processing,
BlockSignatureStrategy, ConsensusContext, StateProcessingStrategy, VerifyBlockRoot,
BlockSignatureStrategy, ConsensusContext, EpochCache, StateProcessingStrategy, VerifyBlockRoot,
};
use std::borrow::Cow;
use std::fs::File;
Expand Down
28 changes: 21 additions & 7 deletions testing/ef_tests/src/case_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use super::*;
use compare_fields::{CompareFields, Comparison, FieldComparison};
use std::fmt::Debug;
use std::path::{Path, PathBuf};
use types::{beacon_state::BeaconStateDiff, milhouse::diff::Diff, BeaconState};
use store::hdiff::{HDiff, HDiffBuffer};
use types::BeaconState;

pub const MAX_VALUE_STRING_LEN: usize = 500;

Expand Down Expand Up @@ -121,15 +122,28 @@ where
pub fn check_state_diff<T: EthSpec>(
pre_state: &BeaconState<T>,
opt_post_state: &Option<BeaconState<T>>,
spec: &ChainSpec,
) -> 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");
// Produce a diff between the pre- and post-states.
let pre_state_buf = HDiffBuffer::from_state(pre_state.clone());
let post_state_buf = HDiffBuffer::from_state(post_state.clone());
let diff = HDiff::compute(&pre_state_buf, &post_state_buf).expect("HDiff should compute");

compare_result_detailed::<_, ()>(&Ok(diffed_state), opt_post_state)
// Apply the diff to the pre-state, ensuring the same post-state is
// regenerated.
let mut reconstructed_buf = HDiffBuffer::from_state(pre_state.clone());
diff.apply(&mut reconstructed_buf)
.expect("HDiff should apply");
let diffed_state = reconstructed_buf
.into_state(spec)
.expect("HDiffDiffer should convert to state");

// Drop the caches on the post-state to assist with equality checking.
let mut post_state_without_caches = post_state.clone();
post_state_without_caches.drop_all_caches().unwrap();

compare_result_detailed::<_, ()>(&Ok(diffed_state), &Some(post_state_without_caches))
} else {
Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion testing/ef_tests/src/cases/epoch_processing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ impl<E: EthSpec, T: EpochTransition<E>> Case for EpochProcessing<E, T> {

let mut result = T::run(&mut state, spec).map(|_| state);

check_state_diff(&pre_state, &expected)?;
check_state_diff(&pre_state, &expected, spec)?;
compare_beacon_state_results_without_caches(&mut result, &mut expected)
}
}
4 changes: 2 additions & 2 deletions testing/ef_tests/src/cases/operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ impl<E: EthSpec, O: Operation<E>> Case for Operations<E, O> {
// NOTE: some of the withdrawals tests have 0 active validators, do not try
// to build the commitee cache in this case.
if O::handler_name() != "withdrawals" {
state.build_all_committee_caches(spec).unwrap();
pre_state.build_all_committee_caches(spec).unwrap();
}

let mut state = pre_state.clone();
Expand All @@ -475,7 +475,7 @@ impl<E: EthSpec, O: Operation<E>> Case for Operations<E, O> {
.apply_to(&mut state, spec, self)
.map(|()| state);

check_state_diff(&pre_state, &expected)?;
check_state_diff(&pre_state, &expected, spec)?;
compare_beacon_state_results_without_caches(&mut result, &mut expected)
}
}
2 changes: 1 addition & 1 deletion testing/ef_tests/src/cases/sanity_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ impl<E: EthSpec> Case for SanityBlocks<E> {
post.build_all_committee_caches(spec).unwrap();
post
});
check_state_diff(&pre, &post)?;
check_state_diff(&pre, &post, spec)?;
Ok(())
}
}
2 changes: 1 addition & 1 deletion testing/ef_tests/src/cases/sanity_slots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,6 @@ impl<E: EthSpec> Case for SanitySlots<E> {
post.build_all_committee_caches(spec).unwrap();
post
});
check_state_diff(&pre, &post)
check_state_diff(&pre, &post, spec)
}
}
Loading

0 comments on commit d56cec8

Please sign in to comment.