From 20e999d64dfab95a013a831a1c84af3fb26ee305 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Tue, 21 May 2024 12:36:05 -0400 Subject: [PATCH] Enable gossiping of newer IPs when connected to an older IP (#3035) --- network/README.md | 1 - network/ip_tracker.go | 54 ++++++++++++++++++++++---------------- network/ip_tracker_test.go | 25 ++++++++++++------ 3 files changed, 48 insertions(+), 32 deletions(-) diff --git a/network/README.md b/network/README.md index eecdd9bc52f6..1b9a4d3235bc 100644 --- a/network/README.md +++ b/network/README.md @@ -173,7 +173,6 @@ A `GetPeerList` message contains the Bloom Filter of the currently known peers a `PeerList` messages are expected to contain `IP:Port` pairs that satisfy all of the following constraints: - The Bloom Filter sent when requesting the `PeerList` message does not contain the node claiming the `IP:Port` pair. - The node claiming the `IP:Port` pair is currently connected. -- The `IP:Port` pair the node shared during the `Handshake` message is the node's most recently known `IP:Port` pair. - The node claiming the `IP:Port` pair is either in the default bootstrapper set, is a current Primary Network validator, is a validator of a tracked Subnet, or is a validator of a Subnet and the peer is a Primary Network validator. #### Example PeerList Gossip diff --git a/network/ip_tracker.go b/network/ip_tracker.go index fee0cd9bd690..58032ff4f202 100644 --- a/network/ip_tracker.go +++ b/network/ip_tracker.go @@ -140,7 +140,12 @@ type gossipableSubnet struct { gossipableIPs []*ips.ClaimedIPPort } -func (s *gossipableSubnet) addGossipableIP(ip *ips.ClaimedIPPort) { +func (s *gossipableSubnet) setGossipableIP(ip *ips.ClaimedIPPort) { + if index, ok := s.gossipableIndices[ip.NodeID]; ok { + s.gossipableIPs[index] = ip + return + } + s.numGossipableIPs.Inc() s.gossipableIndices[ip.NodeID] = len(s.gossipableIPs) s.gossipableIPs = append(s.gossipableIPs, ip) @@ -307,14 +312,21 @@ func (i *ipTracker) ShouldVerifyIP( // subnet. // 2. This IP is newer than the most recent IP we know of for the node. // -// If the previous IP for this node was marked as gossipable, calling this -// function will remove the previous IP from the gossipable set. +// If this IP is replacing a gossipable IP, this IP will also be marked as +// gossipable. func (i *ipTracker) AddIP(ip *ips.ClaimedIPPort) bool { i.lock.Lock() defer i.lock.Unlock() timestampComparison, trackedNode := i.addIP(ip) - return timestampComparison > sameTimestamp && trackedNode.wantsConnection() + if timestampComparison <= sameTimestamp { + return false + } + + if connectedNode, ok := i.connected[ip.NodeID]; ok { + i.setGossipableIP(trackedNode.ip, connectedNode.trackedSubnets) + } + return trackedNode.wantsConnection() } // GetIP returns the most recent IP of the provided nodeID. Returns true if all @@ -343,14 +355,10 @@ func (i *ipTracker) Connected(ip *ips.ClaimedIPPort, trackedSubnets set.Set[ids. trackedSubnets: trackedSubnets, ip: ip, } - if timestampComparison, _ := i.addIP(ip); timestampComparison < sameTimestamp { - return - } - for subnetID := range trackedSubnets { - if subnet, ok := i.subnet[subnetID]; ok && subnet.gossipableIDs.Contains(ip.NodeID) { - subnet.addGossipableIP(ip) - } + timestampComparison, trackedNode := i.addIP(ip) + if timestampComparison != untrackedTimestamp { + i.setGossipableIP(trackedNode.ip, trackedSubnets) } } @@ -364,8 +372,6 @@ func (i *ipTracker) addIP(ip *ips.ClaimedIPPort) (int, *trackedNode) { // This is the first IP we've heard from the validator, so it is the // most recent. i.updateMostRecentTrackedIP(node, ip) - // Because we didn't previously have an IP, we know we aren't currently - // connected to them. return newTimestamp, node } @@ -376,25 +382,29 @@ func (i *ipTracker) addIP(ip *ips.ClaimedIPPort) (int, *trackedNode) { return sameTimestamp, node // This IP is equal to the previously known IP. } + // This IP is newer than the previously known IP. i.updateMostRecentTrackedIP(node, ip) - i.removeGossipableIP(ip.NodeID) return newerTimestamp, node } +func (i *ipTracker) setGossipableIP(ip *ips.ClaimedIPPort, trackedSubnets set.Set[ids.ID]) { + for subnetID := range trackedSubnets { + if subnet, ok := i.subnet[subnetID]; ok && subnet.gossipableIDs.Contains(ip.NodeID) { + subnet.setGossipableIP(ip) + } + } +} + // Disconnected is called when a connection to the peer is closed. func (i *ipTracker) Disconnected(nodeID ids.NodeID) { i.lock.Lock() defer i.lock.Unlock() - i.removeGossipableIP(nodeID) - delete(i.connected, nodeID) -} - -func (i *ipTracker) removeGossipableIP(nodeID ids.NodeID) { connectedNode, ok := i.connected[nodeID] if !ok { return } + delete(i.connected, nodeID) for subnetID := range connectedNode.trackedSubnets { if subnet, ok := i.subnet[subnetID]; ok { @@ -467,11 +477,9 @@ func (i *ipTracker) addGossipableID(nodeID ids.NodeID, subnetID ids.ID, manually return } - if trackedNode, ok := i.tracked[nodeID]; !ok || trackedNode.ip == nil || trackedNode.ip.Timestamp != node.ip.Timestamp { - return + if trackedNode, ok := i.tracked[nodeID]; ok { + subnet.setGossipableIP(trackedNode.ip) } - - subnet.addGossipableIP(node.ip) } func (*ipTracker) OnValidatorWeightChanged(ids.ID, ids.NodeID, uint64, uint64) {} diff --git a/network/ip_tracker_test.go b/network/ip_tracker_test.go index 04f17cf02d0a..93e924ab081e 100644 --- a/network/ip_tracker_test.go +++ b/network/ip_tracker_test.go @@ -544,13 +544,9 @@ func TestIPTracker_AddIP(t *testing.T) { ip: newerIP, expectedUpdatedAndDesired: true, expectedChange: func(tracker *ipTracker) { - tracker.numGossipableIPs.Dec() tracker.tracked[newerIP.NodeID].ip = newerIP tracker.bloomAdditions[newerIP.NodeID] = 2 - - subnet := tracker.subnet[constants.PrimaryNetworkID] - delete(subnet.gossipableIndices, newerIP.NodeID) - subnet.gossipableIPs = subnet.gossipableIPs[:0] + tracker.subnet[constants.PrimaryNetworkID].gossipableIPs[0] = newerIP }, }, { @@ -655,10 +651,17 @@ func TestIPTracker_Connected(t *testing.T) { }, ip: ip, expectedChange: func(tracker *ipTracker) { + tracker.numGossipableIPs.Inc() tracker.connected[ip.NodeID] = &connectedNode{ trackedSubnets: set.Of(constants.PrimaryNetworkID), ip: ip, } + + subnet := tracker.subnet[constants.PrimaryNetworkID] + subnet.gossipableIndices[newerIP.NodeID] = 0 + subnet.gossipableIPs = []*ips.ClaimedIPPort{ + newerIP, + } }, }, { @@ -930,12 +933,18 @@ func TestIPTracker_OnValidatorAdded(t *testing.T) { }, subnetID: constants.PrimaryNetworkID, expectedChange: func(tracker *ipTracker) { + tracker.numGossipableIPs.Inc() tracker.tracked[ip.NodeID].subnets.Add(constants.PrimaryNetworkID) tracker.tracked[ip.NodeID].trackedSubnets.Add(constants.PrimaryNetworkID) tracker.subnet[constants.PrimaryNetworkID] = &gossipableSubnet{ - numGossipableIPs: tracker.numGossipableIPs, - gossipableIDs: set.Of(ip.NodeID), - gossipableIndices: make(map[ids.NodeID]int), + numGossipableIPs: tracker.numGossipableIPs, + gossipableIDs: set.Of(ip.NodeID), + gossipableIndices: map[ids.NodeID]int{ + ip.NodeID: 0, + }, + gossipableIPs: []*ips.ClaimedIPPort{ + newerIP, + }, } }, },