Remove contains from validator manager interface (#2198)

Signed-off-by: Ceyhun Onur <[email protected]> Signed-off-by: Stephen Buttolph <[email protected]> Co-authored-by: Alberto Benegiamo <[email protected]> Co-authored-by: Stephen Buttolph <[email protected]>
ava-labs · Oct 24, 2023 · 7903676 · 7903676
1 parent 963aeb0
commit 7903676
Show file tree

Hide file tree

Showing 18 changed files with 71 additions and 98 deletions.
diff --git a/chains/manager.go b/chains/manager.go
@@ -1359,7 +1359,7 @@ func (m *manager) registerBootstrappedHealthChecks() error {
 		if !m.IsBootstrapped(constants.PlatformChainID) {
 			return "node is currently bootstrapping", nil
 		}
-		if !m.Validators.Contains(constants.PrimaryNetworkID, m.NodeID) {
+		if _, ok := m.Validators.GetValidator(constants.PrimaryNetworkID, m.NodeID); !ok {
 			return "node is not a primary network validator", nil
 		}
 

diff --git a/network/network.go b/network/network.go
@@ -801,7 +801,7 @@ func (n *network) Dispatch() error {
 }
 
 func (n *network) WantsConnection(nodeID ids.NodeID) bool {
-	if n.config.Validators.Contains(constants.PrimaryNetworkID, nodeID) {
+	if _, ok := n.config.Validators.GetValidator(constants.PrimaryNetworkID, nodeID); ok {
 		return true
 	}
 
@@ -864,7 +864,7 @@ func (n *network) getPeers(
 			continue
 		}
 
-		isValidator := n.config.Validators.Contains(subnetID, nodeID)
+		_, isValidator := n.config.Validators.GetValidator(subnetID, nodeID)
 		// check if the peer is allowed to connect to the subnet
 		if !allower.IsAllowed(nodeID, isValidator) {
 			continue
@@ -903,7 +903,7 @@ func (n *network) samplePeers(
 			}
 
 			peerID := p.ID()
-			isValidator := n.config.Validators.Contains(subnetID, peerID)
+			_, isValidator := n.config.Validators.GetValidator(subnetID, peerID)
 			// check if the peer is allowed to connect to the subnet
 			if !allower.IsAllowed(peerID, isValidator) {
 				return false

diff --git a/network/peer/peer.go b/network/peer/peer.go
@@ -853,7 +853,7 @@ func (p *peer) handleVersion(msg *p2p.Version) {
 	p.Metrics.ClockSkew.Observe(clockDifference)
 
 	if clockDifference > p.MaxClockDifference.Seconds() {
-		if p.Beacons.Contains(constants.PrimaryNetworkID, p.id) {
+		if _, ok := p.Beacons.GetValidator(constants.PrimaryNetworkID, p.id); ok {
 			p.Log.Warn("beacon reports out of sync time",
 				zap.Stringer("nodeID", p.id),
 				zap.Uint64("peerTime", msg.MyTime),
@@ -882,7 +882,7 @@ func (p *peer) handleVersion(msg *p2p.Version) {
 	p.version = peerVersion
 
 	if p.VersionCompatibility.Version().Before(peerVersion) {
-		if p.Beacons.Contains(constants.PrimaryNetworkID, p.id) {
+		if _, ok := p.Beacons.GetValidator(constants.PrimaryNetworkID, p.id); ok {
 			p.Log.Info("beacon attempting to connect with newer version. You may want to update your client",
 				zap.Stringer("nodeID", p.id),
 				zap.Stringer("beaconVersion", peerVersion),

diff --git a/node/beacon_manager.go b/node/beacon_manager.go
@@ -25,16 +25,17 @@ type beaconManager struct {
 }
 
 func (b *beaconManager) Connected(nodeID ids.NodeID, nodeVersion *version.Application, subnetID ids.ID) {
-	if constants.PrimaryNetworkID == subnetID &&
-		b.beacons.Contains(constants.PrimaryNetworkID, nodeID) &&
+	_, isBeacon := b.beacons.GetValidator(constants.PrimaryNetworkID, nodeID)
+	if isBeacon &&
+		constants.PrimaryNetworkID == subnetID &&
 		atomic.AddInt64(&b.numConns, 1) >= b.requiredConns {
 		b.timer.Cancel()
 	}
 	b.Router.Connected(nodeID, nodeVersion, subnetID)
 }
 
 func (b *beaconManager) Disconnected(nodeID ids.NodeID) {
-	if b.beacons.Contains(constants.PrimaryNetworkID, nodeID) {
+	if _, isBeacon := b.beacons.GetValidator(constants.PrimaryNetworkID, nodeID); isBeacon {
 		atomic.AddInt64(&b.numConns, -1)
 	}
 	b.Router.Disconnected(nodeID)

diff --git a/snow/engine/common/bootstrapper.go b/snow/engine/common/bootstrapper.go
@@ -304,7 +304,7 @@ func (b *bootstrapper) Startup(ctx context.Context) error {
 	b.sampledBeacons = validators.NewManager()
 	b.pendingSendAcceptedFrontier.Clear()
 	for _, nodeID := range beaconIDs {
-		if !b.sampledBeacons.Contains(b.Ctx.SubnetID, nodeID) {
+		if _, ok := b.sampledBeacons.GetValidator(b.Ctx.SubnetID, nodeID); !ok {
 			// Invariant: We never use the TxID or BLS keys populated here.
 			err = b.sampledBeacons.AddStaker(b.Ctx.SubnetID, nodeID, nil, ids.Empty, 1)
 		} else {

diff --git a/snow/engine/snowman/bootstrap/bootstrapper.go b/snow/engine/snowman/bootstrap/bootstrapper.go
@@ -251,7 +251,7 @@ func (b *bootstrapper) Connected(ctx context.Context, nodeID ids.NodeID, nodeVer
 		return err
 	}
 	// Ensure fetchFrom reflects proper validator list
-	if b.Beacons.Contains(b.Ctx.SubnetID, nodeID) {
+	if _, ok := b.Beacons.GetValidator(b.Ctx.SubnetID, nodeID); ok {
 		b.fetchFrom.Add(nodeID)
 	}
 

diff --git a/snow/engine/snowman/syncer/state_syncer.go b/snow/engine/snowman/syncer/state_syncer.go
@@ -471,7 +471,7 @@ func (ss *stateSyncer) startup(ctx context.Context) error {
 
 	ss.frontierSeeders = validators.NewManager()
 	for _, nodeID := range beaconIDs {
-		if !ss.frontierSeeders.Contains(ss.Ctx.SubnetID, nodeID) {
+		if _, ok := ss.frontierSeeders.GetValidator(ss.Ctx.SubnetID, nodeID); !ok {
 			// Invariant: We never use the TxID or BLS keys populated here.
 			err = ss.frontierSeeders.AddStaker(ss.Ctx.SubnetID, nodeID, nil, ids.Empty, 1)
 		} else {

diff --git a/snow/networking/handler/handler.go b/snow/networking/handler/handler.go
@@ -180,7 +180,8 @@ func (h *handler) Context() *snow.ConsensusContext {
 }
 
 func (h *handler) ShouldHandle(nodeID ids.NodeID) bool {
-	return h.subnet.IsAllowed(nodeID, h.validators.Contains(h.ctx.SubnetID, nodeID))
+	_, ok := h.validators.GetValidator(h.ctx.SubnetID, nodeID)
+	return h.subnet.IsAllowed(nodeID, ok)
 }
 
 func (h *handler) SetEngineManager(engineManager *EngineManager) {

diff --git a/snow/validators/manager.go b/snow/validators/manager.go
@@ -74,10 +74,6 @@ type Manager interface {
 	// If an error is returned, the set will be unmodified.
 	RemoveWeight(subnetID ids.ID, nodeID ids.NodeID, weight uint64) error
 
-	// Contains returns true if there is a validator with the specified ID
-	// currently in the subnet.
-	Contains(subnetID ids.ID, nodeID ids.NodeID) bool
-
 	// Count returns the number of validators currently in the subnet.
 	Count(subnetID ids.ID) int
 
@@ -220,17 +216,6 @@ func (m *manager) RemoveWeight(subnetID ids.ID, nodeID ids.NodeID, weight uint64
 	return nil
 }
 
-func (m *manager) Contains(subnetID ids.ID, nodeID ids.NodeID) bool {
-	m.lock.RLock()
-	set, exists := m.subnetToVdrs[subnetID]
-	m.lock.RUnlock()
-	if !exists {
-		return false
-	}
-
-	return set.Contains(nodeID)
-}
-
 func (m *manager) Count(subnetID ids.ID) int {
 	m.lock.RLock()
 	set, exists := m.subnetToVdrs[subnetID]

diff --git a/snow/validators/manager_test.go b/snow/validators/manager_test.go
@@ -204,22 +204,10 @@ func TestGet(t *testing.T) {
 	require.Equal(nodeID, vdr1.NodeID)
 	require.Equal(pk, vdr1.PublicKey)
 	require.Equal(uint64(2), vdr1.Weight)
-}
-
-func TestContains(t *testing.T) {
-	require := require.New(t)
-
-	m := NewManager()
-	subnetID := ids.GenerateTestID()
 
-	nodeID := ids.GenerateTestNodeID()
-	require.False(m.Contains(subnetID, nodeID))
-
-	require.NoError(m.AddStaker(subnetID, nodeID, nil, ids.Empty, 1))
-
-	require.True(m.Contains(subnetID, nodeID))
-	require.NoError(m.RemoveWeight(subnetID, nodeID, 1))
-	require.False(m.Contains(subnetID, nodeID))
+	require.NoError(m.RemoveWeight(subnetID, nodeID, 2))
+	_, ok = m.GetValidator(subnetID, nodeID)
+	require.False(ok)
 }
 
 func TestLen(t *testing.T) {

diff --git a/snow/validators/overridden_manager.go b/snow/validators/overridden_manager.go
@@ -55,10 +55,6 @@ func (o *overriddenManager) RemoveWeight(_ ids.ID, nodeID ids.NodeID, weight uin
 	return o.manager.RemoveWeight(o.subnetID, nodeID, weight)
 }
 
-func (o *overriddenManager) Contains(_ ids.ID, nodeID ids.NodeID) bool {
-	return o.manager.Contains(o.subnetID, nodeID)
-}
-
 func (o *overriddenManager) Count(ids.ID) int {
 	return o.manager.Count(o.subnetID)
 }

diff --git a/snow/validators/set.go b/snow/validators/set.go
@@ -203,18 +203,6 @@ func (s *vdrSet) get(nodeID ids.NodeID) (*Validator, bool) {
 	return &copiedVdr, true
 }
 
-func (s *vdrSet) Contains(nodeID ids.NodeID) bool {
-	s.lock.RLock()
-	defer s.lock.RUnlock()
-
-	return s.contains(nodeID)
-}
-
-func (s *vdrSet) contains(nodeID ids.NodeID) bool {
-	_, contains := s.vdrs[nodeID]
-	return contains
-}
-
 func (s *vdrSet) Len() int {
 	s.lock.RLock()
 	defer s.lock.RUnlock()

diff --git a/snow/validators/set_test.go b/snow/validators/set_test.go
@@ -155,21 +155,10 @@ func TestSetGet(t *testing.T) {
 	require.Equal(nodeID, vdr1.NodeID)
 	require.Equal(pk, vdr1.PublicKey)
 	require.Equal(uint64(2), vdr1.Weight)
-}
-
-func TestSetContains(t *testing.T) {
-	require := require.New(t)
-
-	s := newSet()
 
-	nodeID := ids.GenerateTestNodeID()
-	require.False(s.Contains(nodeID))
-
-	require.NoError(s.Add(nodeID, nil, ids.Empty, 1))
-
-	require.True(s.Contains(nodeID))
-	require.NoError(s.RemoveWeight(nodeID, 1))
-	require.False(s.Contains(nodeID))
+	require.NoError(s.RemoveWeight(nodeID, 2))
+	_, ok = s.Get(nodeID)
+	require.False(ok)
 }
 
 func TestSetLen(t *testing.T) {

diff --git a/vms/platformvm/block/executor/proposal_block_test.go b/vms/platformvm/block/executor/proposal_block_test.go
@@ -685,20 +685,24 @@ func TestBanffProposalBlockUpdateStakers(t *testing.T) {
 				case pending:
 					_, err := env.state.GetPendingValidator(constants.PrimaryNetworkID, stakerNodeID)
 					require.NoError(err)
-					require.False(env.config.Validators.Contains(constants.PrimaryNetworkID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(constants.PrimaryNetworkID, stakerNodeID)
+					require.False(ok)
 				case current:
 					_, err := env.state.GetCurrentValidator(constants.PrimaryNetworkID, stakerNodeID)
 					require.NoError(err)
-					require.True(env.config.Validators.Contains(constants.PrimaryNetworkID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(constants.PrimaryNetworkID, stakerNodeID)
+					require.True(ok)
 				}
 			}
 
 			for stakerNodeID, status := range test.expectedSubnetStakers {
 				switch status {
 				case pending:
-					require.False(env.config.Validators.Contains(subnetID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(subnetID, stakerNodeID)
+					require.False(ok)
 				case current:
-					require.True(env.config.Validators.Contains(subnetID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(subnetID, stakerNodeID)
+					require.True(ok)
 				}
 			}
 		})
@@ -836,8 +840,10 @@ func TestBanffProposalBlockRemoveSubnetValidator(t *testing.T) {
 	// Check VM Validators are removed successfully
 	require.NoError(propBlk.Accept(context.Background()))
 	require.NoError(commitBlk.Accept(context.Background()))
-	require.False(env.config.Validators.Contains(subnetID, subnetVdr2NodeID))
-	require.False(env.config.Validators.Contains(subnetID, subnetValidatorNodeID))
+	_, ok := env.config.Validators.GetValidator(subnetID, subnetVdr2NodeID)
+	require.False(ok)
+	_, ok = env.config.Validators.GetValidator(subnetID, subnetValidatorNodeID)
+	require.False(ok)
 }
 
 func TestBanffProposalBlockTrackedSubnet(t *testing.T) {
@@ -940,7 +946,8 @@ func TestBanffProposalBlockTrackedSubnet(t *testing.T) {
 
 			require.NoError(propBlk.Accept(context.Background()))
 			require.NoError(commitBlk.Accept(context.Background()))
-			require.Equal(tracked, env.config.Validators.Contains(subnetID, subnetValidatorNodeID))
+			_, ok := env.config.Validators.GetValidator(subnetID, subnetValidatorNodeID)
+			require.Equal(tracked, ok)
 		})
 	}
 }

diff --git a/vms/platformvm/block/executor/standard_block_test.go b/vms/platformvm/block/executor/standard_block_test.go
@@ -348,7 +348,8 @@ func TestBanffStandardBlockUpdatePrimaryNetworkStakers(t *testing.T) {
 
 	// Test VM validators
 	require.NoError(block.Accept(context.Background()))
-	require.True(env.config.Validators.Contains(constants.PrimaryNetworkID, nodeID))
+	_, ok := env.config.Validators.GetValidator(constants.PrimaryNetworkID, nodeID)
+	require.True(ok)
 }
 
 // Ensure semantic verification updates the current and pending staker sets correctly.
@@ -561,20 +562,24 @@ func TestBanffStandardBlockUpdateStakers(t *testing.T) {
 				case pending:
 					_, err := env.state.GetPendingValidator(constants.PrimaryNetworkID, stakerNodeID)
 					require.NoError(err)
-					require.False(env.config.Validators.Contains(constants.PrimaryNetworkID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(constants.PrimaryNetworkID, stakerNodeID)
+					require.False(ok)
 				case current:
 					_, err := env.state.GetCurrentValidator(constants.PrimaryNetworkID, stakerNodeID)
 					require.NoError(err)
-					require.True(env.config.Validators.Contains(constants.PrimaryNetworkID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(constants.PrimaryNetworkID, stakerNodeID)
+					require.True(ok)
 				}
 			}
 
 			for stakerNodeID, status := range test.expectedSubnetStakers {
 				switch status {
 				case pending:
-					require.False(env.config.Validators.Contains(subnetID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(subnetID, stakerNodeID)
+					require.False(ok)
 				case current:
-					require.True(env.config.Validators.Contains(subnetID, stakerNodeID))
+					_, ok := env.config.Validators.GetValidator(subnetID, stakerNodeID)
+					require.True(ok)
 				}
 			}
 		})
@@ -675,8 +680,10 @@ func TestBanffStandardBlockRemoveSubnetValidator(t *testing.T) {
 
 	// Check VM Validators are removed successfully
 	require.NoError(block.Accept(context.Background()))
-	require.False(env.config.Validators.Contains(subnetID, subnetVdr2NodeID))
-	require.False(env.config.Validators.Contains(subnetID, subnetValidatorNodeID))
+	_, ok := env.config.Validators.GetValidator(subnetID, subnetVdr2NodeID)
+	require.False(ok)
+	_, ok = env.config.Validators.GetValidator(subnetID, subnetValidatorNodeID)
+	require.False(ok)
 }
 
 func TestBanffStandardBlockTrackedSubnet(t *testing.T) {
@@ -739,7 +746,8 @@ func TestBanffStandardBlockTrackedSubnet(t *testing.T) {
 			// update staker set
 			require.NoError(block.Verify(context.Background()))
 			require.NoError(block.Accept(context.Background()))
-			require.Equal(tracked, env.config.Validators.Contains(subnetID, subnetValidatorNodeID))
+			_, ok := env.config.Validators.GetValidator(subnetID, subnetValidatorNodeID)
+			require.Equal(tracked, ok)
 		})
 	}
 }

diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go
@@ -1985,7 +1985,8 @@ func (s *Service) nodeValidates(blockchainID ids.ID) bool {
 		return false
 	}
 
-	return s.vm.Validators.Contains(chain.SubnetID, s.vm.ctx.NodeID)
+	_, isValidator := s.vm.Validators.GetValidator(chain.SubnetID, s.vm.ctx.NodeID)
+	return isValidator
 }
 
 func (s *Service) chainExists(ctx context.Context, blockID ids.ID, chainID ids.ID) (bool, error) {