Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the MaxRequestBlocks type #4562

Closed
AgeManning opened this issue Aug 2, 2023 · 3 comments
Closed

Remove the MaxRequestBlocks type #4562

AgeManning opened this issue Aug 2, 2023 · 3 comments
Assignees

Comments

@AgeManning
Copy link
Member

Description

#4426 shifts a lot of the network configuration constants into the configuration file. There are some configurations still remaining to shift. One is the max_request_blocks configuration.

This one is a little bit trickier because we have specified a MaxRequestBlocks type as typenum::U1024 and we use it inside an ssz_types::VariableList.

The way that I see we can remedy this, is in most cases to use a VariableList<_,Usize::max> kind of deal. The second type parameter just sets an upper bound on how many elements we can ssz decode. Although it would be nice to set this to whatever is in the configuration file, this might not be easy to do. There are two easy paths:

  1. We can set some upper limit, which the configuration file cannot exceed. i.e U1024. The configuration can set lower bounds and it will be fine.
  2. We leave the struct unbounded by setting the largest possible value there.

In principle, if we are careful, we shouldn't need to rely on this type to limit the number of elements to decode. The inbound RPC should be bounded by the configuration file and we shouldn't be adding excess elements anyway. So although this type bound is nice, it shouldn't be strictly necessary, provided we have the checks when reading or requesting these objects on the RPC.

This needs to be checked.

Also open to any other solutions people may have. Maybe we can convert an arbitrary uint from configuration to its closest (rounded up) typenum and use that value?

@michaelsproul
Copy link
Member

michaelsproul commented Oct 6, 2023

Tagging this as Deneb, because some consumers of our /eth/v1/config/spec endpoint are expecting MAX_REQUEST_BLOCKS to be there, and cross-client VC<>BN interop may break if we don't remedy this before Deneb happens

@realbigsean realbigsean mentioned this issue Oct 12, 2023
@realbigsean
Copy link
Member

realbigsean commented Oct 12, 2023

I've started working on this. Essentially going with this suggestion:

We leave the struct unbounded by setting the largest possible value there.

Except in implementation, I'm making a wrapper around a vec that does the length check and requires it at construction and during decoding. Looks like this:

EDIT: I initially had from_ssz_bytes wrong here (corrected now), it's a little more complicated than I thought. Could be made easier if we made a helper method in the ssz crate. But I'm no longer sure this is worth doing over using a really big vec

use ssz::{Decode, Encode};
use ssz_derive::Encode;

#[derive(Debug, Clone, PartialEq, Encode)]
#[ssz(struct_behaviour = "transparent")]
pub struct RuntimeVariableList<T: Encode> {
    vec: Vec<T>,
    #[ssz(skip_serializing, skip_deserializing)]
    max_len: usize,
}

impl<T: Encode + Decode + Clone> RuntimeVariableList<T> {
    pub fn new(vec: Vec<T>, max_len: usize) -> Result<Self, ssz_types::Error> {
        if vec.len() <= max_len {
            Ok(Self { vec, max_len })
        } else {
            Err(ssz_types::Error::OutOfBounds {
                i: vec.len(),
                len: max_len,
            })
        }
    }

    pub fn to_vec(&self) -> Vec<T> {
        self.vec.clone()
    }

    pub fn len(&self) -> usize {
        self.vec.len()
    }

    pub fn from_ssz_bytes(bytes: &[u8], max_len: usize) -> Result<Self, ssz::DecodeError> {
        let vec = if bytes.is_empty() {
            vec![]
        } else if T::is_ssz_fixed_len() {
            let num_items = bytes
                .len()
                .checked_div(T::ssz_fixed_len())
                .ok_or(ssz::DecodeError::ZeroLengthItem)?;

            if num_items > max_len {
                return Err(ssz::DecodeError::BytesInvalid(format!(
                    "VariableList of {} items exceeds maximum of {}",
                    num_items, max_len
                )));
            }

            bytes
                .chunks(T::ssz_fixed_len())
                .try_fold(Vec::with_capacity(num_items), |mut vec, chunk| {
                    vec.push(T::from_ssz_bytes(chunk)?);
                    Ok(vec)
                })
                .map(Into::into)
        } else {
            ssz::decode_list_of_variable_length_items(bytes, Some(max_len))?
        };
        Ok(Self { vec, max_len })
    }
}

@realbigsean
Copy link
Member

implemented in #4841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants