upgrade clap to v4.5

[eserilev]

Issue Addressed

#5087

Proposed Changes

This PR upgrades clap from version 2.x to 4.5.1. There are some breaking changes between the two versions, v4.x is a bit stricter when it comes to flag definitions and has a slightly different api. Once we upgrade clap to v4 we can begin working on migrating to clap derive, which gives us the following benefits

• easier to read cli flag definitions
• a bit more type safety/compile time guarantees
• less verbose initialization logic
• an easy interface to enable reading cli flags from a config file

We'll migrate to clap derive in separate PR's on a crate by crate basis, to make the PRs a bit more manageable

List of breaking changes:

• latency-measurement-service flag is now deprecated in favor of disable-latency-measurement-service. By default the latency measurement service is enabled, and can be disabled by providing the new disable-latency-measurement-service flag.
• The self-limiter flag no longer accepts arguments. Providing the self-limiter flag enables the outbound rate limiter service. A new flag self-limiter-protocols is introduced to allow for configuring the outbound rate limiter service per protocol. In order to use the new self-limiter-protocols flag, self-limiter must also be provided.
• The inbound-rate-limiter flag is now deprecated in favor of disable-inbound-rate-limiter. By default the inbound rate limiter service is enabled and can be disabled by providing the new disable-inbound-rate-limiter flag. Also, an additional flag inbound-rate-limiter-protocols is introduced to allow for configuring the inbound rate limiter service per protocol. Note that the disable-inbound-rate-limiter flag conflicts with the inbound-rate-limiter-protocols flag, they cannot be used in tandem.
• slasher-broadcast now requires either a true or false value. The flag cannot be used without providing a value.
• The above should cover the big breaking changes introduced by this PR. However its important to note that cli flag definitions are a bit stricter now. Anything listed under Options in the help screen must have an argument provided. Anything listed under Flags in the help screen cannot have an argument provided. If those conditions are violated the program will fail and an error message will be returned specifying what cli rules were violated.

[eserilev]

There are still roughly 10 failing tests here that I need to resolve

[eserilev]

Tests are passing, this should be ready for a review

[chong-he]

I have done some testing on this and some comments below (current refers to the help texts that we have now, while new refers to the help texts from this PR):

1. The help texts work on both Linux and Windows. The help texts are displaying for all commands and subcommands, with green colour on the flag itself, and white colour for the description. Only some current help texts (such as bn and vc) have this colour distinction, while other commands like lighthouse a validator –help and lighthouse a wallet –help do not. So this is a great improvement.

2. The current help texts always generate the commit in the first line, in which @macladson removed it in the cli.sh when putting them in the Lighthouse Book, as if otherwise the cli-check would always fail. The new help texts do not have this commit in the first line anymore, and as such the removal of first line is no longer needed. If we run make cli-local now, the help texts will always start with this:

Usage: lighthouse beacon_node [OPTIONS]

which is not desirable as the important descriptive first line is removed. To solve this, we remove these lines:

https://github.com/sigp/lighthouse/blob/3058b96f2560f1da04ada4f9d8ba8e5651794ff6/scripts/cli.sh#L15-L16

I have tested that after removing the above in cli.sh and running make cli-local, all help texts are reserved and shown.

3. Currently, the flags are separated into 2 groups: flags (do not require a value), options (require a value). With this PR, all flags are listed under options only. Is this an expected change?

4. Currently the flags are arranged in alphabetical order. I am not sure in what order is the new help texts arranged? For example, in lighthouse bn –help, it starts with the flags --network-dir, then --freezer-dir and --logfile which does not seem follow a certain order? It would be nice if they follow a certain order, e.g., the alphabetical order.

5. The new help texts do not start a new line until reaching the monitor width. This unfortunately breaks the word when starting a new line, referring to the screenshot below. The words exceeds and attestations are broken into two lines.

The current help texts do not have this problem because it starts a new line after a certain number of words. Because the new help texts is shown as a single line, when it is displayed in Lighthouse book, it will have a long horizontal scroll at the bottom, which is not very UX friendly:

I wonder if it is a good idea to break into new lines after reaching a certain number of words, like the current help texts?

Thanks!

[eserilev]

@chong-he thanks for the detailed review! I think i was able to fix the issues you brought up

• the cli params are now listed in alphabetic order
• I created a separate "Flag" section for cli params that dont accept a value
• text wrapping should be a bit cleaner
• i updated cli.sh based on your recommendation

[eserilev]

In an attempt to keep all flags set to either ArgAction::Set, ArgAction::Append, or ArgAction::SetTrue I've introduced a few breaking changes

Self Limiter

the self-limiter flag no longer takes a value. If the flag is provided it'll enable the outbound rate limiter. I've also introduced a second flag self-limiter-protocols that takes a list of protocol values (delimited by a ;). If self-limiter is provided, w/o self-limiter-protocols, the outbound rate limiter will be enabled with the default values

Inbound rate limiter

I've deprecated the inbound-rate-limiter flag and instead introduced a disable-inbound-rate-limiter flag. By default inbound rate limiting is enabled, and can only be disabled via this new flag. Additionally I've introduced a inbound-rate-limiter-protocols flag that accepts a list of protocol values (delimited by a ;.

We could make these breaking changes as a separate PR instead, though I'd rather introduce these breaking changes before merging this in.

[chong-he]

@chong-he thanks for the detailed review! I think i was able to fix the issues you brought up

• the cli params are now listed in alphabetic order
• I created a separate "Flag" section for cli params that dont accept a value
• text wrapping should be a bit cleaner
• i updated cli.sh based on your recommendation

Looks great. I can confirm that the flags are now arranged in alphabetical order, and the flags are separated into flags and options.

I notice for some commands, options flags, the description is now side-by-side with the flag, instead of the description below the flag. For example, lighthouse vm move --help:

(bn and vc are still having the description below the flag)

I see that the cli-check failed. To fix this, you can run make cli-local then commit the changes (it will auto update all help_x.md files)

[dapplion]

Do we really want to split flags and options? I never understand the UX benefit of doing so. IMO flags should either:

• be sorted in alphabetical order (flags and options together)
• be grouped in some way (network, log, mev) and then sorted in alphabetical order (flags and options together)

[eserilev]

I really like the idea of grouping the flags as Lion describes (network, log, mev etc.)

[chong-he]

Do we really want to split flags and options? I never understand the UX benefit of doing so. IMO flags should either:

• be sorted in alphabetical order (flags and options together)
• be grouped in some way (network, log, mev) and then sorted in alphabetical order (flags and options together)

Makes sense, I was commenting by comparing to previous help texts arrangement. But grouping them does sound good because if it doesn't require a value, it will be clear by looking at the flag itself

[eserilev]

re: cli param grouping

in subsequent PRs im going to move us to clap derive on a crate by crate basis. At that time I think I'll look to adding more granular/logical cli param grouping. For now I've kept it as is (i.e. flag vs option grouping). If we're against this, I can do no groupings and just have it purely alphabetical.

aside from that, I think everything else here seems to be in a good spot. thank you @chong-he for all the help!

[realbigsean]

confused why the cli-check is failing. It's working for me locally 🤔

[chong-he]

Member

confused why the cli-check is failing. It's working for me locally 🤔

I will look into this

[chong-he]

Some comments after taking another look:

1. Should lighthouse boot_node --enr-address take a value? For now, with lighthouse boot_node --enr-address it doesn’t return an error.

2. lighthouse boot_node --network-dir dummy_directory returns 9000 when a dummy directory is set. Not sure why the 9000 is returned?

3. Should the subcommands follow alphabetical order too? Currently the subcommands, e.g., in lighthouse –help, the commands database_manager, validator_manager etc do not follow the alphabetical order.

4. If we still separate options and flags, the --help should be in flags since it doesn’t take a value. They are in options now.

5. I find that --testnet-dir is appearing first, although if following alphabetical order, it should come after other flags, such as --target-peers in bn, --terminal-xxx in vc. It seems that it has something to do with the fact that .short('t') is put in the --testnet-dir flag?

6. Do we want to consider this? https://github.com/sigp/lighthouse/pull/5273#issuecomment-2038467089, i.e., having the descriptions below the flags. Currently only bn and vc are having the description below.

   Are we also open to having more text wrapping? This is mainly so that the help texts are displayed on the Lighthouse book without a horizontal scroll like what we currently have: https://lighthouse-book.sigmaprime.io/help_vm.html

   The new one will have a horizontal scroll which is not very UX friendly.

[eserilev]

thanks @chong-he

1. fixed, it should now return an error when a value isnt provided
2. the random "9000" has been removed
3. subcommands now display in alphabetical order
4. help should now show up under the flags section
5. yes, the alphabetical order takes into account the short name -t. i dont see a way to override this functionality
6. i've moved descriptions to below the flag everywhere. let me know what you think
7. ive added better text wrapping, i think the book looks a bit better now (see screenshot below)

[chong-he]

thanks @chong-he

1. fixed, it should now return an error when a value isnt provided
2. the random "9000" has been removed
3. subcommands now display in alphabetical order
4. help should now show up under the flags section
5. yes, the alphabetical order takes into account the short name -t. i dont see a way to override this functionality
6. i've moved descriptions to below the flag everywhere. let me know what you think
7. ive added better text wrapping, i think the book looks a bit better now (see screenshot below)

Looks all good to me, thank you!

One way to let the testnet-dir flag follow the alphabetical order is to remove the short(t) I think? But I don't know, do we want to keep the short name there?

[AgeManning]

Should we get this in for 5.2?

[eserilev]

i'll put together a list of breaking changes

[michaelsproul]

@mergifyio queue

[mergify[bot]]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at df983a83e1ccf1fe288b5464569b59b5ac35e38c