cmd/geth: fix cache variable with config files #22476
Conversation
Create new config flag for Cache and check for that flag in the config file early
Make comment text clearer
Fix spelling on comment
Set the config Cache value if provided by command line
petesimard
changed the title
| if err := loadConfig(file, &cfg); err != nil { | ||
| utils.Fatalf("%v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure why the config needs to be loaded another time here. Maybe it's because the code below, which post-processes the cache amount automatically is in the wrong place.
There are two parts to this post-processing. First we have some logic to set the default value based on blockchain if it is not specified as a flag. This part is only related to flags and doesn't need the config file value. Then there is the part where the cache amount is 'sanitized'. We need to apply that to the config file as well. Maybe it should be moved to cmd/utils/flags.go.
There was a problem hiding this comment.
The config file is loaded first here since this code is executed before the config file is normally processed.
From my understanding the problem is the cache has to be set early here before the p2p networking is setup.
I'm still new to the codebase so please let me know how you'd prefer it to be implemented.
There was a problem hiding this comment.
Geth startup works as follows (see it here).
- pre-setup (everything that runs before
makeConfigNode) - config file is loaded
- flags related to
node.Configare processed and override values in config node.Nodeobject is constructed- flags related to
ethconfig.Configare processed and override values in config eth.Ethereumobject is constructed and registered in node- node is started
I think this code here runs during the pre-setup, which is the wrong place to adjust anything in the config.
To make it work properly, this code needs to run during phase 3 or phase 5. The code for those phases is in package cmd/utils. This is why I suggest that the adjusting of the cache amount should be moved there.
There was a problem hiding this comment.
When we discussed this PR in the team today, someone said that adding the Cache field to node.Config is a bad idea. I agree, because we already have config fields for the cache amount in ethconfig.Config:
go-ethereum/eth/ethconfig/config.go
Line 72 in 91726e8
So what really needs to happen here is: the sanity checking of the database cache amount needs to be moved into phase 5, i.e. in the SetEthConfig function
fjl
changed the title
|
Created new pull request with different approach |
3 participants
main: Sample config file for cache value to allow setting Go GC cache values though a config file
node: Add new configuration variable "Cache"
utils: Set configuration value based on command line parameter
When Geth detects it is operating on the mainnet it will bump up cache values. This happens before the configuration file is parsed so the value is unable to be set by the config file and must be set via a command line parameter.
This patch adds a new configuration variable allowing this cache value to be set. In order to read this value, the config file must be parsed early to detect the presence of this value.
Fixes #21090