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

Delete --builder-profit-threshold flag #6131

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

chong-he
Copy link
Member

There is a bug when using the deprecated flag --builder-profit-threshold in Lighthouse 5.2.0 and above, reported by @CouzDelCouzzz on Discord: https://discord.com/channels/605577013327167508/605577013331361793/1262698540358963261

To reproduce, run Lighthouse with the flag, e.g.:

./lighthouse bn --checkpoint-sync-url https://mainnet.checkpoint.sigp.io --builder-profit-threshold 100 --execution-endpoint http://localhost:8551 --execution-jwt /home/hi/.ethereum/geth/jwtsecret

@michaelsproul mentioned that this is due to the parse_flag should only be used when the flag does not take an argument (i.e., the flag is a boolean, such as the --metrics flag):

if parse_flag(cli_args, "disable-enr-auto-update") {
config.discv5_config.enr_update = false;
}

This is not the case for --builder-profit-threshold which takes an argument:

if parse_flag(cli_args, "builder-profit-threshold") {
warn!(
log,
"Ignoring --builder-profit-threshold";
"info" => "this flag is deprecated and will be removed"
);
}

This PR deletes the --builder-profit-threshold flag so that the panic is not possible to manifest.

@chong-he chong-he added bug Something isn't working ready-for-review The code is ready for review v5.3.0 Q3 2024 release with database changes! labels Jul 18, 2024
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.

Nice. Let's do it

@michaelsproul michaelsproul added backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 19, 2024
@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 19, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 647992b

mergify bot added a commit that referenced this pull request Jul 19, 2024
mergify bot added a commit that referenced this pull request Jul 19, 2024
@mergify mergify bot merged commit 647992b into sigp:unstable Jul 19, 2024
29 checks passed
@chong-he chong-he deleted the delete-builder-profit-threshold branch July 31, 2024 06:01
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompat Backwards-incompatible API change bug Something isn't working ready-for-merge This PR is ready to merge. v5.3.0 Q3 2024 release with database changes!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants