feat!: prototype for gatekeeping messages based on their version #3162

cmwaters · 2024-03-06T10:56:55Z

This PR solves the problem of ensuring the messages belonging to modules not part of the current app version aren't executed.

It does this in two ways:

Introducing an antehandler decorator to be predominantly used in CheckTx to immediately reject transactions giving users a useful error message (instead of just failing to execute)
Introduces a CircuitBreaker implementation to the MsgServiceRouter which prevents the execution of messages not belonging to the current app version. We need this because another module may call a message that is not current supported (think a governance proposal)

I had several complications with the wiring of this given the structure of the SDK and tried a few different variants - this one I think being the better.

It uses the configurator which is reponsible for registering services to read all the methods a modules grpc Server supports and extracting out the message names and mapping them to one or more versions that they are supported for.

Note: this is a prototype and is not yet complete but is open to feedback. In order to finish this I will need to write tests and more comprehensive documentation.

There is currently an unresolved problem in that prepare proposal's context does not have access to the app version

…nto cal/migration

Co-authored-by: Rootul P <[email protected]>

app/app.go

cmwaters · 2024-03-06T11:07:44Z

app/module/configurator.go

+// the server wrapper wraps the pbgrpc.Server for registering a service but
+// includes logic to extract all the sdk.Msg types that the service declares
+// in its methods and fires a callback to add them to the configurator.
+// This allows us to create a map of which messages are accepted across which
+// versions
+type serverWrapper struct {
+	addMessages func(msgs []string)
+	msgServer   pbgrpc.Server
+}
+
+func (s *serverWrapper) RegisterService(sd *grpc.ServiceDesc, v interface{}) {
+	msgs := make([]string, len(sd.Methods))
+	for idx, method := range sd.Methods {
+		// we execute the handler to extract the message type
+		_, _ = method.Handler(nil, context.Background(), func(i interface{}) error {
+			msg, ok := i.(sdk.Msg)
+			if !ok {
+				panic(fmt.Errorf("unable to register service method %s/%s: %T does not implement sdk.Msg", sd.ServiceName, method.MethodName, i))
+			}
+			msgs[idx] = sdk.MsgTypeURL(msg)
+			return nil
+		}, noopInterceptor)
+	}
+	s.addMessages(msgs)
+	// call the underlying msg server to actually register the grpc server
+	s.msgServer.RegisterService(sd, v)
+}
+
+func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) {
+	return nil, nil
+}


This is where the fun happens (I basically insert a wrapper to read off the grpc service descriptor before passing it on to the underlying service

rootulp

Seems clever! How does this handle the case where the new version of a module removes a message type? I infer the message will no longer appear on the module service description methods and so it won't get registered in the map for a particular version. Is that right?

Wait on re-read it looks like a module defines a range of versions it supports so addMessages can't selectively remove a message for a module at a particular version.

Maybe I'm asking about a scenario that isn't possible. But if it is possible (and unlikely) I think we need a plan for it.

app/ante/msg_gatekeeper.go

rootulp · 2024-03-07T02:53:35Z

app/module/configurator.go

+// one consensus version of the module to the next. Finally it maps all the messages
+// to the app versions that they are accepted in. This then gets used in the antehandler
+// to prevent users from submitting messages that can not yet be executed.


I had a concern and then realized it's not an issue but wanted to document it. I was originally concerned that we wouldn't be able to submit a crank message to try an upgrade because the v2 crank message would not be acceptable until the app version v2.

But we're using an upgrade height to upgrade from v1 -> v2. The crank message is introduced in app version v2 and will be used to upgrade from v2 -> v3 so it will pass the ante handler b/c all nodes will be on app version v2.

app/module/configurator.go

rootulp · 2024-03-07T03:12:48Z

app/module/configurator.go

+// the server wrapper wraps the pbgrpc.Server for registering a service but
+// includes logic to extract all the sdk.Msg types that the service declares
+// in its methods and fires a callback to add them to the configurator.
+// This allows us to create a map of which messages are accepted across which
+// versions
+type serverWrapper struct {
+	addMessages func(msgs []string)
+	msgServer   pbgrpc.Server
+}
+
+func (s *serverWrapper) RegisterService(sd *grpc.ServiceDesc, v interface{}) {
+	msgs := make([]string, len(sd.Methods))
+	for idx, method := range sd.Methods {
+		// we execute the handler to extract the message type
+		_, _ = method.Handler(nil, context.Background(), func(i interface{}) error {
+			msg, ok := i.(sdk.Msg)
+			if !ok {
+				panic(fmt.Errorf("unable to register service method %s/%s: %T does not implement sdk.Msg", sd.ServiceName, method.MethodName, i))
+			}
+			msgs[idx] = sdk.MsgTypeURL(msg)
+			return nil
+		}, noopInterceptor)
+	}
+	s.addMessages(msgs)
+	// call the underlying msg server to actually register the grpc server
+	s.msgServer.RegisterService(sd, v)
+}
+
+func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) {
+	return nil, nil
+}


