From fff4dd6311695c1d772a9d6991463915edf223d5 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 7 Apr 2022 23:45:38 +0000 Subject: [PATCH] Fix rpc limits version 2 (#3146) ## Issue Addressed N/A ## Proposed Changes https://github.com/sigp/lighthouse/pull/3133 changed the rpc type limits to be fork aware i.e. if our current fork based on wall clock slot is Altair, then we apply only altair rpc type limits. This is a bug because phase0 blocks can still be sent over rpc and phase 0 block minimum size is smaller than altair block minimum size. So a phase0 block with `size < SIGNED_BEACON_BLOCK_ALTAIR_MIN` will return an `InvalidData` error as it doesn't pass the rpc types bound check. This error can be seen when we try syncing pre-altair blocks with size smaller than `SIGNED_BEACON_BLOCK_ALTAIR_MIN`. This PR fixes the issue by also accounting for forks earlier than current_fork in the rpc limits calculation in the `rpc_block_limits_by_fork` function. I decided to hardcode the limits in the function because that seemed simpler than calculating previous forks based on current fork and doing a min across forks. Adding a new fork variant is simple and can the limits can be easily checked in a review. Adds unit tests and modifies the syncing simulator to check the syncing from across fork boundaries. The syncing simulator's block 1 would always be of phase 0 minimum size (404 bytes) which is smaller than altair min block size (since block 1 contains no attestations). --- .../src/rpc/codec/ssz_snappy.rs | 72 ++++++++++++++----- .../lighthouse_network/src/rpc/protocol.rs | 48 ++++++------- testing/simulator/src/sync_sim.rs | 5 ++ 3 files changed, 83 insertions(+), 42 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs b/beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs index 188ae59b6f9..6bd4a96fb5f 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs @@ -659,9 +659,11 @@ mod tests { ForkContext::new::(current_slot, Hash256::zero(), &chain_spec) } - fn base_block() -> SignedBeaconBlock { - let full_block = BeaconBlock::Base(BeaconBlockBase::::full(&Spec::default_spec())); - SignedBeaconBlock::from_block(full_block, Signature::empty()) + /// Smallest sized block across all current forks. Useful for testing + /// min length check conditions. + fn empty_base_block() -> SignedBeaconBlock { + let empty_block = BeaconBlock::Base(BeaconBlockBase::::empty(&Spec::default_spec())); + SignedBeaconBlock::from_block(empty_block, Signature::empty()) } fn altair_block() -> SignedBeaconBlock { @@ -830,10 +832,12 @@ mod tests { encode_then_decode( Protocol::BlocksByRange, Version::V1, - RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(empty_base_block()))), ForkName::Base, ), - Ok(Some(RPCResponse::BlocksByRange(Box::new(base_block())))) + Ok(Some(RPCResponse::BlocksByRange(Box::new( + empty_base_block() + )))) ); assert!( @@ -854,10 +858,12 @@ mod tests { encode_then_decode( Protocol::BlocksByRoot, Version::V1, - RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(empty_base_block()))), ForkName::Base, ), - Ok(Some(RPCResponse::BlocksByRoot(Box::new(base_block())))) + Ok(Some(RPCResponse::BlocksByRoot( + Box::new(empty_base_block()) + ))) ); assert!( @@ -941,10 +947,27 @@ mod tests { encode_then_decode( Protocol::BlocksByRange, Version::V2, - RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(empty_base_block()))), ForkName::Base, ), - Ok(Some(RPCResponse::BlocksByRange(Box::new(base_block())))) + Ok(Some(RPCResponse::BlocksByRange(Box::new( + empty_base_block() + )))) + ); + + // Decode the smallest possible base block when current fork is altair + // This is useful for checking that we allow for blocks smaller than + // the current_fork's rpc limit + assert_eq!( + encode_then_decode( + Protocol::BlocksByRange, + Version::V2, + RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(empty_base_block()))), + ForkName::Altair, + ), + Ok(Some(RPCResponse::BlocksByRange(Box::new( + empty_base_block() + )))) ); assert_eq!( @@ -996,10 +1019,27 @@ mod tests { encode_then_decode( Protocol::BlocksByRoot, Version::V2, - RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(empty_base_block()))), ForkName::Base, ), - Ok(Some(RPCResponse::BlocksByRoot(Box::new(base_block())))), + Ok(Some(RPCResponse::BlocksByRoot( + Box::new(empty_base_block()) + ))), + ); + + // Decode the smallest possible base block when current fork is altair + // This is useful for checking that we allow for blocks smaller than + // the current_fork's rpc limit + assert_eq!( + encode_then_decode( + Protocol::BlocksByRoot, + Version::V2, + RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(empty_base_block()))), + ForkName::Altair, + ), + Ok(Some(RPCResponse::BlocksByRoot( + Box::new(empty_base_block()) + ))) ); assert_eq!( @@ -1073,7 +1113,7 @@ mod tests { let mut encoded_bytes = encode( Protocol::BlocksByRange, Version::V2, - RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(empty_base_block()))), ForkName::Base, ) .unwrap(); @@ -1094,7 +1134,7 @@ mod tests { let mut encoded_bytes = encode( Protocol::BlocksByRoot, Version::V2, - RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(empty_base_block()))), ForkName::Base, ) .unwrap(); @@ -1116,7 +1156,7 @@ mod tests { let mut encoded_bytes = encode( Protocol::BlocksByRange, Version::V2, - RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRange(Box::new(empty_base_block()))), ForkName::Altair, ) .unwrap(); @@ -1186,7 +1226,7 @@ mod tests { let mut encoded_bytes = encode( Protocol::BlocksByRoot, Version::V2, - RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(empty_base_block()))), ForkName::Altair, ) .unwrap(); @@ -1210,7 +1250,7 @@ mod tests { let mut encoded_bytes = encode( Protocol::BlocksByRoot, Version::V2, - RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(base_block()))), + RPCCodedResponse::Success(RPCResponse::BlocksByRoot(Box::new(empty_base_block()))), ForkName::Altair, ) .unwrap(); diff --git a/beacon_node/lighthouse_network/src/rpc/protocol.rs b/beacon_node/lighthouse_network/src/rpc/protocol.rs index d88f93de49f..1639d179412 100644 --- a/beacon_node/lighthouse_network/src/rpc/protocol.rs +++ b/beacon_node/lighthouse_network/src/rpc/protocol.rs @@ -118,6 +118,26 @@ pub fn max_rpc_size(fork_context: &ForkContext) -> usize { } } +/// Returns the rpc limits for beacon_block_by_range and beacon_block_by_root responses. +/// +/// Note: This function should take care to return the min/max limits accounting for all +/// previous valid forks when adding a new fork variant. +pub fn rpc_block_limits_by_fork(current_fork: ForkName) -> RpcLimits { + match ¤t_fork { + ForkName::Base => { + RpcLimits::new(*SIGNED_BEACON_BLOCK_BASE_MIN, *SIGNED_BEACON_BLOCK_BASE_MAX) + } + ForkName::Altair => RpcLimits::new( + *SIGNED_BEACON_BLOCK_BASE_MIN, // Base block is smaller than altair blocks + *SIGNED_BEACON_BLOCK_ALTAIR_MAX, // Altair block is larger than base blocks + ), + ForkName::Merge => RpcLimits::new( + *SIGNED_BEACON_BLOCK_BASE_MIN, // Base block is smaller than altair and merge blocks + *SIGNED_BEACON_BLOCK_MERGE_MAX, // Merge block is larger than base and altair blocks + ), + } +} + /// Protocol names to be used. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Protocol { @@ -281,32 +301,8 @@ impl ProtocolId { ::ssz_fixed_len(), ), Protocol::Goodbye => RpcLimits::new(0, 0), // Goodbye request has no response - Protocol::BlocksByRange => match fork_context.current_fork() { - ForkName::Base => { - RpcLimits::new(*SIGNED_BEACON_BLOCK_BASE_MIN, *SIGNED_BEACON_BLOCK_BASE_MAX) - } - ForkName::Altair => RpcLimits::new( - *SIGNED_BEACON_BLOCK_ALTAIR_MIN, - *SIGNED_BEACON_BLOCK_ALTAIR_MAX, - ), - ForkName::Merge => RpcLimits::new( - *SIGNED_BEACON_BLOCK_MERGE_MIN, - *SIGNED_BEACON_BLOCK_MERGE_MAX, - ), - }, - Protocol::BlocksByRoot => match fork_context.current_fork() { - ForkName::Base => { - RpcLimits::new(*SIGNED_BEACON_BLOCK_BASE_MIN, *SIGNED_BEACON_BLOCK_BASE_MAX) - } - ForkName::Altair => RpcLimits::new( - *SIGNED_BEACON_BLOCK_ALTAIR_MIN, - *SIGNED_BEACON_BLOCK_ALTAIR_MAX, - ), - ForkName::Merge => RpcLimits::new( - *SIGNED_BEACON_BLOCK_MERGE_MIN, - *SIGNED_BEACON_BLOCK_MERGE_MAX, - ), - }, + Protocol::BlocksByRange => rpc_block_limits_by_fork(fork_context.current_fork()), + Protocol::BlocksByRoot => rpc_block_limits_by_fork(fork_context.current_fork()), Protocol::Ping => RpcLimits::new( ::ssz_fixed_len(), diff --git a/testing/simulator/src/sync_sim.rs b/testing/simulator/src/sync_sim.rs index e328938db1d..3bb460c9fee 100644 --- a/testing/simulator/src/sync_sim.rs +++ b/testing/simulator/src/sync_sim.rs @@ -62,6 +62,9 @@ fn syncing_sim( let end_after_checks = true; let eth1_block_time = Duration::from_millis(15_000 / speed_up_factor); + // Set fork epochs to test syncing across fork boundaries + spec.altair_fork_epoch = Some(Epoch::new(1)); + spec.bellatrix_fork_epoch = Some(Epoch::new(2)); spec.seconds_per_slot /= speed_up_factor; spec.seconds_per_slot = max(1, spec.seconds_per_slot); spec.eth1_follow_distance = 16; @@ -86,6 +89,8 @@ fn syncing_sim( beacon_config.dummy_eth1_backend = true; beacon_config.sync_eth1_chain = true; + beacon_config.http_api.allow_sync_stalled = true; + beacon_config.network.enr_address = Some(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1))); // Generate the directories and keystores required for the validator clients.