limit deneb block requests (by range and by root)

[mehdi-aouadi]

PR Description

No more than MAX_REQUEST_BLOCKS_DENEB may be requested at a time (BeaconBlocksByRange and BeaconBlocksByRoot)

Fixed Issue(s)

#6870

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[StefanBratanov]

Hmm think this is not correct. MAX_BLOCK_BY_RANGE_REQUEST_SIZE is used for rate limiting. Think can leave this value as it is for Deneb. We should see the usage of MAX_REQUEST_BLOCKS instead and change it accordingly for the Deneb fork.

[zilm13]

@StefanBratanov is MAX_BLOCK_BY_RANGE_REQUEST_SIZE anywhere in specs? I've not find. And 128 is smaller than 200 so we just rate limiting even more

[mehdi-aouadi]

@StefanBratanov is MAX_BLOCK_BY_RANGE_REQUEST_SIZE anywhere in specs? I've not find. And 128 is smaller than 200 so we just rate limiting even more

Actually MAX_BLOCK_BY_RANGE_REQUEST_SIZE is defined internally for peers sync limitation. Not needed anymore

[mehdi-aouadi]

Hmm think this is not correct. MAX_BLOCK_BY_RANGE_REQUEST_SIZE is used for rate limiting. Think can leave this value as it is for Deneb. We should see the usage of MAX_REQUEST_BLOCKS instead and change it accordingly for the Deneb fork.

Actually MAX_REQUEST_BLOCKS_DENEB < MAX_BLOCK_BY_RANGE_REQUEST_SIZE as @zilm13 mentioned so it doesn't make sense to use different limits in the method and in the peer, the smaller one will be enforced anyway. I added a unit test for the BeaconBlockByRange method when the count is greater than MAX_REQUEST_BLOCKS.
There is no MAX_REQUEST_BLOCKS check in the BeaconBlockByRoot because the BeaconBlocksByRootRequestMessage cannot contain more than MAX_REQUEST_BLOCKS elements anyway.

[zilm13]

LGTM

[StefanBratanov]

Hmm, not sure why MAX_BLOCK_BY_RANGE_REQUEST_SIZE was introduced. But there are usages of MAX_REQUEST_BLOCKS still which are definied in phase0 specs https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/p2p-interface.md. In my mind this constant is getting overriden for Deneb https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/p2p-interface.md so all usages of MAX_REQUEST_BLOCKS should be replaced by MAX_REQUEST_BLOCKS_DENEB once Deneb is active. Maybe @tbenr can jump in and have a look as well if he agrees with me.

[tbenr]

@mehdi-aouadi @StefanBratanov

MAX_BLOCK_BY_RANGE_REQUEST_SIZE has been introduced way back here #1061

I agree with @StefanBratanov that we should change at runtime the limit from MAX_REQUEST_BLOCKS to MAX_REQUEST_BLOCKS_DENEB

[mehdi-aouadi]

Done in: #6938