Skip to content

Commit d235f2c

Browse files
michaelsproulmichaelsproul
authored
Delete RuntimeVariableList::from_vec (sigp#7930)
This method is a footgun because it truncates the list. It is the source of a recent bug: - sigp#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`).
1 parent ccf03e1 commit d235f2c

File tree

15 files changed

+89
-60
lines changed

15 files changed

+89
-60
lines changed

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎beacon_node/lighthouse_network/src/rpc/codec.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1089,11 +1089,11 @@ mod tests {
10891089
}
10901090

10911091
fn bbroot_request_v1(fork_name: ForkName, spec: &ChainSpec) -> BlocksByRootRequest {
1092-
BlocksByRootRequest::new_v1(vec![Hash256::zero()], &fork_context(fork_name, spec))
1092+
BlocksByRootRequest::new_v1(vec![Hash256::zero()], &fork_context(fork_name, spec)).unwrap()
10931093
}
10941094

10951095
fn bbroot_request_v2(fork_name: ForkName, spec: &ChainSpec) -> BlocksByRootRequest {
1096-
BlocksByRootRequest::new(vec![Hash256::zero()], &fork_context(fork_name, spec))
1096+
BlocksByRootRequest::new(vec![Hash256::zero()], &fork_context(fork_name, spec)).unwrap()
10971097
}
10981098

10991099
fn blbroot_request(fork_name: ForkName, spec: &ChainSpec) -> BlobsByRootRequest {
@@ -1104,6 +1104,7 @@ mod tests {
11041104
}],
11051105
&fork_context(fork_name, spec),
11061106
)
1107+
.unwrap()
11071108
}
11081109

