Skip to content

Fix BlobsByRange by reverting PR6462 #6526

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
Merged
merged 1 commit into from
Oct 21, 2024
Merged

Fix BlobsByRange by reverting PR6462 #6526

merged 1 commit into from
Oct 21, 2024

Conversation

michaelsproul
Copy link
Member

Issue Addressed

This reverts PR:

That PR accidentally broke BlobsByRange requests by adding a non-standard field to the SSZ representation (sent over the network). The result is that peers would not respond to Lighthouse's BlobsByRange requests and Lighthouse would be unable to sync.

@michaelsproul michaelsproul added bug Something isn't working ready-for-review The code is ready for review v6.0.0 New major release for hierarchical state diffs labels Oct 21, 2024
@michaelsproul michaelsproul changed the title Revert PR #6462 Fix BlobsByRange by reverting PR6462 Oct 21, 2024
jimmygchen
jimmygchen approved these changes Oct 21, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

LGTM! Nice find 🎉
We can revisit the change once #6329 is merged.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 21, 2024
@jimmygchen
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Oct 21, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The merge 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.

mergify bot added a commit that referenced this pull request Oct 21, 2024
@mergify
Merge of #6526
c52bde1
@mergify mergify bot mentioned this pull request Oct 21, 2024
Closed
5 tasks
@michaelsproul
Copy link
Member Author

@mergify requeue

@mergify Mergify
Copy link

mergify bot commented Oct 21, 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 Oct 21, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 9aefb55

mergify bot added a commit that referenced this pull request Oct 21, 2024
@mergify
Merge of #6526
5ed7db9
@mergify mergify bot mentioned this pull request Oct 21, 2024
Closed
5 tasks
@mergify mergify bot merged commit 9aefb55 into sigp:unstable Oct 21, 2024
29 checks passed
@michaelsproul michaelsproul deleted the revert-6462 branch October 21, 2024 22:12
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
@michaelsproul @chong-he
Fix BlobsByRange by reverting PR6462 (sigp#6526)
6c737e9 
* Revert "Remove generic E from RequestId (sigp#6462)"

This reverts commit 772929f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmygchen jimmygchen jimmygchen approved these changes

@dapplion dapplion dapplion approved these changes

Assignees
No one assigned
Labels
bug Something isn't working Networking ready-for-merge This PR is ready to merge. v6.0.0 New major release for hierarchical state diffs
Projects
None yet
Milestone
No milestone
Development

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