Set/Update cct from blockchain. #821
Conversation
Codecov Report
@@ Coverage Diff @@ ## staging #821 +/- ## ======================================== Coverage 24.28% 24.28% ======================================== Files 80 80 Lines 8095 8095 ======================================== Hits 1966 1966 Misses 5854 5854 Partials 275 275
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
code/go/0chain.net/blobber/config.go Outdated
| config.Configuration.ChallengeCompletionTime = viper.GetDuration("challenge_completion_time") | ||
| err := setCCTFromChain() | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Why use a panic here? If I understand, an error here indicates an issue with 0chain rather than the blobber.
Maybe the problem here is that the challenge completion time does not belong in the configuration file anymore on account of being removed, and we should move the challenge completion time elsewhere.
https://github.com/0chain/blobber/blob/staging/code/go/0chain.net/blobber/main.go#L12 Looking at blobber/main.go, the first thing the blobber does is read the config file and use it to set the config object; Setting the challenge completion time in setupConfig seems out of place. Can we guarantee 0chain is reachable at this point?
There was a problem hiding this comment.
Though it is not blobber error but of 0chain, we cannot start blobber with 0 cct.
What is better here?
Also the value is retrieved from storage-config endpoint. It is also termed as storageSCconfig and isn't it better to put in configuration file?
Don't understand what you mean by 0chain is reachable at this point. Basically it is only asking for storageSCConfig which is publicly available without wallet.
There was a problem hiding this comment.
The challenge competition time is no longer in the configuration file, so it smells bad to keep it in the configuration file's model. And setting it in the method designed to read in the configuration file smells just as bad. Better create a sister method to setupConfig to get settings from 0chain.
If there is no object for settings read from 0chain yet then we should create one. We could create a parent object for config.Configuration, and have 0chain settings as a sister. Anyway, check with someone more familiar with the blobber.
| BlobberUpdateInterval time.Duration | ||
| | ||
| ChallengeCompletionTime time.Duration | ||
| CCT time.Duration // challenge completion time // retrieved from storage SC config |
There was a problem hiding this comment.
Why the change to CCT? ChallengeCompletionTime looks clearer to me.
You can remove the extra //.
| // Upon updating cct, we only log error because cct is not 0 | ||
| // We should try to submit challenge as soon as possible regardless | ||
| // of cct value. | ||
| err := setCCTFromChain() |
There was a problem hiding this comment.
If we set the challenge completion time here, why do we need to set it in setupConfig where it does not seem to belong?
There was a problem hiding this comment.
Here it is done to update CCT. It can have been changed dynamically (which is not the current case though).
Do you think it can be dynamic? Or will it be dead constant?
There was a problem hiding this comment.
The challenge completion time can change with an update settings transaction. https://github.com/0chain/zwalletcli/blob/staging/cmd/sc-update-config.go#L14
So the blobber needs to be able to respond.
code/go/0chain.net/blobber/config.go Outdated
| config.Configuration.Geolocation.Longitude = viper.GetFloat64("geolocation.longitude") | ||
| | ||
| config.Configuration.ChallengeCompletionTime = viper.GetDuration("challenge_completion_time") | ||
| err := setCCTFromChain() |
There was a problem hiding this comment.
Can we move this outside of setupConfig, this has nothing to do with setting up the config file. Maybe have the main function call setupSettings that calls setupConfig and setCCTFromChain? Or alternately add a setupSettingsFrom0Chain method to the main funtion.
| Manual system tests [success] with the following config
|
Changes
This PR will change setting of cct value from config file to api leveraged by blockchain.
Will fix #818
Tests
Tasks to complete before merging PR:
Associated PRs (Link as appropriate):