Cleanup ids.NodeID usage (#2280)

ava-labs · Nov 9, 2023 · 151621f · 151621f
1 parent ebaf9d4
commit 151621f
Show file tree

Hide file tree

Showing 33 changed files with 157 additions and 124 deletions.
diff --git a/ids/test_generator.go b/ids/test_generator.go
@@ -23,3 +23,12 @@ func GenerateTestShortID() ShortID {
 func GenerateTestNodeID() NodeID {
 	return NodeID(GenerateTestShortID())
 }
+
+// BuildTestNodeID is an utility to build NodeID from bytes in UTs
+// It must not be used in production code. In production code we should
+// use ToNodeID, which performs proper length checking.
+func BuildTestNodeID(src []byte) NodeID {
+	res := NodeID{}
+	copy(res[:], src)
+	return res
+}
diff --git a/network/peer/set_test.go b/network/peer/set_test.go
@@ -18,24 +18,24 @@ func TestSet(t *testing.T) {
 	set := NewSet()
 
 	peer1 := &peer{
-		id:              ids.NodeID{0x01},
+		id:              ids.BuildTestNodeID([]byte{0x01}),
 		observedUptimes: map[ids.ID]uint32{constants.PrimaryNetworkID: 0},
 	}
 	updatedPeer1 := &peer{
-		id:              ids.NodeID{0x01},
+		id:              ids.BuildTestNodeID([]byte{0x01}),
 		observedUptimes: map[ids.ID]uint32{constants.PrimaryNetworkID: 1},
 	}
 	peer2 := &peer{
-		id: ids.NodeID{0x02},
+		id: ids.BuildTestNodeID([]byte{0x02}),
 	}
 	unknownPeer := &peer{
-		id: ids.NodeID{0xff},
+		id: ids.BuildTestNodeID([]byte{0xff}),
 	}
 	peer3 := &peer{
-		id: ids.NodeID{0x03},
+		id: ids.BuildTestNodeID([]byte{0x03}),
 	}
 	peer4 := &peer{
-		id: ids.NodeID{0x04},
+		id: ids.BuildTestNodeID([]byte{0x04}),
 	}
 
 	// add of first peer is handled
@@ -105,10 +105,10 @@ func TestSetSample(t *testing.T) {
 	set := NewSet()
 
 	peer1 := &peer{
-		id: ids.NodeID{0x01},
+		id: ids.BuildTestNodeID([]byte{0x01}),
 	}
 	peer2 := &peer{
-		id: ids.NodeID{0x02},
+		id: ids.BuildTestNodeID([]byte{0x02}),
 	}
 
 	// Case: Empty

diff --git a/network/peer/upgrader.go b/network/peer/upgrader.go
@@ -60,12 +60,12 @@ func (t *tlsClientUpgrader) Upgrade(conn net.Conn) (ids.NodeID, net.Conn, *staki
 
 func connToIDAndCert(conn *tls.Conn, invalidCerts prometheus.Counter) (ids.NodeID, net.Conn, *staking.Certificate, error) {
 	if err := conn.Handshake(); err != nil {
-		return ids.NodeID{}, nil, nil, err
+		return ids.EmptyNodeID, nil, nil, err
 	}
 
 	state := conn.ConnectionState()
 	if len(state.PeerCertificates) == 0 {
-		return ids.NodeID{}, nil, nil, errNoCert
+		return ids.EmptyNodeID, nil, nil, errNoCert
 	}
 
 	tlsCert := state.PeerCertificates[0]
@@ -75,7 +75,7 @@ func connToIDAndCert(conn *tls.Conn, invalidCerts prometheus.Counter) (ids.NodeI
 	peerCert, err := staking.ParseCertificate(tlsCert.Raw)
 	if err != nil {
 		invalidCerts.Inc()
-		return ids.NodeID{}, nil, nil, err
+		return ids.EmptyNodeID, nil, nil, err
 	}
 
 	// We validate the certificate here to attempt to make the validity of the
@@ -84,7 +84,7 @@ func connToIDAndCert(conn *tls.Conn, invalidCerts prometheus.Counter) (ids.NodeI
 	// healthy.
 	if err := staking.ValidateCertificate(peerCert); err != nil {
 		invalidCerts.Inc()
-		return ids.NodeID{}, nil, nil, err
+		return ids.EmptyNodeID, nil, nil, err
 	}
 
 	nodeID := ids.NodeIDFromCert(peerCert)

diff --git a/node/insecure_validator_manager.go b/node/insecure_validator_manager.go
@@ -27,7 +27,7 @@ func (i *insecureValidatorManager) Connected(vdrID ids.NodeID, nodeVersion *vers
 		// peer as a validator. Because each validator needs a txID associated
 		// with it, we hack one together by padding the nodeID with zeroes.
 		dummyTxID := ids.Empty
-		copy(dummyTxID[:], vdrID[:])
+		copy(dummyTxID[:], vdrID.Bytes())
 
 		err := i.vdrs.AddStaker(constants.PrimaryNetworkID, vdrID, nil, dummyTxID, i.weight)
 		if err != nil {

diff --git a/node/node.go b/node/node.go
@@ -304,7 +304,7 @@ func (n *Node) initNetworking() error {
 		// a validator. Because each validator needs a txID associated with it,
 		// we hack one together by just padding our nodeID with zeroes.
 		dummyTxID := ids.Empty
-		copy(dummyTxID[:], n.ID[:])
+		copy(dummyTxID[:], n.ID.Bytes())
 
 		err := n.vdrs.AddStaker(
 			constants.PrimaryNetworkID,

diff --git a/snow/consensus/snowman/poll/set_test.go b/snow/consensus/snowman/poll/set_test.go
@@ -21,11 +21,11 @@ var (
 	blkID3 = ids.ID{3}
 	blkID4 = ids.ID{4}
 
-	vdr1 = ids.NodeID{1}
-	vdr2 = ids.NodeID{2}
-	vdr3 = ids.NodeID{3}
-	vdr4 = ids.NodeID{4}
-	vdr5 = ids.NodeID{5}
+	vdr1 = ids.BuildTestNodeID([]byte{0x01})
+	vdr2 = ids.BuildTestNodeID([]byte{0x02})
+	vdr3 = ids.BuildTestNodeID([]byte{0x03})
+	vdr4 = ids.BuildTestNodeID([]byte{0x04})
+	vdr5 = ids.BuildTestNodeID([]byte{0x05}) // k = 5
 )
 
 func TestNewSetErrorOnPollsMetrics(t *testing.T) {

diff --git a/snow/engine/common/appsender/appsender_client.go b/snow/engine/common/appsender/appsender_client.go
@@ -52,8 +52,7 @@ func (c *Client) SendAppRequest(ctx context.Context, nodeIDs set.Set[ids.NodeID]
 	nodeIDsBytes := make([][]byte, nodeIDs.Len())
 	i := 0
 	for nodeID := range nodeIDs {
-		nodeID := nodeID // Prevent overwrite in next iteration
-		nodeIDsBytes[i] = nodeID[:]
+		nodeIDsBytes[i] = nodeID.Bytes()
 		i++
 	}
 	_, err := c.client.SendAppRequest(
@@ -71,7 +70,7 @@ func (c *Client) SendAppResponse(ctx context.Context, nodeID ids.NodeID, request
 	_, err := c.client.SendAppResponse(
 		ctx,
 		&appsenderpb.SendAppResponseMsg{
-			NodeId:    nodeID[:],
+			NodeId:    nodeID.Bytes(),
 			RequestId: requestID,
 			Response:  response,
 		},
@@ -93,8 +92,7 @@ func (c *Client) SendAppGossipSpecific(ctx context.Context, nodeIDs set.Set[ids.
 	nodeIDsBytes := make([][]byte, nodeIDs.Len())
 	i := 0
 	for nodeID := range nodeIDs {
-		nodeID := nodeID // Prevent overwrite in next iteration
-		nodeIDsBytes[i] = nodeID[:]
+		nodeIDsBytes[i] = nodeID.Bytes()
 		i++
 	}
 	_, err := c.client.SendAppGossipSpecific(

diff --git a/snow/engine/common/requests_test.go b/snow/engine/common/requests_test.go
@@ -30,7 +30,7 @@ func TestRequests(t *testing.T) {
 	_, removed = req.Remove(ids.EmptyNodeID, 1)
 	require.False(removed)
 
-	_, removed = req.Remove(ids.NodeID{1}, 0)
+	_, removed = req.Remove(ids.BuildTestNodeID([]byte{0x01}), 0)
 	require.False(removed)
 
 	require.True(req.Contains(ids.Empty))
@@ -42,7 +42,7 @@ func TestRequests(t *testing.T) {
 	_, removed = req.Remove(ids.EmptyNodeID, 1)
 	require.False(removed)
 
-	_, removed = req.Remove(ids.NodeID{1}, 0)
+	_, removed = req.Remove(ids.BuildTestNodeID([]byte{0x01}), 0)
 	require.False(removed)
 
 	require.True(req.Contains(ids.Empty))

diff --git a/snow/engine/snowman/bootstrap/bootstrapper_test.go b/snow/engine/snowman/bootstrap/bootstrapper_test.go
@@ -405,7 +405,7 @@ func TestBootstrapperUnknownByzantineResponse(t *testing.T) {
 	require.NoError(bs.Ancestors(context.Background(), peerID, *requestID+1, [][]byte{blkBytes1})) // respond with wrong request ID
 	require.Equal(oldReqID, *requestID)
 
-	require.NoError(bs.Ancestors(context.Background(), ids.NodeID{1, 2, 3}, *requestID, [][]byte{blkBytes1})) // respond from wrong peer
+	require.NoError(bs.Ancestors(context.Background(), ids.BuildTestNodeID([]byte{1, 2, 3}), *requestID, [][]byte{blkBytes1})) // respond from wrong peer
 	require.Equal(oldReqID, *requestID)
 
 	require.NoError(bs.Ancestors(context.Background(), peerID, *requestID, [][]byte{blkBytes0})) // respond with wrong block

diff --git a/snow/networking/sender/sender_test.go b/snow/networking/sender/sender_test.go
@@ -312,7 +312,7 @@ func TestReliableMessages(t *testing.T) {
 
 	ctx := snow.DefaultConsensusContextTest()
 	vdrs := validators.NewManager()
-	require.NoError(vdrs.AddStaker(ctx.SubnetID, ids.NodeID{1}, nil, ids.Empty, 1))
+	require.NoError(vdrs.AddStaker(ctx.SubnetID, ids.BuildTestNodeID([]byte{1}), nil, ids.Empty, 1))
 	benchlist := benchlist.NewNoBenchlist()
 	tm, err := timeout.NewManager(
 		&timer.AdaptiveTimeoutConfig{
@@ -443,7 +443,7 @@ func TestReliableMessages(t *testing.T) {
 
 	go func() {
 		for i := 0; i < queriesToSend; i++ {
-			vdrIDs := set.Of(ids.NodeID{1})
+			vdrIDs := set.Of(ids.BuildTestNodeID([]byte{1}))
 
 			sender.SendPullQuery(context.Background(), vdrIDs, uint32(i), ids.Empty, 0)
 			time.Sleep(time.Duration(rand.Float64() * float64(time.Microsecond))) // #nosec G404

diff --git a/snow/networking/timeout/manager_test.go b/snow/networking/timeout/manager_test.go
@@ -39,7 +39,7 @@ func TestManagerFire(t *testing.T) {
 	wg.Add(1)
 
 	manager.RegisterRequest(
-		ids.NodeID{},
+		ids.EmptyNodeID,
 		ids.ID{},
 		true,
 		ids.RequestID{},

diff --git a/snow/networking/tracker/resource_tracker_test.go b/snow/networking/tracker/resource_tracker_test.go
@@ -48,8 +48,8 @@ func TestCPUTracker(t *testing.T) {
 	tracker, err := NewResourceTracker(prometheus.NewRegistry(), mockUser, meter.ContinuousFactory{}, time.Second)
 	require.NoError(err)
 
-	node1 := ids.NodeID{1}
-	node2 := ids.NodeID{2}
+	node1 := ids.BuildTestNodeID([]byte{1})
+	node2 := ids.BuildTestNodeID([]byte{2})
 
 	// Note that all the durations between start and end are [halflife].
 	startTime1 := time.Now()

diff --git a/snow/networking/tracker/targeter_test.go b/snow/networking/tracker/targeter_test.go
@@ -46,10 +46,10 @@ func TestNewTargeter(t *testing.T) {
 func TestTarget(t *testing.T) {
 	ctrl := gomock.NewController(t)
 
-	vdr := ids.NodeID{1}
+	vdr := ids.BuildTestNodeID([]byte{1})
 	vdrWeight := uint64(1)
 	totalVdrWeight := uint64(10)
-	nonVdr := ids.NodeID{2}
+	nonVdr := ids.BuildTestNodeID([]byte{2})
 	vdrs := validators.NewManager()
 	require.NoError(t, vdrs.AddStaker(constants.PrimaryNetworkID, vdr, nil, ids.Empty, 1))
 	require.NoError(t, vdrs.AddStaker(constants.PrimaryNetworkID, ids.GenerateTestNodeID(), nil, ids.Empty, totalVdrWeight-vdrWeight))

diff --git a/snow/validators/gvalidators/validator_state_server.go b/snow/validators/gvalidators/validator_state_server.go
@@ -66,7 +66,7 @@ func (s *Server) GetValidatorSet(ctx context.Context, req *pb.GetValidatorSetReq
 	i := 0
 	for _, vdr := range vdrs {
 		vdrPB := &pb.Validator{
-			NodeId: vdr.NodeID[:],
+			NodeId: vdr.NodeID.Bytes(),
 			Weight: vdr.Weight,
 		}
 		if vdr.PublicKey != nil {

diff --git a/snow/validators/manager_test.go b/snow/validators/manager_test.go
@@ -324,9 +324,9 @@ func TestGetMap(t *testing.T) {
 func TestWeight(t *testing.T) {
 	require := require.New(t)
 
-	vdr0 := ids.NodeID{1}
+	vdr0 := ids.BuildTestNodeID([]byte{1})
 	weight0 := uint64(93)
-	vdr1 := ids.NodeID{2}
+	vdr1 := ids.BuildTestNodeID([]byte{2})
 	weight1 := uint64(123)
 
 	m := NewManager()
@@ -411,7 +411,7 @@ func TestString(t *testing.T) {
 func TestAddCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	sk0, err := bls.NewSecretKey()
 	require.NoError(err)
 	pk0 := bls.PublicFromSecretKey(sk0)
@@ -442,7 +442,7 @@ func TestAddCallback(t *testing.T) {
 func TestAddWeightCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	txID0 := ids.GenerateTestID()
 	weight0 := uint64(1)
 	weight1 := uint64(93)
@@ -480,7 +480,7 @@ func TestAddWeightCallback(t *testing.T) {
 func TestRemoveWeightCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	txID0 := ids.GenerateTestID()
 	weight0 := uint64(93)
 	weight1 := uint64(92)
@@ -518,7 +518,7 @@ func TestRemoveWeightCallback(t *testing.T) {
 func TestValidatorRemovedCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	txID0 := ids.GenerateTestID()
 	weight0 := uint64(93)
 

diff --git a/snow/validators/set_test.go b/snow/validators/set_test.go
@@ -273,9 +273,9 @@ func TestSetMap(t *testing.T) {
 func TestSetWeight(t *testing.T) {
 	require := require.New(t)
 
-	vdr0 := ids.NodeID{1}
+	vdr0 := ids.BuildTestNodeID([]byte{1})
 	weight0 := uint64(93)
-	vdr1 := ids.NodeID{2}
+	vdr1 := ids.BuildTestNodeID([]byte{2})
 	weight1 := uint64(123)
 
 	s := newSet()
@@ -332,10 +332,10 @@ func TestSetString(t *testing.T) {
 	require := require.New(t)
 
 	nodeID0 := ids.EmptyNodeID
-	nodeID1 := ids.NodeID{
+	nodeID1 := ids.BuildTestNodeID([]byte{
 		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
 		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
-	}
+	})
 
 	s := newSet()
 	require.NoError(s.Add(nodeID0, nil, ids.Empty, 1))
@@ -385,7 +385,7 @@ func (c *callbackListener) OnValidatorWeightChanged(nodeID ids.NodeID, oldWeight
 func TestSetAddCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	sk0, err := bls.NewSecretKey()
 	require.NoError(err)
 	pk0 := bls.PublicFromSecretKey(sk0)
@@ -413,7 +413,7 @@ func TestSetAddCallback(t *testing.T) {
 func TestSetAddWeightCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	txID0 := ids.GenerateTestID()
 	weight0 := uint64(1)
 	weight1 := uint64(93)
@@ -447,7 +447,7 @@ func TestSetAddWeightCallback(t *testing.T) {
 func TestSetRemoveWeightCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	txID0 := ids.GenerateTestID()
 	weight0 := uint64(93)
 	weight1 := uint64(92)
@@ -481,7 +481,7 @@ func TestSetRemoveWeightCallback(t *testing.T) {
 func TestSetValidatorRemovedCallback(t *testing.T) {
 	require := require.New(t)
 
-	nodeID0 := ids.NodeID{1}
+	nodeID0 := ids.BuildTestNodeID([]byte{1})
 	txID0 := ids.GenerateTestID()
 	weight0 := uint64(93)
 

diff --git a/utils/beacon/set_test.go b/utils/beacon/set_test.go
@@ -16,9 +16,9 @@ import (
 func TestSet(t *testing.T) {
 	require := require.New(t)
 
-	id0 := ids.NodeID{0}
-	id1 := ids.NodeID{1}
-	id2 := ids.NodeID{2}
+	id0 := ids.BuildTestNodeID([]byte{0})
+	id1 := ids.BuildTestNodeID([]byte{1})
+	id2 := ids.BuildTestNodeID([]byte{2})
 
 	ip0 := ips.IPPort{
 		IP:   net.IPv4zero,

diff --git a/vms/platformvm/api/static_service_test.go b/vms/platformvm/api/static_service_test.go
@@ -18,7 +18,7 @@ import (
 
 func TestBuildGenesisInvalidUTXOBalance(t *testing.T) {
 	require := require.New(t)
-	nodeID := ids.NodeID{1, 2, 3}
+	nodeID := ids.BuildTestNodeID([]byte{1, 2, 3})
 	addr, err := address.FormatBech32(constants.UnitTestHRP, nodeID.Bytes())
 	require.NoError(err)
 
@@ -62,7 +62,7 @@ func TestBuildGenesisInvalidUTXOBalance(t *testing.T) {
 
 func TestBuildGenesisInvalidStakeWeight(t *testing.T) {
 	require := require.New(t)
-	nodeID := ids.NodeID{1, 2, 3}
+	nodeID := ids.BuildTestNodeID([]byte{1, 2, 3})
 	addr, err := address.FormatBech32(constants.UnitTestHRP, nodeID.Bytes())
 	require.NoError(err)
 
@@ -106,7 +106,7 @@ func TestBuildGenesisInvalidStakeWeight(t *testing.T) {
 
 func TestBuildGenesisInvalidEndtime(t *testing.T) {
 	require := require.New(t)
-	nodeID := ids.NodeID{1, 2, 3}
+	nodeID := ids.BuildTestNodeID([]byte{1, 2, 3})
 	addr, err := address.FormatBech32(constants.UnitTestHRP, nodeID.Bytes())
 	require.NoError(err)
 
@@ -151,7 +151,7 @@ func TestBuildGenesisInvalidEndtime(t *testing.T) {
 
 func TestBuildGenesisReturnsSortedValidators(t *testing.T) {
 	require := require.New(t)
-	nodeID := ids.NodeID{1}
+	nodeID := ids.BuildTestNodeID([]byte{1})
 	addr, err := address.FormatBech32(constants.UnitTestHRP, nodeID.Bytes())
 	require.NoError(err)