Skip to content

Commit

Permalink
Fixes for passing tests:
Browse files Browse the repository at this point in the history 
- Update `consensus-spec-tests` test vector submodule to v1.4.0 and implement fixes in fork choice.
- Remove several unnecessary fork choice extra tests.
- Slashing protection tests:
  Test block proposals and attestations against `should_succeed_complete` from test vectors,
  since we have a complete slashing protection DB, not a minimal one.
  • Loading branch information
@povi
povi committed Mar 22, 2024
1 parent 32c32e1 commit c4beb15
Show file tree
Hide file tree
Showing 12 changed files with 37 additions and 395 deletions.
1 change: 0 additions & 1 deletion Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion builder_api/src/api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ fn validate_phase(computed: Phase, in_response: Phase) -> Result<()> {
}

#[cfg(test)]
#[cfg(feature = "eth2-cache")]
mod tests {
use eth2_cache_utils::mainnet;
use reqwest::{Client, Url};
Expand Down Expand Up @@ -320,7 +321,6 @@ mod tests {
43, [42, 21, 0] => Err(BuilderApiError::RollingEpochMissingBlocks { missing_blocks: 30 });
"more missing blocks than allowed in first gap alone"
)]
#[cfg(feature = "eth2-cache")]
fn circuit_breaker_conditions(
slot: Slot,
nonempty_slots: impl IntoIterator<Item = Slot>,
Expand Down
2 changes: 1 addition & 1 deletion consensus-spec-tests
Submodule consensus-spec-tests updated 6 files
+0 −3 tests/minimal/deneb/transition/core/pyspec_tests/higher_churn_limit_to_lower__without_block/meta.yaml
+ − tests/minimal/deneb/transition/core/pyspec_tests/higher_churn_limit_to_lower__without_block/post.ssz_snappy
+ − tests/minimal/deneb/transition/core/pyspec_tests/higher_churn_limit_to_lower__without_block/pre.ssz_snappy
+0 −3 tests/minimal/eip6110/transition/core/pyspec_tests/higher_churn_limit_to_lower__without_block/meta.yaml
+ − tests/minimal/eip6110/transition/core/pyspec_tests/higher_churn_limit_to_lower__without_block/post.ssz_snappy
+ − tests/minimal/eip6110/transition/core/pyspec_tests/higher_churn_limit_to_lower__without_block/pre.ssz_snappy
1 change: 0 additions & 1 deletion fork_choice_control/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ eth2_cache_utils = { workspace = true }
factory = { workspace = true }
fs-err = { workspace = true }
serde-aux = { workspace = true }
serde_json = { workspace = true }
spec_test_utils = { workspace = true }
tap = { workspace = true }
test-generator = { workspace = true }
Expand Down
316 changes: 13 additions & 303 deletions fork_choice_control/src/extra_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,29 @@
#![allow(clippy::too_many_lines)]

use core::ops::Not as _;
use std::{path::PathBuf, sync::Arc};
#[cfg(feature = "eth2-cache")]
use std::sync::Arc;

use anyhow::Result;
use eth2_cache_utils::{mainnet, medalla, withdrawal_devnet_3};
#[cfg(feature = "eth2-cache")]
use eth2_cache_utils::medalla;
#[cfg(feature = "eth2-cache")]
use eth2_libp2p::GossipId;
use execution_engine::PayloadStatusV1;
use fork_choice_store::PayloadStatus;
use helper_functions::misc;
#[cfg(feature = "eth2-cache")]
use std_ext::ArcExt as _;
#[cfg(feature = "eth2-cache")]
use types::{config::Config, preset::Medalla};
use types::{
combined::SignedBeaconBlock,
config::Config,
phase0::{consts::GENESIS_SLOT, primitives::H256},
preset::{Medalla, Minimal},
preset::Minimal,
traits::SignedBeaconBlock as _,
};

use crate::{
helpers::{is_at_start_of_epoch, start_of_epoch, Context, Status},
specialized::TestController,
};
use crate::helpers::{is_at_start_of_epoch, start_of_epoch, Context, Status};

#[cfg(feature = "eth2-cache")]
use crate::specialized::TestController;

// This test was added to reproduce the bug described in
// <https://github.com/ethereum/consensus-specs/issues/1887>.
Expand Down Expand Up @@ -87,180 +89,6 @@ fn processes_blocks_from_medalla_in_their_slots() {
}
}

