-
Notifications
You must be signed in to change notification settings - Fork 919
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't overwrite DefaultFeeRecipient config param at runtime #10720
Comments
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? |
Hi @timg512372 , thanks for volunteering! I assigned you to the issue. |
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 // 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 |
cc @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. |
@james-prysm I just had a look at where 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? |
@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. |
Gotcha, thanks for clarifying |
馃拵 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.
The text was updated successfully, but these errors were encountered: