Skip to content

Commit

Permalink
add new ef tests
Browse files Browse the repository at this point in the history
  • Loading branch information
realbigsean committed Nov 19, 2023
1 parent d683ef0 commit 76b56e2
Show file tree
Hide file tree
Showing 8 changed files with 224 additions and 25 deletions.
34 changes: 19 additions & 15 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4410,23 +4410,27 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
&self,
canonical_forkchoice_params: ForkchoiceUpdateParameters,
) -> Result<ForkchoiceUpdateParameters, Error> {
self.overridden_forkchoice_update_params_or_failure_reason(&canonical_forkchoice_params)
.or_else(|e| match e {
ProposerHeadError::DoNotReOrg(reason) => {
trace!(
self.log,
"Not suppressing fork choice update";
"reason" => %reason,
);
Ok(canonical_forkchoice_params)
}
ProposerHeadError::Error(e) => Err(e),
})
self.overridden_forkchoice_update_params_or_failure_reason(
&canonical_forkchoice_params,
false,
)
.or_else(|e| match e {
ProposerHeadError::DoNotReOrg(reason) => {
trace!(
self.log,
"Not suppressing fork choice update";
"reason" => %reason,
);
Ok(canonical_forkchoice_params)
}
ProposerHeadError::Error(e) => Err(e),
})
}

