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

feat: prototype versioned app module upgrades #3729

Draft
wants to merge 133 commits into
base: main
Choose a base branch
from

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jul 24, 2024

Closes #3625

Issues encountered

  1. Modules register errors via errors.Register. Since the multiplexer imports multiple celestia-app versions, all errors get registered twice resulting in a: panic: error with code 2 is already registered: "duplicate". To resolve, we need to:
    1. Make errors unique so that they don't conflict across app versions
    2. Modify errors.Register to not panic if a duplicate error is registered. Did this in https://github.com/rootulp/cosmos-sdk/releases/tag/errors%2Fv1.4.0
  2. panic: concrete type *types.MsgPayForBlobs has already been registered under typeURL /, cannot register *types.MsgRegisterEVMAddress under same typeURL. This usually means that there are conflicting modules registering different concrete types for a same interface implementation.
    1. Option A: bump the proto package versions (did in this PR)
    2. Option B: Create a unique go.mod for each package and de-duplicate imports. Note: we'll have to extract the x/blob package from this repo so that it isn't present in the root-level celestia-app go module. Use the v2 version because it has types that aren't present in the v1 version.
    3. Option C: Make the registries distinct per state machine

Remaining tasks

  • Remove v3 upgrade height because the upgrade from v2 -> v3 will use the signaling mechanism.
  • Extract the testnode refactors to a separate PR or remove them entirely
  • Update this branch to latest on main. Extract the previous changes to a new v2 branch.
  • Revert the unnecessary replace statements in celestia-app/node/go.mod
  • Add a logger to multiplexer. Convert print statements to logger.Debug statements.
  • Figure out a long-term solution so that two state machines (v2 and v3) can invoke config.Seal() or move it to some place where it is only invoked once. Resolved via feat: expose config.IsSealed cosmos-sdk#414
  • Figure out a long term solution for proto conflicts for SDK registry.
  • Consider removing the module manager from v3 entirely.
  • Remove the diff in v2 that extends the range for all modules to v3. Why do module migrations need to occur in Commit of the block height before the upgrade height? In order to satisfy that constraint, the v2 state machine has to be updated so that modules support the range v1 to v3. If we remove v2 supporting v3 modules then we hit this error
  • Support module migrations
  • Modify the multiplexer CLI so that it replaces the existing celestia-appd start CLI
  • Add unit tests
  • Add e2e tests
  • Follow-up: remove most of the app version checks from v3 state machine
  • Follow-up: remove versioned constants from v3 state machine
  • Follow-up: Make v2 state machine only support v2 (?) b/c we might not do this
  • Update Multiplexer to not hardcode v2 app version intialization and instead run Info() get the version and then start that state machine

Notes

  • Previous branches: v1.x and v3.x .
  • To run this locally you need the v2.x locally then from the repo root:
make build-node && ./build/node

@rootulp rootulp self-assigned this Jul 24, 2024
$ ./node
panic: error with code 2 is already registered: "duplicate"

goroutine 1 [running]:
cosmossdk.io/errors.RegisterWithGRPCCode({0x105b93b3f, 0x3}, 0x2, 0x2, {0x105b9a8f5, 0x9})
	/Users/rootulp/go/pkg/mod/cosmossdk.io/[email protected]/errors.go:43 +0x240
cosmossdk.io/errors.Register(...)
	/Users/rootulp/go/pkg/mod/cosmossdk.io/[email protected]/errors.go:35
github.com/celestiaorg/celestia-app/x/qgb/types.init()
	/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/qgb/types/errors.go:8 +0x128
@rootulp

This comment was marked as resolved.

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 24, 2024

New issue:

panic: concrete type *types.MsgPayForBlobs has already been registered under typeURL /, cannot register *types.MsgRegisterEVMAddress under same typeURL. This usually means that there are conflicting modules registering different concrete types for a same interface implementation

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/codec/types.(*interfaceRegistry).registerImpl(0x14000181dd0, {0x104623f80?, 0x0?}, {0x104536430, 0x1}, {0x104aaaab0, 0x1400126c9c0})
	/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/codec/types/interface_registry.go:151 +0x2ec
