feat: implement EIP-7691 increase blob throughput #7309

ensi321 · 2024-12-17T01:23:19Z

Implement EIP-7691.

Outstanding items that is not covered in this PR:

Make callers of requestSszTypeByMethod to provide fork info
Make InboundRateLimitQuota fork-aware so byPeer limit can be correctly set for BlobSidecarsByRange and BlobSidecarsByRoot.
Make ReqRespRateLimiter to re-set the rate limit on fork activation

codecov · 2024-12-18T22:48:25Z

Codecov Report

Attention: Patch coverage is 43.95604% with 51 lines in your changes missing coverage. Please review.

Project coverage is 48.60%. Comparing base (4b8de08) to head (760fbb8).

Additional details and impacted files

@@             Coverage Diff              @@
##           devnet-5    #7309      +/-   ##
============================================
- Coverage     48.61%   48.60%   -0.01%     
============================================
  Files           603      604       +1     
  Lines         40457    40516      +59     
  Branches       2065     2065              
============================================
+ Hits          19667    19694      +27     
- Misses        20752    20784      +32     
  Partials         38       38

ensi321 · 2024-12-18T23:08:58Z

Please ignore E2E Tests and spec tests as they will be addressed in the next PR.

nflaig · 2024-12-19T13:03:54Z

packages/beacon-node/src/network/network.ts

