feat: support migrations during version upgrades #3112

cmwaters · 2024-02-15T12:32:01Z

Closes: #3105

rootulp

Kinda makes sense to me. My takeaway is this introduces a new versioned module type that is aware of the version range it supports. That seems reasonable. The logic around running migrations also seems sound so no big blocking feedback

rootulp · 2024-02-15T17:55:29Z

app/app.go

@@ -190,7 +191,7 @@ type App struct {
 	mm *module.Manager

 	// module configurator


[optional] this comment doesn't add any value so it's just visual noise

Suggested change

// module configurator

rootulp · 2024-02-15T17:59:20Z

app/module/module.go

+
+type VersionedModule struct {
+	module                 sdkmodule.AppModule
+	fromVersion, toVersion uint64


[blocking] Go docs would be helpful for these two fields. Does the range defined by [fromVersion, toVersion] describe the versions this module supports?

rootulp · 2024-02-15T18:00:25Z

app/app.go

+		module.NewVersionedModule(transfer.NewAppModule(app.TransferKeeper), v1.Version, v2.Version),
+		module.NewVersionedModule(blob.NewAppModule(appCodec, app.BlobKeeper), v1.Version, v2.Version),
+		module.NewVersionedModule(blobstream.NewAppModule(appCodec, app.BlobstreamKeeper), v1.Version, v2.Version),
+		module.NewVersionedModule(upgrade.NewAppModule(app.UpgradeKeeper), v2.Version, v2.Version),


[question] why is v2.Version used as both the second and third param? Is it because the upgrade module only supports v2 and isn't enabled in v1?

See other comment but Go docs on what the fromVersion and toVersion describe would help.

rootulp · 2024-02-15T18:01:51Z

app/app.go

 		}
 	}
 	return res
 }

+func (app *App) Upgrade(ctx sdk.Context, version uint64) error {
+	app.SetAppVersion(ctx, version)
+	return app.mm.RunMigrations(ctx, app.configurator, version, version)


[question] why is version used for both fromVersion and toVersion? If we're upgrading, from a version to a different version, I would expect these two versions to be distinct.

Can a clarifying comment be added?

rootulp · 2024-02-15T18:04:29Z

app/module/configurator.go

+// RegisterMigration implements the Configurator.RegisterMigration method
+func (c configurator) RegisterMigration(moduleName string, fromVersion uint64, handler module.MigrationHandler) error {
+	if fromVersion == 0 {
+		return sdkerrors.ErrInvalidVersion.Wrap("module migration versions should start at 1")


does this mean no migrations can occur from v0 -> v1? Does other code already assume version starts at v1?

Correct. Yes we started on v1 and there are assumptions that v0 doesn't exist and implies that the version has forgotten to be set

thanks!

[tangent] Is there already an assertion somewhere that fails on chain start up if a chain is initialized with v0? Asking because I haven't seen an assertion like that but I expect it exists somewhere.

Yes I believe I've added it. I'll double check

app/module/module.go

rootulp · 2024-02-15T18:12:37Z

app/module/module.go

+	m.OrderBeginBlockers = moduleNames
+}
+
+// SetOrderEndBlockers sets the order of set end-blocker calls


Suggested change

// SetOrderEndBlockers sets the order of set end-blocker calls

// SetOrderEndBlockers sets the order of end-blocker calls

app/module/module.go

rootulp · 2024-02-15T18:22:10Z

app/module/module.go

+		OrderInitGenesis:   modulesStr,
+		OrderExportGenesis: modulesStr,
+		OrderBeginBlockers: modulesStr,
+		OrderEndBlockers:   modulesStr,


[tangent] I don't think we need to do this at the same time as this PR but I wonder if all these could use the same order: #3052.

Yeah I was merely copying across from the SDK

cmwaters · 2024-02-16T13:14:10Z

My takeaway is this introduces a new versioned module type that is aware of the version range it supports

Yeah it's basically that and it multiplexes based on the version it expects to be on

…nto cal/migration

coderabbitai · 2024-02-29T12:48:32Z

Walkthrough

The changes introduce significant updates to the application structure and module management, with a particular focus on enhancing the upgrade process and module migrations. A new dependency for managing modules and an update to existing imports reflect a broader architectural shift towards better modularity and upgradeability. The addition of migration support during the upgrade process, as indicated by the introduction of an Upgrade method and adjustments in key functions, aligns with the objectives to streamline version transitions and module management.

Changes

Files	Change Summary
`app/app.go`	- New import - Import rename - New constant - Refactorings and method adjustments
`app/module/module.go`	- Introduction of module manager
`go.mod`	- New dependency added
`go.work.sum`	- New modules and versions added

Assessment against linked issues

Objective	Addressed	Explanation
Add support for migrations during the upgrade process (#3105)	✅

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Note: Auto-reply has been disabled for this repository by the repository owner. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

coderabbitai · 2024-02-29T12:51:34Z

app/module/module.go

+// RunMigrations performs in-place store migrations for all modules. This
+// function MUST be called when the state machine changes appVersion
+func (m Manager) RunMigrations(ctx sdk.Context, cfg sdkmodule.Configurator, fromVersion, toVersion uint64) error {
+	c, ok := cfg.(configurator)
+	if !ok {
+		return sdkerrors.ErrInvalidType.Wrapf("expected %T, got %T", configurator{}, cfg)
+	}
+	modules := m.OrderMigrations
+	if modules == nil {
+		modules = DefaultMigrationsOrder(m.ModuleNames(toVersion))
+	}
+	currentVersionModules, exists := m.versionedModules[fromVersion]
+	if !exists {
+		return sdkerrors.ErrInvalidVersion.Wrapf("version %d not supported", fromVersion)
+	}
+	nextVersionModules, exists := m.versionedModules[toVersion]
+	if !exists {
+		return sdkerrors.ErrInvalidVersion.Wrapf("version %d not supported", toVersion)
+	}
+
+	for _, moduleName := range modules {
+		_, currentModuleExists := currentVersionModules[moduleName]
+		nextModule, nextModuleExists := nextVersionModules[moduleName]
+
+		// if the module exists for both upgrades
+		if currentModuleExists && nextModuleExists {
+			err := c.runModuleMigrations(ctx, moduleName, fromVersion, toVersion)
+			if err != nil {
+				return err
+			}
+		} else if !currentModuleExists && nextModuleExists {
+			ctx.Logger().Info(fmt.Sprintf("adding a new module: %s", moduleName))
+			moduleValUpdates := nextModule.InitGenesis(ctx, c.cdc, nextModule.DefaultGenesis(c.cdc))
+			// The module manager assumes only one module will update the
+			// validator set, and it can't be a new module.
+			if len(moduleValUpdates) > 0 {
+				return sdkerrors.ErrLogic.Wrap("validator InitGenesis update is already set by another module")
+			}
+		}
+		// TODO: handle the case where a module is no longer supported (i.e. removed from the state machine)
+	}
+
+	return nil


The RunMigrations method is a key component of the versioning system, allowing for in-place store migrations between versions. The method's logic, including handling new modules and ensuring that only one module can update the validator set during migration, is correctly implemented. However, the TODO comment on line 245 indicates that handling the removal of modules is not yet addressed, which is an important aspect to consider for future development.

Would you like me to open a GitHub issue to track the implementation of handling module removals during migrations?

coderabbitai · 2024-02-29T12:51:34Z

test/tokenfilter/tokenfilter_test.go

+	// FIXME: this is because the ibctesting framework we use
+	// doesn't set the version in the header which we require
+	// to know which state machine to execute against
+	t.Skip("token filter tests are currently not supported")


Skipping the token filter tests due to the missing version in the header is a temporary workaround. It's important to address this limitation in the testing framework to ensure that token filter functionality can be thoroughly tested, especially in the context of version-specific behavior.

Consider enhancing the testing framework to include version information in the header, allowing these tests to be re-enabled and ensuring comprehensive test coverage.

coderabbitai · 2024-02-29T12:51:34Z

app/module/module_test.go

+func TestManager_InitGenesis(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	t.Cleanup(mockCtrl.Finish)
+
+	mockAppModule1 := mocks.NewMockAppModule(mockCtrl)
+	mockAppModule2 := mocks.NewMockAppModule(mockCtrl)
+	mockAppModule1.EXPECT().Name().Times(2).Return("module1")
+	mockAppModule2.EXPECT().Name().Times(2).Return("module2")
+	mm, err := module.NewManager(module.NewVersionedModule(mockAppModule1, 1, 1), module.NewVersionedModule(mockAppModule2, 1, 1))
+	require.NoError(t, err)
+	require.NotNil(t, mm)
+	require.Equal(t, 2, len(mm.ModuleNames(1)))
+
+	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
+	interfaceRegistry := types.NewInterfaceRegistry()
+	cdc := codec.NewProtoCodec(interfaceRegistry)
+	genesisData := map[string]json.RawMessage{"module1": json.RawMessage(`{"key": "value"}`)}
+
+	// this should panic since the validator set is empty even after init genesis
+	mockAppModule1.EXPECT().InitGenesis(gomock.Eq(ctx), gomock.Eq(cdc), gomock.Eq(genesisData["module1"])).Times(1).Return(nil)
+	require.Panics(t, func() { mm.InitGenesis(ctx, cdc, genesisData, 1) })
+
+	// test panic
+	genesisData = map[string]json.RawMessage{
+		"module1": json.RawMessage(`{"key": "value"}`),
+		"module2": json.RawMessage(`{"key": "value"}`),
+	}
+	mockAppModule1.EXPECT().InitGenesis(gomock.Eq(ctx), gomock.Eq(cdc), gomock.Eq(genesisData["module1"])).Times(1).Return([]abci.ValidatorUpdate{{}})
+	mockAppModule2.EXPECT().InitGenesis(gomock.Eq(ctx), gomock.Eq(cdc), gomock.Eq(genesisData["module2"])).Times(1).Return([]abci.ValidatorUpdate{{}})
+	require.Panics(t, func() { mm.InitGenesis(ctx, cdc, genesisData, 1) })
+}


The TestManager_InitGenesis function tests the initialization of genesis state by modules. This includes a check for panic when the validator set is empty, which is a critical test case for ensuring the robustness of the genesis initialization process. However, the comment about expecting a panic due to an empty validator set might be misleading since the panic is actually triggered by the test setup and not by the InitGenesis method itself.

Consider clarifying the comment regarding the expected panic in TestManager_InitGenesis to accurately reflect the reason for the panic, ensuring that it does not mislead readers into thinking it's an inherent behavior of the InitGenesis method.

coderabbitai · 2024-02-29T12:51:34Z

app/module/module_test.go

+func TestManager_EndBlock(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	t.Cleanup(mockCtrl.Finish)
+
+	mockAppModule1 := mocks.NewMockAppModule(mockCtrl)
+	mockAppModule2 := mocks.NewMockAppModule(mockCtrl)
+	mockAppModule1.EXPECT().Name().Times(2).Return("module1")
+	mockAppModule2.EXPECT().Name().Times(2).Return("module2")
+	mm, err := module.NewManager(module.NewVersionedModule(mockAppModule1, 1, 1), module.NewVersionedModule(mockAppModule2, 1, 1))
+	require.NoError(t, err)
+	require.NotNil(t, mm)
+	require.Equal(t, 2, len(mm.ModuleNames(1)))
+
+	req := abci.RequestEndBlock{Height: 10}
+
+	mockAppModule1.EXPECT().EndBlock(gomock.Any(), gomock.Eq(req)).Times(1).Return([]abci.ValidatorUpdate{{}})
+	mockAppModule2.EXPECT().EndBlock(gomock.Any(), gomock.Eq(req)).Times(1)
+	ctx := sdk.NewContext(nil, tmproto.Header{
+		Version: tmversion.Consensus{App: 1},
+	}, false, log.NewNopLogger())
+	ret := mm.EndBlock(ctx, req)
+	require.Equal(t, []abci.ValidatorUpdate{{}}, ret.ValidatorUpdates)
+
+	// test panic
+	mockAppModule1.EXPECT().EndBlock(gomock.Any(), gomock.Eq(req)).Times(1).Return([]abci.ValidatorUpdate{{}})
+	mockAppModule2.EXPECT().EndBlock(gomock.Any(), gomock.Eq(req)).Times(1).Return([]abci.ValidatorUpdate{{}})
+	require.Panics(t, func() { mm.EndBlock(ctx, req) })
+}


The TestManager_EndBlock function tests the handling of the end block event by modules. This includes testing the aggregation of validator updates, which is crucial for the application's consensus mechanism. The test also includes a case for expecting a panic, which is important for ensuring the robustness of the end block handling. However, similar to the InitGenesis test, the reason for the expected panic should be clarified to avoid confusion.

Clarify the comment regarding the expected panic in TestManager_EndBlock to accurately reflect the reason for the panic, ensuring it does not mislead readers into thinking it's an inherent behavior of the EndBlock method.

app/app.go

evan-forbes

this LGTM

the mocks are great essential unit tests, and this might be planned and I just missed it, but it seems v2 blocking to have some test that will break if things outside of the Manager change. Similar to #3137, if there are other things that are necessary for the complete integration, can we make a point to document them before merging and then discuss them in the next v2 sync? cc @rootulp

evan-forbes · 2024-02-29T13:48:44Z

app/module/module.go

+		fromVersion: fromVersion,
+		toVersion:   toVersion,


[not blocking question]

are there scenarios where from and to are not consecutive?

It's meant as a range. The same module could be used over several app versions

app/module/module.go

Co-authored-by: Rootul P <rootulp@gmail.com>

rootulp · 2024-03-01T20:53:18Z

app/app.go

+func (app *App) Upgrade(ctx sdk.Context, version uint64) error {
+	app.SetAppVersion(ctx, version)
+	return app.mm.RunMigrations(ctx, app.configurator, app.AppVersion(ctx), version)
+}


[optional] some log or event here

rootulp · 2024-03-01T21:17:45Z

app/app.go

+	// genesis must always contain the consensus params. The validator set howerver is derived from the
+	// initial genesis state


I don't understand this comment, can it be rephrased? The consensus params and the validator set seem orthogonal.

Also the validator set doesn't seem applicable to the following three lines so if it's not relevant in this context, proposal to delete that portion of the comment.

I could perhaps move this comment to the function signature. I think it's important since normally there are multiple ways of setting the consensus params and validator set that we clarify which one we use

staheri14

LGTM, added some suggestions and questions, will defer to other reviewers for the final approval.

staheri14 · 2024-03-02T01:54:49Z

app/module/module.go

+
+// NewManager creates a new Manager object
+func NewManager(modules ...VersionedModule) (*Manager, error) {
+	moduleMap := make(map[uint64]map[string]sdkmodule.AppModule)


Could be beneficial to add a comment about the interpretation of the keys and values in this moduleMap i.e., version -> [module name -> module]

staheri14 · 2024-03-02T02:00:03Z

app/module/module.go

+		if module.fromVersion > module.toVersion {
+			return nil, sdkerrors.ErrLogic.Wrapf("fromVersion can not be greater than toVersion for module %s", module.module.Name())
+		}
+		for version := module.fromVersion; version <= module.toVersion; version++ {


Mind explaining in the code what fromVersion and toVersion indicates for each module?

staheri14 · 2024-03-02T02:10:04Z

app/module/module.go

+	moduleMap := make(map[uint64]map[string]sdkmodule.AppModule)
+	allModules := make([]sdkmodule.AppModule, len(modules))
+	modulesStr := make([]string, 0, len(modules))
+	firstVersion, lastVersion := uint64(0), uint64(0)


A description for what firstVersion and lastVersion hold would be helpful.

staheri14 · 2024-03-02T02:12:48Z

app/module/module.go

+			if moduleMap[version] == nil {
+				moduleMap[version] = make(map[string]sdkmodule.AppModule)
+			}
+			moduleMap[version][module.module.Name()] = module.module


[question] Why the fromVersion is taken into account but not the toVersion (I guess it is related to my earlier question about the usage of fromVersion and toVersion).

I don't quite understand this question. All version fromVersion to toVersion are added to the moduleMap

What I meant to say is that the version variable contains the value obtained from module.fromModule. In the map, however, we only store the module corresponding to fromVersion and not toVersion. This leads to my confusion: shouldn't the same process apply to all versions ranging from fromVersion to toVersion? This mostly relates to my earlier question regarding the purpose of toVersion.

app/module/module.go

Closes: celestiaorg#3105 --------- Co-authored-by: Rootul P <rootulp@gmail.com>

cmwaters added 2 commits February 15, 2024 13:31

create a versioned module manager

50dabdb

fix some tests

2633565

rootulp reviewed Feb 15, 2024

View reviewed changes

cmwaters added 8 commits February 18, 2024 09:24

chore: bump cometbft to v1.34.0

2200857

Merge branch 'cal/bump-comet' into cal/migration

36bc349

Merge branch 'main' into cal/migration

9f44974

checkpoint

83c28cf

Merge branch 'cal/migration' of github.com:celestiaorg/celestia-app i…

e535264

…nto cal/migration

Merge branch 'main' into cal/migration

0187da6

lint and fix tests

4b194c1

disable token filter test

197098c

cmwaters mentioned this pull request Feb 28, 2024

ibctesting framework does not support multiple versions #3137

Closed

cmwaters marked this pull request as ready for review February 29, 2024 12:48

cmwaters requested review from evan-forbes, staheri14 and ninabarbakadze as code owners February 29, 2024 12:48

Merge branch 'main' into cal/migration

a5a5f62

celestia-bot requested a review from a team February 29, 2024 12:48

coderabbitai bot reviewed Feb 29, 2024

View reviewed changes

evan-forbes previously approved these changes Feb 29, 2024

View reviewed changes

evan-forbes mentioned this pull request Feb 29, 2024

Handle the case where a module is no longer supported #3139

Closed

Apply suggestions from code review

af73b30

Co-authored-by: Rootul P <rootulp@gmail.com>

cmwaters dismissed evan-forbes’s stale review via af73b30 February 29, 2024 17:37

celestia-bot requested a review from a team February 29, 2024 17:37

evan-forbes previously approved these changes Feb 29, 2024

View reviewed changes

rootulp assigned cmwaters Mar 1, 2024

rootulp mentioned this pull request Mar 1, 2024

EPIC: Single Binary Syncs #1568

Closed

1 task

rootulp previously approved these changes Mar 1, 2024

View reviewed changes

staheri14 reviewed Mar 2, 2024

View reviewed changes

Merge branch 'main' into cal/migration

0c4410c

celestia-bot requested review from a team and rach-id and removed request for a team March 4, 2024 10:10

add more comments

c130787

cmwaters dismissed stale reviews from evan-forbes and rootulp via c130787 March 4, 2024 10:39

celestia-bot requested a review from a team March 4, 2024 10:39

cmwaters requested review from staheri14, rootulp and evan-forbes March 4, 2024 14:07

rootulp previously approved these changes Mar 4, 2024

View reviewed changes

evan-forbes previously approved these changes Mar 4, 2024

View reviewed changes

Merge branch 'main' into cal/migration

c97ba6e

cmwaters dismissed stale reviews from evan-forbes and rootulp via c97ba6e March 5, 2024 07:58

celestia-bot requested a review from a team March 5, 2024 07:58

evan-forbes approved these changes Mar 5, 2024

View reviewed changes

rootulp approved these changes Mar 5, 2024

View reviewed changes

cmwaters merged commit 9a920ba into main Mar 5, 2024
33 checks passed

cmwaters deleted the cal/migration branch March 5, 2024 17:52

ninabarbakadze pushed a commit to ninabarbakadze/celestia-app that referenced this pull request Apr 2, 2024

feat: support migrations during version upgrades (celestiaorg#3112)

6ee5795

Closes: celestiaorg#3105 --------- Co-authored-by: Rootul P <rootulp@gmail.com>

		@@ -190,7 +191,7 @@ type App struct {
		mm *module.Manager

		// module configurator

	// SetOrderEndBlockers sets the order of set end-blocker calls
	// SetOrderEndBlockers sets the order of end-blocker calls

		// genesis must always contain the consensus params. The validator set howerver is derived from the
		// initial genesis state

feat: support migrations during version upgrades #3112

feat: support migrations during version upgrades #3112

Conversation

cmwaters commented Feb 15, 2024

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters commented Feb 16, 2024

coderabbitai bot commented Feb 29, 2024 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

coderabbitai bot Feb 29, 2024

Choose a reason for hiding this comment

coderabbitai bot Feb 29, 2024

Choose a reason for hiding this comment

coderabbitai bot Feb 29, 2024

Choose a reason for hiding this comment

coderabbitai bot Feb 29, 2024

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot commented Feb 29, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)