Delete RuntimeVariableList::from_vec (#7930)

This method is a footgun because it truncates the list. It is the source of a recent bug:

- https://github.com/sigp/lighthouse/pull/7927


  - Delete uses of `RuntimeVariableList::from_vec` and replace them with `::new` which does validation and can fail.
- Propagate errors where possible, unwrap in tests and use `expect` for obviously-safe uses (in `chain_spec.rs`).

consensus/types/src/blob_sidecar.rs

@@ -84,6 +84,7 @@ pub enum BlobSidecarError {
     MissingKzgCommitment,
     BeaconState(BeaconStateError),
     MerkleTree(MerkleTreeError),
+    SszTypes(ssz_types::Error),
     ArithError(ArithError),
 }
 
@@ -283,10 +284,11 @@ impl<E: EthSpec> BlobSidecar<E> {
             let blob_sidecar = BlobSidecar::new(i, blob, block, *kzg_proof)?;
             blob_sidecars.push(Arc::new(blob_sidecar));
         }
-        Ok(RuntimeVariableList::from_vec(
+        RuntimeVariableList::new(
             blob_sidecars,
             spec.max_blobs_per_block(block.epoch()) as usize,
-        ))
+        )
+        .map_err(BlobSidecarError::SszTypes)
     }
 }
 

consensus/types/src/chain_spec.rs

@@ -2001,10 +2001,11 @@ const fn default_min_epochs_for_data_column_sidecars_requests() -> u64 {
 
 fn max_blocks_by_root_request_common(max_request_blocks: u64) -> usize {
     let max_request_blocks = max_request_blocks as usize;
-    RuntimeVariableList::<Hash256>::from_vec(
+    RuntimeVariableList::<Hash256>::new(
         vec![Hash256::zero(); max_request_blocks],
         max_request_blocks,
     )
+    .expect("creating a RuntimeVariableList of size `max_request_blocks` should succeed")
     .as_ssz_bytes()
     .len()
 }
@@ -2016,10 +2017,11 @@ fn max_blobs_by_root_request_common(max_request_blob_sidecars: u64) -> usize {
         index: 0,
     };
 
-    RuntimeVariableList::<BlobIdentifier>::from_vec(
+    RuntimeVariableList::<BlobIdentifier>::new(
         vec![empty_blob_identifier; max_request_blob_sidecars],
         max_request_blob_sidecars,
     )
+    .expect("creating a RuntimeVariableList of size `max_request_blob_sidecars` should succeed")
     .as_ssz_bytes()
     .len()
 }
@@ -2032,10 +2034,11 @@ fn max_data_columns_by_root_request_common<E: EthSpec>(max_request_blocks: u64)
         columns: VariableList::from(vec![0; E::number_of_columns()]),
     };
 
-    RuntimeVariableList::<DataColumnsByRootIdentifier<E>>::from_vec(
+    RuntimeVariableList::<DataColumnsByRootIdentifier<E>>::new(
         vec![empty_data_columns_by_root_id; max_request_blocks],
         max_request_blocks,
     )
+    .expect("creating a RuntimeVariableList of size `max_request_blocks` should succeed")
     .as_ssz_bytes()
     .len()
 }

consensus/types/src/runtime_var_list.rs

@@ -23,15 +23,15 @@ use tree_hash::{Hash256, MerkleHasher, PackedEncoding, TreeHash, TreeHashType};
 /// let base: Vec<u64> = vec![1, 2, 3, 4];
 ///
 /// // Create a `RuntimeVariableList` from a `Vec` that has the expected length.
-/// let exact: RuntimeVariableList<_> = RuntimeVariableList::from_vec(base.clone(), 4);
+/// let exact: RuntimeVariableList<_> = RuntimeVariableList::new(base.clone(), 4).unwrap();
 /// assert_eq!(&exact[..], &[1, 2, 3, 4]);
 ///
-/// // Create a `RuntimeVariableList` from a `Vec` that is too long and the `Vec` is truncated.
-/// let short: RuntimeVariableList<_> = RuntimeVariableList::from_vec(base.clone(), 3);
-/// assert_eq!(&short[..], &[1, 2, 3]);
+/// // Create a `RuntimeVariableList` from a `Vec` that is too long you'll get an error.
+/// let err = RuntimeVariableList::new(base.clone(), 3).unwrap_err();
+/// assert_eq!(err, ssz_types::Error::OutOfBounds { i: 4, len: 3 });
 ///
 /// // Create a `RuntimeVariableList` from a `Vec` that is shorter than the maximum.
-/// let mut long: RuntimeVariableList<_> = RuntimeVariableList::from_vec(base, 5);
+/// let mut long: RuntimeVariableList<_> = RuntimeVariableList::new(base, 5).unwrap();
 /// assert_eq!(&long[..], &[1, 2, 3, 4]);
 ///
 /// // Push a value to if it does not exceed the maximum
@@ -65,12 +65,6 @@ impl<T> RuntimeVariableList<T> {
         }
     }
 
