-
Notifications
You must be signed in to change notification settings - Fork 741
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
Make max_blobs_per_block a config parameter #6329
base: unstable
Are you sure you want to change the base?
Conversation
8b665b5
to
52bb581
Compare
let (block, blobs_vec) = | ||
generate_rand_block_and_blobs::<E>(ForkName::Deneb, NumBlobs::Random, &mut rng); | ||
let mut blobs: FixedVector<_, <E as EthSpec>::MaxBlobsPerBlock> = FixedVector::default(); | ||
let max_len = spec.max_blobs_per_block(block.epoch()) as usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The epoch here will be random right?
@@ -214,6 +262,85 @@ where | |||
} | |||
} | |||
|
|||
/// Emulates a SSZ `Vector`. | |||
#[derive(Clone, Debug)] | |||
pub struct RuntimeFixedList<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh we should probably move this to a separate file
@@ -676,7 +676,8 @@ impl<E: EthSpec> ExecutionBlockGenerator<E> { | |||
ForkName::Deneb | ForkName::Electra => { | |||
// get random number between 0 and Max Blobs | |||
let mut rng = self.rng.lock(); | |||
let num_blobs = rng.gen::<usize>() % (E::max_blobs_per_block() + 1); | |||
// TODO(pawan): thread the chainspec value here somehow | |||
let num_blobs = rng.gen::<usize>() % 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let num_blobs = rng.gen::<usize>() % 6; | |
let num_blobs = rng.gen::<usize>() % (6 + 1); |
if self.is_peer_das_enabled_for_epoch(epoch) { | ||
self.max_blobs_per_block | ||
} else { | ||
default_max_blobs_per_block() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? If we're changing this on devnets, then all clients would be using MAX_BLOBS_PER_BLOCK
from deneb, and we won't be able to interop with other clients. This config value is already configurable for some clients for Deneb, so if we're overriding this value, other clients would be using this value even before PeerDAS.
For PeerDAS purpose I guess we'd be using a new config value when the time comes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see what you're trying to do here - this function returns the max_blobs_per_block
for the epoch, so in the future it could return MAX_BLOBS_PER_BLOCK
for deneb, and MAX_BLOBS_PER_BLOCK_ELECTRA
if we're in Electra?
The issue i see with returning the default
is that if we start a devnet on Deneb
with a different MAX_BLOBS_PER_BLOCK
, we won't be able to interop with other clients (which is the main purpose of making this field configurable)
Self { | ||
block_root, | ||
verified_blobs: FixedVector::default(), | ||
// TODO(pawan): just make this a vec potentially |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can - this was previous an SSZ type because of overflow LRU cache and persisting to disk, but that's been dropped.
@@ -214,6 +262,85 @@ where | |||
} | |||
} | |||
|
|||
/// Emulates a SSZ `Vector`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really cover SSZ encoding and decoding
I've just added the |
} | ||
|
||
pub fn set_max_len(&mut self, max_len: usize) { | ||
self.max_len = Some(max_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should try to catch unexpected mutations after the max_len
is already set, or only allow it to be set once (would OnceCell
be an overkill for this?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this function is not used at all - maybe we could drop it? it feels like it could be a potential footgun
/// | ||
/// No mutating operation can be performed on an uninitialized instance | ||
/// without first setting `max_len`. | ||
pub fn empty_uninitialized() -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like this function is for convenience? the little downside is having to make max_len
optional when it's known most of the time. But I guess the convenience is worth it if we don't need to mutate it all.
@@ -296,17 +296,24 @@ impl BlockId { | |||
)) | |||
})? | |||
} else { | |||
BlobSidecarList::default() | |||
BlobSidecarList::empty_uninitialized() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max_len
is not specified here - i think it's not an issue since it's not encoded when BlobSidecarList
is used as a standalone object
.map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? | ||
if let Some(max_len) = list | ||
.first() | ||
.map(|sidecar| chain.spec.max_blobs_per_block(sidecar.epoch())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could use the block
slot so we could avoid using first
?
pub fn max_blobs_requested<E: EthSpec>(&self) -> u64 { | ||
self.count.saturating_mul(E::max_blobs_per_block() as u64) | ||
self.count.saturating_mul(MAX_BLOBS_PER_BLOCK_CEILING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may cause the node to hit the rate limit quicker because each range blob request will now consume max_blobs_requested
tokens from the quota
/// bounds checking and other non-consensus critical operations. | ||
/// | ||
/// For exact value, we should always check the chainspec. | ||
pub const MAX_BLOBS_PER_BLOCK_CEILING: u64 = 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could even go with a higher number if it's for non critical checks and preventing DOS, so we don't run into issues in upcoming upgrades and we don't have to frequently update this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this for #6462, but as far as I understood this value may change per fork right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it can change, but like i mentioned in the comment, it is to be used only for rate limiting and bounds checking of the incoming rpc request, so we can bump the number up in preparation for a fork
let max_len = if let Some(blob) = blobs.first() { | ||
self.chain.spec.max_blobs_per_block(blob.epoch()) as usize | ||
} else { | ||
6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_response
only returns Some
if all blob sidecars have been received, I think this may be unreachable? if we reach here it means add_response
is buggy and might be worth doing some error handling?
BlobSidecarList::from_vec(blobs, max_blobs_per_block as usize) | ||
} else { | ||
// This always implies that there were no blobs for this block_root | ||
BlobSidecarList::empty_uninitialized() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for now I think, but note it may break if we end up encoding this within another object like CacheItem
or AvailableBlock
(I don't see any use of encoding these rn)
We also need to move |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments, let me know what you think, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Pawan, this is awesome, I wasn't aware of this PR but this is what I had in mind with #6462, left a comment
/// bounds checking and other non-consensus critical operations. | ||
/// | ||
/// For exact value, we should always check the chainspec. | ||
pub const MAX_BLOBS_PER_BLOCK_CEILING: u64 = 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this for #6462, but as far as I understood this value may change per fork right?
Issue Addressed
N/A
Proposed Changes
Change
max_blobs_per_block
from a preset value to a config value. This affects a lot of the codebase where we have to use runtime variants of ssz List and Vector.