github.com/cosmos/cosmos-sdk/codec/types.(*interfaceRegistry).RegisterImplementations(0x14000181dd0, {0x104623f80, 0x0}, {0x140012dc4b0?, 0x104abe2e8?, 0x14000181dd0?})
	/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/codec/types/interface_registry.go:115 +0x84
github.com/celestiaorg/celestia-app/x/qgb/types.RegisterInterfaces({0x104abe2e8, 0x14000181dd0})
	/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/qgb/types/codec.go:17 +0x9c
github.com/celestiaorg/celestia-app/x/qgb.AppModuleBasic.RegisterInterfaces(...)
	/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/qgb/module.go:58
github.com/celestiaorg/celestia-app/app/encoding.MakeConfig({0x1400107ea00, 0x14, 0x10466f380?})
	/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/app/encoding/encoding.go:37 +0x1cc
github.com/celestiaorg/celestia-app/node/utils.NewAppV1()
	/Users/rootulp/git/rootulp/celestiaorg/celestia-app/node/utils/apps.go:27 +0x170
github.com/celestiaorg/celestia-app/node/utils.GetApps()
	/Users/rootulp/git/rootulp/celestiaorg/celestia-app/node/utils/apps.go:14 +0x1c
github.com/celestiaorg/celestia-app/node/cmd.init.func1(0x1400078b900?, {0x10390c42e?, 0x4?, 0x10390c432?})
	/Users/rootulp/git/rootulp/celestiaorg/celestia-app/node/cmd/root.go:29 +0x24
github.com/spf13/cobra.(*Command).execute(0x105d7b520, {0x14000114060, 0x0, 0x0})
	/Users/rootulp/go/pkg/mod/github.com/spf13/[email protected]/command.go:985 +0x840
github.com/spf13/cobra.(*Command).ExecuteC(0x105d7b520)
	/Users/rootulp/go/pkg/mod/github.com/spf13/[email protected]/command.go:1117 +0x344
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/rootulp/go/pkg/mod/github.com/spf13/[email protected]/command.go:1041
github.com/celestiaorg/celestia-app/node/cmd.Execute()
	/Users/rootulp/git/rootulp/celestiaorg/celestia-app/node/cmd/root.go:46 +0x24
main.main()
	/Users/rootulp/git/rootulp/celestiaorg/celestia-app/node/main.go:6 +0x1c

@cmwaters
Copy link
Contributor

This was the error that I first got when trying to unfork the SDK. There is a global proto registry and thus when we import multiple versions we end up with this panic. One option would be to go mod the modules that way there's not two copies of x/blob for instance but both state machines point to the same version. The other option would be to modify the register code such that if it saw the same type url name, it would check the fields (using reflect maybe) and so long as they were exactly the same it would ignore the panic

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 25, 2024

Thanks for your help!

One option would be to go mod the modules that way there's not two copies of x/blob for instance but both state machines point to the same version.

What if celestia-app v3 needs x/blob/v2 and celestia-app v1 needs x/blob/v1, then I think we'll hit the same issue where both modules try to register the same types in the global proto registry.

The other option would be to modify the register code such that if it saw the same type url name, it would check the fields (using reflect maybe) and so long as they were exactly the same it would ignore the panic

This one seems more hacky but potentially do-able. The concern is if celestia-app v2's x/blob actually does modify a field from a message type and the two types conflict on MsgTypeUrl but differ on fields. IMO if an SDK message's fields do change, that would be a breaking change of the x/blob package which we can workaround by defining a v2 for that type and module. In other words if MsgPayForBlobs changes we need to introduce MsgPayForBlobsV2 with a unique MsgTypeURL.

@cmwaters
Copy link
Contributor

What if celestia-app v3 needs x/blob/v2 and celestia-app v1 needs x/blob/v1, then I think we'll hit the same issue where both modules try to register the same types in the global proto registry.

