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 PeerDAS RPC handlers #6237

Merged
merged 4 commits into from
Aug 15, 2024
Merged

Implement PeerDAS RPC handlers #6237

merged 4 commits into from
Aug 15, 2024

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Aug 7, 2024 •
edited by jimmygchen
Loading

Issue Addressed

Continue porting network changes of das branch. Extends #6238

Part of

all branches are on the sipg/lighthouse repo to start another PR conga :)

Proposed Changes

  • Implement serving RPC handlers for data_columns_by_root and data_columns_by_range.
  • This PR excludes the feature to reprocess early sampling requests.

Upstream PRs:

@dapplion dapplion added do-not-merge das Data Availability Sampling das-unstable-merge labels Aug 7, 2024
@jimmygchen jimmygchen mentioned this pull request Aug 7, 2024
Closed
@jimmygchen jimmygchen changed the base branch from peerdas-network-boilerplate to unstable August 13, 2024 05:30
@jimmygchen jimmygchen changed the base branch from unstable to peerdas-network-processor August 13, 2024 05:42
@jimmygchen
Copy link
Member

Oops I thought this was the first PR in the conga line and incorrectly set the base branch to unstable. I've reverted my merge commit and it's now back to what it was before.

@jimmygchen
Copy link
Member

I've merged latest base branch into this branch so i can extend the conga line.

@jimmygchen jimmygchen mentioned this pull request Aug 13, 2024
Merged
realbigsean
realbigsean reviewed Aug 13, 2024
View reviewed changes
beacon_node/network/src/network_beacon_processor/rpc_methods.rs
@@ -320,14 +320,53 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
pub fn handle_data_columns_by_root_request(
Copy link
Member

Choose a reason for hiding this comment

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

should this use terminate_response_stream like we use in other similar methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in aa0b903

realbigsean
realbigsean reviewed Aug 13, 2024
View reviewed changes
beacon_node/network/src/network_beacon_processor/rpc_methods.rs
self.log,
"Received DataColumnsByRoot Request";
"peer" => %peer_id,
"request" => ?request.group_by_ordered_block_root(),
Copy link
Member

Choose a reason for hiding this comment

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

what is the benefit of ordering by block roots in this log?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I introduced group_by_ordered_block_root originally to handle the reprocessing of requests. I left it here as it produces a more succint output. The raw request repeats the root for every column. Usually peers requests multiple indexes for the same root, so the output of this is nicer.

realbigsean
realbigsean approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

one small suggestion and one question but looks good

@dapplion dapplion changed the base branch from peerdas-network-processor to unstable August 13, 2024 21:37
@dapplion
Copy link
Collaborator Author

@jimmygchen I have forced push a rebase of this branch against unstable, as the commit from #6238 had leaked into this PR

@dapplion dapplion added ready-for-review The code is ready for review and removed do-not-merge labels Aug 13, 2024
@realbigsean
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Aug 15, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@realbigsean
Copy link
Member

@mergify refresh

@mergify Mergify
Copy link

mergify bot commented Aug 15, 2024

refresh

✅ Pull request refreshed

@realbigsean
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Aug 15, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@realbigsean
Copy link
Member

@mergify dequeue

@mergify Mergify
Copy link

mergify bot commented Aug 15, 2024

dequeue

☑️ The pull request is not queued

@realbigsean
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Aug 15, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@realbigsean
Copy link
Member

@mergify requeue

@mergify Mergify
Copy link

mergify bot commented Aug 15, 2024

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

@mergify Mergify
Copy link

mergify bot commented Aug 15, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 6566705

@mergify mergify bot merged commit 6566705 into unstable Aug 15, 2024
28 checks passed
@mergify mergify bot deleted the peerdas-network-rpc-handler branch August 15, 2024 21:15
AgeManning pushed a commit to AgeManning/lighthouse that referenced this pull request Sep 3, 2024
@dapplion @AgeManning
Implement PeerDAS RPC handlers (sigp#6237)
e7b1d5e 
* Implement PeerDAS RPC handlers

* use terminate_response_stream

* Merge branch 'unstable' of https://github.com/sigp/lighthouse into peerdas-network-rpc-handler

* cargo fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@realbigsean realbigsean realbigsean approved these changes

@jimmygchen jimmygchen Awaiting requested review from jimmygchen

Assignees
No one assigned
Labels
das Data Availability Sampling das-unstable-merge ready-for-review The code is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dapplion @jimmygchen @realbigsean