feat: make MAX_REQUEST_BLOB_SIDECARS and MAX_BLOBS_PER_BLOCK configurable
#7294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ensi321
changed the title
MAX_REQUEST_BLOB_SIDECARS and MAX_BLOBS_PER_BLOCK configurableMAX_REQUEST_BLOB_SIDECARS and MAX_BLOBS_PER_BLOCK configurable
ensi321
changed the title
MAX_REQUEST_BLOB_SIDECARS and MAX_BLOBS_PER_BLOCK configurableMAX_REQUEST_BLOB_SIDECARS and MAX_BLOBS_PER_BLOCK configurable
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #7294 +/- ##
============================================
- Coverage 48.91% 48.76% -0.15%
============================================
Files 601 601
Lines 40188 40195 +7
Branches 2061 2061
============================================
- Hits 19658 19602 -56
- Misses 20492 20555 +63
Partials 38 38 |
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
approved these changes
Dec 13, 2024
| @@ -198,7 +197,6 @@ export const SYNC_COMMITTEE_SUBNET_SIZE = Math.floor(SYNC_COMMITTEE_SIZE / SYNC_ | |||
|
|
|||
| export const MAX_REQUEST_BLOCKS = 2 ** 10; // 1024 | |||
| export const MAX_REQUEST_BLOCKS_DENEB = 2 ** 7; // 128 | |||
There was a problem hiding this comment.
as per spec those should be part of config as well, see configs/mainnet.yaml#L146
| @@ -12,3 +13,7 @@ export const signedBLSToExecutionChangeVersionedType = new ContainerType( | |||
| {jsonCase: "eth2", typeName: "SignedBLSToExecutionChangeVersionedType"} | |||
| ); | |||
| export type SignedBLSToExecutionChangeVersioned = ValueOf<typeof signedBLSToExecutionChangeVersionedType>; | |||
|
|
|||
| export const BlobSidecarsByRootRequestType = (config: Pick<ChainConfig, "MAX_REQUEST_BLOB_SIDECARS">) => | |||
There was a problem hiding this comment.
I don't think it's helpful to pick just the specific key, this is actually more error prone than making sure the proper config object is passed
| @@ -213,7 +213,7 @@ export function getCoreTopicsAtFork( | |||
|
|
|||
| // After Deneb also track blob_sidecar_{index} | |||
| if (ForkSeq[fork] >= ForkSeq.deneb) { | |||
| for (let index = 0; index < MAX_BLOBS_PER_BLOCK; index++) { | |||
| for (let index = 0; index < config.MAX_BLOBS_PER_BLOCK; index++) { | |||
There was a problem hiding this comment.
just a side note, we should use BLOB_SIDECAR_SUBNET_COUNT here instead, currently this does not matter as BLOB_SIDECAR_SUBNET_COUNT == MAX_BLOBS_PER_BLOCK but would be good to clean up some open todos left over from Deneb
Will put up a separate PR for that
| @@ -62,8 +61,6 @@ export const BlobIdentifier = new ContainerType( | |||
| {typeName: "BlobIdentifier", jsonCase: "eth2"} | |||
| ); | |||
|
|
|||
| export const BlobSidecarsByRootRequest = new ListCompositeType(BlobIdentifier, MAX_REQUEST_BLOB_SIDECARS); | |||
There was a problem hiding this comment.
moving this out of types should be fine as it's not a ssz container defined in the spec, only specs/deneb/p2p-interface.md?plain=1#L284
nflaig
requested changes
Dec 13, 2024
nflaig
approved these changes
Dec 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We need to make these two parameters configurable before we implement EIP-7691
MAX_BLOBS_PER_BLOCKwill need to haveconfigpassed inBlobSidecarsByRootRequestout ofpackages/typestopackages/beacon-nodesince it is now "dynamic' depending onconfig.MAX_REQUEST_BLOB_SIDECARS.BlobSidecarsByRootRequestwill need to accessconfig.Closes #7172