Skip to content

Commit

Permalink
v0.8: updates to make the EF tests pass
Browse files Browse the repository at this point in the history
* Remove redundant max operation checks.
* Always supply both messages when checking attestation signatures -- allowing
  verification of an attestation with no signatures.
* Swap the order of the fork and domain constant in `get_domain`, to match
  the spec.
  • Loading branch information
michaelsproul committed Jul 26, 2019
1 parent d0301ba commit 81b42f3
Show file tree
Hide file tree
Showing 9 changed files with 33 additions and 68 deletions.
26 changes: 0 additions & 26 deletions eth2/state_processing/src/per_block_processing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,6 @@ pub fn process_proposer_slashings<T: EthSpec>(
proposer_slashings: &[ProposerSlashing],
spec: &ChainSpec,
) -> Result<(), Error> {
// TODO(freeze): this check is kind of redundant with the VariableList impl
verify!(
proposer_slashings.len() <= T::MaxProposerSlashings::to_usize(),
Invalid::MaxProposerSlashingsExceeded
);

// Verify proposer slashings in parallel.
proposer_slashings
.par_iter()
Expand Down Expand Up @@ -257,11 +251,6 @@ pub fn process_attester_slashings<T: EthSpec>(
attester_slashings: &[AttesterSlashing<T>],
spec: &ChainSpec,
) -> Result<(), Error> {
verify!(
attester_slashings.len() <= T::MaxAttesterSlashings::to_usize(),
Invalid::MaxAttesterSlashingsExceed
);

// Verify the `IndexedAttestation`s in parallel (these are the resource-consuming objects, not
// the `AttesterSlashing`s themselves).
let mut indexed_attestations: Vec<&_> = Vec::with_capacity(attester_slashings.len() * 2);
Expand Down Expand Up @@ -314,11 +303,6 @@ pub fn process_attestations<T: EthSpec>(
attestations: &[Attestation<T>],
spec: &ChainSpec,
) -> Result<(), Error> {
verify!(
attestations.len() <= T::MaxAttestations::to_usize(),
Invalid::MaxAttestationsExceeded
);

// Ensure the previous epoch cache exists.
state.build_committee_cache(RelativeEpoch::Previous, spec)?;

Expand Down Expand Up @@ -441,11 +425,6 @@ pub fn process_exits<T: EthSpec>(
voluntary_exits: &[VoluntaryExit],
spec: &ChainSpec,
) -> Result<(), Error> {
verify!(
voluntary_exits.len() <= T::MaxVoluntaryExits::to_usize(),
Invalid::MaxExitsExceeded
);

// Verify exits in parallel.
voluntary_exits
.par_iter()
Expand Down Expand Up @@ -479,11 +458,6 @@ pub fn process_transfers<T: EthSpec>(
Invalid::DuplicateTransfers
);

verify!(
transfers.len() <= T::MaxTransfers::to_usize(),
Invalid::MaxTransfersExceed
);

transfers
.par_iter()
.enumerate()
Expand Down
3 changes: 3 additions & 0 deletions eth2/state_processing/src/per_block_processing/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ pub enum BlockInvalid {
DepositProcessingFailed(usize),
ExitInvalid(usize, ExitInvalid),
TransferInvalid(usize, TransferInvalid),
// NOTE: this is only used in tests, normally a state root mismatch is handled
// in the beacon_chain rather than in state_processing
StateRootMismatch,
}

impl From<ssz_types::Error> for BlockProcessingError {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,8 @@ fn is_valid_indexed_attestation_signature<T: EthSpec>(
}
.tree_hash_root();

let mut messages = vec![];
let mut keys = vec![];

if !indexed_attestation.custody_bit_0_indices.is_empty() {
messages.push(&message_0[..]);
keys.push(&bit_0_pubkey);
}
if !indexed_attestation.custody_bit_1_indices.is_empty() {
messages.push(&message_1[..]);
keys.push(&bit_1_pubkey);
}
let messages = vec![&message_0[..], &message_1[..]];
let keys = vec![&bit_0_pubkey, &bit_1_pubkey];

let domain = spec.get_domain(
indexed_attestation.data.target.epoch,
Expand Down
1 change: 1 addition & 0 deletions eth2/types/src/beacon_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ where
pub randao_mixes: FixedVector<Hash256, T::EpochsPerHistoricalVector>,
#[compare_fields(as_slice)]
active_index_roots: FixedVector<Hash256, T::EpochsPerHistoricalVector>,
#[compare_fields(as_slice)]
compact_committees_roots: FixedVector<Hash256, T::EpochsPerHistoricalVector>,

// Slashings
Expand Down
10 changes: 5 additions & 5 deletions eth2/types/src/chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ pub struct ChainSpec {
impl ChainSpec {
/// Get the domain number that represents the fork meta and signature domain.
///
/// Spec v0.6.3
/// Spec v0.8.1
pub fn get_domain(&self, epoch: Epoch, domain: Domain, fork: &Fork) -> u64 {
let domain_constant = match domain {
Domain::BeaconProposer => self.domain_beacon_proposer,
Expand All @@ -109,8 +109,8 @@ impl ChainSpec {
Domain::Transfer => self.domain_transfer,
};

let mut bytes: Vec<u8> = fork.get_fork_version(epoch).to_vec();
bytes.append(&mut int_to_bytes4(domain_constant));
let mut bytes: Vec<u8> = int_to_bytes4(domain_constant);
bytes.append(&mut fork.get_fork_version(epoch).to_vec());

let mut fork_and_domain = [0; 8];
fork_and_domain.copy_from_slice(&bytes);
Expand Down Expand Up @@ -237,8 +237,8 @@ mod tests {

let domain = spec.get_domain(epoch, domain_type, &fork);

let mut expected = fork.get_fork_version(epoch).to_vec();
expected.append(&mut int_to_bytes4(raw_domain));
let mut expected = int_to_bytes4(raw_domain);
expected.append(&mut fork.get_fork_version(epoch).to_vec());

assert_eq!(int_to_bytes8(domain), expected);
}
Expand Down
2 changes: 2 additions & 0 deletions eth2/types/src/test_utils/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ macro_rules! ssz_tests {
macro_rules! cached_tree_hash_tests {
($type: ty) => {
#[test]
#[ignore]
// FIXME: re-enable https://github.com/sigp/lighthouse/issues/440
pub fn test_cached_tree_hash() {
use crate::test_utils::{SeedableRng, TestRandom, XorShiftRng};
use tree_hash::TreeHash;
Expand Down
6 changes: 0 additions & 6 deletions tests/ef_tests/src/cases/bls_g2_compressed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,6 @@ impl YamlDecode for BlsG2Compressed {

impl Case for BlsG2Compressed {
fn result(&self, _case_index: usize) -> Result<(), Error> {
// FIXME: re-enable in v0.7
// https://github.com/ethereum/eth2.0-spec-tests/issues/3
if _case_index == 4 {
return Err(Error::SkippedKnownFailure);
}

// Convert message and domain to required types
let msg = hex::decode(&self.input.message[2..])
.map_err(|e| Error::FailedToParseTest(format!("{:?}", e)))?;
Expand Down
26 changes: 13 additions & 13 deletions tests/ef_tests/src/cases/sanity_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use super::*;
use crate::bls_setting::BlsSetting;
use crate::case_result::compare_beacon_state_results_without_caches;
use serde_derive::Deserialize;
use state_processing::{per_block_processing, per_slot_processing};
use state_processing::{
per_block_processing, per_slot_processing, BlockInvalid, BlockProcessingError,
};
use types::{BeaconBlock, BeaconState, EthSpec, RelativeEpoch};

#[derive(Debug, Clone, Deserialize)]
Expand All @@ -26,19 +28,9 @@ impl<E: EthSpec> Case for SanityBlocks<E> {
self.description.clone()
}

fn result(&self, case_index: usize) -> Result<(), Error> {
fn result(&self, _case_index: usize) -> Result<(), Error> {
self.bls_setting.unwrap_or_default().check()?;

// FIXME: re-enable these tests in v0.7
let known_failures = vec![
0, // attestation: https://github.com/ethereum/eth2.0-spec-tests/issues/6
10, // transfer: https://github.com/ethereum/eth2.0-spec-tests/issues/7
11, // voluntary exit: signature is invalid, don't know why
];
if known_failures.contains(&case_index) {
return Err(Error::SkippedKnownFailure);
}

let mut state = self.pre.clone();
let mut expected = self.post.clone();
let spec = &E::default_spec();
Expand All @@ -58,7 +50,15 @@ impl<E: EthSpec> Case for SanityBlocks<E> {
.build_committee_cache(RelativeEpoch::Current, spec)
.unwrap();

per_block_processing(&mut state, block, spec)
per_block_processing(&mut state, block, spec)?;

if block.state_root == state.canonical_root() {
Ok(())
} else {
Err(BlockProcessingError::Invalid(
BlockInvalid::StateRootMismatch,
))
}
})
.map(|_| state);

Expand Down
14 changes: 7 additions & 7 deletions tests/ef_tests/src/cases/ssz_static.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
use super::*;
use crate::case_result::compare_result;
use cached_tree_hash::{CachedTreeHash, TreeHashCache};
use cached_tree_hash::CachedTreeHash;
use serde_derive::Deserialize;
use ssz::{Decode, Encode};
use std::fmt::Debug;
use std::marker::PhantomData;
use tree_hash::TreeHash;
use types::{
test_utils::{SeedableRng, TestRandom, XorShiftRng},
Attestation, AttestationData, AttestationDataAndCustodyBit, AttesterSlashing, BeaconBlock,
BeaconBlockBody, BeaconBlockHeader, BeaconState, Checkpoint, CompactCommittee, Crosslink,
Deposit, DepositData, Eth1Data, EthSpec, Fork, Hash256, HistoricalBatch, IndexedAttestation,
PendingAttestation, ProposerSlashing, Transfer, Validator, VoluntaryExit,
test_utils::TestRandom, Attestation, AttestationData, AttestationDataAndCustodyBit,
AttesterSlashing, BeaconBlock, BeaconBlockBody, BeaconBlockHeader, BeaconState, Checkpoint,
CompactCommittee, Crosslink, Deposit, DepositData, Eth1Data, EthSpec, Fork, Hash256,
HistoricalBatch, IndexedAttestation, PendingAttestation, ProposerSlashing, Transfer, Validator,
VoluntaryExit,
};

// Enum variant names are used by Serde when deserializing the test YAML
Expand Down Expand Up @@ -64,7 +64,7 @@ impl<E: EthSpec + serde::de::DeserializeOwned> YamlDecode for SszStatic<E> {
}

impl<E: EthSpec> Case for SszStatic<E> {
fn result(&self, case_index: usize) -> Result<(), Error> {
fn result(&self, _case_index: usize) -> Result<(), Error> {
use self::SszStatic::*;

match *self {
Expand Down

0 comments on commit 81b42f3

Please sign in to comment.