Skip to content

RequestType::max_responses for LightClientUpdatesByRange #6534

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

Merged

Conversation

ackintosh
Copy link
Member

Issue Addressed

In case of RequestType::LightClientUpdatesByRange, RequestType::max_responses() returns the number that LightClientUpdatesByRangeRequest::max_requested() returns, which is a constant value representing the maximum number of LightClientUpdate instances in a single request. I think it should return the actual number of instances the request requires, instead of the constant value defined in the spec.

@ackintosh ackintosh added the ready-for-review The code is ready for review label Oct 22, 2024
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.

Good catch!

This is also consistent with the spec, with the max size enforced in the handling method:

The response MUST NOT contain more than min(MAX_REQUEST_LIGHT_CLIENT_UPDATES, count) results

@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 22, 2024
@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Oct 22, 2024

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.

@jimmygchen
Copy link
Member

Failed due to crates.io download issue, will requeue.

@jimmygchen
Copy link
Member

@mergify requeue

Copy link

mergify bot commented Oct 22, 2024

requeue

✅ This pull request will be re-embarked automatically

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

Copy link

mergify bot commented Oct 22, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 40d3423

mergify bot added a commit that referenced this pull request Oct 22, 2024
@mergify mergify bot merged commit 40d3423 into sigp:unstable Oct 22, 2024
29 checks passed
@ackintosh ackintosh deleted the max-response-lightclient-updates-by-range branch October 23, 2024 20:29
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
* return the actual number of instances the request requires
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants