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

Runtime rpc request sizes #4841

Merged
merged 19 commits into from
Jan 8, 2024
Merged

Runtime rpc request sizes #4841

merged 19 commits into from
Jan 8, 2024

Conversation

realbigsean
Copy link
Member

Issue Addressed

#4562

Proposed Changes

  • Moves networking config from constants / types to ChainSpec type
  • Introduces a new ssz type RuntimeVariableList that's used in RpcRequests

Questions

  • Should RuntimeVariableList be added to the ssz types crate?
  • I added ChainSpec as well as some values we derive from new ChainSpec values to ForkContext, should these be moved elsewhere?
  • A potential benefit of leaving these derived values (like blocks_by_root_request_max) in ForkContext is that if they change across a fork we can dynamically update them in RpcLimits for example
  • Sort of related to the last point - do we want to dynamically update blocks_by_root_request_max across the fork? Same question for max_request_blocks. max_request_blocks decreases from 1024 to 128 in deneb. Another option would be to just use the higher value until we are through the deneb fork and in the next release, release the lower value. Or do the opposite and use the lower value in the deneb release.

@realbigsean realbigsean added ready-for-review The code is ready for review deneb labels Oct 13, 2023
@realbigsean realbigsean mentioned this pull request Oct 13, 2023
Merged
pawanjay176
pawanjay176 requested changes Oct 13, 2023
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Partial review, but I'm keen to get the switching of the max request values at the fork.

I know @AgeManning disagreed with this approach earlier so keen to hear his thoughts too.

beacon_node/beacon_processor/src/lib.rs Show resolved Hide resolved
consensus/types/src/chain_spec.rs Show resolved Hide resolved
beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs Outdated Show resolved Hide resolved
beacon_node/lighthouse_network/src/rpc/config.rs Show resolved Hide resolved
beacon_node/lighthouse_network/src/rpc/config.rs Show resolved Hide resolved
@realbigsean
Copy link
Member Author

Ok, think I've addressed everything so far!

@michaelsproul michaelsproul deleted the branch sigp:unstable October 16, 2023 23:58
@michaelsproul michaelsproul reopened this Oct 17, 2023
@michaelsproul michaelsproul changed the base branch from deneb-free-blobs to unstable October 17, 2023 00:01
@michaelsproul michaelsproul mentioned this pull request Oct 25, 2023
Closed
@realbigsean realbigsean added the v4.6.0 ETA Q1 2024 label Jan 5, 2024
@jimmygchen jimmygchen self-assigned this Jan 7, 2024
jimmygchen
jimmygchen requested changes Jan 8, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, looks good to me.
I've got two minor comments above, I'll create a PR for the changes if I have time today!

EDIT: sorry looks like i won't have time to get to this today.

paulhauner added a commit to paulhauner/lighthouse that referenced this pull request Jan 8, 2024
@paulhauner
Runtime rpc request sizes (sigp#4841)
da59ed2 
Squashed commit of the following:

commit a0c5343
Merge: 3a63dc4 0c97762
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 8 10:37:01 2024 +1100

    Merge branch 'unstable' into runtime-rpc-request-sizes

commit 3a63dc4
Merge: 3326584 01994c4
Author: realbigsean <seananderson33@GMAIL.com>
Date:   Fri Jan 5 11:23:23 2024 -0500

    Merge branch 'unstable' of https://github.com/sigp/lighthouse into runtime-rpc-request-sizes

commit 3326584
Author: realbigsean <seananderson33@gmail.com>
Date:   Fri Dec 8 16:18:47 2023 -0500

    fix compile

commit d97b161
Merge: d350ea3 46184e5
Author: realbigsean <seananderson33@gmail.com>
Date:   Fri Dec 8 16:09:57 2023 -0500

    Merge branch 'unstable' of https://github.com/sigp/lighthouse into runtime-rpc-request-sizes

commit d350ea3
Author: realbigsean <seananderson33@gmail.com>
Date:   Mon Oct 16 16:09:17 2023 -0400

    get values off chain spec

commit b9c5926
Author: realbigsean <seananderson33@gmail.com>
Date:   Mon Oct 16 13:37:54 2023 -0400

    move methods for per-fork-spec to chainspec

commit dadfa1b
Author: realbigsean <seananderson33@gmail.com>
Date:   Mon Oct 16 13:08:02 2023 -0400

    add docs fix compilt

commit 936af46
Author: realbigsean <seananderson33@gmail.com>
Date:   Mon Oct 16 12:58:14 2023 -0400

    add new config to `Config` api struct

commit 7b9f51e
Merge: 150a1d8 ba0567d
Author: realbigsean <seananderson33@gmail.com>
Date:   Mon Oct 16 11:59:25 2023 -0400

    Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse into runtime-rpc-request-sizes

commit 150a1d8
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Oct 12 22:12:35 2023 -0400

    fix decode impl

commit bc064f1
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Oct 12 20:18:44 2023 -0400

    git rid of old const usage

commit d6126cd
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Oct 12 19:52:10 2023 -0400

    remove todos

commit 2cac7b5
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Oct 12 19:51:37 2023 -0400

    fix tests and lints

commit 67782ca
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Oct 12 17:10:27 2023 -0400

    git rid of max request blocks type

commit ac5eb9a
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Oct 12 16:04:13 2023 -0400

    add configs to ChainSpec

commit d0fe2ce
Author: realbigsean <seananderson33@gmail.com>
Date:   Thu Oct 12 15:46:47 2023 -0400

    add runtime variable list type
@paulhauner paulhauner mentioned this pull request Jan 8, 2024
Merged
@jimmygchen jimmygchen 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 Jan 8, 2024
@realbigsean realbigsean mentioned this pull request Jan 8, 2024
Open
@realbigsean realbigsean added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 8, 2024
michaelsproul
michaelsproul approved these changes Jan 8, 2024
View reviewed changes
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.

LGTM

jimmygchen
jimmygchen approved these changes Jan 8, 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.

Looks good to me too! Thanks

@jimmygchen jimmygchen merged commit b47e3f2 into sigp:unstable Jan 8, 2024
28 checks passed
@realbigsean realbigsean mentioned this pull request Jan 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgeManning AgeManning AgeManning left review comments

@pawanjay176 pawanjay176 pawanjay176 requested changes

@michaelsproul michaelsproul michaelsproul approved these changes

@jimmygchen jimmygchen jimmygchen approved these changes

Assignees

@jimmygchen jimmygchen

Labels
deneb ready-for-review The code is ready for review v4.6.0 ETA Q1 2024
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@realbigsean @jimmygchen @michaelsproul @AgeManning @pawanjay176