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

Expose additional builder booster related flags in the vc #5086

Merged
merged 18 commits into from
Jan 24, 2024
Merged

Expose additional builder booster related flags in the vc #5086

merged 18 commits into from
Jan 24, 2024

Conversation

eserilev
Copy link
Collaborator

Issue Addressed

#5039

Proposed Changes

Add two new flags to the VC

prefer_builder_proposals
builder_boost_factor

Use these new flags plus the existing builder_proposals flag to 'calculate' the correct boost factor.

Enable these flags to be set via cli and relevant validator management api endpoints

@eserilev eserilev changed the title Expose builder booster flags vc Expose additional builder booster flags in the vc Jan 19, 2024
@eserilev eserilev changed the title Expose additional builder booster flags in the vc Expose additional builder booster related flags in the vc Jan 19, 2024
@michaelsproul michaelsproul added the v4.6.0 ETA Q1 2024 label Jan 19, 2024
paulhauner
paulhauner reviewed Jan 19, 2024
View reviewed changes
book/src/help_vc.md Outdated Show resolved Hide resolved
@dapplion dapplion added the ready-for-review The code is ready for review label Jan 21, 2024
@paulhauner paulhauner self-assigned this Jan 22, 2024
paulhauner
paulhauner reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Looking great! I just a have a few minor suggestions. There are also some docs over in the Lighthouse book that might be nice to update, eg https://github.com/sigp/lighthouse/blob/stable/book/src/api-vc-endpoints.md#patch-lighthousevalidatorsvoting_pubkey

validator_client/src/validator_store.rs Outdated Show resolved Hide resolved
validator_client/src/config.rs Outdated Show resolved Hide resolved
validator_client/src/block_service.rs Outdated Show resolved Hide resolved
@eserilev
Copy link
Collaborator Author

thanks for the review Paul. I've made changes based on your feedback and added some additional documentation to the Lighthouse book.

paulhauner
paulhauner approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Nice! Once we have approval from @realbigsean as well we can merge!

realbigsean
realbigsean approved these changes Jan 23, 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.

Awesome!

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 23, 2024
michaelsproul
michaelsproul requested changes Jan 23, 2024
View reviewed changes
validator_client/src/block_service.rs Outdated Show resolved Hide resolved
validator_client/src/validator_store.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul added under-review A reviewer has only partially completed a review. and removed ready-for-merge This PR is ready to merge. labels Jan 23, 2024
@jimmygchen
Copy link
Member

I've added a new test (locally) that covers the scenario where process defaults is present, and checking that the per-validator value overrides them - the test is failing though, I'm investigating now.

paulhauner added a commit that referenced this pull request Jan 24, 2024
@paulhauner
Expose additional builder booster related flags in the vc (#5086)
f217082 
Squashed commit of the following:

commit 4df4663
Author: Paul Hauner <[email protected]>
Date:   Wed Jan 24 18:23:20 2024 +1100

    Fix issue with PATCH request

commit 45d7ea5
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 24 17:26:25 2024 +1100

    Add http test for builder boost factor with process defaults.

commit 800e29c
Merge: b225328 2401fd7
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 24 12:29:28 2024 +1100

    Merge branch 'expose-builder-booster-flags-vc' of github.com:eserilev/lighthouse into fork/eserilev/expose-builder-booster-flags-vc

commit b225328
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 24 11:37:14 2024 +1100

    Prioritise per validator builder boost configurations over CLI flags.

commit 2401fd7
Author: Mac L <[email protected]>
Date:   Wed Jan 24 11:03:31 2024 +1100

    Fix CLI help text

commit c852a51
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 03:34:39 2024 +0200

    typo

commit 59af312
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 03:32:56 2024 +0200

    typo

commit 24a522b
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 02:55:33 2024 +0200

    fmt

commit f3a1b30
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 02:02:45 2024 +0200

    assume builder-proosals flag if one of other two vc builder flags are present

commit 2b2c9af
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Jan 22 19:43:27 2024 +0200

    fix typos

commit d1591fe
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Jan 22 19:43:20 2024 +0200

    fix typos

commit dc7a870
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Jan 22 19:25:51 2024 +0200

    remove unneeded val, change how boost factor flag logic in the vc, add some additional documentation

commit 7a214f7
Author: Eitan Seri-Levi <[email protected]>
Date:   Sat Jan 20 21:57:36 2024 +0200

    fix issues related to CreateConfig and MoveConfig

commit 89ade68
Author: Eitan Seri-Levi <[email protected]>
Date:   Sat Jan 20 01:52:42 2024 +0200

    resolve failing test

commit 5641d24
Merge: 03d68f5 185646a
Author: Eitan Seri-Levi <[email protected]>
Date:   Fri Jan 19 04:30:54 2024 +0200

    Merge branch 'unstable' of https://github.com/sigp/lighthouse into expose-builder-booster-flags-vc

commit 03d68f5
Author: Eitan Seri-Levi <[email protected]>
Date:   Fri Jan 19 04:30:39 2024 +0200

    expose builder booster flags in vc, enable options in validator endpoints, update tests
@paulhauner paulhauner mentioned this pull request Jan 24, 2024
Closed
2 tasks
paulhauner added a commit that referenced this pull request Jan 24, 2024
@paulhauner
Expose additional builder booster related flags in the vc (#5086)
4d51fde 
Squashed commit of the following:

commit 6c6f6b7
Author: Paul Hauner <[email protected]>
Date:   Wed Jan 24 19:06:33 2024 +1100

    Add prefer builder proposals

commit 4df4663
Author: Paul Hauner <[email protected]>
Date:   Wed Jan 24 18:23:20 2024 +1100

    Fix issue with PATCH request

commit 45d7ea5
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 24 17:26:25 2024 +1100

    Add http test for builder boost factor with process defaults.

commit 800e29c
Merge: b225328 2401fd7
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 24 12:29:28 2024 +1100

    Merge branch 'expose-builder-booster-flags-vc' of github.com:eserilev/lighthouse into fork/eserilev/expose-builder-booster-flags-vc

commit b225328
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 24 11:37:14 2024 +1100

    Prioritise per validator builder boost configurations over CLI flags.

commit 2401fd7
Author: Mac L <[email protected]>
Date:   Wed Jan 24 11:03:31 2024 +1100

    Fix CLI help text

commit c852a51
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 03:34:39 2024 +0200

    typo

commit 59af312
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 03:32:56 2024 +0200

    typo

commit 24a522b
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 02:55:33 2024 +0200

    fmt

commit f3a1b30
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jan 23 02:02:45 2024 +0200

    assume builder-proosals flag if one of other two vc builder flags are present

commit 2b2c9af
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Jan 22 19:43:27 2024 +0200

    fix typos

commit d1591fe
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Jan 22 19:43:20 2024 +0200

    fix typos

commit dc7a870
Author: Eitan Seri-Levi <[email protected]>
Date:   Mon Jan 22 19:25:51 2024 +0200

    remove unneeded val, change how boost factor flag logic in the vc, add some additional documentation

commit 7a214f7
Author: Eitan Seri-Levi <[email protected]>
Date:   Sat Jan 20 21:57:36 2024 +0200

    fix issues related to CreateConfig and MoveConfig

commit 89ade68
Author: Eitan Seri-Levi <[email protected]>
Date:   Sat Jan 20 01:52:42 2024 +0200

    resolve failing test

commit 5641d24
Merge: 03d68f5 185646a
Author: Eitan Seri-Levi <[email protected]>
Date:   Fri Jan 19 04:30:54 2024 +0200

    Merge branch 'unstable' of https://github.com/sigp/lighthouse into expose-builder-booster-flags-vc

commit 03d68f5
Author: Eitan Seri-Levi <[email protected]>
Date:   Fri Jan 19 04:30:39 2024 +0200

    expose builder booster flags in vc, enable options in validator endpoints, update tests
@eserilev
Copy link
Collaborator Author

thanks for helping get this across the finish line everyone!

@jimmygchen
Copy link
Member

I've added some more tests and it's looking good now after Paul's fixes!
Perhaps another approval from @realbigsean again and we can merge?

@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed under-review A reviewer has only partially completed a review. labels Jan 24, 2024
realbigsean
realbigsean approved these changes Jan 24, 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.

new changes look good!

michaelsproul
michaelsproul approved these changes Jan 24, 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, let's ship 🚀

@paulhauner paulhauner merged commit f9e36c9 into sigp:unstable Jan 24, 2024
29 checks passed
@jimmygchen jimmygchen mentioned this pull request Jan 24, 2024
Closed
@chenrui333 chenrui333 mentioned this pull request Jan 28, 2024
Merged
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Feb 14, 2024
@eserilev @macladson
@jimmygchen @paulhauner @danielrachi1
Expose additional builder booster related flags in the vc (sigp#5086)
4f4a1b0 
* expose builder booster flags in vc, enable options in validator endpoints, update tests

* resolve failing test

* fix issues related to CreateConfig and MoveConfig

* remove unneeded val, change how boost factor flag logic in the vc, add some additional documentation

* fix typos

* fix typos

* assume builder-proosals flag if one of other two vc builder flags are present

* fmt

* typo

* typo

* Fix CLI help text

* Prioritise per validator builder boost configurations over CLI flags.

* Add http test for builder boost factor with process defaults.

* Fix issue with PATCH request

* Add prefer builder proposals

* Add more builder boost factor tests.

---------

Co-authored-by: Mac L <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmygchen jimmygchen jimmygchen left review comments

@paulhauner paulhauner paulhauner approved these changes

@realbigsean realbigsean realbigsean approved these changes

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees

@paulhauner paulhauner

Labels
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.
7 participants
@eserilev @jimmygchen @michaelsproul @realbigsean @paulhauner @dapplion @macladson