Added checks to avoid decoding messages as handshakes (#2681)

Sometimes, we end up decoding a message as a handshake and IsHandshake is not useful after decoding has happened. IsHandshake in Handshake implementations has been improved to check if the value of `Roles` is valid. If we decode bytes of handshake to handshake Roles would have a valid value (1,2, 4) otherwise it would have some random value.
ChainSafe · Aug 8, 2022 · b21a8a1 · b21a8a1
1 parent c5dda51
commit b21a8a1
Show file tree

Hide file tree

Showing 29 changed files with 103 additions and 84 deletions.
diff --git a/chain/dev/defaults.go b/chain/dev/defaults.go
@@ -5,6 +5,7 @@ package dev
 
 import (
 	"github.com/ChainSafe/gossamer/internal/log"
+	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/genesis"
 	"github.com/ChainSafe/gossamer/lib/runtime/wasmer"
 )
@@ -52,7 +53,7 @@ var (
 	// DefaultAuthority is true if the node is a block producer and a grandpa authority
 	DefaultAuthority = true
 	// DefaultRoles Default node roles
-	DefaultRoles = byte(4) // authority node (see Table D.2)
+	DefaultRoles = common.AuthorityRole // authority node (see Table D.2)
 	// DefaultBabeAuthority is true if the node is a block producer (overwrites previous settings)
 	DefaultBabeAuthority = true
 	// DefaultGrandpaAuthority is true if the node is a grandpa authority (overwrites previous settings)

diff --git a/chain/gssmr/defaults.go b/chain/gssmr/defaults.go
@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/ChainSafe/gossamer/internal/log"
+	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/genesis"
 	"github.com/ChainSafe/gossamer/lib/runtime/wasmer"
 )
@@ -54,7 +55,7 @@ var (
 	// DefaultAuthority is true if the node is a block producer and a grandpa authority
 	DefaultAuthority = true
 	// DefaultRoles Default node roles
-	DefaultRoles = byte(4) // authority node (see Table D.2)
+	DefaultRoles = common.AuthorityRole // authority node (see Table D.2)
 	// DefaultBabeAuthority is true if the node is a block producer (overwrites previous settings)
 	DefaultBabeAuthority = true
 	// DefaultGrandpaAuthority is true if the node is a grandpa authority (overwrites previous settings)

diff --git a/chain/kusama/defaults.go b/chain/kusama/defaults.go
@@ -5,6 +5,7 @@ package kusama
 
 import (
 	"github.com/ChainSafe/gossamer/internal/log"
+	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/genesis"
 	"github.com/ChainSafe/gossamer/lib/runtime/wasmer"
 )
@@ -52,7 +53,7 @@ var (
 	// DefaultAuthority true if BABE block producer
 	DefaultAuthority = false
 	// DefaultRoles Default node roles
-	DefaultRoles = byte(1) // full node (see Table D.2)
+	DefaultRoles = common.FullNodeRole // full node (see Table D.2)
 	// DefaultWasmInterpreter is the name of the wasm interpreter to use by default
 	DefaultWasmInterpreter = wasmer.Name
 

diff --git a/chain/polkadot/defaults.go b/chain/polkadot/defaults.go
@@ -5,6 +5,7 @@ package polkadot
 
 import (
 	"github.com/ChainSafe/gossamer/internal/log"
+	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/genesis"
 	"github.com/ChainSafe/gossamer/lib/runtime/wasmer"
 )
@@ -49,7 +50,7 @@ var (
 	// DefaultAuthority is true if the node is a block producer and a grandpa authority
 	DefaultAuthority = true
 	// DefaultRoles Default node roles
-	DefaultRoles = byte(1) // authority node (see Table D.2)
+	DefaultRoles = common.FullNodeRole // authority node (see Table D.2)
 	// DefaultBabeAuthority is true if the node is a block producer (overwrites previous settings)
 	DefaultBabeAuthority = true
 	// DefaultGrandpaAuthority is true if the node is a grandpa authority (overwrites previous settings)

diff --git a/cmd/gossamer/config.go b/cmd/gossamer/config.go
@@ -16,7 +16,6 @@ import (
 	ctoml "github.com/ChainSafe/gossamer/dot/config/toml"
 	"github.com/ChainSafe/gossamer/dot/state"
 	"github.com/ChainSafe/gossamer/dot/state/pruner"
-	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/internal/log"
 	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/genesis"
@@ -572,9 +571,9 @@ func setDotAccountConfig(ctx *cli.Context, tomlCfg ctoml.AccountConfig, cfg *dot
 
 // setDotCoreConfig sets dot.CoreConfig using flag values from the cli context
 func setDotCoreConfig(ctx *cli.Context, tomlCfg ctoml.CoreConfig, cfg *dot.CoreConfig) {
-	cfg.Roles = tomlCfg.Roles
-	cfg.BabeAuthority = tomlCfg.Roles == types.AuthorityRole
-	cfg.GrandpaAuthority = tomlCfg.Roles == types.AuthorityRole
+	cfg.Roles = common.Roles(tomlCfg.Roles)
+	cfg.BabeAuthority = common.Roles(tomlCfg.Roles) == common.AuthorityRole
+	cfg.GrandpaAuthority = common.Roles(tomlCfg.Roles) == common.AuthorityRole
 	cfg.GrandpaInterval = time.Second * time.Duration(tomlCfg.GrandpaInterval)
 
 	cfg.BABELead = tomlCfg.BABELead
@@ -586,14 +585,14 @@ func setDotCoreConfig(ctx *cli.Context, tomlCfg ctoml.CoreConfig, cfg *dot.CoreC
 	if roles := ctx.GlobalString(RolesFlag.Name); roles != "" {
 		// convert string to byte
 		n, err := strconv.Atoi(roles)
-		b := byte(n)
+		b := common.Roles(n)
 		if err != nil {
 			logger.Errorf("failed to convert Roles to byte: %s", err)
-		} else if b == types.AuthorityRole {
+		} else if b == common.AuthorityRole {
 			// if roles byte is 4, act as an authority (see Table D.2)
 			logger.Debug("authority enabled (roles=4)")
 			cfg.Roles = b
-		} else if b > types.AuthorityRole {
+		} else if b > common.AuthorityRole {
 			// if roles byte is greater than 4, invalid roles byte (see Table D.2)
 			logger.Errorf("invalid roles option provided, authority disabled (roles=%d)", b)
 		} else {
@@ -605,15 +604,15 @@ func setDotCoreConfig(ctx *cli.Context, tomlCfg ctoml.CoreConfig, cfg *dot.CoreC
 
 	// to turn on BABE but not grandpa, cfg.Roles must be set to 4
 	// and cfg.GrandpaAuthority must be set to false
-	if cfg.Roles == types.AuthorityRole && !tomlCfg.BabeAuthority {
+	if cfg.Roles == common.AuthorityRole && !tomlCfg.BabeAuthority {
 		cfg.BabeAuthority = false
 	}
 
-	if cfg.Roles == types.AuthorityRole && !tomlCfg.GrandpaAuthority {
+	if cfg.Roles == common.AuthorityRole && !tomlCfg.GrandpaAuthority {
 		cfg.GrandpaAuthority = false
 	}
 
-	if cfg.Roles != types.AuthorityRole {
+	if cfg.Roles != common.AuthorityRole {
 		cfg.BabeAuthority = false
 		cfg.GrandpaAuthority = false
 	}
@@ -805,9 +804,9 @@ func setSystemInfoConfig(ctx *cli.Context, cfg *dot.Config) {
 func updateDotConfigFromGenesisJSONRaw(tomlCfg ctoml.Config, cfg *dot.Config) {
 	cfg.Account.Key = tomlCfg.Account.Key
 	cfg.Account.Unlock = tomlCfg.Account.Unlock
-	cfg.Core.Roles = tomlCfg.Core.Roles
-	cfg.Core.BabeAuthority = tomlCfg.Core.Roles == types.AuthorityRole
-	cfg.Core.GrandpaAuthority = tomlCfg.Core.Roles == types.AuthorityRole
+	cfg.Core.Roles = common.Roles(tomlCfg.Core.Roles)
+	cfg.Core.BabeAuthority = common.Roles(tomlCfg.Core.Roles) == common.AuthorityRole
+	cfg.Core.GrandpaAuthority = common.Roles(tomlCfg.Core.Roles) == common.AuthorityRole
 
 	// use default genesis file if genesis configuration not provided, for example,
 	// if we load a toml configuration file without a defined genesis init value or

diff --git a/cmd/gossamer/config_test.go b/cmd/gossamer/config_test.go
@@ -21,6 +21,7 @@ import (
 	"github.com/ChainSafe/gossamer/dot/state"
 	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/internal/log"
+	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/genesis"
 	"github.com/ChainSafe/gossamer/lib/runtime"
 	"github.com/ChainSafe/gossamer/lib/utils"
@@ -974,7 +975,7 @@ func TestGlobalNodeName_WhenNodeAlreadyHasStoredName(t *testing.T) {
 	err = os.WriteFile(genPath, genData, os.ModePerm)
 	require.NoError(t, err)
 
-	cfg.Core.Roles = types.FullNodeRole
+	cfg.Core.Roles = common.FullNodeRole
 	cfg.Core.BabeAuthority = false
 	cfg.Core.GrandpaAuthority = false
 	cfg.Init.Genesis = genPath

diff --git a/cmd/gossamer/export.go b/cmd/gossamer/export.go
@@ -101,7 +101,7 @@ func dotConfigToToml(dcfg *dot.Config) *ctoml.Config {
 	}
 
 	cfg.Core = ctoml.CoreConfig{
-		Roles:            dcfg.Core.Roles,
+		Roles:            byte(dcfg.Core.Roles),
 		BabeAuthority:    dcfg.Core.BabeAuthority,
 		GrandpaAuthority: dcfg.Core.GrandpaAuthority,
 		GrandpaInterval:  uint32(dcfg.Core.GrandpaInterval / time.Second),

diff --git a/dot/config.go b/dot/config.go
@@ -16,6 +16,7 @@ import (
 	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/internal/log"
 	"github.com/ChainSafe/gossamer/internal/pprof"
+	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/genesis"
 )
 
@@ -106,7 +107,7 @@ type NetworkConfig struct {
 
 // CoreConfig is to marshal/unmarshal toml core config vars
 type CoreConfig struct {
-	Roles            byte
+	Roles            common.Roles
 	BabeAuthority    bool
 	BABELead         bool
 	GrandpaAuthority bool
@@ -163,7 +164,7 @@ type StateConfig struct {
 
 // networkServiceEnabled returns true if the network service is enabled
 func networkServiceEnabled(cfg *Config) bool {
-	return cfg.Core.Roles != byte(0)
+	return cfg.Core.Roles != common.NoNetworkRole
 }
 
 // PprofConfig is the configuration for the pprof HTTP server.

diff --git a/dot/config_test.go b/dot/config_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/ChainSafe/gossamer/internal/log"
 	"github.com/ChainSafe/gossamer/internal/pprof"
+	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -50,7 +51,7 @@ func TestConfig(t *testing.T) {
 					Key: "alice",
 				},
 				Core: CoreConfig{
-					Roles:            byte(4),
+					Roles:            common.AuthorityRole,
 					BabeAuthority:    true,
 					BABELead:         true,
 					GrandpaAuthority: true,
@@ -109,7 +110,7 @@ func TestConfig(t *testing.T) {
 				},
 				Account: AccountConfig{},
 				Core: CoreConfig{
-					Roles:            byte(4),
+					Roles:            common.AuthorityRole,
 					BabeAuthority:    true,
 					GrandpaAuthority: true,
 					WasmInterpreter:  "wasmer",
@@ -170,7 +171,7 @@ func TestConfig(t *testing.T) {
 				},
 				Account: AccountConfig{},
 				Core: CoreConfig{
-					Roles:           byte(1),
+					Roles:           common.FullNodeRole,
 					WasmInterpreter: "wasmer",
 					GrandpaInterval: 0,
 				},
@@ -227,7 +228,7 @@ func TestConfig(t *testing.T) {
 				},
 				Init: InitConfig{Genesis: "./chain/polkadot/genesis.json"},
 				Core: CoreConfig{
-					Roles:           byte(1),
+					Roles:           common.FullNodeRole,
 					WasmInterpreter: "wasmer",
 				},
 				Network: NetworkConfig{

diff --git a/dot/network/block_announce.go b/dot/network/block_announce.go
@@ -15,6 +15,7 @@ import (
 	"github.com/libp2p/go-libp2p-core/peer"
 )
 
+var errInvalidRole = errors.New("invalid role")
 var (
 	_ NotificationsMessage = &BlockAnnounceMessage{}
 	_ NotificationsMessage = &BlockAnnounceHandshake{}
@@ -103,7 +104,7 @@ func decodeBlockAnnounceMessage(in []byte) (NotificationsMessage, error) {
 
 // BlockAnnounceHandshake is exchanged by nodes that are beginning the BlockAnnounce protocol
 type BlockAnnounceHandshake struct {
-	Roles           byte
+	Roles           common.Roles
 	BestBlockNumber uint32
 	BestBlockHash   common.Hash
 	GenesisHash     common.Hash
@@ -173,6 +174,12 @@ func (s *Service) validateBlockAnnounceHandshake(from peer.ID, hs Handshake) err
 		return errors.New("invalid handshake type")
 	}
 
+	switch bhs.Roles {
+	case common.FullNodeRole, common.LightClientRole, common.AuthorityRole:
+	default:
+		return fmt.Errorf("%w: %d", errInvalidRole, bhs.Roles)
+	}
+
 	if !bhs.GenesisHash.Equal(s.blockState.GenesisHash()) {
 		s.host.cm.peerSetHandler.ReportPeer(peerset.ReputationChange{
 			Value:  peerset.GenesisMismatch,

diff --git a/dot/network/block_announce_test.go b/dot/network/block_announce_test.go
@@ -165,6 +165,7 @@ func TestValidateBlockAnnounceHandshake(t *testing.T) {
 	nodeA.notificationsProtocols[BlockAnnounceMsgType].peersData.setInboundHandshakeData(testPeerID, &handshakeData{})
 
 	err := nodeA.validateBlockAnnounceHandshake(testPeerID, &BlockAnnounceHandshake{
+		Roles:           common.FullNodeRole,
 		BestBlockNumber: 100,
 		GenesisHash:     nodeA.blockState.GenesisHash(),
 	})

diff --git a/dot/network/config.go b/dot/network/config.go
@@ -13,6 +13,7 @@ import (
 	"github.com/ChainSafe/gossamer/dot/telemetry"
 	"github.com/ChainSafe/gossamer/internal/log"
 	"github.com/ChainSafe/gossamer/internal/metrics"
+	"github.com/ChainSafe/gossamer/lib/common"
 )
 
 const (
@@ -32,7 +33,7 @@ const (
 	DefaultProtocolID = "/gossamer/gssmr/0"
 
 	// DefaultRoles the default value for Config.Roles (0 = no network, 1 = full node)
-	DefaultRoles = byte(1)
+	DefaultRoles = common.FullNodeRole
 
 	// DefaultMinPeerCount is the default minimum peer count
 	DefaultMinPeerCount = 5
@@ -58,7 +59,7 @@ type Config struct {
 	// BasePath the data directory for the node
 	BasePath string
 	// Roles a bitmap value that represents the different roles for the sender node (see Table D.2)
-	Roles byte
+	Roles common.Roles
 
 	// Service interfaces
 	BlockState         BlockState

diff --git a/dot/network/host_test.go b/dot/network/host_test.go
@@ -330,12 +330,11 @@ func TestStreamCloseMetadataCleanup(t *testing.T) {
 	require.NoError(t, err)
 
 	const (
-		roles           byte   = 4
 		bestBlockNumber uint32 = 77
 	)
 
 	testHandshake := &BlockAnnounceHandshake{
-		Roles:           roles,
+		Roles:           common.AuthorityRole,
 		BestBlockNumber: bestBlockNumber,
 		BestBlockHash:   common.Hash{1},
 		GenesisHash:     nodeB.blockState.GenesisHash(),

diff --git a/dot/network/notifications.go b/dot/network/notifications.go
@@ -154,6 +154,8 @@ func (s *Service) createNotificationsMessageHandler(
 
 			hs, ok := msg.(Handshake)
 			if !ok {
+				// NOTE: As long as, Handshake interface and NotificationMessage interfaces are same,
+				// this error would never happen.
 				return errMessageIsNotHandshake
 			}
 

diff --git a/dot/network/service.go b/dot/network/service.go
@@ -616,7 +616,7 @@ func (s *Service) RemoveReservedPeers(addrs ...string) error {
 }
 
 // NodeRoles Returns the roles the node is running as.
-func (s *Service) NodeRoles() byte {
+func (s *Service) NodeRoles() common.Roles {
 	return s.cfg.Roles
 }
 

diff --git a/dot/node_integration_test.go b/dot/node_integration_test.go
@@ -15,7 +15,6 @@ import (
 
 	"github.com/ChainSafe/gossamer/dot/core"
 	"github.com/ChainSafe/gossamer/dot/state"
-	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/lib/babe"
 	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/crypto/sr25519"
@@ -92,7 +91,7 @@ func TestNewNodeIntegration(t *testing.T) {
 	err = keystore.LoadKeystore("alice", ks.Babe)
 	require.NoError(t, err)
 
-	cfg.Core.Roles = types.FullNodeRole
+	cfg.Core.Roles = common.FullNodeRole
 
 	node, err := NewNode(cfg, ks)
 	require.NoError(t, err)
@@ -121,7 +120,7 @@ func TestNewNode_Authority(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, 1, ks.Babe.Size())
 
-	cfg.Core.Roles = types.AuthorityRole
+	cfg.Core.Roles = common.AuthorityRole
 
 	node, err := NewNode(cfg, ks)
 	require.NoError(t, err)
@@ -150,7 +149,7 @@ func TestStartStopNode(t *testing.T) {
 	err = keystore.LoadKeystore("alice", ks.Babe)
 	require.NoError(t, err)
 
-	cfg.Core.Roles = types.FullNodeRole
+	cfg.Core.Roles = common.FullNodeRole
 
 	node, err := NewNode(cfg, ks)
 	require.NoError(t, err)
@@ -168,7 +167,7 @@ func TestInitNode_LoadStorageRoot(t *testing.T) {
 
 	genPath := newTestGenesisAndRuntime(t)
 
-	cfg.Core.Roles = types.FullNodeRole
+	cfg.Core.Roles = common.FullNodeRole
 	cfg.Core.BabeAuthority = false
 	cfg.Core.GrandpaAuthority = false
 	cfg.Init.Genesis = genPath
@@ -217,7 +216,7 @@ func TestInitNode_LoadBalances(t *testing.T) {
 
 	genPath := newTestGenesisAndRuntime(t)
 
-	cfg.Core.Roles = types.FullNodeRole
+	cfg.Core.Roles = common.FullNodeRole
 	cfg.Core.BabeAuthority = false
 	cfg.Core.GrandpaAuthority = false
 	cfg.Init.Genesis = genPath
@@ -256,7 +255,7 @@ func TestNode_PersistGlobalName_WhenInitialize(t *testing.T) {
 	cfg := NewTestConfig(t)
 	cfg.Global.Name = globalName
 
-	cfg.Core.Roles = types.FullNodeRole
+	cfg.Core.Roles = common.FullNodeRole
 	cfg.Core.BabeAuthority = false
 	cfg.Core.GrandpaAuthority = false
 	cfg.Init.Genesis = newTestGenesisAndRuntime(t)