fn overridden_forkchoice_update_params_or_failure_reason(
pub fn overridden_forkchoice_update_params_or_failure_reason(
&self,
canonical_forkchoice_params: &ForkchoiceUpdateParameters,
testing: bool,
) -> Result<ForkchoiceUpdateParameters, ProposerHeadError<Error>> {
let _timer = metrics::start_timer(&metrics::FORK_CHOICE_OVERRIDE_FCU_TIMES);

Expand Down Expand Up @@ -4478,7 +4482,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
}

// Only attempt a re-org if we have a proposer registered for the re-org slot.
let proposing_at_re_org_slot = {
let proposing_at_re_org_slot = testing || {
// The proposer shuffling has the same decision root as the next epoch attestation
// shuffling. We know our re-org block is not on the epoch boundary, so it has the
// same proposer shuffling as the head (but not necessarily the parent which may lie
Expand Down Expand Up @@ -4533,7 +4537,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
// current slot, which would be necessary for determining its weight.
let head_block_late =
self.block_observed_after_attestation_deadline(head_block_root, head_slot);
if !head_block_late {
if !head_block_late && !testing {
return Err(DoNotReOrg::HeadNotLate.into());
}

Expand Down
2 changes: 1 addition & 1 deletion consensus/fork_choice/src/fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ pub enum AttestationFromBlock {
}

/// Parameters which are cached between calls to `ForkChoice::get_head`.
#[derive(Clone, Copy)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct ForkchoiceUpdateParameters {
/// The most recent result of running `ForkChoice::get_head`.
pub head_root: Hash256,
Expand Down
4 changes: 3 additions & 1 deletion consensus/fork_choice/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,6 @@ pub use crate::fork_choice::{
QueuedAttestation, ResetPayloadStatuses,
};
pub use fork_choice_store::ForkChoiceStore;
pub use proto_array::{Block as ProtoBlock, ExecutionStatus, InvalidationOperation};
pub use proto_array::{
Block as ProtoBlock, ExecutionStatus, InvalidationOperation, ProposerHeadError,
};
6 changes: 3 additions & 3 deletions consensus/proto_array/src/proto_array_fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ where
}

/// Information about the proposer head used for opportunistic re-orgs.
#[derive(Clone)]
#[derive(Debug, Clone)]
pub struct ProposerHeadInfo {
/// Information about the *current* head block, which may be re-orged.
pub head_node: ProtoNode,
Expand All @@ -206,7 +206,7 @@ pub struct ProposerHeadInfo {
///
/// This type intentionally does not implement `Debug` so that callers are forced to handle the
/// enum.
#[derive(Clone, PartialEq)]
#[derive(Debug, Clone, PartialEq)]
pub enum ProposerHeadError<E> {
DoNotReOrg(DoNotReOrg),
Error(E),
Expand Down Expand Up @@ -243,7 +243,7 @@ impl<E1> ProposerHeadError<E1> {
/// Reasons why a re-org should not be attempted.
///
/// This type intentionally does not implement `Debug` so that the `Display` impl must be used.
#[derive(Clone, PartialEq)]
#[derive(Debug, Clone, PartialEq)]
pub enum DoNotReOrg {
MissingHeadOrParentNode,
MissingHeadFinalizedCheckpoint,
Expand Down
82 changes: 81 additions & 1 deletion testing/ef_tests/src/cases/fork_choice.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use super::*;
use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file};
use ::fork_choice::PayloadVerificationStatus;
use ::fork_choice::{PayloadVerificationStatus, ProposerHeadError};
use beacon_chain::blob_verification::GossipBlobError;
use beacon_chain::chain_config::{
DisallowedReOrgOffsets, DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, DEFAULT_RE_ORG_THRESHOLD,
};
use beacon_chain::slot_clock::SlotClock;
use beacon_chain::{
attestation_verification::{
Expand Down Expand Up @@ -39,6 +42,13 @@ pub struct Head {
root: Hash256,
}

#[derive(Debug, Clone, Copy, PartialEq, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct ShouldOverrideFcu {
validator_is_connected: bool,
result: bool,
}

#[derive(Debug, Clone, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct Checks {
Expand All @@ -51,6 +61,8 @@ pub struct Checks {
u_justified_checkpoint: Option<Checkpoint>,
u_finalized_checkpoint: Option<Checkpoint>,
proposer_boost_root: Option<Hash256>,
get_proposer_head: Option<Hash256>,
should_override_forkchoice_update: Option<ShouldOverrideFcu>,
}

#[derive(Debug, Clone, Deserialize)]
Expand Down Expand Up @@ -257,6 +269,8 @@ impl<E: EthSpec> Case for ForkChoiceTest<E> {
u_justified_checkpoint,
u_finalized_checkpoint,
proposer_boost_root,
get_proposer_head,
should_override_forkchoice_update: should_override_fcu,
} = checks.as_ref();

if let Some(expected_head) = head {
Expand Down Expand Up @@ -295,6 +309,14 @@ impl<E: EthSpec> Case for ForkChoiceTest<E> {
if let Some(expected_proposer_boost_root) = proposer_boost_root {
tester.check_expected_proposer_boost_root(*expected_proposer_boost_root)?;
}

if let Some(expected_proposer_head) = get_proposer_head {
tester.check_expected_proposer_head(*expected_proposer_head)?;
}

if let Some(should_override_fcu) = should_override_fcu {
tester.check_should_override_fcu(*should_override_fcu)?;
}
}
}
}
Expand Down Expand Up @@ -707,6 +729,64 @@ impl<E: EthSpec> Tester<E> {
expected_proposer_boost_root,
)
}

pub fn check_expected_proposer_head(
&self,
expected_proposer_head: Hash256,
) -> Result<(), Error> {
let mut fc = self.harness.chain.canonical_head.fork_choice_write_lock();
let slot = self.harness.chain.slot().unwrap();
let canonical_head = fc.get_head(slot, &self.harness.spec).unwrap();
let proposer_head_result = fc.get_proposer_head(
slot,
canonical_head,
DEFAULT_RE_ORG_THRESHOLD,
&DisallowedReOrgOffsets::default(),
DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION,
);
let proposer_head = match proposer_head_result {
Ok(head) => head.parent_node.root,
Err(ProposerHeadError::DoNotReOrg(_)) => canonical_head,
_ => panic!("Unexpected error in get proposer head"),
};

check_equal("proposer_head", proposer_head, expected_proposer_head)
}

pub fn check_should_override_fcu(
&self,
expected_should_override_fcu: ShouldOverrideFcu,
) -> Result<(), Error> {
let canonical_fcu_params = self
.harness
.chain
.canonical_head
.cached_head()
.forkchoice_update_parameters();
let fcu_params_result = self
.harness
.chain
.overridden_forkchoice_update_params_or_failure_reason(&canonical_fcu_params, true);

let should_override = match fcu_params_result {
Ok(_) => true,
Err(ProposerHeadError::DoNotReOrg(_)) => false,
_ => panic!("Unexpected error in fcu override"),
};

let should_override_fcu = ShouldOverrideFcu {
// Testing this doesn't really make sense because we don't call `override_forkchoice_update_params`
// unless a validator is connected.
validator_is_connected: expected_should_override_fcu.validator_is_connected,
result: should_override,
};

check_equal(
"proposer_head",
should_override_fcu,
expected_should_override_fcu,
)
}
}

/// Checks that the `head` checkpoint from the beacon chain head matches the `fc` checkpoint gleaned
Expand Down
64 changes: 62 additions & 2 deletions testing/ef_tests/src/cases/merkle_proof_validity.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use super::*;
use crate::decode::{ssz_decode_state, yaml_decode_file};
use crate::decode::{ssz_decode_file, ssz_decode_state, yaml_decode_file};
use serde::Deserialize;
use std::path::Path;
use tree_hash::Hash256;
use types::{BeaconState, EthSpec, ForkName};
use types::{BeaconBlockBody, BeaconBlockBodyDeneb, BeaconState, EthSpec, ForkName};

#[derive(Debug, Clone, Deserialize)]
pub struct Metadata {
Expand Down Expand Up @@ -82,3 +82,63 @@ impl<E: EthSpec> Case for MerkleProofValidity<E> {
Ok(())
}
}

#[derive(Debug, Clone, Deserialize)]
#[serde(bound = "E: EthSpec")]
pub struct KzgInclusionMerkleProofValidity<E: EthSpec> {
pub metadata: Option<Metadata>,
pub block: BeaconBlockBody<E>,
pub merkle_proof: MerkleProof,
}

impl<E: EthSpec> LoadCase for KzgInclusionMerkleProofValidity<E> {
fn load_from_dir(path: &Path, _fork_name: ForkName) -> Result<Self, Error> {
//TODO(sean) make this work for all forks
let block = ssz_decode_file::<BeaconBlockBodyDeneb<E>>(&path.join("object.ssz_snappy"))?;
let merkle_proof = yaml_decode_file(&path.join("proof.yaml"))?;
// Metadata does not exist in these tests but it is left like this just in case.
let meta_path = path.join("meta.yaml");
let metadata = if meta_path.exists() {
Some(yaml_decode_file(&meta_path)?)
} else {
None
};

Ok(Self {
metadata,
block: block.into(),
merkle_proof,
})
}
}

impl<E: EthSpec> Case for KzgInclusionMerkleProofValidity<E> {
fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> {
let Ok(proof) = self.block.to_ref().kzg_commitment_merkle_proof(0) else {
return Err(Error::FailedToParseTest(
"Could not retrieve merkle proof".to_string(),
));
};
let proof_len = proof.len();
let branch_len = self.merkle_proof.branch.len();
if proof_len != branch_len {
return Err(Error::NotEqual(format!(
"Branches not equal in length computed: {}, expected {}",
proof_len, branch_len
)));
}

for (i, proof_leaf) in proof.iter().enumerate().take(proof_len) {
let expected_leaf = self.merkle_proof.branch[i];
if *proof_leaf != expected_leaf {
return Err(Error::NotEqual(format!(
"Leaves not equal in merke proof computed: {}, expected: {}",
hex::encode(proof_leaf),
hex::encode(expected_leaf)
)));
}
}

Ok(())
}
}
35 changes: 35 additions & 0 deletions testing/ef_tests/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,13 @@ impl<E: EthSpec + TypeName> Handler for ForkChoiceHandler<E> {
return false;
}

// No FCU override tests prior to bellatrix.
if self.handler_name == "should_override_forkchoice_update"
&& (fork_name == ForkName::Base || fork_name == ForkName::Altair)
{
return false;
}

// These tests check block validity (which may include signatures) and there is no need to
// run them with fake crypto.
cfg!(not(feature = "fake_crypto"))
Expand Down Expand Up @@ -786,6 +793,34 @@ impl<E: EthSpec + TypeName> Handler for MerkleProofValidityHandler<E> {
}
}

#[derive(Derivative)]
#[derivative(Default(bound = ""))]
pub struct KzgInclusionMerkleProofValidityHandler<E>(PhantomData<E>);

impl<E: EthSpec + TypeName> Handler for KzgInclusionMerkleProofValidityHandler<E> {
type Case = cases::KzgInclusionMerkleProofValidity<E>;

fn config_name() -> &'static str {
E::name()
}

fn runner_name() -> &'static str {
"merkle_proof"
}

fn handler_name(&self) -> String {
"single_merkle_proof".into()
}

fn is_enabled_for_fork(&self, fork_name: ForkName) -> bool {
// Enabled in Deneb
fork_name != ForkName::Base
&& fork_name != ForkName::Altair
&& fork_name != ForkName::Merge
&& fork_name != ForkName::Capella
}
}

#[derive(Derivative)]
#[derivative(Default(bound = ""))]
pub struct OperationsHandler<E, O>(PhantomData<(E, O)>);
Expand Down
22 changes: 20 additions & 2 deletions testing/ef_tests/tests/tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#![cfg(feature = "ef_tests")]

use ef_tests::*;
use types::*;
use ef_tests::{KzgInclusionMerkleProofValidityHandler, *};
use types::{MainnetEthSpec, MinimalEthSpec, *};

// Check that the hand-computed multiplications on EthSpec are correctly computed.
// This test lives here because one is most likely to muck these up during a spec update.
Expand Down Expand Up @@ -540,6 +540,18 @@ fn fork_choice_withholding() {
// There is no mainnet variant for this test.
}

#[test]
fn fork_choice_should_override_forkchoice_update() {
ForkChoiceHandler::<MinimalEthSpec>::new("should_override_forkchoice_update").run();
ForkChoiceHandler::<MainnetEthSpec>::new("should_override_forkchoice_update").run();
}

#[test]
fn fork_choice_get_proposer_head() {
ForkChoiceHandler::<MinimalEthSpec>::new("get_proposer_head").run();
ForkChoiceHandler::<MainnetEthSpec>::new("get_proposer_head").run();
}

#[test]
fn optimistic_sync() {
OptimisticSyncHandler::<MinimalEthSpec>::default().run();
Expand Down Expand Up @@ -592,6 +604,12 @@ fn merkle_proof_validity() {
MerkleProofValidityHandler::<MainnetEthSpec>::default().run();
}

#[test]
fn kzg_inclusion_merkle_proof_validity() {
KzgInclusionMerkleProofValidityHandler::<MainnetEthSpec>::default().run();
KzgInclusionMerkleProofValidityHandler::<MinimalEthSpec>::default().run();
}

#[test]
fn rewards() {
for handler in &["basic", "leak", "random"] {
Expand Down

0 comments on commit 76b56e2

Please sign in to comment.