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

Fix bug in --builder-proposals #5151

Merged
merged 5 commits into from
Jan 31, 2024
Merged

Fix bug in --builder-proposals #5151

merged 5 commits into from
Jan 31, 2024

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Jan 30, 2024 •
edited
Loading

Issue Addressed

The validator client was using the wrong logic for the default builder_boost_factor when the --builder-proposals flag was provided. Rather than setting the boost to 0 when builder-propsals was false, it was setting it to 0 when builder-proposals was true.

Proposed Changes

  • Fix the condition
  • Add another test case to cover this specifically

@michaelsproul michaelsproul added bug Something isn't working val-client Relates to the validator client binary v5.0.0 Q1 2024 labels Jan 30, 2024
@jimmygchen jimmygchen self-assigned this Jan 31, 2024
jimmygchen
jimmygchen approved these changes Jan 31, 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! Thanks for the fix.
Pushed a fix for the duplicate test case as discussed.

@jimmygchen jimmygchen added the ready-for-merge This PR is ready to merge. label Jan 31, 2024
@michaelsproul
Copy link
Member Author

Thanks for noticing & fixing the test. I pushed a new commit to get cargo fmt passing.

Let's merge

@michaelsproul
Copy link
Member Author

@Mergifyio queue

@mergify Mergify
Copy link

mergify bot commented Jan 31, 2024

queue

🛑 The pull request has been removed from the queue default

The pull request #5151 has been manually updated.

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.

@michaelsproul
Copy link
Member Author

@Mergifyio refresh

@mergify Mergify
Copy link

mergify bot commented Jan 31, 2024

refresh

✅ Pull request refreshed

@michaelsproul
Copy link
Member Author

@Mergifyio requeue

@mergify Mergify
Copy link

mergify bot commented Jan 31, 2024

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@michaelsproul
Copy link
Member Author

@Mergifyio queue

@mergify Mergify
Copy link

mergify bot commented Jan 31, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at d2aef1b

mergify bot added a commit that referenced this pull request Jan 31, 2024
@mergify
Merge of #5151
3bbf012
This was referenced Jan 31, 2024
Closed
Merged
@mergify mergify bot merged commit d2aef1b into sigp:unstable Jan 31, 2024
28 of 29 checks passed
@michaelsproul michaelsproul deleted the fix-builder-proposals branch January 31, 2024 05:52
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Feb 14, 2024
@michaelsproul @danielrachi1
Fix bug in --builder-proposals (sigp#5151)
8bd0625 
* Fix bug in `--builder-proposals`

* Add tests

* More sensible test order

* Fix duplicate builder-boost test case

* Cargo fmt and rename
@chenrui333 chenrui333 mentioned this pull request Feb 22, 2024
Merged
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

Assignees

@jimmygchen jimmygchen

Labels
bug Something isn't working ready-for-merge This PR is ready to merge. v5.0.0 Q1 2024 val-client Relates to the validator client binary
Projects
None yet
Milestone
No milestone
Development

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