If we created a x/blob/2 we would bump the proto version so the msg url would look like celestia.blob.v2.MsgPayForBlob

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 31, 2024

I've tried a few times to create a Go module from x/blob but the gopls in VSCode messes up the import paths. Currently hitting:

$ make build-node
--> Building celestia-app/node and outputting binary to build/node
# github.com/celestiaorg/celestia-app/v2/app/ante
../app/ante/ante.go:69:12: undefined: blobante.NewMaxTotalBlobSizeDecorator
../app/ante/ante.go:73:12: undefined: blobante.NewBlobShareDecorator

error while importing github.com/celestiaorg/celestia-app/v2/app: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob in multiple modules:
github.com/celestiaorg/celestia-app v1.13.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob)
github.com/celestiaorg/celestia-app/x/blob v0.0.0 (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob)
@rootulp
Copy link
Collaborator Author

rootulp commented Aug 1, 2024

I thought the issue might be because v1.x has an x/blob that isn't a Go module yet so I repeated the steps above on the v1.x branch and hit this issue:

go: github.com/celestiaorg/celestia-app/x/blob: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob in multiple modules:
	github.com/celestiaorg/celestia-app v1.14.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob)
	github.com/celestiaorg/celestia-app/x/blob (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob)
go: github.com/celestiaorg/celestia-app/x/blob/ante: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob/ante in multiple modules:
	github.com/celestiaorg/celestia-app v1.14.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob/ante)
	github.com/celestiaorg/celestia-app/x/blob (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob/ante)
go: github.com/celestiaorg/celestia-app/x/blob/client/cli: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob/client/cli in multiple modules:
	github.com/celestiaorg/celestia-app v1.14.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob/client/cli)
	github.com/celestiaorg/celestia-app/x/blob (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob/client/cli)
go: github.com/celestiaorg/celestia-app/x/blob/client/testutil: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob/client/testutil in multiple modules:
	github.com/celestiaorg/celestia-app v1.14.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob/client/testutil)
	github.com/celestiaorg/celestia-app/x/blob (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob/client/testutil)
go: github.com/celestiaorg/celestia-app/x/blob/keeper: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob/keeper in multiple modules:
	github.com/celestiaorg/celestia-app v1.14.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob/keeper)
	github.com/celestiaorg/celestia-app/x/blob (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob/keeper)
go: github.com/celestiaorg/celestia-app/x/blob/types: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob/types in multiple modules:
	github.com/celestiaorg/celestia-app v1.14.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob/types)
	github.com/celestiaorg/celestia-app/x/blob (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob/types)
go: github.com/celestiaorg/celestia-app/x/blob/types/test: ambiguous import: found package github.com/celestiaorg/celestia-app/x/blob/types/test in multiple modules:
	github.com/celestiaorg/celestia-app v1.14.0 (/Users/rootulp/go/pkg/mod/github.com/celestiaorg/[email protected]/x/blob/types/test)
	github.com/celestiaorg/celestia-app/x/blob (/Users/rootulp/git/rootulp/celestiaorg/celestia-app/x/blob/types/test)

I tried cleaning the go mod cache and a few different variations of importing the x/blob package from the root go.mod. See https://github.com/rootulp/celestia-app/tree/rp/go-mod-x-blob

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 1, 2024

The original error message

panic: concrete type *types.MsgPayForBlobs has already been registered under typeURL /, cannot register *types.MsgRegisterEVMAddress under same typeURL. This usually means that there are conflicting modules registering different concrete types for a same interface implementation

makes me think that both the types MsgPayForBlobs and MsgRegisterEVMAddress don't have a proto.MessageName defined because this line should suffix it to / so I would've expected to his this when registering both of those types in just one application.

@cmwaters
Copy link
Contributor

cmwaters commented Aug 2, 2024

MsgPayForBlobs and MsgRegisterEVMAddress don't have a proto.MessageName defined

That's strange. This isn't a problem in main. These two would be registered under different names

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prototype versioned app module rolling upgrades
2 participants