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.

Sign up for GitHub

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

Jump to bottom

6778 blob sidecars by range #6835

Merged
merged 30 commits into from
Feb 28, 2023
Merged

6778 blob sidecars by range #6835

merged 30 commits into from
Feb 28, 2023

Conversation

mehdi-aouadi
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi commented Feb 17, 2023 •
edited
Loading

PR Description

Add BlobSidecarsByRange RPC method
SPEC

Fixed Issue(s)

#6778

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.

@mehdi-aouadi mehdi-aouadi force-pushed the 6778-blob-sidecars-by-range branch 2 times, most recently from 0e24838 to 446ee96 Compare February 17, 2023 20:20
@mehdi-aouadi mehdi-aouadi marked this pull request as draft February 20, 2023 07:48
@mehdi-aouadi mehdi-aouadi self-assigned this Feb 20, 2023
@mehdi-aouadi mehdi-aouadi force-pushed the 6778-blob-sidecars-by-range branch from 446ee96 to 27cc5a4 Compare February 20, 2023 17:56
@mehdi-aouadi mehdi-aouadi marked this pull request as ready for review February 21, 2023 08:53
zilm13
zilm13 reviewed Feb 21, 2023
View reviewed changes
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not looked at BlobSidecarsByRangeMessageHandlerTest yet but it may need some changes too in a line with spec logic

...egasys/teku/spec/datastructures/networking/libp2p/rpc/BlobSidecarsByRangeRequestMessage.java Outdated Show resolved Hide resolved
...c/main/java/tech/pegasys/teku/beacon/sync/forward/multipeer/chains/ThrottlingSyncSource.java Outdated Show resolved Hide resolved
...c/main/java/tech/pegasys/teku/beacon/sync/forward/multipeer/chains/ThrottlingSyncSource.java Outdated Show resolved Hide resolved
.../pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRangeMessageHandler.java Show resolved Hide resolved
.../pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRangeMessageHandler.java Outdated Show resolved Hide resolved
.../pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRangeMessageHandler.java Outdated Show resolved Hide resolved
.../pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRangeMessageHandler.java Outdated Show resolved Hide resolved
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/Eth2PeerFactory.java Outdated Show resolved Hide resolved
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/DefaultEth2Peer.java Show resolved Hide resolved
ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/generator/ChainBuilder.java Show resolved Hide resolved
@mehdi-aouadi mehdi-aouadi requested a review from zilm13 February 23, 2023 10:32
@StefanBratanov StefanBratanov mentioned this pull request Feb 23, 2023
Merged
2 tasks
zilm13
zilm13 reviewed Feb 26, 2023
View reviewed changes
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I sound too picky on this, our network behavior could lead to bad relations with other clients and I want to have a good relations :)

...c/main/java/tech/pegasys/teku/beacon/sync/forward/multipeer/chains/ThrottlingSyncSource.java Outdated Show resolved Hide resolved
...c/main/java/tech/pegasys/teku/beacon/sync/forward/multipeer/chains/ThrottlingSyncSource.java Outdated Show resolved Hide resolved
...st/java/tech/pegasys/teku/beacon/sync/forward/multipeer/chains/ThrottlingSyncSourceTest.java Outdated Show resolved Hide resolved
...egasys/teku/spec/datastructures/networking/libp2p/rpc/BlobSidecarsByRangeRequestMessage.java Outdated Show resolved Hide resolved
...sys/teku/networking/eth2/rpc/beaconchain/methods/BlobsSidecarsByRangeMessageHandlerTest.java Outdated Show resolved Hide resolved
networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/peers/Eth2PeerTest.java Outdated Show resolved Hide resolved
...rking/eth2/src/testFixtures/java/tech/pegasys/teku/networking/eth2/peers/StubSyncSource.java Outdated Show resolved Hide resolved
@mehdi-aouadi mehdi-aouadi mentioned this pull request Feb 27, 2023
Closed
StefanBratanov
StefanBratanov requested changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw we need to add blobSidecarsByRange.ifPresent(allMethods::add) in the BeaconChainMethods constructor

This was referenced Feb 28, 2023
Closed
Closed
@mehdi-aouadi
Copy link
Contributor Author

Btw we need to add blobSidecarsByRange.ifPresent(allMethods::add) in the BeaconChainMethods constructor

Good catch. I added it ;)

@mehdi-aouadi
Handle requested blobs count zero
7d4e3ac
@mehdi-aouadi
remove request validation
5adb5ee
@mehdi-aouadi
remove unused method
d385f18
@mehdi-aouadi
cleanup unused methods
b940370
@mehdi-aouadi
remove unused variables
cd75523
@mehdi-aouadi
add slot and indices iterator tests
dbe05d2
@mehdi-aouadi
fix imports
e173788
@mehdi-aouadi
refactor slot and indices iteration
1e8e492
@mehdi-aouadi
refactor getBlobSidecarByBlockRootAndIndex
bbbc5d7
@mehdi-aouadi
rename supplier
611aea9
@mehdi-aouadi
remove unused parent block root
8117138
@mehdi-aouadi
rename getMaximumRequestChunks
d197efd
@mehdi-aouadi
remove unused method
95d35c7
@mehdi-aouadi
do no send request to peer when count is zero
9fc9c56
@mehdi-aouadi
snake case
cb4f734
@mehdi-aouadi
remove irrelevant comment
1db1110
@mehdi-aouadi
ignore request when count is zero
f06aa43
@mehdi-aouadi
move peers requests throttling delay to constants
4980eb1
@mehdi-aouadi
remove unnecessary test
1bf25a2
@mehdi-aouadi
use global duration
c1e69fe
@mehdi-aouadi
add blob sidecars by range to the beacon chain method
e6437c2
@mehdi-aouadi
rebase fixes
afdcef8
@mehdi-aouadi mehdi-aouadi force-pushed the 6778-blob-sidecars-by-range branch from 7f85387 to afdcef8 Compare February 28, 2023 10:54
StefanBratanov
StefanBratanov approved these changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@StefanBratanov StefanBratanov left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will improve the iteration and fetching of blob sidecars in a next PR.

@mehdi-aouadi mehdi-aouadi merged commit 63d0367 into Consensys:master Feb 28, 2023
@mehdi-aouadi mehdi-aouadi deleted the 6778-blob-sidecars-by-range branch February 28, 2023 11:18
rolfyone pushed a commit to rolfyone/teku that referenced this pull request Mar 1, 2023
@mehdi-aouadi @rolfyone
6778 blob sidecars by range (Consensys#6835)
164a68b 
* Add BlobSidecarsByRange RPC method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanBratanov StefanBratanov StefanBratanov approved these changes

@zilm13 zilm13 Awaiting requested review from zilm13

@tbenr tbenr Awaiting requested review from tbenr

Assignees

@mehdi-aouadi mehdi-aouadi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mehdi-aouadi @zilm13 @StefanBratanov @tbenr