From eba34f03274985babbed38874b4f2783293776c9 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Thu, 8 Feb 2024 13:08:21 -0500 Subject: [PATCH] Update to consensus spec v1.4.0-beta.6 (#5094) * get latest ef tests passing * fix tests * Fix invalid payload recovery tests * Merge branch 'unstable' into update-to-spec-v1.4.0-beta.6 * Revert "fix tests" This reverts commit 0c875b02e032ebd9bebd822183a1c3d10d333a4c. * Fix fork choice def. tests * Update beacon_node/beacon_chain/tests/payload_invalidation.rs --- .../tests/payload_invalidation.rs | 107 ++++++++++-------- .../ffg_updates.rs | 53 +++++++-- .../fork_choice_test_definition/no_votes.rs | 14 ++- consensus/proto_array/src/proto_array.rs | 13 +-- testing/ef_tests/Makefile | 2 +- testing/ef_tests/check_all_files_accessed.py | 3 +- testing/ef_tests/tests/tests.rs | 3 +- 7 files changed, 119 insertions(+), 76 deletions(-) diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index a0b7fbd365..597d53fddd 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1820,81 +1820,94 @@ struct InvalidHeadSetup { } impl InvalidHeadSetup { + /// This function aims to produce two things: + /// + /// 1. A chain where the only viable head block has an invalid execution payload. + /// 2. A block (`fork_block`) which will become the head of the chain when + /// it is imported. async fn new() -> InvalidHeadSetup { + let slots_per_epoch = E::slots_per_epoch(); let mut rig = InvalidPayloadRig::new().enable_attestations(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. - // Import blocks until the first time the chain finalizes. + // Import blocks until the first time the chain finalizes. This avoids + // some edge-cases around genesis. while rig.cached_head().finalized_checkpoint().epoch == 0 { rig.import_block(Payload::Syncing).await; } - let slots_per_epoch = E::slots_per_epoch(); - let start_slot = rig.cached_head().head_slot() + 1; - let mut opt_fork_block = None; - - assert_eq!(start_slot % slots_per_epoch, 1); - for i in 0..slots_per_epoch - 1 { - let slot = start_slot + i; - let slot_offset = slot.as_u64() % slots_per_epoch; - - rig.harness.set_current_slot(slot); - - if slot_offset == slots_per_epoch - 1 { - // Optimistic head block right before epoch boundary. - let is_valid = Payload::Syncing; - rig.import_block_parametric(is_valid, is_valid, Some(slot), |error| { - matches!( - error, - BlockError::ExecutionPayloadError( - ExecutionPayloadError::RejectedByExecutionEngine { .. } - ) - ) - }) - .await; - } else if 3 * slot_offset < 2 * slots_per_epoch { - // Valid block in previous epoch. - rig.import_block(Payload::Valid).await; - } else if slot_offset == slots_per_epoch - 2 { - // Fork block one slot prior to invalid head, not applied immediately. - let parent_state = rig - .harness - .chain - .state_at_slot(slot - 1, StateSkipConfig::WithStateRoots) - .unwrap(); - let (fork_block_tuple, _) = rig.harness.make_block(parent_state, slot).await; - opt_fork_block = Some(fork_block_tuple.0); - } else { - // Skipped slot. - }; + // Define a helper function. + let chain = rig.harness.chain.clone(); + let get_unrealized_justified_epoch = move || { + chain + .canonical_head + .fork_choice_read_lock() + .unrealized_justified_checkpoint() + .epoch + }; + + // Import more blocks until there is a new and higher unrealized + // justified checkpoint. + // + // The result will be a single chain where the head block has a higher + // unrealized justified checkpoint than all other blocks in the chain. + let initial_unrealized_justified = get_unrealized_justified_epoch(); + while get_unrealized_justified_epoch() == initial_unrealized_justified { + rig.import_block(Payload::Syncing).await; } + // Create a forked block that competes with the head block. Both the + // head block and this fork block will share the same parent. + // + // The fork block and head block will both have an unrealized justified + // checkpoint at epoch `N` whilst their parent is at `N - 1`. + let head_slot = rig.cached_head().head_slot(); + let parent_slot = head_slot - 1; + let fork_block_slot = head_slot + 1; + let parent_state = rig + .harness + .chain + .state_at_slot(parent_slot, StateSkipConfig::WithStateRoots) + .unwrap(); + let (fork_block_tuple, _) = rig.harness.make_block(parent_state, fork_block_slot).await; + let fork_block = fork_block_tuple.0; + let invalid_head = rig.cached_head(); - assert_eq!( - invalid_head.head_slot() % slots_per_epoch, - slots_per_epoch - 1 - ); - // Advance clock to new epoch to realize the justification of soon-to-be-invalid head block. - rig.harness.set_current_slot(invalid_head.head_slot() + 1); + // Advance the chain forward two epochs past the current head block. + // + // This ensures that `voting_source.epoch + 2 >= current_epoch` is + // `false` in the `node_is_viable_for_head` function. In effect, this + // ensures that no other block but the current head block is viable as a + // head block. + let invalid_head_epoch = invalid_head.head_slot().epoch(slots_per_epoch); + let new_wall_clock_epoch = invalid_head_epoch + 2; + rig.harness + .set_current_slot(new_wall_clock_epoch.start_slot(slots_per_epoch)); // Invalidate the head block. rig.invalidate_manually(invalid_head.head_block_root()) .await; + // Since our setup ensures that there is only a single, invalid block + // that's viable for head (according to FFG filtering), setting the + // head block as invalid should not result in another head being chosen. + // Rather, it should fail to run fork choice and leave the invalid block as + // the head. assert!(rig .canonical_head() .head_execution_status() .unwrap() .is_invalid()); - // Finding a new head should fail since the only possible head is not valid. + // Ensure that we're getting the correct error when trying to find a new + // head. rig.assert_get_head_error_contains("InvalidBestNode"); Self { rig, - fork_block: opt_fork_block.unwrap(), + fork_block, invalid_head, } } diff --git a/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs b/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs index 77211a86a7..3b31616145 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs @@ -59,7 +59,7 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { expected_head: get_root(3), }); - // Ensure that with justified epoch 1 we find 2 + // Ensure that with justified epoch 1 we find 3 // // 0 // | @@ -68,11 +68,15 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { // 2 <- start // | // 3 <- head + // + // Since https://github.com/ethereum/consensus-specs/pull/3431 it is valid + // to elect head blocks that have a higher justified checkpoint than the + // store. ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(1), finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), - expected_head: get_root(2), + expected_head: get_root(3), }); // Ensure that with justified epoch 2 we find 3 @@ -247,14 +251,19 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { justified_state_balances: balances.clone(), expected_head: get_root(10), }); - // Same as above, but with justified epoch 3 (should be invalid). - ops.push(Operation::InvalidFindHead { + // Same as above, but with justified epoch 3. + // + // Since https://github.com/ethereum/consensus-specs/pull/3431 it is valid + // to elect head blocks that have a higher justified checkpoint than the + // store. + ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(3), root: get_root(6), }, finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), + expected_head: get_root(10), }); // Add a vote to 1. @@ -305,14 +314,19 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { justified_state_balances: balances.clone(), expected_head: get_root(9), }); - // Save as above but justified epoch 3 (should fail). - ops.push(Operation::InvalidFindHead { + // Save as above but justified epoch 3. + // + // Since https://github.com/ethereum/consensus-specs/pull/3431 it is valid + // to elect head blocks that have a higher justified checkpoint than the + // store. + ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(3), root: get_root(5), }, finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), + expected_head: get_root(9), }); // Add a vote to 2. @@ -363,14 +377,19 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { justified_state_balances: balances.clone(), expected_head: get_root(10), }); - // Same as above but justified epoch 3 (should fail). - ops.push(Operation::InvalidFindHead { + // Same as above but justified epoch 3. + // + // Since https://github.com/ethereum/consensus-specs/pull/3431 it is valid + // to elect head blocks that have a higher justified checkpoint than the + // store. + ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(3), root: get_root(6), }, finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), + expected_head: get_root(10), }); // Ensure that if we start at 1 we find 9 (just: 0, fin: 0). @@ -405,14 +424,19 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { justified_state_balances: balances.clone(), expected_head: get_root(9), }); - // Same as above but justified epoch 3 (should fail). - ops.push(Operation::InvalidFindHead { + // Same as above but justified epoch 3. + // + // Since https://github.com/ethereum/consensus-specs/pull/3431 it is valid + // to elect head blocks that have a higher justified checkpoint than the + // store. + ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(3), root: get_root(5), }, finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), + expected_head: get_root(9), }); // Ensure that if we start at 2 we find 10 (just: 0, fin: 0). @@ -444,14 +468,19 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { justified_state_balances: balances.clone(), expected_head: get_root(10), }); - // Same as above but justified epoch 3 (should fail). - ops.push(Operation::InvalidFindHead { + // Same as above but justified epoch 3. + // + // Since https://github.com/ethereum/consensus-specs/pull/3431 it is valid + // to elect head blocks that have a higher justified checkpoint than the + // store. + ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(3), root: get_root(6), }, finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances, + expected_head: get_root(10), }); // END OF TESTS diff --git a/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs b/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs index a60b3e6b36..27a7969e49 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs @@ -184,7 +184,7 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { root: Hash256::zero(), }, }, - // Ensure the head is still 4 whilst the justified epoch is 0. + // Ensure the head is now 5 whilst the justified epoch is 0. // // 0 // / \ @@ -203,9 +203,10 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { root: Hash256::zero(), }, justified_state_balances: balances.clone(), - expected_head: get_root(4), + expected_head: get_root(5), }, - // Ensure there is an error when starting from a block that has the wrong justified epoch. + // Ensure there is no error when starting from a block that has the + // wrong justified epoch. // // 0 // / \ @@ -214,7 +215,11 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // 4 3 // | // 5 <- starting from 5 with justified epoch 0 should error. - Operation::InvalidFindHead { + // + // Since https://github.com/ethereum/consensus-specs/pull/3431 it is valid + // to elect head blocks that have a higher justified checkpoint than the + // store. + Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(1), root: get_root(5), @@ -224,6 +229,7 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { root: Hash256::zero(), }, justified_state_balances: balances.clone(), + expected_head: get_root(5), }, // Set the justified epoch to 2 and the start block to 5 and ensure 5 is the head. // diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 4726715a16..7c2ecfe26a 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -961,16 +961,9 @@ impl ProtoArray { node_justified_checkpoint }; - let mut correct_justified = self.justified_checkpoint.epoch == genesis_epoch - || voting_source.epoch == self.justified_checkpoint.epoch; - - if let Some(node_unrealized_justified_checkpoint) = node.unrealized_justified_checkpoint { - if !correct_justified && self.justified_checkpoint.epoch + 1 == current_epoch { - correct_justified = node_unrealized_justified_checkpoint.epoch - >= self.justified_checkpoint.epoch - && voting_source.epoch + 2 >= current_epoch; - } - } + let correct_justified = self.justified_checkpoint.epoch == genesis_epoch + || voting_source.epoch == self.justified_checkpoint.epoch + || voting_source.epoch + 2 >= current_epoch; let correct_finalized = self.finalized_checkpoint.epoch == genesis_epoch || self.is_finalized_checkpoint_or_descendant::(node.root); diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index e42db1801d..508c284275 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := v1.4.0-beta.4 +TESTS_TAG := v1.4.0-beta.6 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index a5ab897c33..1d1f2fa49a 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -51,7 +51,8 @@ "bls12-381-tests/deserialization_G1", "bls12-381-tests/deserialization_G2", "bls12-381-tests/hash_to_G2", - "tests/.*/eip6110" + "tests/.*/eip6110", + "tests/.*/whisk" ] diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index dd25dba8b6..5ed657c652 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -1,6 +1,6 @@ #![cfg(feature = "ef_tests")] -use ef_tests::{KzgInclusionMerkleProofValidityHandler, *}; +use ef_tests::*; use types::{MainnetEthSpec, MinimalEthSpec, *}; // Check that the hand-computed multiplications on EthSpec are correctly computed. @@ -605,6 +605,7 @@ fn merkle_proof_validity() { } #[test] +#[cfg(feature = "fake_crypto")] fn kzg_inclusion_merkle_proof_validity() { KzgInclusionMerkleProofValidityHandler::::default().run(); KzgInclusionMerkleProofValidityHandler::::default().run();