ADR-016: Add protocol Version to NodeInfo (#2654)

* p2p: add protocol Version to NodeInfo

* update node pkg. remove extraneous version files

* update changelog and docs

* fix test

* p2p: Version -> ProtocolVersion; more ValidateBasic and tests

CHANGELOG_PENDING.md

@@ -14,6 +14,8 @@ BREAKING CHANGES:
   * [privval] \#2459 Split `SocketPVMsg`s implementations into Request and Response, where the Response may contain a error message (returned by the remote signer)
   * [state] \#2644 Add Version field to State, breaking the format of State as
     encoded on disk.
+  * [rpc] \#2654 Remove all `node_info.other.*_version` fields in `/status` and
+    `/net_info`
 
 * Apps
   * [abci] \#2298 ResponseQuery.Proof is now a structured merkle.Proof, not just
@@ -43,6 +45,9 @@ BREAKING CHANGES:
   * [state] \#2644 Require block.Version to match state.Version
 
 * P2P Protocol
+  * [p2p] \#2654 Add `ProtocolVersion` struct with protocol versions to top of
+    DefaultNodeInfo and require `ProtocolVersion.Block` to match during peer handshake
+
 
 FEATURES:
 - [crypto/merkle] \#2298 General Merkle Proof scheme for chaining various types of Merkle trees together

benchmarks/codec_test.go

@@ -14,14 +14,15 @@ import (
 
 func testNodeInfo(id p2p.ID) p2p.DefaultNodeInfo {
 	return p2p.DefaultNodeInfo{
-		ID_:        id,
-		Moniker:    "SOMENAME",
-		Network:    "SOMENAME",
-		ListenAddr: "SOMEADDR",
-		Version:    "SOMEVER",
+		ProtocolVersion: p2p.InitProtocolVersion,
+		ID_:             id,
+		Moniker:         "SOMENAME",
+		Network:         "SOMENAME",
+		ListenAddr:      "SOMEADDR",
+		Version:         "SOMEVER",
 		Other: p2p.DefaultNodeInfoOther{
-			AminoVersion: "SOMESTRING",
-			P2PVersion:   "OTHERSTRING",
+			TxIndex:    "on",
+			RPCAddress: "0.0.0.0:26657",
 		},
 	}
 }

consensus/version.go

@@ -1,11 +0,0 @@
-package consensus
-
-import "fmt"
-
-// kind of arbitrary
-var Spec = "1"     // async
-var Major = "0"    //
-var Minor = "2"    // replay refactor
-var Revision = "2" // validation -> commit
-
-var Version = fmt.Sprintf("v%s/%s.%s.%s", Spec, Major, Minor, Revision)

docs/spec/p2p/peer.md

@@ -75,22 +75,25 @@ The Tendermint Version Handshake allows the peers to exchange their NodeInfo:
 
 ```golang
 type NodeInfo struct {
+  Version    p2p.Version
   ID         p2p.ID
   ListenAddr string
 
   Network    string
-  Version    string
+  SoftwareVersion    string
   Channels   []int8
 
   Moniker    string
   Other      NodeInfoOther
 }
 
+type Version struct {
+	P2P uint64
+	Block uint64
+	App uint64
+}
+
 type NodeInfoOther struct {
-	AminoVersion     string
-	P2PVersion       string
-	ConsensusVersion string
-	RPCVersion       string
 	TxIndex          string
 	RPCAddress       string
 }
@@ -99,8 +102,7 @@ type NodeInfoOther struct {
 The connection is disconnected if:
 
 - `peer.NodeInfo.ID` is not equal `peerConn.ID`
-- `peer.NodeInfo.Version` is not formatted as `X.X.X` where X are integers known as Major, Minor, and Revision
-- `peer.NodeInfo.Version` Major is not the same as ours
+- `peer.NodeInfo.Version.Block` does not match ours
 - `peer.NodeInfo.Network` is not the same as ours
 - `peer.Channels` does not intersect with our known Channels.
 - `peer.NodeInfo.ListenAddr` is malformed or is a DNS host that cannot be

node/node.go

@@ -32,7 +32,6 @@ import (
 	rpccore "github.com/tendermint/tendermint/rpc/core"
 	ctypes "github.com/tendermint/tendermint/rpc/core/types"
 	grpccore "github.com/tendermint/tendermint/rpc/grpc"
-	"github.com/tendermint/tendermint/rpc/lib"
 	"github.com/tendermint/tendermint/rpc/lib/server"
 	sm "github.com/tendermint/tendermint/state"
 	"github.com/tendermint/tendermint/state/txindex"
@@ -771,9 +770,10 @@ func makeNodeInfo(
 		txIndexerStatus = "off"
 	}
 	nodeInfo := p2p.DefaultNodeInfo{
-		ID_:     nodeID,
-		Network: chainID,
-		Version: version.Version,
+		ProtocolVersion: p2p.InitProtocolVersion,
+		ID_:             nodeID,
+		Network:         chainID,
+		Version:         version.TMCoreSemVer,
 		Channels: []byte{
 			bc.BlockchainChannel,
 			cs.StateChannel, cs.DataChannel, cs.VoteChannel, cs.VoteSetBitsChannel,
@@ -782,12 +782,8 @@ func makeNodeInfo(
 		},
 		Moniker: config.Moniker,
 		Other: p2p.DefaultNodeInfoOther{
-			AminoVersion:     amino.Version,
-			P2PVersion:       p2p.Version,
-			ConsensusVersion: cs.Version,
-			RPCVersion:       fmt.Sprintf("%v/%v", rpc.Version, rpccore.Version),
-			TxIndex:          txIndexerStatus,
-			RPCAddress:       config.RPC.ListenAddress,
+			TxIndex:    txIndexerStatus,
+			RPCAddress: config.RPC.ListenAddress,
 		},
 	}
 

p2p/node_info.go

@@ -3,9 +3,9 @@ package p2p
 import (
 	"fmt"
 	"reflect"
-	"strings"
 
 	cmn "github.com/tendermint/tendermint/libs/common"
+	"github.com/tendermint/tendermint/version"
 )
 
 const (
@@ -18,8 +18,10 @@ func MaxNodeInfoSize() int {
 	return maxNodeInfoSize
 }
 
+//-------------------------------------------------------------
+
 // NodeInfo exposes basic info of a node
-// and determines if we're compatible
+// and determines if we're compatible.
 type NodeInfo interface {
 	nodeInfoAddress
 	nodeInfoTransport
@@ -31,16 +33,38 @@ type nodeInfoAddress interface {
 	NetAddress() *NetAddress
 }
 
-// nodeInfoTransport is validates a nodeInfo and checks
+// nodeInfoTransport validates a nodeInfo and checks
 // our compatibility with it. It's for use in the handshake.
 type nodeInfoTransport interface {
 	ValidateBasic() error
 	CompatibleWith(other NodeInfo) error
 }
 
+//-------------------------------------------------------------
+
+// ProtocolVersion contains the protocol versions for the software.
+type ProtocolVersion struct {
+	P2P   version.Protocol `json:"p2p"`
+	Block version.Protocol `json:"block"`
+	App   version.Protocol `json:"app"`
+}
+
+var InitProtocolVersion = ProtocolVersion{
+	P2P:   version.P2PProtocol,
+	Block: version.BlockProtocol,
+	App:   0,
+}
+
+//-------------------------------------------------------------
+
+// Assert DefaultNodeInfo satisfies NodeInfo
+var _ NodeInfo = DefaultNodeInfo{}
+
 // DefaultNodeInfo is the basic node information exchanged
 // between two peers during the Tendermint P2P handshake.
 type DefaultNodeInfo struct {
+	ProtocolVersion ProtocolVersion `json:"protocol_version"`
+
 	// Authenticate
 	// TODO: replace with NetAddress
 	ID_        ID     `json:"id"`          // authenticated identifier
@@ -59,12 +83,8 @@ type DefaultNodeInfo struct {
 
 // DefaultNodeInfoOther is the misc. applcation specific data
 type DefaultNodeInfoOther struct {
-	AminoVersion     string `json:"amino_version"`
-	P2PVersion       string `json:"p2p_version"`
-	ConsensusVersion string `json:"consensus_version"`
-	RPCVersion       string `json:"rpc_version"`
-	TxIndex          string `json:"tx_index"`
-	RPCAddress       string `json:"rpc_address"`
+	TxIndex    string `json:"tx_index"`
+	RPCAddress string `json:"rpc_address"`
 }
 
 // ID returns the node's peer ID.
@@ -86,35 +106,28 @@ func (info DefaultNodeInfo) ID() ID {
 // url-encoding), and we just need to be careful with how we handle that in our
 // clients. (e.g. off by default).
 func (info DefaultNodeInfo) ValidateBasic() error {
-	if len(info.Channels) > maxNumChannels {
-		return fmt.Errorf("info.Channels is too long (%v). Max is %v", len(info.Channels), maxNumChannels)
-	}
 
-	// Sanitize ASCII text fields.
-	if !cmn.IsASCIIText(info.Moniker) || cmn.ASCIITrim(info.Moniker) == "" {
-		return fmt.Errorf("info.Moniker must be valid non-empty ASCII text without tabs, but got %v", info.Moniker)
-	}
+	// ID is already validated.
 
-	// Sanitize versions
-	// XXX: Should we be more strict about version and address formats?
-	other := info.Other
-	versions := []string{
-		other.AminoVersion,
-		other.P2PVersion,
-		other.ConsensusVersion,
-		other.RPCVersion}
-	for i, v := range versions {
-		if cmn.ASCIITrim(v) != "" && !cmn.IsASCIIText(v) {
-			return fmt.Errorf("info.Other[%d]=%v must be valid non-empty ASCII text without tabs", i, v)
-		}
-	}
-	if cmn.ASCIITrim(other.TxIndex) != "" && (other.TxIndex != "on" && other.TxIndex != "off") {
-		return fmt.Errorf("info.Other.TxIndex should be either 'on' or 'off', got '%v'", other.TxIndex)
+	// Validate ListenAddr.
+	_, err := NewNetAddressString(IDAddressString(info.ID(), info.ListenAddr))
+	if err != nil {
+		return err
 	}
-	if cmn.ASCIITrim(other.RPCAddress) != "" && !cmn.IsASCIIText(other.RPCAddress) {
-		return fmt.Errorf("info.Other.RPCAddress=%v must be valid non-empty ASCII text without tabs", other.RPCAddress)
+
+	// Network is validated in CompatibleWith.
+
+	// Validate Version
+	if len(info.Version) > 0 &&
+		(!cmn.IsASCIIText(info.Version) || cmn.ASCIITrim(info.Version) == "") {
+
+		return fmt.Errorf("info.Version must be valid ASCII text without tabs, but got %v", info.Version)
 	}
 
+	// Validate Channels - ensure max and check for duplicates.
+	if len(info.Channels) > maxNumChannels {
+		return fmt.Errorf("info.Channels is too long (%v). Max is %v", len(info.Channels), maxNumChannels)
+	}
 	channels := make(map[byte]struct{})
 	for _, ch := range info.Channels {
 		_, ok := channels[ch]
@@ -124,13 +137,30 @@ func (info DefaultNodeInfo) ValidateBasic() error {
 		channels[ch] = struct{}{}
 	}
 
-	// ensure ListenAddr is good
-	_, err := NewNetAddressString(IDAddressString(info.ID(), info.ListenAddr))
-	return err
+	// Validate Moniker.
+	if !cmn.IsASCIIText(info.Moniker) || cmn.ASCIITrim(info.Moniker) == "" {
+		return fmt.Errorf("info.Moniker must be valid non-empty ASCII text without tabs, but got %v", info.Moniker)
+	}
+
+	// Validate Other.
+	other := info.Other
+	txIndex := other.TxIndex
+	switch txIndex {
+	case "", "on", "off":
+	default:
+		return fmt.Errorf("info.Other.TxIndex should be either 'on' or 'off', got '%v'", txIndex)
+	}
+	// XXX: Should we be more strict about address formats?
+	rpcAddr := other.RPCAddress
+	if len(rpcAddr) > 0 && (!cmn.IsASCIIText(rpcAddr) || cmn.ASCIITrim(rpcAddr) == "") {
+		return fmt.Errorf("info.Other.RPCAddress=%v must be valid ASCII text without tabs", rpcAddr)
+	}
+
+	return nil
 }
 
 // CompatibleWith checks if two DefaultNodeInfo are compatible with eachother.
-// CONTRACT: two nodes are compatible if the major version matches and network match
+// CONTRACT: two nodes are compatible if the Block version and network match
 // and they have at least one channel in common.
 func (info DefaultNodeInfo) CompatibleWith(other_ NodeInfo) error {
 	other, ok := other_.(DefaultNodeInfo)
@@ -138,22 +168,9 @@ func (info DefaultNodeInfo) CompatibleWith(other_ NodeInfo) error {
 		return fmt.Errorf("wrong NodeInfo type. Expected DefaultNodeInfo, got %v", reflect.TypeOf(other_))
 	}
 
-	iMajor, _, _, iErr := splitVersion(info.Version)
-	oMajor, _, _, oErr := splitVersion(other.Version)
-
-	// if our own version number is not formatted right, we messed up
-	if iErr != nil {
-		return iErr
-	}
-
-	// version number must be formatted correctly ("x.x.x")
-	if oErr != nil {
-		return oErr
-	}
-
-	// major version must match
-	if iMajor != oMajor {
-		return fmt.Errorf("Peer is on a different major version. Got %v, expected %v", oMajor, iMajor)
+	if info.ProtocolVersion.Block != other.ProtocolVersion.Block {
+		return fmt.Errorf("Peer is on a different Block version. Got %v, expected %v",
+			other.ProtocolVersion.Block, info.ProtocolVersion.Block)
 	}
 
 	// nodes must be on the same network
@@ -201,11 +218,3 @@ func (info DefaultNodeInfo) NetAddress() *NetAddress {
 	}
 	return netAddr
 }
-
-func splitVersion(version string) (string, string, string, error) {
-	spl := strings.Split(version, ".")
-	if len(spl) != 3 {
-		return "", "", "", fmt.Errorf("Invalid version format %v", version)
-	}
-	return spl[0], spl[1], spl[2], nil
-}

p2p/node_info_test.go

@@ -0,0 +1,123 @@
+package p2p
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/tendermint/tendermint/crypto/ed25519"
+)
+
+func TestNodeInfoValidate(t *testing.T) {
+
+	// empty fails
+	ni := DefaultNodeInfo{}
+	assert.Error(t, ni.ValidateBasic())
+
+	channels := make([]byte, maxNumChannels)
+	for i := 0; i < maxNumChannels; i++ {
+		channels[i] = byte(i)
+	}
+	dupChannels := make([]byte, 5)
+	copy(dupChannels[:], channels[:5])
+	dupChannels = append(dupChannels, testCh)
+
+	nonAscii := "¢§µ"
+	emptyTab := fmt.Sprintf("\t")
+	emptySpace := fmt.Sprintf("  ")
+
+	testCases := []struct {
+		testName         string
+		malleateNodeInfo func(*DefaultNodeInfo)
+		expectErr        bool
+	}{
+		{"Too Many Channels", func(ni *DefaultNodeInfo) { ni.Channels = append(channels, byte(maxNumChannels)) }, true},
+		{"Duplicate Channel", func(ni *DefaultNodeInfo) { ni.Channels = dupChannels }, true},
+		{"Good Channels", func(ni *DefaultNodeInfo) { ni.Channels = ni.Channels[:5] }, false},
+
+		{"Invalid NetAddress", func(ni *DefaultNodeInfo) { ni.ListenAddr = "not-an-address" }, true},
+		{"Good NetAddress", func(ni *DefaultNodeInfo) { ni.ListenAddr = "0.0.0.0:26656" }, false},
+
+		{"Non-ASCII Version", func(ni *DefaultNodeInfo) { ni.Version = nonAscii }, true},
+		{"Empty tab Version", func(ni *DefaultNodeInfo) { ni.Version = emptyTab }, true},
+		{"Empty space Version", func(ni *DefaultNodeInfo) { ni.Version = emptySpace }, true},
+		{"Empty Version", func(ni *DefaultNodeInfo) { ni.Version = "" }, false},
+
+		{"Non-ASCII Moniker", func(ni *DefaultNodeInfo) { ni.Moniker = nonAscii }, true},
+		{"Empty tab Moniker", func(ni *DefaultNodeInfo) { ni.Moniker = emptyTab }, true},
+		{"Empty space Moniker", func(ni *DefaultNodeInfo) { ni.Moniker = emptySpace }, true},
+		{"Empty Moniker", func(ni *DefaultNodeInfo) { ni.Moniker = "" }, true},
+		{"Good Moniker", func(ni *DefaultNodeInfo) { ni.Moniker = "hey its me" }, false},
+
+		{"Non-ASCII TxIndex", func(ni *DefaultNodeInfo) { ni.Other.TxIndex = nonAscii }, true},
+		{"Empty tab TxIndex", func(ni *DefaultNodeInfo) { ni.Other.TxIndex = emptyTab }, true},
+		{"Empty space TxIndex", func(ni *DefaultNodeInfo) { ni.Other.TxIndex = emptySpace }, true},
+		{"Empty TxIndex", func(ni *DefaultNodeInfo) { ni.Other.TxIndex = "" }, false},
+		{"Off TxIndex", func(ni *DefaultNodeInfo) { ni.Other.TxIndex = "off" }, false},
+
+		{"Non-ASCII RPCAddress", func(ni *DefaultNodeInfo) { ni.Other.RPCAddress = nonAscii }, true},
+		{"Empty tab RPCAddress", func(ni *DefaultNodeInfo) { ni.Other.RPCAddress = emptyTab }, true},
+		{"Empty space RPCAddress", func(ni *DefaultNodeInfo) { ni.Other.RPCAddress = emptySpace }, true},
+		{"Empty RPCAddress", func(ni *DefaultNodeInfo) { ni.Other.RPCAddress = "" }, false},
+		{"Good RPCAddress", func(ni *DefaultNodeInfo) { ni.Other.RPCAddress = "0.0.0.0:26657" }, false},
+	}
+
+	nodeKey := NodeKey{PrivKey: ed25519.GenPrivKey()}
+	name := "testing"
+
+	// test case passes
+	ni = testNodeInfo(nodeKey.ID(), name).(DefaultNodeInfo)
+	ni.Channels = channels
+	assert.NoError(t, ni.ValidateBasic())
+
+	for _, tc := range testCases {
+		ni := testNodeInfo(nodeKey.ID(), name).(DefaultNodeInfo)
+		ni.Channels = channels
+		tc.malleateNodeInfo(&ni)
+		err := ni.ValidateBasic()
+		if tc.expectErr {
+			assert.Error(t, err, tc.testName)
+		} else {
+			assert.NoError(t, err, tc.testName)
+		}
+	}
+
+}
+
+func TestNodeInfoCompatible(t *testing.T) {
+
+	nodeKey1 := NodeKey{PrivKey: ed25519.GenPrivKey()}
+	nodeKey2 := NodeKey{PrivKey: ed25519.GenPrivKey()}
+	name := "testing"
+
+	var newTestChannel byte = 0x2
+
+	// test NodeInfo is compatible
+	ni1 := testNodeInfo(nodeKey1.ID(), name).(DefaultNodeInfo)
+	ni2 := testNodeInfo(nodeKey2.ID(), name).(DefaultNodeInfo)
+	assert.NoError(t, ni1.CompatibleWith(ni2))
+
+	// add another channel; still compatible
+	ni2.Channels = []byte{newTestChannel, testCh}
+	assert.NoError(t, ni1.CompatibleWith(ni2))
+
+	// wrong NodeInfo type is not compatible
+	_, netAddr := CreateRoutableAddr()
+	ni3 := mockNodeInfo{netAddr}
+	assert.Error(t, ni1.CompatibleWith(ni3))
+
+	testCases := []struct {
+		testName         string
+		malleateNodeInfo func(*DefaultNodeInfo)
+	}{
+		{"Wrong block version", func(ni *DefaultNodeInfo) { ni.ProtocolVersion.Block += 1 }},
+		{"Wrong network", func(ni *DefaultNodeInfo) { ni.Network += "-wrong" }},
+		{"No common channels", func(ni *DefaultNodeInfo) { ni.Channels = []byte{newTestChannel} }},
+	}
+
+	for _, tc := range testCases {
+		ni := testNodeInfo(nodeKey2.ID(), name).(DefaultNodeInfo)
+		tc.malleateNodeInfo(&ni)
+		assert.Error(t, ni1.CompatibleWith(ni))
+	}
+}

p2p/peer_test.go

@@ -207,11 +207,12 @@ func (rp *remotePeer) accept(l net.Listener) {
 
 func (rp *remotePeer) nodeInfo(l net.Listener) NodeInfo {
 	return DefaultNodeInfo{
-		ID_:        rp.Addr().ID,
-		Moniker:    "remote_peer",
-		Network:    "testing",
-		Version:    "123.123.123",
-		ListenAddr: l.Addr().String(),
-		Channels:   rp.channels,
+		ProtocolVersion: InitProtocolVersion,
+		ID_:             rp.Addr().ID,
+		ListenAddr:      l.Addr().String(),
+		Network:         "testing",
+		Version:         "1.2.3-rc0-deadbeef",
+		Channels:        rp.channels,
+		Moniker:         "remote_peer",
 	}
 }

p2p/test_util.go

@@ -247,11 +247,16 @@ func testNodeInfo(id ID, name string) NodeInfo {
 
 func testNodeInfoWithNetwork(id ID, name, network string) NodeInfo {
 	return DefaultNodeInfo{
-		ID_:        id,
-		ListenAddr: fmt.Sprintf("127.0.0.1:%d", cmn.RandIntn(64512)+1023),
-		Moniker:    name,
-		Network:    network,
-		Version:    "123.123.123",
-		Channels:   []byte{testCh},
+		ProtocolVersion: InitProtocolVersion,
+		ID_:             id,
+		ListenAddr:      fmt.Sprintf("127.0.0.1:%d", cmn.RandIntn(64512)+1023),
+		Network:         network,
+		Version:         "1.2.3-rc0-deadbeef",
+		Channels:        []byte{testCh},
+		Moniker:         name,
+		Other: DefaultNodeInfoOther{
+			TxIndex:    "on",
+			RPCAddress: fmt.Sprintf("127.0.0.1:%d", cmn.RandIntn(64512)+1023),
+		},
 	}
 }

p2p/version.go

@@ -1,3 +0,0 @@
-package p2p
-
-const Version = "0.5.0"

rpc/core/status.go

@@ -31,6 +31,11 @@ import (
 // "id": "",
 // "result": {
 //   "node_info": {
+//   		"protocol_version": {
+//   			"p2p": "4",
+//   			"block": "7",
+//   			"app": "0"
+//   		},
 //   		"id": "53729852020041b956e86685e24394e0bee4373f",
 //   		"listen_addr": "10.0.2.15:26656",
 //   		"network": "test-chain-Y1OHx6",
@@ -38,10 +43,6 @@ import (
 //   		"channels": "4020212223303800",
 //   		"moniker": "ubuntu-xenial",
 //   		"other": {
-//   			"amino_version": "0.12.0",
-//   			"p2p_version": "0.5.0",
-//   			"consensus_version": "v1/0.2.2",
-//   			"rpc_version": "0.7.0/3",
 //   			"tx_index": "on",
 //   			"rpc_addr": "tcp://0.0.0.0:26657"
 //   		}

rpc/core/version.go

@@ -1,5 +0,0 @@
-package core
-
-// a single integer is sufficient here
-
-const Version = "3" // rpc routes for profiling, setting config

rpc/lib/version.go

@@ -1,7 +0,0 @@
-package rpc
-
-const Maj = "0"
-const Min = "7"
-const Fix = "0"
-
-const Version = Maj + "." + Min + "." + Fix