diff --git a/Cargo.lock b/Cargo.lock index 7c637a1847..5e89dc2a90 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8978,6 +8978,7 @@ dependencies = [ "safe_arith", "serde", "smallvec", + "ssz_types", "state_processing", "strum", "superstruct", diff --git a/beacon_node/lighthouse_network/src/rpc/codec.rs b/beacon_node/lighthouse_network/src/rpc/codec.rs index 91f9960d47..acb0188456 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec.rs @@ -1089,11 +1089,11 @@ mod tests { } fn bbroot_request_v1(fork_name: ForkName, spec: &ChainSpec) -> BlocksByRootRequest { - BlocksByRootRequest::new_v1(vec![Hash256::zero()], &fork_context(fork_name, spec)) + BlocksByRootRequest::new_v1(vec![Hash256::zero()], &fork_context(fork_name, spec)).unwrap() } fn bbroot_request_v2(fork_name: ForkName, spec: &ChainSpec) -> BlocksByRootRequest { - BlocksByRootRequest::new(vec![Hash256::zero()], &fork_context(fork_name, spec)) + BlocksByRootRequest::new(vec![Hash256::zero()], &fork_context(fork_name, spec)).unwrap() } fn blbroot_request(fork_name: ForkName, spec: &ChainSpec) -> BlobsByRootRequest { @@ -1104,6 +1104,7 @@ mod tests { }], &fork_context(fork_name, spec), ) + .unwrap() } fn ping_message() -> Ping { diff --git a/beacon_node/lighthouse_network/src/rpc/methods.rs b/beacon_node/lighthouse_network/src/rpc/methods.rs index 39078e8d9e..9319973e59 100644 --- a/beacon_node/lighthouse_network/src/rpc/methods.rs +++ b/beacon_node/lighthouse_network/src/rpc/methods.rs @@ -481,20 +481,22 @@ pub struct BlocksByRootRequest { } impl BlocksByRootRequest { - pub fn new(block_roots: Vec, fork_context: &ForkContext) -> Self { + pub fn new(block_roots: Vec, fork_context: &ForkContext) -> Result { let max_request_blocks = fork_context .spec .max_request_blocks(fork_context.current_fork_name()); - let block_roots = RuntimeVariableList::from_vec(block_roots, max_request_blocks); - Self::V2(BlocksByRootRequestV2 { block_roots }) + let block_roots = RuntimeVariableList::new(block_roots, max_request_blocks) + .map_err(|e| format!("BlocksByRootRequestV2 too many roots: {e:?}"))?; + Ok(Self::V2(BlocksByRootRequestV2 { block_roots })) } - pub fn new_v1(block_roots: Vec, fork_context: &ForkContext) -> Self { + pub fn new_v1(block_roots: Vec, fork_context: &ForkContext) -> Result { let max_request_blocks = fork_context .spec .max_request_blocks(fork_context.current_fork_name()); - let block_roots = RuntimeVariableList::from_vec(block_roots, max_request_blocks); - Self::V1(BlocksByRootRequestV1 { block_roots }) + let block_roots = RuntimeVariableList::new(block_roots, max_request_blocks) + .map_err(|e| format!("BlocksByRootRequestV1 too many roots: {e:?}"))?; + Ok(Self::V1(BlocksByRootRequestV1 { block_roots })) } } @@ -506,12 +508,13 @@ pub struct BlobsByRootRequest { } impl BlobsByRootRequest { - pub fn new(blob_ids: Vec, fork_context: &ForkContext) -> Self { + pub fn new(blob_ids: Vec, fork_context: &ForkContext) -> Result { let max_request_blob_sidecars = fork_context .spec .max_request_blob_sidecars(fork_context.current_fork_name()); - let blob_ids = RuntimeVariableList::from_vec(blob_ids, max_request_blob_sidecars); - Self { blob_ids } + let blob_ids = RuntimeVariableList::new(blob_ids, max_request_blob_sidecars) + .map_err(|e| format!("BlobsByRootRequestV1 too many blob IDs: {e:?}"))?; + Ok(Self { blob_ids }) } } @@ -526,9 +529,10 @@ impl DataColumnsByRootRequest { pub fn new( data_column_ids: Vec>, max_request_blocks: usize, - ) -> Self { - let data_column_ids = RuntimeVariableList::from_vec(data_column_ids, max_request_blocks); - Self { data_column_ids } + ) -> Result { + let data_column_ids = RuntimeVariableList::new(data_column_ids, max_request_blocks) + .map_err(|_| "DataColumnsByRootRequest too many column IDs")?; + Ok(Self { data_column_ids }) } pub fn max_requested(&self) -> usize { diff --git a/beacon_node/lighthouse_network/tests/rpc_tests.rs b/beacon_node/lighthouse_network/tests/rpc_tests.rs index 098d7efadb..e37f4131a7 100644 --- a/beacon_node/lighthouse_network/tests/rpc_tests.rs +++ b/beacon_node/lighthouse_network/tests/rpc_tests.rs @@ -831,7 +831,7 @@ fn test_tcp_blocks_by_root_chunked_rpc() { // BlocksByRoot Request let rpc_request = RequestType::BlocksByRoot(BlocksByRootRequest::V2(BlocksByRootRequestV2 { - block_roots: RuntimeVariableList::from_vec( + block_roots: RuntimeVariableList::new( vec![ Hash256::zero(), Hash256::zero(), @@ -841,7 +841,8 @@ fn test_tcp_blocks_by_root_chunked_rpc() { Hash256::zero(), ], spec.max_request_blocks(current_fork_name), - ), + ) + .unwrap(), })); // BlocksByRoot Response @@ -991,7 +992,8 @@ fn test_tcp_columns_by_root_chunked_rpc() { max_request_blocks ], max_request_blocks, - ); + ) + .unwrap(); let req_bytes = req.data_column_ids.as_ssz_bytes(); let req_decoded = DataColumnsByRootRequest { data_column_ids: >>::from_ssz_bytes( @@ -1281,7 +1283,7 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() { // BlocksByRoot Request let rpc_request = RequestType::BlocksByRoot(BlocksByRootRequest::V2(BlocksByRootRequestV2 { - block_roots: RuntimeVariableList::from_vec( + block_roots: RuntimeVariableList::new( vec![ Hash256::zero(), Hash256::zero(), @@ -1295,7 +1297,8 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() { Hash256::zero(), ], spec.max_request_blocks(current_fork), - ), + ) + .unwrap(), })); // BlocksByRoot Response diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 1f4a14b4bf..07462a01fe 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -865,10 +865,15 @@ impl SyncNetworkContext { // - RPCError(request_id): handled by `Self::on_single_block_response` // - Disconnect(peer_id) handled by `Self::peer_disconnected``which converts it to a // ` RPCError(request_id)`event handled by the above method + let network_request = RequestType::BlocksByRoot( + request + .into_request(&self.fork_context) + .map_err(RpcRequestSendError::InternalError)?, + ); self.network_send .send(NetworkMessage::SendRequest { peer_id, - request: RequestType::BlocksByRoot(request.into_request(&self.fork_context)), + request: network_request, app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }), }) .map_err(|_| RpcRequestSendError::InternalError("network send error".to_owned()))?; @@ -959,10 +964,16 @@ impl SyncNetworkContext { }; // Lookup sync event safety: Refer to `Self::block_lookup_request` `network_send.send` call + let network_request = RequestType::BlobsByRoot( + request + .clone() + .into_request(&self.fork_context) + .map_err(RpcRequestSendError::InternalError)?, + ); self.network_send .send(NetworkMessage::SendRequest { peer_id, - request: RequestType::BlobsByRoot(request.clone().into_request(&self.fork_context)), + request: network_request, app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }), }) .map_err(|_| RpcRequestSendError::InternalError("network send error".to_owned()))?; diff --git a/beacon_node/network/src/sync/network_context/requests/blobs_by_root.rs b/beacon_node/network/src/sync/network_context/requests/blobs_by_root.rs index 0d176e2d8c..39886d814e 100644 --- a/beacon_node/network/src/sync/network_context/requests/blobs_by_root.rs +++ b/beacon_node/network/src/sync/network_context/requests/blobs_by_root.rs @@ -11,7 +11,7 @@ pub struct BlobsByRootSingleBlockRequest { } impl BlobsByRootSingleBlockRequest { - pub fn into_request(self, spec: &ForkContext) -> BlobsByRootRequest { + pub fn into_request(self, spec: &ForkContext) -> Result { BlobsByRootRequest::new( self.indices .into_iter() diff --git a/beacon_node/network/src/sync/network_context/requests/blocks_by_root.rs b/beacon_node/network/src/sync/network_context/requests/blocks_by_root.rs index 6d7eabf909..8cb7f53ac5 100644 --- a/beacon_node/network/src/sync/network_context/requests/blocks_by_root.rs +++ b/beacon_node/network/src/sync/network_context/requests/blocks_by_root.rs @@ -9,7 +9,8 @@ use super::{ActiveRequestItems, LookupVerifyError}; pub struct BlocksByRootSingleRequest(pub Hash256); impl BlocksByRootSingleRequest { - pub fn into_request(self, fork_context: &ForkContext) -> BlocksByRootRequest { + pub fn into_request(self, fork_context: &ForkContext) -> Result { + // This should always succeed (single block root), but we return a `Result` for safety. BlocksByRootRequest::new(vec![self.0], fork_context) } } diff --git a/beacon_node/network/src/sync/network_context/requests/data_columns_by_root.rs b/beacon_node/network/src/sync/network_context/requests/data_columns_by_root.rs index 253e8940b2..34df801eaa 100644 --- a/beacon_node/network/src/sync/network_context/requests/data_columns_by_root.rs +++ b/beacon_node/network/src/sync/network_context/requests/data_columns_by_root.rs @@ -21,13 +21,13 @@ impl DataColumnsByRootSingleBlockRequest { ) -> Result, &'static str> { let columns = VariableList::new(self.indices) .map_err(|_| "Number of indices exceeds total number of columns")?; - Ok(DataColumnsByRootRequest::new( + DataColumnsByRootRequest::new( vec![DataColumnsByRootIdentifier { block_root: self.block_root, columns, }], spec.max_request_blocks(fork_name), - )) + ) } } diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index 4180025096..b5bc10851d 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -2303,7 +2303,7 @@ mod deneb_only { block, self.unknown_parent_blobs .take() - .map(|vec| RuntimeVariableList::from_vec(vec, max_len)), + .map(|vec| RuntimeVariableList::new(vec, max_len).unwrap()), ) .unwrap(); self.rig.parent_block_processed( diff --git a/beacon_node/store/Cargo.toml b/beacon_node/store/Cargo.toml index 13df83efab..61a8474a73 100644 --- a/beacon_node/store/Cargo.toml +++ b/beacon_node/store/Cargo.toml @@ -25,6 +25,7 @@ redb = { version = "2.1.3", optional = true } safe_arith = { workspace = true } serde = { workspace = true } smallvec = { workspace = true } +ssz_types = { workspace = true } state_processing = { workspace = true } strum = { workspace = true } superstruct = { workspace = true } diff --git a/beacon_node/store/src/errors.rs b/beacon_node/store/src/errors.rs index 51b4bfef83..f62647ae54 100644 --- a/beacon_node/store/src/errors.rs +++ b/beacon_node/store/src/errors.rs @@ -69,6 +69,7 @@ pub enum Error { CacheBuildError(EpochCacheError), RandaoMixOutOfBounds, MilhouseError(milhouse::Error), + SszTypesError(ssz_types::Error), Compression(std::io::Error), FinalizedStateDecreasingSlot, FinalizedStateUnaligned, @@ -161,6 +162,12 @@ impl From for Error { } } +impl From for Error { + fn from(e: ssz_types::Error) -> Self { + Self::SszTypesError(e) + } +} + impl From for Error { fn from(e: hdiff::Error) -> Self { Self::Hdiff(e) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 8116596aa0..7b390b39f3 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -2478,7 +2478,7 @@ impl, Cold: ItemStore> HotColdDB .first() .map(|blob| self.spec.max_blobs_per_block(blob.epoch())) { - let blobs = BlobSidecarList::from_vec(blobs, max_blobs_per_block as usize); + let blobs = BlobSidecarList::new(blobs, max_blobs_per_block as usize)?; self.block_cache .lock() .put_blobs(*block_root, blobs.clone()); diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index d65ad9a3e0..2e8c257897 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/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 BlobSidecar { 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) } } diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 709b4f28fe..a1005d904a 100644 --- a/consensus/types/src/chain_spec.rs +++ b/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::::from_vec( + RuntimeVariableList::::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::::from_vec( + RuntimeVariableList::::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(max_request_blocks: u64) columns: VariableList::from(vec![0; E::number_of_columns()]), }; - RuntimeVariableList::>::from_vec( + RuntimeVariableList::>::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() } diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 2a8899e203..dcb98538b7 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -23,15 +23,15 @@ use tree_hash::{Hash256, MerkleHasher, PackedEncoding, TreeHash, TreeHashType}; /// let base: Vec = 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 RuntimeVariableList { } } - pub fn from_vec(mut vec: Vec, 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 using the Vec impl you already have let vec: Vec = 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 = RuntimeVariableList::from_vec(vec.clone(), 8192); + let mut fixed: RuntimeVariableList = + 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 = RuntimeVariableList::from_vec(vec.clone(), 4); - assert_eq!(&fixed[..], &vec[0..4]); + let err = RuntimeVariableList::::new(vec.clone(), 4).unwrap_err(); + assert_eq!(err, Error::OutOfBounds { i: 5, len: 4 }); let vec = vec![42; 3]; - let fixed: RuntimeVariableList = RuntimeVariableList::from_vec(vec.clone(), 4); + let fixed: RuntimeVariableList = 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 = RuntimeVariableList::from_vec(vec, 4); + let fixed: RuntimeVariableList = RuntimeVariableList::new(vec, 4).unwrap(); assert_eq!(&fixed[..], &[] as &[u64]); } #[test] fn deref() { let vec = vec![0, 2, 4, 6]; - let fixed: RuntimeVariableList = RuntimeVariableList::from_vec(vec, 4); + let fixed: RuntimeVariableList = 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 = RuntimeVariableList::from_vec(vec![0; 2], 2); + let vec: RuntimeVariableList = RuntimeVariableList::new(vec![0; 2], 2).unwrap(); assert_eq!(vec.as_ssz_bytes(), vec![0, 0, 0, 0]); assert_eq!( as Encode>::ssz_fixed_len(), 4); } @@ -378,7 +373,7 @@ mod test { #[test] fn u16_len_8() { - round_trip::(RuntimeVariableList::from_vec(vec![42; 8], 8)); - round_trip::(RuntimeVariableList::from_vec(vec![0; 8], 8)); + round_trip::(RuntimeVariableList::new(vec![42; 8], 8).unwrap()); + round_trip::(RuntimeVariableList::new(vec![0; 8], 8).unwrap()); } }