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

v0.46.6: GetConfig doesn't read the store or streamers sections. #13955

Closed
SpicyLemon opened this issue Nov 21, 2022 · 5 comments · Fixed by #14082 or #14125
Closed

v0.46.6: GetConfig doesn't read the store or streamers sections. #13955

SpicyLemon opened this issue Nov 21, 2022 · 5 comments · Fixed by #14082 or #14125
Assignees
Labels

Comments

@SpicyLemon
Copy link
Collaborator

Summary of Bug

The GetConfig function in server/config/config.go does not read the store or streamers sections into config.Store and config.Streamers structs.

Version

Only v0.46.6. This does not affect main due to #13651.

Steps to Reproduce

This was caught in one of our unit tests that does the following:

  1. Write the config file using WriteConfigFile(..., DefaultConfig()).
  2. Read the config into viper.
  3. Call the GetConfig function.
  4. Assert that the config returned by GetConfig is exactly the same as the one returned by DefaultConfig().

Expected results:
The result of GetConfig is exactly the same as DefaultConfig().

Actual results:

            	Error:      	Not equal:
            	            	expected: config.Config{BaseConfig:config.BaseConfig{MinGasPrices:"42manager", Pruning:"default", PruningKeepRecent:"0", PruningInterval:"0", HaltHeight:0x0, HaltTime:0x0, MinRetainBlocks:0x0, InterBlockCache:true, IndexEvents:[]string{}, IAVLCacheSize:0xbebc2, IAVLDisableFastNode:true, AppDBBackend:""}, Telemetry:telemetry.Config{ServiceName:"", Enabled:false, EnableHostname:false, EnableHostnameLabel:false, EnableServiceLabel:false, PrometheusRetentionTime:0, GlobalLabels:[][]string{}}, API:config.APIConfig{Enable:false, Swagger:false, EnableUnsafeCORS:false, Address:"tcp://0.0.0.0:1317", MaxOpenConnections:0x3e8, RPCReadTimeout:0xa, RPCWriteTimeout:0x0, RPCMaxBodyBytes:0xf4240}, GRPC:config.GRPCConfig{Enable:true, Address:"0.0.0.0:9090", MaxRecvMsgSize:10485760, MaxSendMsgSize:2147483647}, Rosetta:config.RosettaConfig{Address:":8080", Blockchain:"app", Network:"network", Retries:3, Enable:false, Offline:false, EnableFeeSuggestion:false, GasToSuggest:200000, DenomToSuggest:"uatom"}, GRPCWeb:config.GRPCWebConfig{Enable:true, Address:"0.0.0.0:9091", EnableUnsafeCORS:false}, StateSync:config.StateSyncConfig{SnapshotInterval:0x0, SnapshotKeepRecent:0x2}, Store:config.StoreConfig{Streamers:[]string{}}, Streamers:config.StreamersConfig{File:config.FileStreamerConfig{Keys:[]string{"*"}, WriteDir:"", Prefix:""}}}
            	            	actual  : config.Config{BaseConfig:config.BaseConfig{MinGasPrices:"42manager", Pruning:"default", PruningKeepRecent:"0", PruningInterval:"0", HaltHeight:0x0, HaltTime:0x0, MinRetainBlocks:0x0, InterBlockCache:true, IndexEvents:[]string{}, IAVLCacheSize:0xbebc2, IAVLDisableFastNode:true, AppDBBackend:""}, Telemetry:telemetry.Config{ServiceName:"", Enabled:false, EnableHostname:false, EnableHostnameLabel:false, EnableServiceLabel:false, PrometheusRetentionTime:0, GlobalLabels:[][]string{}}, API:config.APIConfig{Enable:false, Swagger:false, EnableUnsafeCORS:false, Address:"tcp://0.0.0.0:1317", MaxOpenConnections:0x3e8, RPCReadTimeout:0xa, RPCWriteTimeout:0x0, RPCMaxBodyBytes:0xf4240}, GRPC:config.GRPCConfig{Enable:true, Address:"0.0.0.0:9090", MaxRecvMsgSize:10485760, MaxSendMsgSize:2147483647}, Rosetta:config.RosettaConfig{Address:":8080", Blockchain:"app", Network:"network", Retries:3, Enable:false, Offline:false, EnableFeeSuggestion:false, GasToSuggest:200000, DenomToSuggest:"uatom"}, GRPCWeb:config.GRPCWebConfig{Enable:true, Address:"0.0.0.0:9091", EnableUnsafeCORS:false}, StateSync:config.StateSyncConfig{SnapshotInterval:0x0, SnapshotKeepRecent:0x2}, Store:config.StoreConfig{Streamers:[]string(nil)}, Streamers:config.StreamersConfig{File:config.FileStreamerConfig{Keys:[]string(nil), WriteDir:"", Prefix:""}}}

            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -63,4 +63,3 @@
            	            	  Store: (config.StoreConfig) {
            	            	-  Streamers: ([]string) {
            	            	-  }
            	            	+  Streamers: ([]string) <nil>
            	            	  },
            	            	@@ -68,5 +67,3 @@
            	            	   File: (config.FileStreamerConfig) {
            	            	-   Keys: ([]string) (len=1) {
            	            	-    (string) (len=1) "*"
            	            	-   },
            	            	+   Keys: ([]string) <nil>,
            	            	    WriteDir: (string) "",

Basically, the config struct returned by GetConfig didn't read the Store.Streamers or Streamers.Keys fields. It has the Store and Streamers structs because they are concrete (not references), so they're made any time a new Config struct is made. But the Store.Streamers and Streamers.Keys slices are nil because those fields aren't being read.

@tac0turtle
Copy link
Member

tac0turtle commented Nov 21, 2022

did you find this with the tool you mentioned in the other issue? we should add this test case

@SpicyLemon
Copy link
Collaborator Author

I didn't find it directly from the config command, but from a unit test associated with the command: https://github.com/provenance-io/provenance/blob/main/cmd/provenanced/config/manager_test.go#L106-L122.
That test failed once we pulled the v0.46.6 changes into our fork. I then noticed that some config fields were added to the Config struct that the GetConfig function didn't know about.

@tac0turtle
Copy link
Member

can we add a test case to prevent this from happening?its not the first time its happened

@tac0turtle tac0turtle reopened this Dec 1, 2022
Repository owner moved this from 👏 Done to 📝 Todo in Cosmos-SDK Dec 1, 2022
@alexanderbez alexanderbez self-assigned this Dec 1, 2022
@alexanderbez
Copy link
Contributor

So why don't we just backport #13651 to 0.46.x?

@tac0turtle
Copy link
Member

it was back ported

Repository owner moved this from 📝 Todo to 👏 Done in Cosmos-SDK Dec 1, 2022
@tac0turtle tac0turtle removed this from Cosmos-SDK Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants