Throw an error on duplicate registration (#7729)

* Panic on registering a service twice

* Panic if we register twice

* Fix test

* Fix test

* Add clearer panic message

* Add comment

* Fix test

baseapp/grpcrouter.go

@@ -54,13 +54,31 @@ func (qrt *GRPCQueryRouter) Route(path string) GRPCQueryHandler {
 }
 
 // RegisterService implements the gRPC Server.RegisterService method. sd is a gRPC
-// service description, handler is an object which implements that gRPC service
+// service description, handler is an object which implements that gRPC service/
+//
+// This functions PANICS:
+// - if a protobuf service is registered twice.
 func (qrt *GRPCQueryRouter) RegisterService(sd *grpc.ServiceDesc, handler interface{}) {
 	// adds a top-level query handler based on the gRPC service name
 	for _, method := range sd.Methods {
 		fqName := fmt.Sprintf("/%s/%s", sd.ServiceName, method.MethodName)
 		methodHandler := method.Handler
 
+		// Check that each service is only registered once. If a service is
+		// registered more than once, then we should error. Since we can't
+		// return an error (`Server.RegisterService` interface restriction) we
+		// panic (at startup).
+		_, found := qrt.routes[fqName]
+		if found {
+			panic(
+				fmt.Errorf(
+					"gRPC query service %s has already been registered. Please make sure to only register each service once. "+
+						"This usually means that there are conflicting modules registering the same gRPC query service",
+					fqName,
+				),
+			)
+		}
+
 		qrt.routes[fqName] = func(ctx sdk.Context, req abci.RequestQuery) (abci.ResponseQuery, error) {
 			// call the method handler from the service description with the handler object,
 			// a wrapped sdk.Context with proto-unmarshaled data from the ABCI request data

baseapp/grpcrouter_helpers.go

@@ -18,7 +18,7 @@ import (
 // service client.
 type QueryServiceTestHelper struct {
 	*GRPCQueryRouter
-	ctx sdk.Context
+	Ctx sdk.Context
 }
 
 var (
@@ -31,7 +31,7 @@ var (
 func NewQueryServerTestHelper(ctx sdk.Context, interfaceRegistry types.InterfaceRegistry) *QueryServiceTestHelper {
 	qrt := NewGRPCQueryRouter()
 	qrt.SetInterfaceRegistry(interfaceRegistry)
-	return &QueryServiceTestHelper{GRPCQueryRouter: qrt, ctx: ctx}
+	return &QueryServiceTestHelper{GRPCQueryRouter: qrt, Ctx: ctx}
 }
 
 // Invoke implements the grpc ClientConn.Invoke method
@@ -45,7 +45,7 @@ func (q *QueryServiceTestHelper) Invoke(_ gocontext.Context, method string, args
 		return err
 	}
 
-	res, err := querier(q.ctx, abci.RequestQuery{Data: reqBz})
+	res, err := querier(q.Ctx, abci.RequestQuery{Data: reqBz})
 	if err != nil {
 		return err
 	}

baseapp/grpcrouter_test.go

@@ -1,24 +1,29 @@
-package baseapp
+package baseapp_test
 
 import (
 	"context"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"github.com/tendermint/tendermint/libs/log"
+	dbm "github.com/tendermint/tm-db"
 
+	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/codec/types"
+	"github.com/cosmos/cosmos-sdk/simapp"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 func TestGRPCRouter(t *testing.T) {
-	qr := NewGRPCQueryRouter()
+	qr := baseapp.NewGRPCQueryRouter()
 	interfaceRegistry := testdata.NewTestInterfaceRegistry()
 	qr.SetInterfaceRegistry(interfaceRegistry)
 	testdata.RegisterQueryServer(qr, testdata.QueryImpl{})
-	helper := &QueryServiceTestHelper{
+	helper := &baseapp.QueryServiceTestHelper{
 		GRPCQueryRouter: qr,
-		ctx:             sdk.Context{}.WithContext(context.Background()),
+		Ctx:             sdk.Context{}.WithContext(context.Background()),
 	}
 	client := testdata.NewQueryClient(helper)
 
@@ -44,3 +49,28 @@ func TestGRPCRouter(t *testing.T) {
 	require.NotNil(t, res3)
 	require.Equal(t, spot, res3.HasAnimal.Animal.GetCachedValue())
 }
+
+func TestRegisterQueryServiceTwice(t *testing.T) {
+	// Setup baseapp.
+	db := dbm.NewMemDB()
+	encCfg := simapp.MakeTestEncodingConfig()
+	app := baseapp.NewBaseApp("test", log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, encCfg.TxConfig.TxDecoder())
+	app.SetInterfaceRegistry(encCfg.InterfaceRegistry)
+	testdata.RegisterInterfaces(encCfg.InterfaceRegistry)
+
+	// First time registering service shouldn't panic.
+	require.NotPanics(t, func() {
+		testdata.RegisterQueryServer(
+			app.GRPCQueryRouter(),
+			testdata.QueryImpl{},
+		)
+	})
+
+	// Second time should panic.
+	require.Panics(t, func() {
+		testdata.RegisterQueryServer(
+			app.GRPCQueryRouter(),
+			testdata.QueryImpl{},
+		)
+	})
+}

baseapp/msg_service_router.go

@@ -40,8 +40,10 @@ func (msr *MsgServiceRouter) Handler(methodName string) MsgServiceHandler {
 // RegisterService implements the gRPC Server.RegisterService method. sd is a gRPC
 // service description, handler is an object which implements that gRPC service.
 //
-// This function PANICs if it is called before the service `Msg`s have been
-// registered using RegisterInterfaces.
+// This function PANICs:
+// - if it is called before the service `Msg`s have been registered using
+//   RegisterInterfaces,
+// - or if a service is being registered twice.
 func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler interface{}) {
 	// Adds a top-level query handler based on the gRPC service name.
 	for _, method := range sd.Methods {
@@ -66,6 +68,21 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter
 			)
 		}
 
+		// Check that each service is only registered once. If a service is
+		// registered more than once, then we should error. Since we can't
+		// return an error (`Server.RegisterService` interface restriction) we
+		// panic (at startup).
+		_, found := msr.routes[fqMethod]
+		if found {
+			panic(
+				fmt.Errorf(
+					"msg service %s has already been registered. Please make sure to only register each service once. "+
+						"This usually means that there are conflicting modules registering the same msg service",
+					fqMethod,
+				),
+			)
+		}
+
 		msr.routes[fqMethod] = func(ctx sdk.Context, req sdk.MsgRequest) (*sdk.Result, error) {
 			ctx = ctx.WithEventManager(sdk.NewEventManager())
 			interceptor := func(goCtx context.Context, _ interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {

baseapp/msg_service_router_test.go

@@ -18,7 +18,7 @@ import (
 	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 )
 
-func TestRegisterService(t *testing.T) {
+func TestRegisterMsgService(t *testing.T) {
 	db := dbm.NewMemDB()
 
 	// Create an encoding config that doesn't register testdata Msg services.
@@ -42,6 +42,31 @@ func TestRegisterService(t *testing.T) {
 	})
 }
 
+func TestRegisterMsgServiceTwice(t *testing.T) {
+	// Setup baseapp.
+	db := dbm.NewMemDB()
+	encCfg := simapp.MakeTestEncodingConfig()
+	app := baseapp.NewBaseApp("test", log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, encCfg.TxConfig.TxDecoder())
+	app.SetInterfaceRegistry(encCfg.InterfaceRegistry)
+	testdata.RegisterInterfaces(encCfg.InterfaceRegistry)
+
+	// First time registering service shouldn't panic.
+	require.NotPanics(t, func() {
+		testdata.RegisterMsgServer(
+			app.MsgServiceRouter(),
+			testdata.MsgServerImpl{},
+		)
+	})
+
+	// Second time should panic.
+	require.Panics(t, func() {
+		testdata.RegisterMsgServer(
+			app.MsgServiceRouter(),
+			testdata.MsgServerImpl{},
+		)
+	})
+}
+
 func TestMsgService(t *testing.T) {
 	priv, _, _ := testdata.KeyTestPubAddr()
 	encCfg := simapp.MakeTestEncodingConfig()

client/grpc/reflection/reflection_test.go

@@ -21,14 +21,9 @@ type IntegrationTestSuite struct {
 func (s *IntegrationTestSuite) SetupSuite() {
 	app := simapp.Setup(false)
 
-	srv := reflection.NewReflectionServiceServer(app.InterfaceRegistry())
-
 	sdkCtx := app.BaseApp.NewContext(false, tmproto.Header{})
 	queryHelper := baseapp.NewQueryServerTestHelper(sdkCtx, app.InterfaceRegistry())
-
-	reflection.RegisterReflectionServiceServer(queryHelper, srv)
 	queryClient := reflection.NewReflectionServiceClient(queryHelper)
-
 	s.queryClient = queryClient
 }
 

codec/types/interface_registry.go

@@ -115,17 +115,32 @@ func (registry *interfaceRegistry) RegisterInterface(protoName string, iface int
 	registry.RegisterImplementations(iface, impls...)
 }
 
+// RegisterImplementations registers a concrete proto Message which implements
+// the given interface.
+//
+// This function PANICs if different concrete types are registered under the
+// same typeURL.
 func (registry *interfaceRegistry) RegisterImplementations(iface interface{}, impls ...proto.Message) {
 	for _, impl := range impls {
 		typeURL := "/" + proto.MessageName(impl)
 		registry.registerImpl(iface, typeURL, impl)
 	}
 }
 
+// RegisterCustomTypeURL registers a concrete type which implements the given
+// interface under `typeURL`.
+//
+// This function PANICs if different concrete types are registered under the
+// same typeURL.
 func (registry *interfaceRegistry) RegisterCustomTypeURL(iface interface{}, typeURL string, impl proto.Message) {
 	registry.registerImpl(iface, typeURL, impl)
 }
 
+// registerImpl registers a concrete type which implements the given
+// interface under `typeURL`.
+//
+// This function PANICs if different concrete types are registered under the
+// same typeURL.
 func (registry *interfaceRegistry) registerImpl(iface interface{}, typeURL string, impl proto.Message) {
 	ityp := reflect.TypeOf(iface).Elem()
 	imap, found := registry.interfaceImpls[ityp]
@@ -138,6 +153,24 @@ func (registry *interfaceRegistry) registerImpl(iface interface{}, typeURL strin
 		panic(fmt.Errorf("type %T doesn't actually implement interface %+v", impl, ityp))
 	}
 
+	// Check if we already registered something under the given typeURL. It's
+	// okay to register the same concrete type again, but if we are registering
+	// a new concrete type under the same typeURL, then we throw an error (here,
+	// we panic).
+	foundImplType, found := imap[typeURL]
+	if found && foundImplType != implType {
+		panic(
+			fmt.Errorf(
+				"concrete type %s has already been registered under typeURL %s, cannot register %s under same typeURL. "+
+					"This usually means that there are conflicting modules registering different concrete types "+
+					"for a same interface implementation",
+				foundImplType,
+				typeURL,
+				implType,
+			),
+		)
+	}
+
 	imap[typeURL] = implType
 	registry.typeURLMap[typeURL] = implType
 

codec/types/types_test.go

@@ -48,22 +48,61 @@ type TestI interface {
 	DoSomething()
 }
 
+// A struct that has the same typeURL as testdata.Dog, but is actually another
+// concrete type.
+type FakeDog struct{}
+
+var (
+	_ proto.Message   = &FakeDog{}
+	_ testdata.Animal = &FakeDog{}
+)
+
+// dummy implementation of proto.Message and testdata.Animal
+func (dog FakeDog) Reset()                  {}
+func (dog FakeDog) String() string          { return "fakedog" }
+func (dog FakeDog) ProtoMessage()           {}
+func (dog FakeDog) XXX_MessageName() string { return proto.MessageName(&testdata.Dog{}) }
+func (dog FakeDog) Greet() string           { return "fakedog" }
+
 func TestRegister(t *testing.T) {
 	registry := types.NewInterfaceRegistry()
 	registry.RegisterInterface("Animal", (*testdata.Animal)(nil))
 	registry.RegisterInterface("TestI", (*TestI)(nil))
+
+	// Happy path.
 	require.NotPanics(t, func() {
 		registry.RegisterImplementations((*testdata.Animal)(nil), &testdata.Dog{})
 	})
+
+	// testdata.Dog doesn't implement TestI
 	require.Panics(t, func() {
 		registry.RegisterImplementations((*TestI)(nil), &testdata.Dog{})
 	})
+
+	// nil proto message
 	require.Panics(t, func() {
 		registry.RegisterImplementations((*TestI)(nil), nil)
 	})
+
+	// Not an interface.
 	require.Panics(t, func() {
 		registry.RegisterInterface("not_an_interface", (*testdata.Dog)(nil))
 	})
+
+	// Duplicate registration with same concrete type.
+	require.NotPanics(t, func() {
+		registry.RegisterImplementations((*testdata.Animal)(nil), &testdata.Dog{})
+	})
+
+	// Duplicate registration with different concrete type on same typeURL.
+	require.PanicsWithError(
+		t,
+		"concrete type *testdata.Dog has already been registered under typeURL /testdata.Dog, cannot register *types_test.FakeDog under same typeURL. "+
+			"This usually means that there are conflicting modules registering different concrete types for a same interface implementation",
+		func() {
+			registry.RegisterImplementations((*testdata.Animal)(nil), &FakeDog{})
+		},
+	)
 }
 
 func TestUnpackInterfaces(t *testing.T) {