@@ -501,17 +501,30 @@ export class Network implements INetwork {
    peerId: PeerIdStr,
    request: deneb.BlobSidecarsByRangeRequest
  ): Promise<deneb.BlobSidecar[]> {
+    const fork = this.config.getForkName(this.clock.currentSlot);


need to check if it's correct to use clock slot here, what if we range sync and wanna get deneb blob sidecars but clock slot is already electra?

Good catch. I think we should use request.startSlot here. And if the request start slot is in deneb but the end slot is in electra, we still want to send out V1.

nflaig · 2024-12-19T13:16:52Z

packages/beacon-node/src/chain/validation/blobSidecar.ts

 ): Promise<void> {
  const blobSlot = blobSidecar.signedBlockHeader.message.slot;

  // [REJECT] The sidecar's index is consistent with `MAX_BLOBS_PER_BLOCK` -- i.e. `blob_sidecar.index < MAX_BLOBS_PER_BLOCK`.
-  if (blobSidecar.index >= chain.config.MAX_BLOBS_PER_BLOCK) {
+  const maxBlobsPerBlock = isForkPostElectra(fork)


Assigning maxBlobsPerBlock is done in multiple places, we could think about adding a helper for that as there will be likely more values in the future since we intend to bump the blob count further (likely in the next hard fork)

Maybe something similar to what the CL spec uses in their tests get_max_blob_count

Added constantsHelper.ts for the purpose of reducing number of conditional assignments in the codebase.

Can expand the scope of constantsHelper to outside of blobs in another PR

nflaig · 2024-12-20T00:47:32Z

packages/config/src/constantsHelper.ts

I wonder if those could just be part of ChainForkConfig, like config.getForkName() and only take in the fork

refactor: move helper to get max blobs per block to fork config #7322 (to see comparison in terms of semantics)

nflaig · 2024-12-20T00:50:51Z

packages/config/src/constantsHelper.ts

+  return isForkPostElectra(fork) ? config.MAX_BLOBS_PER_BLOCK_ELECTRA : config.MAX_BLOBS_PER_BLOCK;
+}
+
+export function getBlobSidecarSubnetCount(fork: ForkName, config: ChainConfig): number {


This isn't really used much compared to max blob per blob, also will likely go away with peerdas, might get away with not adding a helper

I removed this in my PR #7322 as it's already pretty encapsulated where it is used

packages/state-transition/src/block/processExecutionPayload.ts

packages/beacon-node/src/chain/validation/blobSidecar.ts

g11tech · 2024-12-20T08:34:27Z

packages/beacon-node/src/network/network.ts

+      this.sendReqRespRequest(
+        peerId,
+        ReqRespMethod.BlobSidecarsByRoot,
+        [isForkPostElectra(fork) ? Version.V2 : Version.V1],


whats different in v2?

packages/beacon-node/src/network/processor/gossipHandlers.ts

g11tech · 2024-12-20T08:38:54Z

packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts

@@ -207,7 +207,8 @@ export class ReqRespBeaconNode extends ReqResp {
    versions: number[],
    request: Req
  ): AsyncIterable<ResponseIncoming> {
-    const requestType = requestSszTypeByMethod(this.config)[method];
+    const fork = ForkName[ForkSeq[this.currentRegisteredFork] as keyof typeof ForkName];
+    const requestType = requestSszTypeByMethod(this.config, fork)[method];


we need this for v1/v2?

g11tech · 2024-12-20T08:41:15Z

packages/beacon-node/src/network/reqresp/handlers/index.ts

@@ -38,7 +39,8 @@ export function getReqRespHandlers({db, chain}: {db: IBeaconDb; chain: IBeaconCh
      return onBeaconBlocksByRoot(body, chain, db);
    },
    [ReqRespMethod.BlobSidecarsByRoot]: (req) => {
-      const body = BlobSidecarsByRootRequestType(chain.config).deserialize(req.data);
+      const fork = req.version === Version.V2 ? ForkName.electra : ForkName.deneb;
+      const body = BlobSidecarsByRootRequestType(fork, chain.config).deserialize(req.data);


wouldn't type be the same just that v2 will have context bytes/fork type?

g11tech · 2024-12-20T08:43:51Z

packages/params/src/index.ts

@@ -255,11 +255,11 @@ export const BLOB_TX_TYPE = 0x03;
 export const VERSIONED_HASH_VERSION_KZG = 0x01;

 // ssz.deneb.BeaconBlockBody.getPathInfo(['blobKzgCommitments',0]).gindex
-export const KZG_COMMITMENT_GINDEX0 = ACTIVE_PRESET === PresetName.minimal ? 864 : 221184;
+export const KZG_COMMITMENT_GINDEX0 = ACTIVE_PRESET === PresetName.minimal ? 1728 : 221184;


this changed for electra or was the value incorrect? then gindex0 constant needs to be forkaware

in the spec PR the minimal preset values for deneb were changed here, I thought you have to add new constants for that but apparently not

but agree the gindex should be fork aware

then this will break the current deneb minimal devnets

ahh because we retrospectively changed deneb max vals, sounds very random to me to do (in spec)

ensi321 force-pushed the devnet-5 branch from 324562a to 031214e Compare December 17, 2024 06:22

ensi321 added 2 commits December 16, 2024 22:22

Init

13d6448

Add reqresp v2 definition

a8d9f73

ensi321 force-pushed the nc/eip-7691 branch from dc398e5 to a8d9f73 Compare December 17, 2024 06:22

ensi321 added 7 commits December 16, 2024 22:25

Update validateGossipBlock

e6984f4

Partial commit

6b48588

Reqresp. Add todos

1722971

polish

f031dc3

Fork-aware requestSszTypeByMethod

a8f34b8

Fixed minimal constants

d3361b4

Bump config test version

ebc2aae

ensi321 marked this pull request as ready for review December 18, 2024 23:07

ensi321 requested a review from a team as a code owner December 18, 2024 23:07

nflaig added 7 commits December 19, 2024 12:16

Merge branch 'devnet-5' into nc/eip-7691

7a51d37

Update blob sidecar subnet computation

13ad885

Pass proper commitment limit to block gossip error

2c68274

Update blob sidecar index check

c22f78b

Lint

aa8a218

Update kzg unit test

41be261

Subscribe to correct number of blob sidecar subnets

1c2b5ed

nflaig reviewed Dec 19, 2024

View reviewed changes

ensi321 added 2 commits December 19, 2024 16:06

Refactor constants getter to constantsHelper

a267c34

address comment

e813294

nflaig reviewed Dec 20, 2024

View reviewed changes

g11tech reviewed Dec 20, 2024

View reviewed changes

packages/beacon-node/src/chain/validation/blobSidecar.ts Outdated Show resolved Hide resolved

g11tech reviewed Dec 20, 2024

View reviewed changes

packages/beacon-node/src/network/processor/gossipHandlers.ts Outdated Show resolved Hide resolved

g11tech reviewed Dec 20, 2024

View reviewed changes

nflaig added 2 commits December 20, 2024 10:07

Pass fork as first arg

744c0be

Update packages/state-transition/src/block/processExecutionPayload.ts

760fbb8

nflaig mentioned this pull request Dec 20, 2024

refactor: move helper to get max blobs per block to fork config #7322

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement EIP-7691 increase blob throughput #7309

feat: implement EIP-7691 increase blob throughput #7309

ensi321 commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

ensi321 commented Dec 18, 2024

nflaig Dec 19, 2024

ensi321 Dec 20, 2024

nflaig Dec 19, 2024

ensi321 Dec 20, 2024

nflaig Dec 20, 2024

nflaig Dec 20, 2024

nflaig Dec 20, 2024

nflaig Dec 20, 2024

g11tech Dec 20, 2024

g11tech Dec 20, 2024 •

edited

Loading

g11tech Dec 20, 2024

g11tech Dec 20, 2024

nflaig Dec 20, 2024

g11tech Dec 23, 2024

g11tech Dec 23, 2024

feat: implement EIP-7691 increase blob throughput #7309

Are you sure you want to change the base?

feat: implement EIP-7691 increase blob throughput #7309

Conversation

ensi321 commented Dec 17, 2024 • edited Loading

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

ensi321 commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ensi321 commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

g11tech Dec 20, 2024 •

edited

Loading