app/module/configurator.go

cmwaters · 2024-03-07T10:39:00Z

I infer the message will no longer appear on the module service description methods and so it won't get registered in the map for a particular version. Is that right?

Yeah that's correct. Say we remove staking.MsgDelegate. It will be registered for v1 and not part of v2

evan-forbes

overall makes a lot of sense! still chewing on a few questions that I listed below

app/ante/msg_gatekeeper.go

evan-forbes · 2024-03-07T22:23:24Z

app/ante/msg_gatekeeper.go

+
+func (mgk MsgVersioningGateKeeper) IsAllowed(ctx context.Context, msgName string) (bool, error) {
+	appVersion := sdk.UnwrapSDKContext(ctx).BlockHeader().Version.App
+	acceptedMsgs, exists := mgk.acceptedMsgs[appVersion]


is this making the assumption that app version has been checked to be supported before hand? I think this is the case, and don't think its an issue, but just want to confirm

Yes that assumption is made but should be caught in the block before if we are going to upgrade to a version that the node doesn't currently support

evan-forbes · 2024-03-07T22:28:17Z

app/app.go

+		},
+		{
+			Module:      gov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper),
+			FromVersion: v1, ToVersion: v2,


I like the formatting change a lot

unrelated to this PR, does switching the ToVersion here to 0 allow for any future version to work? if so, can we do that fix the testground issues (app version == to some large number to have large block sizes)?

does switching the ToVersion here to 0 allow for any future version to work?

No everything currently must be explicit (and continuous). I think it's better if we have a different mechanism for testing the large block sizes - we could disconnect them from the appversion by simply having a mapping that is introduced in the app constructor

cmwaters · 2024-03-12T15:48:50Z

Blocked on celestiaorg/cosmos-sdk#376

coderabbitai · 2024-03-13T15:12:07Z

Walkthrough

The update enhances the application with module migration tests, message versioning gatekeeper functionality, and significant changes to the configurator.go file in the module package. These changes collectively improve version handling, migration execution, and message acceptance based on application versions.

Changes

Files	Change Summary
`app/module/migrations_test.go`	Introduces tests for module migration functionality, covering version management, migration execution, and message type registration using mock modules and configurators.
`app/ante/msg_gatekeeper.go`	Introduces a message versioning gatekeeper to control accepted transactions based on application version, with methods in `MsgVersioningGateKeeper` for message handling and version checking.
`app/module/configurator.go`	Renames `configurator` struct to `VersionedConfigurator`, adds version handling fields and methods, replaces `grpc.Server` with `pbgrpc.Server` in function signatures, and introduces methods for managing message acceptance across versions.

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-03-13T15:22:39Z

app/module/configurator.go