-    pub fn from_vec(mut vec: Vec<T>, max_len: usize) -> Self {
-        vec.truncate(max_len);
-
-        Self { vec, max_len }
-    }
-
     /// Create an empty list with the given `max_len`.
     pub fn empty(max_len: usize) -> Self {
         Self {
@@ -231,14 +225,13 @@ where
     {
         // first parse out a Vec<C> using the Vec<C> impl you already have
         let vec: Vec<T> = Vec::context_deserialize(deserializer, context.0)?;
-        if vec.len() > context.1 {
-            return Err(DeError::custom(format!(
-                "RuntimeVariableList lengh {} exceeds max_len {}",
-                vec.len(),
-                context.1
-            )));
-        }
-        Ok(RuntimeVariableList::from_vec(vec, context.1))
+        let vec_len = vec.len();
+        RuntimeVariableList::new(vec, context.1).map_err(|e| {
+            DeError::custom(format!(
+                "RuntimeVariableList length {} exceeds max_len {}: {e:?}",
+                vec_len, context.1,
+            ))
+        })
     }
 }
 
@@ -323,7 +316,8 @@ mod test {
     fn indexing() {
         let vec = vec![1, 2];
 
-        let mut fixed: RuntimeVariableList<u64> = RuntimeVariableList::from_vec(vec.clone(), 8192);
+        let mut fixed: RuntimeVariableList<u64> =
+            RuntimeVariableList::new(vec.clone(), 8192).unwrap();
 
         assert_eq!(fixed[0], 1);
         assert_eq!(&fixed[0..1], &vec[0..1]);
@@ -335,24 +329,25 @@ mod test {
 
     #[test]
     fn length() {
+        // Too long.
         let vec = vec![42; 5];
-        let fixed: RuntimeVariableList<u64> = RuntimeVariableList::from_vec(vec.clone(), 4);
-        assert_eq!(&fixed[..], &vec[0..4]);
+        let err = RuntimeVariableList::<u64>::new(vec.clone(), 4).unwrap_err();
+        assert_eq!(err, Error::OutOfBounds { i: 5, len: 4 });
 
         let vec = vec![42; 3];
-        let fixed: RuntimeVariableList<u64> = RuntimeVariableList::from_vec(vec.clone(), 4);
+        let fixed: RuntimeVariableList<u64> = RuntimeVariableList::new(vec.clone(), 4).unwrap();
         assert_eq!(&fixed[0..3], &vec[..]);
         assert_eq!(&fixed[..], &vec![42, 42, 42][..]);
 
         let vec = vec![];
-        let fixed: RuntimeVariableList<u64> = RuntimeVariableList::from_vec(vec, 4);
+        let fixed: RuntimeVariableList<u64> = RuntimeVariableList::new(vec, 4).unwrap();
         assert_eq!(&fixed[..], &[] as &[u64]);
     }
 
     #[test]
     fn deref() {
         let vec = vec![0, 2, 4, 6];
-        let fixed: RuntimeVariableList<u64> = RuntimeVariableList::from_vec(vec, 4);
+        let fixed: RuntimeVariableList<u64> = RuntimeVariableList::new(vec, 4).unwrap();
 
         assert_eq!(fixed.first(), Some(&0));
         assert_eq!(fixed.get(3), Some(&6));
@@ -361,7 +356,7 @@ mod test {
 
     #[test]
     fn encode() {
-        let vec: RuntimeVariableList<u16> = RuntimeVariableList::from_vec(vec![0; 2], 2);
+        let vec: RuntimeVariableList<u16> = RuntimeVariableList::new(vec![0; 2], 2).unwrap();
         assert_eq!(vec.as_ssz_bytes(), vec![0, 0, 0, 0]);
         assert_eq!(<RuntimeVariableList<u16> as Encode>::ssz_fixed_len(), 4);
     }
@@ -378,7 +373,7 @@ mod test {
 
     #[test]
     fn u16_len_8() {
-        round_trip::<u16>(RuntimeVariableList::from_vec(vec![42; 8], 8));
-        round_trip::<u16>(RuntimeVariableList::from_vec(vec![0; 8], 8));
+        round_trip::<u16>(RuntimeVariableList::new(vec![42; 8], 8).unwrap());
+        round_trip::<u16>(RuntimeVariableList::new(vec![0; 8], 8).unwrap());
     }
 }