11091110
fn ping_message() -> Ping {

‎beacon_node/lighthouse_network/src/rpc/methods.rs‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -481,20 +481,22 @@ pub struct BlocksByRootRequest {
481481
}
482482

483483
impl BlocksByRootRequest {
484-
pub fn new(block_roots: Vec<Hash256>, fork_context: &ForkContext) -> Self {
484+
pub fn new(block_roots: Vec<Hash256>, fork_context: &ForkContext) -> Result<Self, String> {
485485
let max_request_blocks = fork_context
486486
.spec
487487
.max_request_blocks(fork_context.current_fork_name());
488-
let block_roots = RuntimeVariableList::from_vec(block_roots, max_request_blocks);
489-
Self::V2(BlocksByRootRequestV2 { block_roots })
488+
let block_roots = RuntimeVariableList::new(block_roots, max_request_blocks)
489+
.map_err(|e| format!("BlocksByRootRequestV2 too many roots: {e:?}"))?;
490+
Ok(Self::V2(BlocksByRootRequestV2 { block_roots }))
490491
}
491492

492-
pub fn new_v1(block_roots: Vec<Hash256>, fork_context: &ForkContext) -> Self {
493+
pub fn new_v1(block_roots: Vec<Hash256>, fork_context: &ForkContext) -> Result<Self, String> {
493494
let max_request_blocks = fork_context
494495
.spec
495496
.max_request_blocks(fork_context.current_fork_name());
496-
let block_roots = RuntimeVariableList::from_vec(block_roots, max_request_blocks);
497-
Self::V1(BlocksByRootRequestV1 { block_roots })
497+
let block_roots = RuntimeVariableList::new(block_roots, max_request_blocks)
498+
.map_err(|e| format!("BlocksByRootRequestV1 too many roots: {e:?}"))?;
499+
Ok(Self::V1(BlocksByRootRequestV1 { block_roots }))
498500
}
499501
}
500502

@@ -506,12 +508,13 @@ pub struct BlobsByRootRequest {
506508
}
507509

508510
impl BlobsByRootRequest {
509-
pub fn new(blob_ids: Vec<BlobIdentifier>, fork_context: &ForkContext) -> Self {
511+
pub fn new(blob_ids: Vec<BlobIdentifier>, fork_context: &ForkContext) -> Result<Self, String> {
510512
let max_request_blob_sidecars = fork_context
511513
.spec
512514
.max_request_blob_sidecars(fork_context.current_fork_name());
513-
let blob_ids = RuntimeVariableList::from_vec(blob_ids, max_request_blob_sidecars);
514-
Self { blob_ids }
515+
let blob_ids = RuntimeVariableList::new(blob_ids, max_request_blob_sidecars)
516+
.map_err(|e| format!("BlobsByRootRequestV1 too many blob IDs: {e:?}"))?;
517+
Ok(Self { blob_ids })
515518
}
516519
}
517520

@@ -526,9 +529,10 @@ impl<E: EthSpec> DataColumnsByRootRequest<E> {
526529
pub fn new(
527530
data_column_ids: Vec<DataColumnsByRootIdentifier<E>>,
528531
max_request_blocks: usize,
529-
) -> Self {
530-
let data_column_ids = RuntimeVariableList::from_vec(data_column_ids, max_request_blocks);
531-
Self { data_column_ids }
532+
) -> Result<Self, &'static str> {
533+
let data_column_ids = RuntimeVariableList::new(data_column_ids, max_request_blocks)
534+
.map_err(|_| "DataColumnsByRootRequest too many column IDs")?;
535+
Ok(Self { data_column_ids })
532536
}
533537

534538
pub fn max_requested(&self) -> usize {

‎beacon_node/lighthouse_network/tests/rpc_tests.rs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -831,7 +831,7 @@ fn test_tcp_blocks_by_root_chunked_rpc() {
831831
// BlocksByRoot Request
832832
let rpc_request =
833833
RequestType::BlocksByRoot(BlocksByRootRequest::V2(BlocksByRootRequestV2 {
834-
block_roots: RuntimeVariableList::from_vec(
834+
block_roots: RuntimeVariableList::new(
835835
vec![
836836
Hash256::zero(),
837837
Hash256::zero(),
@@ -841,7 +841,8 @@ fn test_tcp_blocks_by_root_chunked_rpc() {
841841
Hash256::zero(),
842842
],
843843
spec.max_request_blocks(current_fork_name),
844-
),
844+
)
845+
.unwrap(),
845846
}));
846847

847848
// BlocksByRoot Response
@@ -991,7 +992,8 @@ fn test_tcp_columns_by_root_chunked_rpc() {
991992
max_request_blocks
992993
],
993994
max_request_blocks,
994-
);
995+
)
996+
.unwrap();
995997
let req_bytes = req.data_column_ids.as_ssz_bytes();
996998
let req_decoded = DataColumnsByRootRequest {
997999
data_column_ids: <RuntimeVariableList<DataColumnsByRootIdentifier<E>>>::from_ssz_bytes(
@@ -1281,7 +1283,7 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() {
12811283
// BlocksByRoot Request
12821284
let rpc_request =
12831285
RequestType::BlocksByRoot(BlocksByRootRequest::V2(BlocksByRootRequestV2 {
1284-
block_roots: RuntimeVariableList::from_vec(
1286+
block_roots: RuntimeVariableList::new(
12851287
vec![
12861288
Hash256::zero(),
12871289
Hash256::zero(),
@@ -1295,7 +1297,8 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() {
12951297
Hash256::zero(),
12961298
],
12971299
spec.max_request_blocks(current_fork),
1298-
),
1300+
)
1301+
.unwrap(),
12991302
}));
13001303

13011304
// BlocksByRoot Response

‎beacon_node/network/src/sync/network_context.rs‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -865,10 +865,15 @@ impl<T: BeaconChainTypes> SyncNetworkContext<T> {
865865
// - RPCError(request_id): handled by `Self::on_single_block_response`
866866
// - Disconnect(peer_id) handled by `Self::peer_disconnected``which converts it to a
867867
// ` RPCError(request_id)`event handled by the above method
868+
let network_request = RequestType::BlocksByRoot(
869+
request
870+
.into_request(&self.fork_context)
871+
.map_err(RpcRequestSendError::InternalError)?,
872+
);
868873
self.network_send
869874
.send(NetworkMessage::SendRequest {
870875
peer_id,
871-
request: RequestType::BlocksByRoot(request.into_request(&self.fork_context)),
876+
request: network_request,
872877
app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }),
873878
})
874879
.map_err(|_| RpcRequestSendError::InternalError("network send error".to_owned()))?;
@@ -959,10 +964,16 @@ impl<T: BeaconChainTypes> SyncNetworkContext<T> {
959964
};
960965

961966
// Lookup sync event safety: Refer to `Self::block_lookup_request` `network_send.send` call
967+
let network_request = RequestType::BlobsByRoot(
968+
request
969+
.clone()
970+
.into_request(&self.fork_context)
971+
.map_err(RpcRequestSendError::InternalError)?,
972+
);
962973
self.network_send
963974
.send(NetworkMessage::SendRequest {
964975
peer_id,
965-
request: RequestType::BlobsByRoot(request.clone().into_request(&self.fork_context)),
976+
request: network_request,
966977
app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }),
967978
})
968979
.map_err(|_| RpcRequestSendError::InternalError("network send error".to_owned()))?;

‎beacon_node/network/src/sync/network_context/requests/blobs_by_root.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ pub struct BlobsByRootSingleBlockRequest {
1111
}
1212

1313
impl BlobsByRootSingleBlockRequest {
14-
pub fn into_request(self, spec: &ForkContext) -> BlobsByRootRequest {
14+
pub fn into_request(self, spec: &ForkContext) -> Result<BlobsByRootRequest, String> {
1515
BlobsByRootRequest::new(
1616
self.indices
1717
.into_iter()

‎beacon_node/network/src/sync/network_context/requests/blocks_by_root.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ use super::{ActiveRequestItems, LookupVerifyError};
99
pub struct BlocksByRootSingleRequest(pub Hash256);
1010

1111
impl BlocksByRootSingleRequest {
12-
pub fn into_request(self, fork_context: &ForkContext) -> BlocksByRootRequest {
12+
pub fn into_request(self, fork_context: &ForkContext) -> Result<BlocksByRootRequest, String> {
13+
// This should always succeed (single block root), but we return a `Result` for safety.
1314
BlocksByRootRequest::new(vec![self.0], fork_context)
1415
}
1516
}

‎beacon_node/network/src/sync/network_context/requests/data_columns_by_root.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ impl DataColumnsByRootSingleBlockRequest {
2121
) -> Result<DataColumnsByRootRequest<E>, &'static str> {
2222
let columns = VariableList::new(self.indices)
2323
.map_err(|_| "Number of indices exceeds total number of columns")?;
24-
Ok(DataColumnsByRootRequest::new(
24+
DataColumnsByRootRequest::new(
2525
vec![DataColumnsByRootIdentifier {
2626
block_root: self.block_root,
2727
columns,
2828
}],
2929
spec.max_request_blocks(fork_name),
30-
))
30+
)
3131
}
3232
}
3333

‎beacon_node/network/src/sync/tests/lookups.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2303,7 +2303,7 @@ mod deneb_only {
23032303
block,
23042304
self.unknown_parent_blobs
23052305
.take()
2306-
.map(|vec| RuntimeVariableList::from_vec(vec, max_len)),
2306+
.map(|vec| RuntimeVariableList::new(vec, max_len).unwrap()),
23072307
)
23082308
.unwrap();
23092309
self.rig.parent_block_processed(

‎beacon_node/store/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ redb = { version = "2.1.3", optional = true }
2525
safe_arith = { workspace = true }
2626
serde = { workspace = true }
2727
smallvec = { workspace = true }
28+
ssz_types = { workspace = true }
2829
state_processing = { workspace = true }
2930
strum = { workspace = true }
3031
superstruct = { workspace = true }

0 commit comments

Comments
 (0)