+func NewConfigurator(cdc codec.Codec, msgServer, queryServer pbgrpc.Server) VersionedConfigurator {
+	return VersionedConfigurator{
+		cdc:              cdc,
+		msgServer:        msgServer,
+		queryServer:      queryServer,
+		migrations:       map[string]map[uint64]module.MigrationHandler{},
+		acceptedMessages: map[uint64]map[string]struct{}{},


The introduction of VersionedConfigurator with added fields and methods for version handling and message registration is a crucial step towards improving the application's capability to manage message versions. It's recommended to ensure comprehensive testing and documentation for these new features to guarantee their effectiveness and usability.

Ensure comprehensive testing and documentation for the new version handling and message registration features introduced by VersionedConfigurator.

app/ante/msg_gatekeeper.go

app/ante/msg_gatekeeper_test.go

coderabbitai · 2024-03-13T15:22:39Z

app/ante/ante.go

+	msgVersioningGateKeeper *MsgVersioningGateKeeper,
 ) sdk.AnteHandler {
 	return sdk.ChainAnteDecorators(
 		// Wraps the panic with the string format of the transaction
 		NewHandlePanicDecorator(),
+		// Prevents messages that don't belong to the correct app version
+		// from being executed
+		msgVersioningGateKeeper,


The integration of msgVersioningGateKeeper into the NewAnteHandler function is a logical step to enforce message version compatibility at the ante handler level. It's recommended to add tests to ensure the functionality works as expected and that the msgVersioningGateKeeper is properly integrated into the ante handler chain.

Would you like assistance in creating test cases for the integration of msgVersioningGateKeeper into the NewAnteHandler function?

app/module/migrations_test.go

coderabbitai · 2024-03-13T15:22:40Z

x/upgrade/keeper.go

 	// safely be ported over without any migration
 	storeKey storetypes.StoreKey

-	// in memory copy of the upgrade height if any. This is local per node
-	// and configured from the config. Used just for V2
-	upgradeHeight int64
-
 	// quorumVersion is the version that has received a quorum of validators
 	// to signal for it. This variable is relevant just for the scope of the
 	// lifetime of the block


📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]

Removing the upgradeHeight field from the Keeper struct and adjusting related methods simplifies the keeper's structure and reflects a more flexible approach to handling upgrades. Ensure that the new upgrade logic is thoroughly tested and documented.

Ensure comprehensive testing and documentation for the new upgrade logic introduced by the changes to the Keeper struct.

app/ante/msg_gatekeeper.go

rootulp · 2024-03-13T18:46:52Z

app/ante/msg_gatekeeper_test.go

+			msg:       &banktypes.MsgSend{},
+			acceptMsg: false,
+			version:   2,
+		},


[optional] do we want a third test case for an app version that isn't supported (i.e. 3) to confirm that the ante handler returns ErrNotSupported

rootulp · 2024-03-13T18:50:57Z

app/app.go

+		return err
+	}
+	if toVersion == v2 {
+		// we need to set the app version in the param store for the first time


[not blocking] is there a diagram of all the places app version is persisted and plumbed through? I don't have a clear picture of where/why there were so many app version related issues in cosmos-sdk / celestia-app.

The crux was that since we pass the sdk context everywhere, we should use that to get the app version. The app version should technically be present in the context because it includes the header that has the app version. The problem however was that in most cases when we create the context we only populate parts of the header (the height for example). So I needed to make several changes in PrepareProposal, ProcessProposal, InitGenesis etc. that we actually populated the app version

app/module/configurator.go

app/module/migrations_test.go

rootulp · 2024-03-13T19:56:41Z

app/module/module.go

+		if module.FromVersion == 0 {
+			return nil, sdkerrors.ErrInvalidVersion.Wrapf("v0 is not a valid version for module %s", module.Module.Name())
 		}


[question] 0 doesn't seem like a valid ToVersion either. Should that be a separate check?

I have the check below that asserts that FromVersion cannot be greater than ToVersion which would catch this scenario

Co-authored-by: Rootul P <[email protected]>

coderabbitai · 2024-03-14T09:52:32Z

app/module/configurator.go

+// VersionedConfigurator is a struct used at startup to register all the message and
+// query servers for all modules. It allows the module to register any migrations from
+// one consensus version of the module to the next. Finally it maps all the messages
+// to the app versions that they are accepted in. This then gets used in the antehandler
+// to prevent users from submitting messages that can not yet be executed.
+type VersionedConfigurator struct {
+	fromVersion, toVersion uint64
+	cdc                    codec.Codec
+	msgServer              pbgrpc.Server
+	queryServer            pbgrpc.Server
+	// acceptedMsgs is a map from appVersion -> msgTypeURL -> struct{}. 
+	acceptedMessages       map[uint64]map[string]struct{}


The introduction of VersionedConfigurator with added fields for version handling and message registration is a significant enhancement. It's crucial to ensure that these new features are well-documented and tested to guarantee their effectiveness and usability.

Ensure comprehensive testing and documentation for the new version handling and message registration features introduced by VersionedConfigurator.

Co-authored-by: Rootul P <[email protected]>

rootulp

Still LGTM

…estiaorg#3162) Ref: celestiaorg#3134 This PR solves the problem of ensuring the messages belonging to modules not part of the current app version aren't executed. It does this in two ways: - Introducing an antehandler decorator to be predominantly used in CheckTx to immediately reject transactions giving users a useful error message (instead of just failing to execute) - Introduces a `CircuitBreaker` implementation to the `MsgServiceRouter` which prevents the execution of messages not belonging to the current app version. We need this because another module may call a message that is not current supported (think a governance proposal) I had several complications with the wiring of this given the structure of the SDK and tried a few different variants - this one I think being the better. It uses the configurator which is reponsible for registering services to read all the methods a modules grpc Server supports and extracting out the message names and mapping them to one or more versions that they are supported for. --------- Co-authored-by: Rootul P <[email protected]>

cmwaters and others added 16 commits February 15, 2024 13:31

create a versioned module manager

50dabdb

fix some tests

2633565

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

Merge branch 'main' into cal/migration

a5a5f62

Apply suggestions from code review

af73b30

Co-authored-by: Rootul P <[email protected]>

Merge branch 'main' into cal/migration

0c4410c

add more comments

c130787

Merge branch 'main' into cal/migration

c97ba6e

prototype for gatekeeping messages based on their version

aef8b75

cmwaters changed the title ~~feat!; prototype for gatekeeping messages based on their version~~ feat!: prototype for gatekeeping messages based on their version Mar 6, 2024

Merge branch 'main' into cal/tx-version-antehandler

8575461

cmwaters commented Mar 6, 2024

View reviewed changes

rootulp assigned cmwaters Mar 6, 2024

rootulp reviewed Mar 7, 2024

View reviewed changes

evan-forbes reviewed Mar 7, 2024

View reviewed changes

evan-forbes mentioned this pull request Mar 11, 2024

Make the encoding require the app version #3134

Closed

cmwaters added 2 commits March 12, 2024 11:27

Merge branch 'main' into cal/tx-version-antehandler

2f89e3a

add tests for migration

018902d

cmwaters added 3 commits March 13, 2024 11:19

update sdk import

c688cc3

Merge branch 'main' into cal/tx-version-antehandler

17338b7

fix tests and upgrade logic

7b182a5

cmwaters added 2 commits March 13, 2024 14:23

add comments

9fb86c1

fix test

55ba050

cmwaters marked this pull request as ready for review March 13, 2024 15:11

cmwaters requested review from staheri14 and ninabarbakadze as code owners March 13, 2024 15:11

cmwaters requested review from rootulp and evan-forbes March 13, 2024 15:11

coderabbitai bot reviewed Mar 13, 2024

View reviewed changes

update cosmos sdk

4ab4092

celestia-bot requested a review from a team March 13, 2024 16:03

rootulp previously approved these changes Mar 13, 2024

View reviewed changes

Apply suggestions from code review

aa584ef

Co-authored-by: Rootul P <[email protected]>

cmwaters dismissed rootulp’s stale review via aa584ef March 14, 2024 09:45

celestia-bot requested a review from a team March 14, 2024 09:45

coderabbitai bot reviewed Mar 14, 2024

View reviewed changes

cmwaters and others added 2 commits March 14, 2024 11:41

Update app/module/migrations_test.go

20458b2

Co-authored-by: Rootul P <[email protected]>

go fmt

3f7b613

evan-forbes approved these changes Mar 14, 2024

View reviewed changes

rootulp approved these changes Mar 14, 2024

View reviewed changes

cmwaters merged commit 91603c4 into main Mar 14, 2024
33 checks passed

cmwaters deleted the cal/tx-version-antehandler branch March 14, 2024 17:21

rootulp mentioned this pull request Mar 14, 2024

fix: throw an error if incorrect ledger is used #3177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: prototype for gatekeeping messages based on their version #3162

feat!: prototype for gatekeeping messages based on their version #3162

cmwaters commented Mar 6, 2024

cmwaters Mar 6, 2024

rootulp Mar 7, 2024

rootulp left a comment

rootulp Mar 7, 2024

rootulp Mar 7, 2024

cmwaters commented Mar 7, 2024

evan-forbes left a comment

evan-forbes Mar 7, 2024

cmwaters Mar 9, 2024

evan-forbes Mar 7, 2024

cmwaters Mar 9, 2024

cmwaters commented Mar 12, 2024

coderabbitai bot commented Mar 13, 2024 •

edited

Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

coderabbitai bot Mar 13, 2024

coderabbitai bot Mar 13, 2024

coderabbitai bot Mar 13, 2024

rootulp Mar 13, 2024

rootulp Mar 13, 2024

cmwaters Mar 14, 2024

rootulp Mar 13, 2024

cmwaters Mar 14, 2024

coderabbitai bot Mar 14, 2024

rootulp left a comment

feat!: prototype for gatekeeping messages based on their version #3162

feat!: prototype for gatekeeping messages based on their version #3162

Conversation

cmwaters commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

cmwaters commented Mar 7, 2024

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

cmwaters commented Mar 12, 2024

coderabbitai bot commented Mar 13, 2024 • edited Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

coderabbitai bot Mar 13, 2024

Choose a reason for hiding this comment

coderabbitai bot Mar 13, 2024

Choose a reason for hiding this comment

coderabbitai bot Mar 13, 2024

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 Mar 14, 2024

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Mar 13, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)