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

feat(consensus): add the max_request_blocks field in ChainSpec #4800

Closed
wants to merge 2 commits into from

Conversation

duguorong009
Copy link
Contributor

Issue Addressed

Proposed Changes

  • Update the ChainSpec struct of consensus crate
    Add the max_request_blocks field

Additional Info

Originally, this PR is for addressing the issue of missing the MAX_REQUEST_BLOCKS info from lighthouse beacon API /eth/v1/config/spec.
While resolving the issue, I found that we don't need to update anything from BN crate code, but update the ChainSpec from consensus crate.
Hence, the PR is for updating the consensus crate, adding one more field to ChainSpec struct and related codes.

@chong-he chong-he added enhancement New feature or request v4.6.0 ETA Q1 2024 ready-for-review The code is ready for review labels Oct 3, 2023
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Sorry, it looks like this problem is a bit more involved than described on the issue. We not only need to include the max_request_blocks in the chain spec, but also need to use this value in place of the existing MAX_REQUEST_BLOCKS constant. This will require refactoring some of the networking code, and should probably happen as part of the Deneb work (to avoid merge conflicts, and because Deneb also touches this code). I'll close the original issue and open a new one.

@divagant-martian
Copy link
Collaborator

There is #4562 @michaelsproul

@michaelsproul
Copy link
Member

Ah! Thanks @divagant-martian, didn't see that!

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 6, 2023
@duguorong009
Copy link
Contributor Author

@michaelsproul
So, what do you want me to do?
As far as I see, this PR is for Deneb work.
So, simply change the merge destination branch to Deneb one?

@divagant-martian
Copy link
Collaborator

divagant-martian commented Oct 12, 2023

HI @duguorong009 bare in mind that I'm not a team member but I know about this to some depth. What @michaelsproul is describing is that the changes you made address the issue partially and we want to address it fully. "Using" this value is what #4562 describes. This has not been done because in terms of code, it's not clear for us what's the best way to do it. Or at least it's definitely not straightforward.

This being said, re-targeting to deneb is not going to be enough, sadly

@duguorong009
Copy link
Contributor Author

duguorong009 commented Oct 12, 2023

Thanks for the quick answer! @divagant-martian
It seems like this PR needs to wait for the discussion result. (currently, leaving as is)

If you and @michaelsproul find a wayout, and think this PR can continue, please let me know.

@michaelsproul
Copy link
Member

Thanks for your work on this @duguorong009, it helped us clarify the issue. I'm going to close your PR now in favour of the more involved fix here: #4841.

@duguorong009 duguorong009 deleted the gr/issue-4711 branch January 25, 2024 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v4.6.0 ETA Q1 2024 waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants