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.

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

Add BlockByRangeV2 request #38

Merged
merged 7 commits into from
Oct 4, 2024
Merged

Add BlockByRangeV2 request #38

merged 7 commits into from
Oct 4, 2024

Conversation

cortze
Copy link
Contributor

@cortze cortze commented Oct 3, 2024

Description

We currently support all the libp2p protocols defined at the spec (when the requests are incoming); however, the current code doesn't allow us to request some of them actively (we miss requesting Blocks and Blobs).

This PR extends the list of Request that Hermes can do to support requesting BlocksByRangeV2:

12:10:41.298 INF got signed_beacon_block block_number=10093818 ssz_block_size=73653 from=16U...W5
12:10:41.581 INF got signed_beacon_block block_number=10093819 ssz_block_size=176430 from=16U...W5
12:10:41.581 INF got signed_beacon_block block_number=10093820 ssz_block_size=101148 from=16U...W5
12:10:41.582 INF got signed_beacon_block block_number=10093821 ssz_block_size=136029 from=16U...W5
12:10:41.787 INF got signed_beacon_block block_number=10093822 ssz_block_size=95587 from=16U...W5
12:10:41.788 INF got signed_beacon_block block_number=10093823 ssz_block_size=113059 from=16U...W5
12:10:41.991 INF got signed_beacon_block block_number=10093824 ssz_block_size=91325 from=16U...W5
12:10:41.991 INF got signed_beacon_block block_number=10093825 ssz_block_size=87732 from=16U...W5
12:10:41.992 INF got signed_beacon_block block_number=10093826 ssz_block_size=112571 from=16U...W5
12:10:42.195 INF got signed_beacon_block block_number=10093827 ssz_block_size=115009 from=16U...W5
12:10:42.196 INF got signed_beacon_block block_number=10093828 ssz_block_size=110787 from=16U...W5
12:10:42.196 INF got signed_beacon_block block_number=10093829 ssz_block_size=100021 from=16U...W5
12:10:42.196 INF got signed_beacon_block block_number=10093830 ssz_block_size=66931 from=16U...W5
12:10:42.403 INF got signed_beacon_block block_number=10093831 ssz_block_size=126812 from=16U...W5
12:10:42.404 INF got signed_beacon_block block_number=10093832 ssz_block_size=203308 from=16U...W5
12:10:42.607 INF got signed_beacon_block block_number=10093833 ssz_block_size=72100 from=16U...W5
12:10:42.608 INF got signed_beacon_block block_number=10093834 ssz_block_size=119006 from=16U...W5
12:10:42.608 INF got signed_beacon_block block_number=10093835 ssz_block_size=85242 from=16U...W5
12:10:42.725 INF got signed_beacon_block block_number=10093836 ssz_block_size=96021 from=16U...W5
12:10:42.726 INF got signed_beacon_block block_number=10093837 ssz_block_size=65819 from=16U...W5
12:10:42.727 INF got signed_beacon_block block_number=10093838 ssz_block_size=163464 from=16U...W5
12:10:42.912 INF got signed_beacon_block block_number=10093839 ssz_block_size=100097 from=16U...W5
12:10:42.913 INF got signed_beacon_block block_number=10093840 ssz_block_size=88027 from=16U...W5
12:10:42.913 INF got signed_beacon_block block_number=10093841 ssz_block_size=95594 from=16U...W5
12:10:42.914 INF got signed_beacon_block block_number=10093842 ssz_block_size=88193 from=16U...W5
12:10:43.118 INF got signed_beacon_block block_number=10093843 ssz_block_size=92441 from=16U...W5
12:10:43.119 INF got signed_beacon_block block_number=10093844 ssz_block_size=98026 from=16U...W5
12:10:43.119 INF got signed_beacon_block block_number=10093845 ssz_block_size=90036 from=16U...W5
12:10:43.321 INF got signed_beacon_block block_number=10093846 ssz_block_size=94171 from=16U...W5
12:10:43.322 INF got signed_beacon_block block_number=10093847 ssz_block_size=86861 from=16U...W5
12:10:43.322 INF got signed_beacon_block block_number=10093848 ssz_block_size=71480 from=16U...W5
12:10:43.523 INF got signed_beacon_block block_number=10093849 ssz_block_size=115651 from=16U...W5

The Trace would look like this:

{
     "Type":"REQUEST_BLOCKS_BY_RANGE",
     "PeerID":"16Uiu2HAkySPxdaiNDt5xiTPYeLjaQmmQer33MFqtd5Bp1VNhxsVj",
     "Timestamp":"2024-10-03T12:54:26.69345185+02:00",
     "Data": {
          "Duration":2253165821,
          "PeerID":"16Uiu2HAkzBWpMC8WQPmqN8FfoVnewnjVk551NVWw7JCVMihNtyW5",
          "ReceivedBlocks":32,
          "RequestedBlocks":32,
      }
}

OPTIONAL: If needed, I could also add the three missing calls (ref): BlocksByRoot, BlobSidecarByRange, and BlobSidecarByRoot), but I'll save the work bandwidth for now.

@cortze cortze requested a review from guillaumemichel October 3, 2024 11:01
@cortze cortze self-assigned this Oct 3, 2024
Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

LGTM, but it seems that BlocksByRangeV2 is never used. Is that normal?

@cortze
Copy link
Contributor Author

cortze commented Oct 3, 2024

There is no need to use it internally while the Hermes is running, but it's useful when you import Hermes as an EthNode service.

I could add some testing function if you think is necessary

@guillaumemichel
Copy link
Contributor

If it isn't too complicated it would be good to add some tests 👍🏻

@cortze
Copy link
Contributor Author

cortze commented Oct 3, 2024

My bad, I've added some tests for all the ReqResp calls, but it relies on a local Prysm node.
It is working locally:

probe-lab/hermes $ go test ./eth 
ok      github.com/probe-lab/hermes/eth 3.230s

Is it okay to Skip the test? I think that adding a docker image of Prysm to the test setup just to run these tests is overkill.

@guillaumemichel
Copy link
Contributor

Yes it is totally fine to skip this test.

@cortze cortze merged commit 3623e70 into main Oct 4, 2024
4 checks passed
@cortze cortze deleted the feat/add-block-reqresp branch October 4, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants