From d79e07902e8d3daf38fb79cd43436d513ca22675 Mon Sep 17 00:00:00 2001 From: Adam Szkoda Date: Thu, 21 May 2020 02:21:44 +0200 Subject: [PATCH] Relax PartialEq constraint on error enums (#1179) --- .../src/attestation_verification.rs | 2 +- .../beacon_chain/src/block_verification.rs | 2 +- beacon_node/beacon_chain/src/errors.rs | 4 +- beacon_node/beacon_chain/src/eth1_chain.rs | 2 +- beacon_node/beacon_chain/src/fork_choice.rs | 2 +- .../tests/attestation_verification.rs | 149 +++++++------ .../beacon_chain/tests/block_verification.rs | 201 ++++++++++-------- .../beacon_chain/tests/persistence_tests.rs | 6 +- beacon_node/beacon_chain/tests/store_tests.rs | 25 +-- beacon_node/beacon_chain/tests/tests.rs | 35 +-- beacon_node/store/src/errors.rs | 2 +- beacon_node/store/src/lib.rs | 8 +- 12 files changed, 251 insertions(+), 187 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 542576fa1..1ca106ce9 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -62,7 +62,7 @@ use types::{ /// other than `BeaconChainError`). /// - The application encountered an internal error whilst attempting to determine validity /// (the `BeaconChainError` variant) -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum Error { /// The attestation is from a slot that is later than the current slot (with respect to the /// gossip clock disparity). diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index cc929e530..a24026425 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -86,7 +86,7 @@ const WRITE_BLOCK_PROCESSING_SSZ: bool = cfg!(feature = "write_ssz_files"); /// /// - The block is malformed/invalid (indicated by all results other than `BeaconChainError`. /// - We encountered an error whilst trying to verify the block (a `BeaconChainError`). -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum BlockError { /// The parent block was unknown. ParentUnknown(Hash256), diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 17ef7c23f..de45f367b 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -26,7 +26,7 @@ macro_rules! easy_from_to { }; } -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum BeaconChainError { InsufficientValidators, UnableToReadSlot, @@ -87,7 +87,7 @@ easy_from_to!(ObservedBlockProducersError, BeaconChainError); easy_from_to!(BlockSignatureVerifierError, BeaconChainError); easy_from_to!(ArithError, BeaconChainError); -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum BlockProductionError { UnableToGetBlockRootFromState, UnableToReadSlot, diff --git a/beacon_node/beacon_chain/src/eth1_chain.rs b/beacon_node/beacon_chain/src/eth1_chain.rs index 5f4434fc5..47929fdee 100644 --- a/beacon_node/beacon_chain/src/eth1_chain.rs +++ b/beacon_node/beacon_chain/src/eth1_chain.rs @@ -19,7 +19,7 @@ use types::{ type BlockNumber = u64; type Eth1DataVoteCount = HashMap<(Eth1Data, BlockNumber), u64>; -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum Error { /// Unable to return an Eth1Data for the given epoch. EpochUnavailable, diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 9f4262ee4..6e1b912c1 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -13,7 +13,7 @@ use types::{BeaconBlock, BeaconState, BeaconStateError, Epoch, Hash256, IndexedA type Result = std::result::Result; -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum Error { MissingBlock(Hash256), MissingState(Hash256), diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 8f4d2da16..224339609 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -199,17 +199,19 @@ fn aggregated_gossip_verification() { get_valid_aggregated_attestation(&harness.chain, valid_attestation); macro_rules! assert_invalid { - ($desc: tt, $attn_getter: expr, $error: expr) => { - assert_eq!( - harness - .chain - .verify_aggregated_attestation_for_gossip($attn_getter) - .err() - .expect(&format!( - "{} should error during verify_aggregated_attestation_for_gossip", - $desc - )), - $error, + ($desc: tt, $attn_getter: expr, $($error: pat) |+ $( if $guard: expr )?) => { + assert!( + matches!( + harness + .chain + .verify_aggregated_attestation_for_gossip($attn_getter) + .err() + .expect(&format!( + "{} should error during verify_aggregated_attestation_for_gossip", + $desc + )), + $( $error ) |+ $( if $guard )? + ), "case: {}", $desc, ); @@ -235,10 +237,8 @@ fn aggregated_gossip_verification() { a.message.aggregate.data.slot = future_slot; a }, - AttnError::FutureSlot { - attestation_slot: future_slot, - latest_permissible_slot: current_slot, - } + AttnError::FutureSlot { attestation_slot, latest_permissible_slot } + if attestation_slot == future_slot && latest_permissible_slot == current_slot ); let early_slot = current_slot @@ -254,11 +254,12 @@ fn aggregated_gossip_verification() { a }, AttnError::PastSlot { - attestation_slot: early_slot, + attestation_slot, // Subtract an additional slot since the harness will be exactly on the start of the // slot and the propagation tolerance will allow an extra slot. - earliest_permissible_slot: current_slot - E::slots_per_epoch() - 1, + earliest_permissible_slot } + if attestation_slot == early_slot && earliest_permissible_slot == current_slot - E::slots_per_epoch() - 1 ); /* @@ -278,8 +279,9 @@ fn aggregated_gossip_verification() { a }, AttnError::UnknownHeadBlock { - beacon_block_root: unknown_root + beacon_block_root } + if beacon_block_root == unknown_root ); /* @@ -379,7 +381,8 @@ fn aggregated_gossip_verification() { a.message.aggregator_index = too_high_index; a }, - AttnError::ValidatorIndexTooHigh(too_high_index as usize) + AttnError::ValidatorIndexTooHigh(index) + if index == too_high_index as usize ); /* @@ -406,8 +409,9 @@ fn aggregated_gossip_verification() { // // However the following error is triggered first: AttnError::AggregatorNotInCommittee { - aggregator_index: unknown_validator + aggregator_index } + if aggregator_index == unknown_validator ); /* @@ -436,8 +440,9 @@ fn aggregated_gossip_verification() { ) }, AttnError::InvalidSelectionProof { - aggregator_index: non_aggregator_index as u64 + aggregator_index: index } + if index == non_aggregator_index as u64 ); assert!( @@ -464,7 +469,8 @@ fn aggregated_gossip_verification() { assert_invalid!( "aggregate with that has already been seen", valid_aggregate.clone(), - AttnError::AttestationAlreadyKnown(valid_aggregate.message.aggregate.tree_hash_root()) + AttnError::AttestationAlreadyKnown(hash) + if hash == valid_aggregate.message.aggregate.tree_hash_root() ); /* @@ -483,7 +489,8 @@ fn aggregated_gossip_verification() { a.message.aggregate.data.beacon_block_root = Hash256::from_low_u64_le(42); a }, - AttnError::AggregatorAlreadyKnown(aggregator_index as u64) + AttnError::AggregatorAlreadyKnown(index) + if index == aggregator_index as u64 ); } @@ -512,21 +519,23 @@ fn unaggregated_gossip_verification() { "the test requires a new epoch to avoid already-seen errors" ); - let (valid_attestation, validator_index, validator_committee_index, validator_sk) = + let (valid_attestation, expected_validator_index, validator_committee_index, validator_sk) = get_valid_unaggregated_attestation(&harness.chain); macro_rules! assert_invalid { - ($desc: tt, $attn_getter: expr, $error: expr) => { - assert_eq!( - harness - .chain - .verify_unaggregated_attestation_for_gossip($attn_getter) - .err() - .expect(&format!( - "{} should error during verify_unaggregated_attestation_for_gossip", - $desc - )), - $error, + ($desc: tt, $attn_getter: expr, $($error: pat) |+ $( if $guard: expr )?) => { + assert!( + matches!( + harness + .chain + .verify_unaggregated_attestation_for_gossip($attn_getter) + .err() + .expect(&format!( + "{} should error during verify_unaggregated_attestation_for_gossip", + $desc + )), + $( $error ) |+ $( if $guard )? + ), "case: {}", $desc, ); @@ -553,9 +562,10 @@ fn unaggregated_gossip_verification() { a }, AttnError::FutureSlot { - attestation_slot: future_slot, - latest_permissible_slot: current_slot, + attestation_slot, + latest_permissible_slot, } + if attestation_slot == future_slot && latest_permissible_slot == current_slot ); let early_slot = current_slot @@ -571,11 +581,12 @@ fn unaggregated_gossip_verification() { a }, AttnError::PastSlot { - attestation_slot: early_slot, + attestation_slot, // Subtract an additional slot since the harness will be exactly on the start of the // slot and the propagation tolerance will allow an extra slot. - earliest_permissible_slot: current_slot - E::slots_per_epoch() - 1, + earliest_permissible_slot, } + if attestation_slot == early_slot && earliest_permissible_slot == current_slot - E::slots_per_epoch() - 1 ); /* @@ -633,8 +644,9 @@ fn unaggregated_gossip_verification() { a }, AttnError::UnknownHeadBlock { - beacon_block_root: unknown_root + beacon_block_root, } + if beacon_block_root == unknown_root ); /* @@ -681,9 +693,10 @@ fn unaggregated_gossip_verification() { "attestation that has already been seen", valid_attestation.clone(), AttnError::PriorAttestationKnown { - validator_index: validator_index as u64, - epoch: current_epoch + validator_index, + epoch, } + if validator_index == expected_validator_index as u64 && epoch == current_epoch ); } @@ -719,7 +732,7 @@ fn fork_choice_verification() { ); let current_slot = chain.slot().expect("should get slot"); - let current_epoch = chain.epoch().expect("should get epoch"); + let expected_current_epoch = chain.epoch().expect("should get epoch"); let attestation = harness .chain @@ -727,17 +740,19 @@ fn fork_choice_verification() { .expect("precondition: should gossip verify attestation"); macro_rules! assert_invalid { - ($desc: tt, $attn_getter: expr, $error: expr) => { - assert_eq!( - harness - .chain - .apply_attestation_to_fork_choice(&$attn_getter) - .err() - .expect(&format!( - "{} should error during apply_attestation_to_fork_choice", - $desc - )), - $error, + ($desc: tt, $attn_getter: expr, $($error: pat) |+ $( if $guard: expr )?) => { + assert!( + matches!( + harness + .chain + .apply_attestation_to_fork_choice(&$attn_getter) + .err() + .expect(&format!( + "{} should error during apply_attestation_to_fork_choice", + $desc + )), + $( $error ) |+ $( if $guard )? + ), "case: {}", $desc, ); @@ -759,10 +774,10 @@ fn fork_choice_verification() { * * Spec v0.11.2 * - * assert target.epoch in [current_epoch, previous_epoch] + * assert target.epoch in [expected_current_epoch, previous_epoch] */ - let future_epoch = current_epoch + 1; + let future_epoch = expected_current_epoch + 1; assert_invalid!( "attestation from future epoch", { @@ -771,17 +786,18 @@ fn fork_choice_verification() { a }, AttnError::FutureEpoch { - attestation_epoch: future_epoch, - current_epoch + attestation_epoch, + current_epoch, } + if attestation_epoch == future_epoch && current_epoch == expected_current_epoch ); assert!( - current_epoch > 1, + expected_current_epoch > 1, "precondition: must be able to have a past epoch" ); - let past_epoch = current_epoch - 2; + let past_epoch = expected_current_epoch - 2; assert_invalid!( "attestation from past epoch", { @@ -790,9 +806,10 @@ fn fork_choice_verification() { a }, AttnError::PastEpoch { - attestation_epoch: past_epoch, - current_epoch + attestation_epoch, + current_epoch, } + if attestation_epoch == past_epoch && current_epoch == expected_current_epoch ); /* @@ -836,7 +853,7 @@ fn fork_choice_verification() { indexed.data.target.root = unknown_root; a }, - AttnError::UnknownTargetRoot(unknown_root) + AttnError::UnknownTargetRoot(hash) if hash == unknown_root ); // NOTE: we're not testing an assert from the spec: @@ -868,8 +885,9 @@ fn fork_choice_verification() { a }, AttnError::UnknownHeadBlock { - beacon_block_root: unknown_root + beacon_block_root } + if beacon_block_root == unknown_root ); let future_block = harness @@ -894,8 +912,9 @@ fn fork_choice_verification() { }, AttnError::AttestsToFutureBlock { block: current_slot, - attestation: current_slot - 1 + attestation: slot, } + if slot == current_slot - 1 ); // Note: we're not checking the "attestations can only affect the fork choice of subsequent diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 0aac2e4d4..9dae527c8 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -193,12 +193,14 @@ fn chain_segment_non_linear_parent_roots() { let mut blocks = chain_segment_blocks(); blocks.remove(2); - assert_eq!( - harness - .chain - .process_chain_segment(blocks.clone()) - .to_block_error(), - Err(BlockError::NonLinearParentRoots), + assert!( + matches!( + harness + .chain + .process_chain_segment(blocks.clone()) + .to_block_error(), + Err(BlockError::NonLinearParentRoots) + ), "should not import chain with missing parent" ); @@ -208,12 +210,14 @@ fn chain_segment_non_linear_parent_roots() { let mut blocks = chain_segment_blocks(); blocks[3].message.parent_root = Hash256::zero(); - assert_eq!( - harness - .chain - .process_chain_segment(blocks.clone()) - .to_block_error(), - Err(BlockError::NonLinearParentRoots), + assert!( + matches!( + harness + .chain + .process_chain_segment(blocks.clone()) + .to_block_error(), + Err(BlockError::NonLinearParentRoots) + ), "should not import chain with a broken parent root link" ); } @@ -233,12 +237,14 @@ fn chain_segment_non_linear_slots() { let mut blocks = chain_segment_blocks(); blocks[3].message.slot = Slot::new(0); - assert_eq!( - harness - .chain - .process_chain_segment(blocks.clone()) - .to_block_error(), - Err(BlockError::NonLinearSlots), + assert!( + matches!( + harness + .chain + .process_chain_segment(blocks.clone()) + .to_block_error(), + Err(BlockError::NonLinearSlots) + ), "should not import chain with a parent that has a lower slot than its child" ); @@ -249,12 +255,14 @@ fn chain_segment_non_linear_slots() { let mut blocks = chain_segment_blocks(); blocks[3].message.slot = blocks[2].message.slot; - assert_eq!( - harness - .chain - .process_chain_segment(blocks.clone()) - .to_block_error(), - Err(BlockError::NonLinearSlots), + assert!( + matches!( + harness + .chain + .process_chain_segment(blocks.clone()) + .to_block_error(), + Err(BlockError::NonLinearSlots) + ), "should not import chain with a parent that has an equal slot to its child" ); } @@ -297,19 +305,23 @@ fn invalid_signatures() { .collect(); // Ensure the block will be rejected if imported in a chain segment. - assert_eq!( - harness.chain.process_chain_segment(blocks).to_block_error(), - Err(BlockError::InvalidSignature), + assert!( + matches!( + harness.chain.process_chain_segment(blocks).to_block_error(), + Err(BlockError::InvalidSignature) + ), "should not import chain segment with an invalid {} signature", item ); // Ensure the block will be rejected if imported on its own (without gossip checking). - assert_eq!( - harness - .chain - .process_block(snapshots[block_index].beacon_block.clone()), - Err(BlockError::InvalidSignature), + assert!( + matches!( + harness + .chain + .process_block(snapshots[block_index].beacon_block.clone()), + Err(BlockError::InvalidSignature) + ), "should not import individual block with an invalid {} signature", item ); @@ -332,17 +344,21 @@ fn invalid_signatures() { .map(|snapshot| snapshot.beacon_block.clone()) .collect(); // Ensure the block will be rejected if imported in a chain segment. - assert_eq!( - harness.chain.process_chain_segment(blocks).to_block_error(), - Err(BlockError::InvalidSignature), + assert!( + matches!( + harness.chain.process_chain_segment(blocks).to_block_error(), + Err(BlockError::InvalidSignature) + ), "should not import chain segment with an invalid gossip signature", ); // Ensure the block will be rejected if imported on its own (without gossip checking). - assert_eq!( - harness - .chain - .process_block(snapshots[block_index].beacon_block.clone()), - Err(BlockError::InvalidSignature), + assert!( + matches!( + harness + .chain + .process_block(snapshots[block_index].beacon_block.clone()), + Err(BlockError::InvalidSignature) + ), "should not import individual block with an invalid gossip signature", ); @@ -467,8 +483,10 @@ fn invalid_signatures() { .map(|snapshot| snapshot.beacon_block.clone()) .collect(); assert!( - harness.chain.process_chain_segment(blocks).to_block_error() - != Err(BlockError::InvalidSignature), + !matches!( + harness.chain.process_chain_segment(blocks).to_block_error(), + Err(BlockError::InvalidSignature) + ), "should not throw an invalid signature error for a bad deposit signature" ); @@ -543,14 +561,17 @@ fn block_gossip_verification() { */ let mut block = CHAIN_SEGMENT[block_index].beacon_block.clone(); - let block_slot = block.message.slot + 1; - block.message.slot = block_slot; - assert_eq!( - unwrap_err(harness.chain.verify_block_for_gossip(block)), - BlockError::FutureSlot { - present_slot: block_slot - 1, - block_slot - }, + let expected_block_slot = block.message.slot + 1; + block.message.slot = expected_block_slot; + assert!( + matches!( + unwrap_err(harness.chain.verify_block_for_gossip(block)), + BlockError::FutureSlot { + present_slot, + block_slot, + } + if present_slot == expected_block_slot - 1 && block_slot == expected_block_slot + ), "should not import a block with a future slot" ); @@ -567,20 +588,23 @@ fn block_gossip_verification() { */ let mut block = CHAIN_SEGMENT[block_index].beacon_block.clone(); - let finalized_slot = harness + let expected_finalized_slot = harness .chain .head_info() .expect("should get head info") .finalized_checkpoint .epoch .start_slot(E::slots_per_epoch()); - block.message.slot = finalized_slot; - assert_eq!( - unwrap_err(harness.chain.verify_block_for_gossip(block)), - BlockError::WouldRevertFinalizedSlot { - block_slot: finalized_slot, - finalized_slot - }, + block.message.slot = expected_finalized_slot; + assert!( + matches!( + unwrap_err(harness.chain.verify_block_for_gossip(block)), + BlockError::WouldRevertFinalizedSlot { + block_slot, + finalized_slot, + } + if block_slot == expected_finalized_slot && finalized_slot == expected_finalized_slot + ), "should not import a block with a finalized slot" ); @@ -595,9 +619,11 @@ fn block_gossip_verification() { let mut block = CHAIN_SEGMENT[block_index].beacon_block.clone(); block.signature = junk_signature(); - assert_eq!( - unwrap_err(harness.chain.verify_block_for_gossip(block)), - BlockError::ProposalSignatureInvalid, + assert!( + matches!( + unwrap_err(harness.chain.verify_block_for_gossip(block)), + BlockError::ProposalSignatureInvalid + ), "should not import a block with an invalid proposal signature" ); @@ -625,21 +651,27 @@ fn block_gossip_verification() { harness.chain.genesis_validators_root, &harness.chain.spec, ); - assert_eq!( - unwrap_err(harness.chain.verify_block_for_gossip(block.clone())), - BlockError::IncorrectBlockProposer { - block: other_proposer, - local_shuffling: expected_proposer - }, + assert!( + matches!( + unwrap_err(harness.chain.verify_block_for_gossip(block.clone())), + BlockError::IncorrectBlockProposer { + block, + local_shuffling, + } + if block == other_proposer && local_shuffling == expected_proposer + ), "should not import a block with the wrong proposer index" ); // Check to ensure that we registered this is a valid block from this proposer. - assert_eq!( - unwrap_err(harness.chain.verify_block_for_gossip(block.clone())), - BlockError::RepeatProposal { - proposer: other_proposer, - slot: block.message.slot - }, + assert!( + matches!( + unwrap_err(harness.chain.verify_block_for_gossip(block.clone())), + BlockError::RepeatProposal { + proposer, + slot, + } + if proposer == other_proposer && slot == block.message.slot + ), "should register any valid signature against the proposer, even if the block failed later verification" ); @@ -659,16 +691,19 @@ fn block_gossip_verification() { */ let block = CHAIN_SEGMENT[block_index].beacon_block.clone(); - assert_eq!( - harness - .chain - .verify_block_for_gossip(block.clone()) - .err() - .expect("should error when processing known block"), - BlockError::RepeatProposal { - proposer: block.message.proposer_index, - slot: block.message.slot, - }, + assert!( + matches!( + harness + .chain + .verify_block_for_gossip(block.clone()) + .err() + .expect("should error when processing known block"), + BlockError::RepeatProposal { + proposer, + slot, + } + if proposer == block.message.proposer_index && slot == block.message.slot + ), "the second proposal by this validator should be rejected" ); } diff --git a/beacon_node/beacon_chain/tests/persistence_tests.rs b/beacon_node/beacon_chain/tests/persistence_tests.rs index e51815bfe..5fa063a73 100644 --- a/beacon_node/beacon_chain/tests/persistence_tests.rs +++ b/beacon_node/beacon_chain/tests/persistence_tests.rs @@ -143,7 +143,11 @@ fn finalizes_after_resuming_from_db() { fn assert_chains_pretty_much_the_same(a: &BeaconChain, b: &BeaconChain) { assert_eq!(a.spec, b.spec, "spec should be equal"); assert_eq!(a.op_pool, b.op_pool, "op_pool should be equal"); - assert_eq!(a.head(), b.head(), "head() should be equal"); + assert_eq!( + a.head().unwrap(), + b.head().unwrap(), + "head() should be equal" + ); assert_eq!(a.heads(), b.heads(), "heads() should be equal"); assert_eq!( a.genesis_block_root, b.genesis_block_root, diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 3f88b0d73..c08edfcfe 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -317,21 +317,22 @@ fn epoch_boundary_state_attestation_processing() { .verify_unaggregated_attestation_for_gossip(attestation.clone()); let current_slot = harness.chain.slot().expect("should get slot"); - let attestation_slot = attestation.data.slot; + let expected_attestation_slot = attestation.data.slot; // Extra -1 to handle gossip clock disparity. - let earliest_permissible_slot = current_slot - E::slots_per_epoch() - 1; + let expected_earliest_permissible_slot = current_slot - E::slots_per_epoch() - 1; - if attestation_slot <= finalized_epoch.start_slot(E::slots_per_epoch()) - || attestation_slot < earliest_permissible_slot + if expected_attestation_slot <= finalized_epoch.start_slot(E::slots_per_epoch()) + || expected_attestation_slot < expected_earliest_permissible_slot { checked_pre_fin = true; - assert_eq!( + assert!(matches!( res.err().unwrap(), AttnError::PastSlot { attestation_slot, earliest_permissible_slot, } - ); + if attestation_slot == expected_attestation_slot && earliest_permissible_slot == expected_earliest_permissible_slot + )); } else { res.expect("should have verified attetation"); } @@ -397,13 +398,13 @@ fn delete_blocks_and_states() { // Delete faulty fork // Attempting to load those states should find them unavailable for (state_root, slot) in &states_to_delete { - assert_eq!(store.delete_state(state_root, *slot), Ok(())); - assert_eq!(store.get_state(state_root, Some(*slot)), Ok(None)); + store.delete_state(state_root, *slot).unwrap(); + assert_eq!(store.get_state(state_root, Some(*slot)).unwrap(), None); } // Double-deleting should also be OK (deleting non-existent things is fine) for (state_root, slot) in &states_to_delete { - assert_eq!(store.delete_state(state_root, *slot), Ok(())); + store.delete_state(state_root, *slot).unwrap(); } // Deleting the blocks from the fork should remove them completely @@ -413,8 +414,8 @@ fn delete_blocks_and_states() { .collect::>(); for (block_root, _) in blocks_to_delete { - assert_eq!(store.delete_block(&block_root), Ok(())); - assert_eq!(store.get_block(&block_root), Ok(None)); + store.delete_block(&block_root).unwrap(); + assert_eq!(store.get_block(&block_root).unwrap(), None); } // Deleting frozen states should do nothing @@ -426,7 +427,7 @@ fn delete_blocks_and_states() { .filter(|(_, slot)| *slot < split_slot); for (state_root, slot) in finalized_states { - assert_eq!(store.delete_state(&state_root, slot), Ok(())); + store.delete_state(&state_root, slot).unwrap(); } // After all that, the chain dump should still be OK diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index bcbe3641d..0cc89d123 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -453,17 +453,19 @@ fn attestations_with_increasing_slots() { .verify_unaggregated_attestation_for_gossip(attestation.clone()); let current_slot = harness.chain.slot().expect("should get slot"); - let attestation_slot = attestation.data.slot; - let earliest_permissible_slot = current_slot - MinimalEthSpec::slots_per_epoch() - 1; + let expected_attestation_slot = attestation.data.slot; + let expected_earliest_permissible_slot = + current_slot - MinimalEthSpec::slots_per_epoch() - 1; - if attestation_slot < earliest_permissible_slot { - assert_eq!( + if expected_attestation_slot < expected_earliest_permissible_slot { + assert!(matches!( res.err().unwrap(), AttnError::PastSlot { attestation_slot, earliest_permissible_slot, } - ) + if attestation_slot == expected_attestation_slot && earliest_permissible_slot == expected_earliest_permissible_slot + )) } else { res.expect("should process attestation"); } @@ -558,19 +560,22 @@ fn run_skip_slot_test(skip_slots: u64) { ); assert_eq!( - harness_b.chain.process_block( - harness_a - .chain - .head() - .expect("should get head") - .beacon_block - .clone(), - ), - Ok(harness_a + harness_b + .chain + .process_block( + harness_a + .chain + .head() + .expect("should get head") + .beacon_block + .clone(), + ) + .unwrap(), + harness_a .chain .head() .expect("should get head") - .beacon_block_root) + .beacon_block_root ); harness_b diff --git a/beacon_node/store/src/errors.rs b/beacon_node/store/src/errors.rs index 9e22530d2..49c4b5144 100644 --- a/beacon_node/store/src/errors.rs +++ b/beacon_node/store/src/errors.rs @@ -3,7 +3,7 @@ use crate::hot_cold_store::HotColdDBError; use ssz::DecodeError; use types::{BeaconStateError, Hash256}; -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum Error { SszDecodeError(DecodeError), VectorChunkError(ChunkError), diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index 494c8977f..b1d622292 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -337,20 +337,20 @@ mod tests { let key = Hash256::random(); let item = StorableThing { a: 1, b: 42 }; - assert_eq!(store.exists::(&key), Ok(false)); + assert_eq!(store.exists::(&key).unwrap(), false); store.put(&key, &item).unwrap(); - assert_eq!(store.exists::(&key), Ok(true)); + assert_eq!(store.exists::(&key).unwrap(), true); let retrieved = store.get(&key).unwrap().unwrap(); assert_eq!(item, retrieved); store.delete::(&key).unwrap(); - assert_eq!(store.exists::(&key), Ok(false)); + assert_eq!(store.exists::(&key).unwrap(), false); - assert_eq!(store.get::(&key), Ok(None)); + assert_eq!(store.get::(&key).unwrap(), None); } #[test]