Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove trackedSubnet check for explicitly named peers in network.Send() #3258

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Remove trackedSubnet check for explicitly named peers in network.Send() #3258

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d00f709
Remove trackedSubnet check for explicitly named peers in network.Send()
iansuvak Aug 1, 2024
34f6892
Merge branch 'master' into isuvak/skip-tracked-subnets-check
iansuvak Aug 1, 2024
aebb1a2
Update network/network.go
iansuvak Aug 2, 2024
07b9a0f
Merge branch 'master' into isuvak/skip-tracked-subnets-check
iansuvak Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions network/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,10 +678,10 @@ func (n *network) track(ip *ips.ClaimedIPPort) error {
//
// - [nodeIDs] the IDs of the peers that should be returned if they are
// connected.
// - [subnetID] the subnetID whose membership should be considered if
// [validatorOnly] is set to true.
// - [validatorOnly] is the flag to drop any nodes from [nodeIDs] that are not
// validators in [subnetID].
// - [subnetID] the subnetID whose membership should be considered to
// determine if the node is a validator.
// - [allower] interface that determines if a node is allowed to connect to
// the subnet based on its validator status.
func (n *network) getPeers(
nodeIDs set.Set[ids.NodeID],
subnetID ids.ID,
Expand All @@ -698,10 +698,6 @@ func (n *network) getPeers(
continue
}

if trackedSubnets := peer.TrackedSubnets(); !trackedSubnets.Contains(subnetID) {
continue
}
Comment on lines -701 to -703
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After going through the code - the one potential downside to this PR is:

If a subnet validator forgets to --track-subnet the subnet that it is validating, the nodes querying them will now need to timeout the request rather than immediately marking the request as failed.

I think that is sufficiently niche that it isn't a concern to merge this PR... But it could be something to keep in mind as a future optimization. This is not a change in any security assumptions, as byzantine nodes could always report that they are tracking the subnet but then just never respond to the request.


_, isValidator := n.config.Validators.GetValidator(subnetID, nodeID)
// check if the peer is allowed to connect to the subnet
if !allower.IsAllowed(nodeID, isValidator) {
Expand Down
Loading