merge queue: embarking unstable (e353358) and #5094 together

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,
         }
     }

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

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.
         //

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::<E>(node.root);

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))
 

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"
 ]
 
 

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::<MainnetEthSpec>::default().run();
     KzgInclusionMerkleProofValidityHandler::<MinimalEthSpec>::default().run();