-
Notifications
You must be signed in to change notification settings - Fork 33
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
refactor!: app version stored in multi store and shared in state sync #265
Changes from 16 commits
fd75a32
5e77bf7
9e2b4ec
85d6390
a94db01
9803be9
b01ce9b
f878d0d
9b7a1aa
3fbae6e
73b5957
950db19
52052eb
5ac95aa
84eb25f
6f2d090
16d062d
d150e7a
135367c
336d5c2
5da1906
a2fd575
2f48af2
9e1bb68
e3b16a9
d5f7551
e912d23
b676090
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
package baseapp | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"reflect" | ||
"strings" | ||
|
@@ -130,7 +129,7 @@ type BaseApp struct { // nolint: maligned | |
indexEvents map[string]struct{} | ||
} | ||
|
||
var _ upgrade.ProtocolVersionManager = (*BaseApp)(nil) | ||
var _ upgrade.AppVersionManager = (*BaseApp)(nil) | ||
|
||
// NewBaseApp returns a reference to an initialized BaseApp. It accepts a | ||
// variadic number of option functions, which act on the BaseApp to set | ||
|
@@ -310,15 +309,12 @@ func (app *BaseApp) init() error { | |
// needed for the export command which inits from store but never calls initchain | ||
app.setCheckState(tmproto.Header{}) | ||
|
||
// If there is no app version set in the store, we should set it to 0. | ||
// Panic on any other error. | ||
// If errMsgNoProtocolVersionSet, we assume that appVersion is assigned to be 0. | ||
appVersion, err := app.GetAppVersion(app.checkState.ctx) | ||
if err != nil && !errors.Is(err, errMsgNoProtocolVersionSet) { | ||
appVersion, err := app.GetAppVersion() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if err := app.SetAppVersion(app.checkState.ctx, appVersion); err != nil { | ||
if err := app.SetAppVersion(appVersion); err != nil { | ||
return err | ||
} | ||
app.Seal() | ||
|
@@ -436,13 +432,13 @@ func (app *BaseApp) GetConsensusParams(ctx sdk.Context) *abci.ConsensusParams { | |
cp.Validator = &vp | ||
} | ||
|
||
if app.paramStore.Has(ctx, ParamStoreKeyVersionParams) { | ||
var vp tmproto.VersionParams | ||
|
||
app.paramStore.Get(ctx, ParamStoreKeyVersionParams, &vp) | ||
cp.Version = &vp | ||
appVersion, err := app.cms.GetAppVersion() | ||
if err != nil { | ||
panic(err) | ||
} | ||
cp.Version = &tmproto.VersionParams{ | ||
AppVersion: appVersion, | ||
} | ||
|
||
return cp | ||
} | ||
|
||
|
@@ -466,9 +462,8 @@ func (app *BaseApp) StoreConsensusParams(ctx sdk.Context, cp *abci.ConsensusPara | |
app.paramStore.Set(ctx, ParamStoreKeyBlockParams, cp.Block) | ||
app.paramStore.Set(ctx, ParamStoreKeyEvidenceParams, cp.Evidence) | ||
app.paramStore.Set(ctx, ParamStoreKeyValidatorParams, cp.Validator) | ||
app.paramStore.Set(ctx, ParamStoreKeyVersionParams, cp.Version) | ||
// We're explicitly not storing the Tendermint app_version in the param store. It's | ||
// stored instead in the x/upgrade store, with its own bump logic. | ||
// We do not store version params here because they are | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit odd to me that we treat What if we instead store it here as we did before but also store it in the multi-store directly? Or does that not seem right either? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I've thought about this as well. It's totally possible to store it in multiple places. However, the long-term problem that I foresee with doing that is that these will go out of sync and get misused. There should really be only one data source of such critical parameters. It seems to me that the original problem with app version breaking state-sync is exactly for this reason - storing it in multiple places and, as a result, not updating correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think this is fine then as long as we have clear documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I would add to the existing comment that the entire reason we do this is to allow state-sync to function properly. |
||
// persisted in the multi-store which is used as the single source of truth. | ||
} | ||
|
||
// getMaximumBlockGas gets the maximum gas from the consensus params. It panics | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is an aliased function and not just a normal function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the convention of storing errors and their messages as vars or constants respectively at the top of the file. It allows identifying these errors more easily and increases the likelihood of reusing them where needed.
The need for making this a function arises at times when we need to format parameters at runtime. That's why I create such aliased functions for these "dynamic" errors.
I'm not sure what the best practice that would allow the consolidation of all errors in one place while removing the need for such aliased functions with runtime parameters.
If you have a suggestion, please let me know
In terms of making this a regular function, I think it would make this error less visible and less likely to be reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterbourgon 's suggestion here seems like a better alternative than this aliased function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What he suggested is the idiomatic approach 👍