Don't overwrite DefaultFeeRecipient config param at runtime

[kasey]

💎 Issue

DefaultFeeRecipient BeaconChainConfig parameter overwritten at runtime.

Background

Configs should be largely immutable, shared by all participants in a given network.

Description

Instead of mutating this config val, we should find a different way to plumb this cli flag to the right places.

[timg512372]

Hey Kasey,

I'm Tim (CS student at UC Berkeley) and I would like to get involved with open source development here. Can I try giving this issue a shot?

[rkapka]

Hi @timg512372 , thanks for volunteering! I assigned you to the issue.

[timg512372]

Hey everyone, apologies for the delay, but I think I found something

First, I just wanted to clarify that the DefaultFeeRecipient is getting overwritten because of the default_config.fee_recipient attribute documented here.

I also believe I found the code block responsible for overwriting the config. In validator/node/node.go lines 488-497:

        // override the default fileConfig with the fileConfig from the command line
	if cliCtx.IsSet(flags.SuggestedFeeRecipientFlag.Name) {
		suggestedFee := cliCtx.String(flags.SuggestedFeeRecipientFlag.Name)
		fileConfig = &validatorServiceConfig.FeeRecipientFileConfig{
			ProposeConfig: nil,
			DefaultConfig: &validatorServiceConfig.FeeRecipientFileOptions{
				FeeRecipient: suggestedFee,
			},
		}
	}

According to the documentation, the DefaultFeeConfig is written in a json file, the path of which is passed into flags.SuggestedFeeRecipientFlag.Name. However, I'm not sure what the best way is to separate the two variables is. Perhaps we could write another function to get the fee recipient that checks if the context is set from the CLI and call it instead of looking up the default fee recipient in the config? Really appreciate all the help! Thank you!

[nisdas]

cc @james-prysm

[james-prysm]

@timg512372 @kasey sorry just taking a look at this, to be more clear this issue was not for the validator client ( which also has this flag) as I am handling it over here it's referring to overriding the beacon config on the beacon node side. you can find this in beacon-chain/node/config.go in the func configureExecutionSetting(cliCtx *cli.Context) error on line 171 c.DefaultFeeRecipient = checksumAddress

[lyzs90]

@james-prysm I just had a look at where DefaultFeeRecipient was supposedly overwritten, and traced it directly to the suggested-fee-recipient beacon node cli flag.

It seems like it's being set for the first time, and done prior to beacon start. Not sure if this still qualifies as a case of runtime mutation?

[james-prysm]

@lyzs90 It's set the first time here, but I think to kasey's point of starting this issue, the beaconConfig was intended to be this read-only config based on network settings ( mainnet, minimal -which is test net, etc) and not setting values at runtime. I'm sure we have other instances of breaking this intention but don't want things to get dangerous and break consensus unintentionally. this was another one of the issues I wanted to start looking at as I address some other UX concerns around fee recipient.

[lyzs90]

Gotcha, thanks for clarifying