// The blocks were not actually invalid.
// We had missed the change to the type of `WithdrawalV1.amount` in `execution-apis`.
#[test]
#[cfg(feature = "eth2-cache")]
fn handles_invalid_blocks_from_withdrawal_devnet_3() -> Result<()> {
let config = Arc::new(Config::withdrawal_devnet_3());

let genesis_block = withdrawal_devnet_3::GENESIS_BEACON_BLOCK
.force()
.clone_arc();

let genesis_state = withdrawal_devnet_3::GENESIS_BEACON_STATE
.force()
.clone_arc();

let last_slot = 672;
let last_valid_slot = 643;
let justified_slot = 640;
let blocks = withdrawal_devnet_3::beacon_blocks(1..=last_slot, 6);

let index_of_slot = |slot| {
blocks
.binary_search_by_key(&slot, |block| block.message().slot())
.unwrap_or_else(|_| panic!("withdrawal-devnet-3 should have a block at slot {slot}"))
};

let last_valid_index = index_of_slot(last_valid_slot);
let justified_index = index_of_slot(justified_slot);
let last_valid_block = &blocks[last_valid_index];
let justified_block = &blocks[justified_index];
let (valid_blocks, invalid_blocks) = blocks.split_at(last_valid_index + 1);
let (invalid_blocks, poison_blocks) = invalid_blocks.split_at(invalid_blocks.len() - 1);

let (controller, _mutator_handle) = TestController::quiet(config, genesis_block, genesis_state);

let mut json_directory = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
assert!(json_directory.pop());
json_directory.push("eth2-cache/withdrawals_devnet_3/json");

let add_block = |block: &Arc<SignedBeaconBlock<_>>| -> Result<()> {
let block_root = block.message().hash_tree_root();
let path = json_directory.join(format!("payload_response_block_root_{block_root:?}.json"));
let bytes = fs_err::read(path)?;
let payload_status = serde_json::from_slice::<PayloadStatusV1>(bytes.as_slice())?;

let execution_block_hash = block
.execution_block_hash()
.expect("every block from withdrawal-devnet-3 should be post-Bellatrix");

controller.on_slot(block.message().slot());
controller.on_gossip_block(block.clone_arc(), GossipId::default());
controller.on_notified_new_payload(execution_block_hash, payload_status);
controller.wait_for_tasks();

Ok(())
};

let assert_head = |block: &Arc<SignedBeaconBlock<_>>| {
let head = controller.head().value;

assert_eq!(head.slot(), block.message().slot());
assert_eq!(head.block_root, block.message().hash_tree_root());
};

for block in valid_blocks {
add_block(block)?;
assert_head(block);
}

assert_head(last_valid_block);

for block in invalid_blocks {
add_block(block)?;
assert_head(last_valid_block);
}

for block in poison_blocks {
add_block(block)?;
assert_head(justified_block);
}

Ok(())
}

// This test was added to verify the comment about anchors above `Store::is_block_viable`.
// We weren't sure how long the store would remain without viable forks after the changes to
// viability checking in `consensus-specs` version 1.3.0-rc.4.
// The test revealed a bug in `Store::apply_tick` that was making the mutator panic.
// `Store::head` was being called while the `Store` was in an inconsistent state.
// That made `assert!(no_viable_segments)` fail, though the end result may have been unaffected.
#[test]
#[cfg(feature = "eth2-cache")]
fn handles_blocks_after_non_genesis_anchor_in_their_slots() {
let config = Arc::new(Config::mainnet());
let anchor_state = mainnet::ALTAIR_BEACON_STATE.force().clone_arc();
let anchor_block = mainnet::ALTAIR_BEACON_BLOCK.force().clone_arc();

let (viable_block, non_viable_blocks) = mainnet::ALTAIR_BEACON_BLOCKS_FROM_32_SLOTS
.force()
.split_last()
.expect("range should contain at least one block");

let (controller, _mutator_handle) =
TestController::quiet(config, anchor_block.clone_arc(), anchor_state);

let add_block = |block: &Arc<SignedBeaconBlock<_>>| {
controller.on_slot(block.message().slot());
controller.on_gossip_block(block.clone_arc(), GossipId::default());
controller.wait_for_tasks();
};

let assert_head = |block: &Arc<SignedBeaconBlock<_>>| {
let head = controller.head().value;

assert_eq!(head.slot(), block.message().slot());
assert_eq!(head.block_root, block.message().hash_tree_root());
};

for block in non_viable_blocks {
add_block(block);
assert_head(&anchor_block);
}

add_block(viable_block);
assert_head(viable_block);
}

#[test]
#[cfg(feature = "eth2-cache")]
fn handles_blocks_after_non_genesis_anchor_in_a_future_slot() {
let config = Arc::new(Config::mainnet());
let anchor_state = mainnet::ALTAIR_BEACON_STATE.force().clone_arc();
let anchor_block = mainnet::ALTAIR_BEACON_BLOCK.force().clone_arc();
let supermajority_block_index = 22;

let (non_viable_blocks, viable_blocks) = mainnet::ALTAIR_BEACON_BLOCKS_FROM_32_SLOTS
.force()
.split_at(supermajority_block_index);

let last_slot = viable_blocks
.last()
.expect("range should contain at least one block")
.message()
.slot();

let (controller, _mutator_handle) =
TestController::quiet(config, anchor_block.clone_arc(), anchor_state);

let add_block = |block: &Arc<SignedBeaconBlock<_>>| {
controller.on_requested_block(block.clone_arc(), None);
controller.wait_for_tasks();
};

let assert_head = |block: &Arc<SignedBeaconBlock<_>>| {
let head = controller.head().value;

assert_eq!(head.slot(), block.message().slot());
assert_eq!(head.block_root, block.message().hash_tree_root());
};

controller.on_slot(last_slot);
controller.wait_for_tasks();

for block in non_viable_blocks {
add_block(block);
assert_head(&anchor_block);
}

for block in viable_blocks {
add_block(block);
assert_head(block);
}
}

// Based on the only fork choice test we had in the repository from 2019.
#[test]
fn handles_happy_path_with_3_blocks_and_height_difference_of_1() {
Expand Down Expand Up @@ -510,124 +338,6 @@ fn best_child_is_updated_when_it_falls_behind_the_2nd_best_one() {
});
}

// ```text
// 0
// / \
// 1 | block in epoch 1, justified between epochs 2 and 3 by blocks 2 and 3
// | |
// 2 | block at the start of epoch 2, with attestations justifying block 1 in epoch 1
// | |
// | 4 block in epoch 2, justified between epochs 2 and 3 by blocks 5 and 6
// | |
// | 5 block at the end of epoch 2, with attestations justifying block 4 in epoch 2
// | |
// 3 | block at the start of epoch 3, causing attestations added by block 2 to be processed
// |
// 6 block at the start of epoch 3, causing attestations added by block 5 to be processed
// ```
//
// There used to be separate tests for this happening early and late in an epoch.
// The checkpoint would only be updated in the first `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` slots.
// The condition was removed from the Fork Choice specification in version 1.3.0-rc.4.
#[test]
fn justified_checkpoint_always_switches_to_later_one() {
let mut context = Context::minimal();

let (_, state_0) = context.genesis();
let (block_1, state_1) = context.empty_block(&state_0, start_of_epoch(1), H256::default());
let (block_2, state_2) = context.block_justifying_previous_epoch(&state_1, 2, H256::default());
let (block_3, _) = context.empty_block(&state_2, start_of_epoch(3), H256::default());
let (block_4, state_4) = context.empty_block(&state_0, start_of_epoch(2), H256::default());
let (block_5, state_5) = context.block_justifying_current_epoch(&state_4, 2, H256::default());
let (block_6, _) = context.empty_block(&state_5, start_of_epoch(3), H256::default());

context.on_slot(start_of_epoch(3));

context.on_acceptable_block(&block_1);

context.assert_status(Status {
head: &block_1,
attesting_validators: Some(0),
store_justified_epoch: 0,
store_finalized_epoch: 0,
fork_count_viable: 1,
fork_count_total: 1,
finalized_block_count: 1,
unfinalized_block_count_in_fork: 1,
unfinalized_block_count_total: 1,
});

context.on_acceptable_block(&block_2);

context.assert_status(Status {
head: &block_2,
attesting_validators: Some(0),
store_justified_epoch: 1,
store_finalized_epoch: 0,
fork_count_viable: 1,
fork_count_total: 1,
finalized_block_count: 1,
unfinalized_block_count_in_fork: 2,
unfinalized_block_count_total: 2,
});

context.on_acceptable_block(&block_3);

context.assert_status(Status {
head: &block_3,
attesting_validators: Some(0),
store_justified_epoch: 1,
store_finalized_epoch: 0,
fork_count_viable: 1,
fork_count_total: 1,
finalized_block_count: 1,
unfinalized_block_count_in_fork: 3,
unfinalized_block_count_total: 3,
});

context.on_acceptable_block(&block_4);

context.assert_status(Status {
head: &block_3,
attesting_validators: Some(0),
store_justified_epoch: 1,
store_finalized_epoch: 0,
fork_count_viable: 1,
fork_count_total: 2,
finalized_block_count: 1,
unfinalized_block_count_in_fork: 3,
unfinalized_block_count_total: 4,
});

context.on_acceptable_block(&block_5);

context.assert_status(Status {
head: &block_5,
attesting_validators: Some(0),
store_justified_epoch: 2,
store_finalized_epoch: 0,
fork_count_viable: 1,
fork_count_total: 2,
finalized_block_count: 1,
unfinalized_block_count_in_fork: 2,
unfinalized_block_count_total: 5,
});

context.on_acceptable_block(&block_6);

context.assert_status(Status {
head: &block_6,
attesting_validators: Some(0),
store_justified_epoch: 2,
store_finalized_epoch: 0,
fork_count_viable: 1,
fork_count_total: 2,
finalized_block_count: 1,
unfinalized_block_count_in_fork: 3,
unfinalized_block_count_total: 6,
});
}

// ```text
// 0
// / \
Expand Down
1 change: 1 addition & 0 deletions fork_choice_control/src/specialized.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ impl<P: Preset> BenchController<P> {

#[cfg(test)]
impl<P: Preset> TestController<P> {
#[cfg(feature = "eth2-cache")]
pub(crate) fn quiet(
chain_config: Arc<ChainConfig>,
anchor_block: Arc<SignedBeaconBlock<P>>,
Expand Down
Loading

0 comments on commit c4beb15

Please sign in to comment.