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

Implement BlobSidecarsByRoot RPC method #6844

Merged
merged 20 commits into from
Feb 27, 2023
Merged

Implement BlobSidecarsByRoot RPC method #6844

merged 20 commits into from
Feb 27, 2023

Conversation

StefanBratanov
Copy link
Contributor

@StefanBratanov StefanBratanov commented Feb 21, 2023 •
edited
Loading

PR Description

  • Will leave the implementation of wantToReceiveBlobSidecars to 6778 blob sidecars by range #6835
  • Created an issue to add more test cases to the integration test when the test fixture storage is updated for blob sidecars

Fixed Issue(s)

fixes #6830

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 StefanBratanov force-pushed the blob_sidecars_by_root_rpc branch from c4d09c7 to 9fb6907 Compare February 22, 2023 08:59
@zilm13 zilm13 mentioned this pull request Feb 22, 2023
Merged
2 tasks
@StefanBratanov StefanBratanov force-pushed the blob_sidecars_by_root_rpc branch from 3a5794a to 5d86d13 Compare February 22, 2023 17:33
@StefanBratanov StefanBratanov marked this pull request as ready for review February 22, 2023 17:46
@StefanBratanov StefanBratanov force-pushed the blob_sidecars_by_root_rpc branch 2 times, most recently from 71e44b0 to 44fcec9 Compare February 23, 2023 09:54
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 23, 2023
View reviewed changes
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/Eth2Peer.java
@@ -111,6 +119,8 @@
boolean wantToReceiveBlobsSidecars(
ResponseCallback<BlobsSidecar> callback, long blobsSidecarsCount);

boolean wantToReceiveBlobSidecars(ResponseCallback<BlobSidecar> callback, long blobSidecarsCount);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'callback' is never used.
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/Eth2Peer.java
@@ -111,6 +119,8 @@
boolean wantToReceiveBlobsSidecars(
ResponseCallback<BlobsSidecar> callback, long blobsSidecarsCount);

boolean wantToReceiveBlobSidecars(ResponseCallback<BlobSidecar> callback, long blobSidecarsCount);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'blobSidecarsCount' is never used.
@StefanBratanov StefanBratanov force-pushed the blob_sidecars_by_root_rpc branch from 5914094 to 2edb25a Compare February 24, 2023 09:31
zilm13
zilm13 reviewed Feb 25, 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.

Looks great, just needs a few more improvements

...pegasys/teku/spec/datastructures/networking/libp2p/rpc/BlobSidecarsByRootRequestMessage.java Outdated Show resolved Hide resolved
...tegration-test/java/tech/pegasys/teku/networking/eth2/BlobSidecarsByRootIntegrationTest.java Show resolved Hide resolved
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/DefaultEth2Peer.java Outdated Show resolved Hide resolved
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/DefaultEth2Peer.java Outdated Show resolved Hide resolved
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/DefaultEth2Peer.java Show resolved Hide resolved
...h/pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRootMessageHandler.java
identifier.getBlockRoot(), identifier.getIndex());
}

private void handleError(final ResponseCallback<BlobSidecar> callback, final Throwable error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks very generic. How difficult is to move it to the parent class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not very much but there are logs which are different and not straightforward to parameterize so thinking of leaving it for now

...h/pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRootMessageHandler.java
final UInt64 requestedEpoch = spec.computeEpochAtSlot(block.getSlot());
final UInt64 minimumRequestEpoch = computeMinimumRequestEpoch(finalizedEpoch);
if (requestedEpoch.isLessThan(minimumRequestEpoch)) {
throw new RpcException(
Copy link
Contributor

Choose a reason for hiding this comment

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

From the spec:
If any root in the request content references a block earlier than minimum_request_epoch, peers MAY respond with error code 3: ResourceUnavailable or not include the blob in the response..

So, it's MAY. Should we be so strict when responding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. It's good to have diversity, so decided to include it, since we already retrieve the block and it's cheap.

networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/peers/DefaultEth2Peer.java Show resolved Hide resolved
@StefanBratanov StefanBratanov force-pushed the blob_sidecars_by_root_rpc branch 2 times, most recently from e60436b to 48b32e9 Compare February 27, 2023 08:58
@StefanBratanov StefanBratanov force-pushed the blob_sidecars_by_root_rpc branch from 1df0182 to c86e2a7 Compare February 27, 2023 10:00
zilm13
zilm13 approved these changes Feb 27, 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.

LGTM,
I'd just ask @tbenr if he is ok with #6844 (comment)

@tbenr
Copy link
Contributor

tbenr commented Feb 27, 2023

LGTM, I'd just ask @tbenr if he is ok with #6844 (comment)

I'm ok with that.

@StefanBratanov StefanBratanov merged commit 40d002b into Consensys:master Feb 27, 2023
@StefanBratanov StefanBratanov deleted the blob_sidecars_by_root_rpc branch February 27, 2023 14:21
tbenr pushed a commit to tbenr/teku that referenced this pull request Feb 27, 2023
@StefanBratanov @tbenr
Implement BlobSidecarsByRoot RPC method (Consensys#6844)
3d8a163
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbenr tbenr tbenr left review comments

@zilm13 zilm13 zilm13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DECOUPLING] Implement BlobSidecarsByRoot RPC method
3 participants
@StefanBratanov @tbenr @zilm13