feat!: use versioned keys for dynamically adding and remove commit stores #3320
Conversation
cmwaters
changed the title
cmwaters
requested review from
rach-id and
evan-forbes
and removed request for
a team
WalkthroughWalkthroughThe recent updates focus on enhancing key management across different versions within the application. This includes restructuring how keys are stored and managed in the Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func (app *App) migrateModules(ctx sdk.Context, fromVersion, toVersion uint64) error { | ||
| return app.mm.RunMigrations(ctx, app.configurator, fromVersion, toVersion) | ||
| } |
There was a problem hiding this comment.
Ensure error handling in module migrations.
The migrateModules function is responsible for running module-specific migrations between versions. It's important to ensure robust error handling within this function to manage any issues that might arise during the migration process. Consider implementing comprehensive logging and error reporting mechanisms to aid in troubleshooting migration issues.
| bApp.SetVersion(version.Version) | ||
| bApp.SetInterfaceRegistry(interfaceRegistry) | ||
|
|
||
| keys := sdk.NewKVStoreKeys( | ||
| authtypes.StoreKey, authzkeeper.StoreKey, banktypes.StoreKey, stakingtypes.StoreKey, | ||
| minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, | ||
| evidencetypes.StoreKey, capabilitytypes.StoreKey, | ||
| blobstreamtypes.StoreKey, | ||
| ibctransfertypes.StoreKey, | ||
| ibchost.StoreKey, | ||
| packetforwardtypes.StoreKey, | ||
| icahosttypes.StoreKey, | ||
| signaltypes.StoreKey, | ||
| ) | ||
| keys := sdk.NewKVStoreKeys(allStoreKeys()...) | ||
| tkeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey) | ||
| memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey) | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [250-260]
Ensure correct initialization of key maps in the New function.
The initialization of keys, tkeys, and memKeys using sdk.NewKVStoreKeys, sdk.NewTransientStoreKeys, and sdk.NewMemoryStoreKeys respectively, is critical. However, it's important to ensure that these functions are correctly populating the maps based on the current application version. This might require additional validation or error handling to ensure that the keys are correctly set up for the specified version.
app/app.go
Outdated
| panic(err) | ||
| } | ||
| app.SetAppVersion(ctx, newVersion) | ||
| app.SignalKeeper.ResetTally(ctx) |
There was a problem hiding this comment.
Correct formatting issue.
- app.SignalKeeper.ResetTally(ctx)
+ app.SignalKeeper.ResetTally(ctx)The indentation here does not follow Go's conventional tab-based indentation. Correcting this will improve the readability and maintain the standard coding style.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| app.SignalKeeper.ResetTally(ctx) | |
| app.SignalKeeper.ResetTally(ctx) |
|
I think this needs a |
| @@ -231,7 +232,6 @@ func New( | |||
| logger log.Logger, | |||
| db dbm.DB, | |||
| traceStore io.Writer, | |||
| loadLatest bool, | |||
There was a problem hiding this comment.
[question][not blocking] out of curiosity, what was this param previously used for?
There was a problem hiding this comment.
I couldn't find any use of it in the code i.e. this was always true.
My theory is that you might want to start at an earlier height if you wanted to query the state at that height
There was a problem hiding this comment.
sounds good, agreed we should remove it then
app/app.go
Outdated
| func (app *App) migrateCommitStore(fromVersion, toVersion uint64) (baseapp.StoreMigrations, error) { | ||
| oldStoreKeys := app.keyVersions[fromVersion] | ||
| newStoreKeys := app.keyVersions[toVersion] | ||
| newMap := make(map[string]bool) | ||
| output := baseapp.StoreMigrations{ | ||
| Added: make(map[string]*storetypes.KVStoreKey), | ||
| Deleted: make(map[string]*storetypes.KVStoreKey), | ||
| } | ||
| if toVersion == v2 { | ||
| // we need to set the app version in the param store for the first time | ||
| app.SetInitialAppVersionInConsensusParams(ctx, toVersion) | ||
| for _, storeKey := range newStoreKeys { | ||
| newMap[storeKey] = false | ||
| } | ||
| app.SetAppVersion(ctx, toVersion) | ||
| app.SignalKeeper.ResetTally(ctx) | ||
| return nil | ||
| for _, storeKey := range oldStoreKeys { | ||
| if _, ok := newMap[storeKey]; !ok { | ||
| output.Deleted[storeKey] = app.keys[storeKey] | ||
| } else { | ||
| // this module exists in both the old and new modules | ||
| newMap[storeKey] = true | ||
| } | ||
| } | ||
| for storeKey, existsInOldModule := range newMap { | ||
| if !existsInOldModule { | ||
| output.Added[storeKey] = app.keys[storeKey] | ||
| } | ||
| } | ||
| return output, nil | ||
| } |
There was a problem hiding this comment.
[optional] Here's a refactored implementation that I thought was easier to read. Defer to you if you want to include it:
func (app *App) MigrateCommitStore(oldStoreKeys []string, newStoreKeys []string) (baseapp.StoreMigrations, error) {
result := baseapp.StoreMigrations{
Added: make(map[string]*storetypes.KVStoreKey),
Deleted: make(map[string]*storetypes.KVStoreKey),
}
for _, oldKey := range oldStoreKeys {
if !slices.Contains(newStoreKeys, oldKey) {
result.Deleted[oldKey] = app.keys[oldKey]
}
}
for _, newKey := range newStoreKeys {
if !slices.Contains(oldStoreKeys, newKey) {
result.Added[newKey] = app.keys[newKey]
}
}
return result, nil
}with unit tests
func Test_migrateCommitStore(t *testing.T) {
logger := log.NewNopLogger()
db := tmdb.NewMemDB()
traceStore := &NoopWriter{}
invCheckPeriod := uint(1)
encodingConfig := encoding.MakeConfig(app.ModuleEncodingRegisters...)
upgradeHeight := int64(0)
appOptions := NoopAppOptions{}
testApp := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, upgradeHeight, appOptions)
type testCase struct {
oldStoreKeys []string
newStoreKeys []string
want baseapp.StoreMigrations
}
testCases := []testCase{
{
oldStoreKeys: []string{"key1", "key2"},
newStoreKeys: []string{"key1", "key2"},
want: baseapp.StoreMigrations{
Added: map[string]*storetypes.KVStoreKey{},
Deleted: map[string]*storetypes.KVStoreKey{},
},
},
{
oldStoreKeys: []string{"key1"},
newStoreKeys: []string{"key1", "key2"},
want: baseapp.StoreMigrations{
Added: map[string]*storetypes.KVStoreKey{"key2": nil},
Deleted: map[string]*storetypes.KVStoreKey{},
},
},
{
oldStoreKeys: []string{"key1", "key2"},
newStoreKeys: []string{"key1"},
want: baseapp.StoreMigrations{
Added: map[string]*storetypes.KVStoreKey{},
Deleted: map[string]*storetypes.KVStoreKey{"key2": nil},
},
},
}
for _, tc := range testCases {
got, err := testApp.MigrateCommitStore(tc.oldStoreKeys, tc.newStoreKeys)
require.NoError(t, err)
assert.Equal(t, tc.want, got)
}
}Note: you'll have to convert the function signature back to using fromVersion / toVersion
There was a problem hiding this comment.
Thanks I definitely need to add the unit tests. It also reads easier and I don't think this is something that needs to be optimized so I will adopt your suggestion
| // we need to know the app version to know what stores to mount | ||
| // thus the paramstore must always be a store that is mounted |
There was a problem hiding this comment.
[not blocking][question] why is app version stored in the param store and consensus params? Is it stored anywhere else?
There was a problem hiding this comment.
No app version is only part of the consensus params in the param store. We load it in memory upon startup for easy reference.
I think the SDK ended up splitting out the param module which will likely be an annoying change to adopt
app/app.go
Outdated
| // versionedStoreKeys returns the store keys for each app version | ||
| // ... I wish there was an easier way than this (like using the modules which are already versioned) | ||
| func versionedStoreKeys() map[uint64][]string { |
There was a problem hiding this comment.
[question] does this mean that to disable blobstream in v2, we have to do:
{
Module: blobstream.NewAppModule(appCodec, app.BlobstreamKeeper),
FromVersion: v1, ToVersion: v1,
},and
2: {
authtypes.StoreKey, authzkeeper.StoreKey, banktypes.StoreKey, stakingtypes.StoreKey,
minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey,
govtypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey,
evidencetypes.StoreKey, capabilitytypes.StoreKey,
- blobstreamtypes.StoreKey, ibctransfertypes.StoreKey, ibchost.StoreKey,
+ ibctransfertypes.StoreKey, ibchost.StoreKey,
packetforwardtypes.StoreKey, icahosttypes.StoreKey, signaltypes.StoreKey,
},There was a problem hiding this comment.
[nit] it may be easier to read subsequent diffs if all the store keys were on new lines:
1: {
authtypes.StoreKey,
authzkeeper.StoreKey,
banktypes.StoreKey,
blobstreamtypes.StoreKey,
blobtypes.StoreKey,
capabilitytypes.StoreKey,
distrtypes.StoreKey,
evidencetypes.StoreKey,
feegrant.StoreKey,
govtypes.StoreKey,
ibchost.StoreKey,
ibctransfertypes.StoreKey,
minttypes.StoreKey,
slashingtypes.StoreKey,
stakingtypes.StoreKey,
upgradetypes.StoreKey,
},
2: {
authtypes.StoreKey,
authzkeeper.StoreKey,
banktypes.StoreKey,
blobstreamtypes.StoreKey,
capabilitytypes.StoreKey,
distrtypes.StoreKey,
evidencetypes.StoreKey,
feegrant.StoreKey,
govtypes.StoreKey,
ibchost.StoreKey,
ibctransfertypes.StoreKey,
icahosttypes.StoreKey, // added in v2
minttypes.StoreKey,
packetforwardtypes.StoreKey, // added in v2
signaltypes.StoreKey, // added in v2
slashingtypes.StoreKey,
stakingtypes.StoreKey,
upgradetypes.StoreKey,
},There was a problem hiding this comment.
Yup, I think that should be everything.
I wonder if we can even remove the directory and import "github.com/celestiaorg/celestia-app/x/blobstream"
I can list it out new lines
There was a problem hiding this comment.
I wonder if we can even remove the directory and import "github.com/celestiaorg/celestia-app/x/blobstream"
Cool idea! I just checked and it looks like we may have to use the old name still b/c the last published release of celestia-app (v1.8.0) still calls it qgb: https://pkg.go.dev/github.com/celestiaorg/[email protected]/x/qgb
Co-authored-by: Rootul P <[email protected]>
|
I've added a few sanity checks upon initialisation to make it more difficult to miss a component when adding modules |
| } | ||
|
|
||
| // versionedStoreKeys returns the store keys for each app version | ||
| // ... I wish there was an easier way than this (like using the modules which are already versioned) |
There was a problem hiding this comment.
[optional]
maybe remove this comment or rephrase. "While this mapping is redundant, unfortunately this has to be done atm because..."
There was a problem hiding this comment.
Yeah I'll remove it in a follow up PR
| @@ -231,7 +232,6 @@ func New( | |||
| logger log.Logger, | |||
| db dbm.DB, | |||
| traceStore io.Writer, | |||
| loadLatest bool, | |||
There was a problem hiding this comment.
sounds good, agreed we should remove it then
app/app.go
Outdated
| // versionedStoreKeys returns the store keys for each app version | ||
| // ... I wish there was an easier way than this (like using the modules which are already versioned) | ||
| func versionedStoreKeys() map[uint64][]string { |
There was a problem hiding this comment.
I wonder if we can even remove the directory and import "github.com/celestiaorg/celestia-app/x/blobstream"
Cool idea! I just checked and it looks like we may have to use the old name still b/c the last published release of celestia-app (v1.8.0) still calls it qgb: https://pkg.go.dev/github.com/celestiaorg/[email protected]/x/qgb
Closes #3392 Opens #3472 Fixes a few bugs: 1. Previously all modules had `ExportGenesis` invoked on them even if they weren't supported by the current app version. Now we only call `ExportGenesis` for the modules that are supported by the current app version 2. The export command wasn't updated to account for the changes in #3320 which force us to mount stores after `app.New()` based on the current app version 3. The minfee module couldn't be exported b/c it didn't register a key table in `ExportGenesis` ## Testing I could export an app on app version 1 and 2. See [output](https://gist.github.com/rootulp/dfea2b5b40f7366b03706fc39321ceee)
Closes celestiaorg/celestia-app#3392 Opens celestiaorg/celestia-app#3472 Fixes a few bugs: 1. Previously all modules had `ExportGenesis` invoked on them even if they weren't supported by the current app version. Now we only call `ExportGenesis` for the modules that are supported by the current app version 2. The export command wasn't updated to account for the changes in celestiaorg/celestia-app#3320 which force us to mount stores after `app.New()` based on the current app version 3. The minfee module couldn't be exported b/c it didn't register a key table in `ExportGenesis` ## Testing I could export an app on app version 1 and 2. See [output](https://gist.github.com/rootulp/dfea2b5b40f7366b03706fc39321ceee)
As was discovered with the apphash mismatch in #3167, by adding both v1 and v2 stores in the constructor we actually end up with a different app hash to
v1.xas now there are a bunch of empy but initialized stores. What's needed is a mechanism that can start with only the v1 stores and then during the upgrade process add or remove the store for v2 as necessary.The main problem with this is that because we are using a branch of state we can't actually modify the stores until we write the new state to disk. Hence we can no longer perform the migration in
EndBlockbut inCommitwhereby we first write state, then create the new stores, then perform the module migrations (like callInitGenesis) and then write it again finally returning the app hash.This PR depends on celestiaorg/cosmos-sdk#387 which introduces the upgrade hooks during
Commit. This PR itself adds the two hooks for updating the stores and performing the migrations.