Subscription filter

[synzhu]

Implements subscription filter. Will not work 100% until libp2p/go-libp2p-pubsub#449 is fixed.

https://github.com/dapperlabs/flow-go/issues/5801

[huitseeker]

This code LGTM.

@smnzhu there's a CI unit test failure on TestCommandRunner that results in a nil dereference. Is there any chance you'd have time to look into it? My hunch is the cleanup / shutdown of the admin server might run first while leaving pending command, so that they probably hit a cleaned up (nil) runner in the end.

[yhassanzadeh13]

Overall clean and neat, just a few comments to preserve the extensibility.

Also, on a separate note, I could not find implementation accounting for https://github.com/dapperlabs/flow-go/issues/5861 in this PR. That is fine, but just wanted to make sure https://github.com/dapperlabs/flow-go/issues/5861 is not a duplicate.
Basically, the subscription filter does not prevent an engine to wrongfully subscribe to a topic that it is not supposed to do, e.g., a consensus node can try registering itself on request-chunks. The aim of https://github.com/dapperlabs/flow-go/issues/5861 is to prevent such scenarios that arising of channelRoleMap getting out of sync or distorted and seems https://github.com/dapperlabs/flow-go/issues/5861 and this PR are serving quite orthogonal purposes. I would be happy to take https://github.com/dapperlabs/flow-go/issues/5861 and apply a fix.

[yhassanzadeh13]

I like the general idea of defining these filter functions (e.g., ClusterChannelRoles), however, I would still recommend keeping the information-centric architecture, i.e., having channelRoleMap as the source-of-truth and then defining these filter functions on top of it, e.g.,

func ClusterChannelRoles() flow.RoleList {
	clusterRoles := flow.RoleList{}
	for channel, roles := range channelRoleMap{
		if _, ok := ClusterChannel(channel); ok{
			clusterRoles.Union(roles)
		}
	}
	
	return clusterRoles
}

By keeping the design information-centric around channelRoleMap instead of ad-hoc functions with hard-coded parameters, we preserve the safety of extensibility in a longer-term outlook.

[synzhu]

right, but channelRoleMap doesn't make sense for cluster-based channels imo because they are unique to the chain ID. ie, the value stored in channelRoleMap is not actually a "real" channel

[synzhu]

I think the real solution is to remove ChannelSyncCluster() and ChannelConsensusCluster entirely, and simply redefine TopicFromChannel to take in a chain ID in addition to a RootBlockID instead. Or to go even further, we can define a closure or a struct ChannelToTopicConverter which does the conversions.

Then we'd be able to rename syncClusterPrefix to simply SyncCluster (similary for consensus) and add it to the channel role map.

[yhassanzadeh13]

ID() method is quite resource-heavy, and that is why we kept it out of the loop there. I could not find any reason we need to recompute it over each iteration. If that is the case, please move it to its old place out of the loop.

[synzhu]

The old place is already inside the loop 🧐

I have only placed it inside the if, which is only saving computation because it will be computed less often.

[yhassanzadeh13]

Similar to my earlier comment about keeping channelRoleMap as the source of truth, I would suggest not to hard code these in the body of functions. Instead, we can have a naming convention that all channels starting with public are public channels, and then do:

// all public channels should start with "public-" prefix.
const publicChannelPrefix = "public-"

// PublicChannels returns all channels that on the public network.
func PublicChannels() network.ChannelList {
	publicChannels := network.ChannelList{}
	for channel := range channelRoleMap {
		if strings.HasPrefix(channel.String(), publicChannelPrefix) {
			publicChannels = append(publicChannels, channel)
		}
	}
	return publicChannels
}

[synzhu]

Imo, channelRoleMap is only relevant to the channels where roles matter, ie private channels.

Public channels can be joined by anyone, regardless of role (and even unstaked nodes who don't have a role whatsoever), so I think it makes sense to treat them separately here.

[vishalchangrani]

shouldn't we have two different filter. One which all the staked node use and one which the full observer node uses and the Staked AN that support the observer node use?

[synzhu]

We could do this, the at a high level, the logic for the subscription filter goes:

A node should only be allowed to subscribe to some set of channels based on its role.

This general rule is the same for any node type, so we can implement the exact same logic for all of them

[vishalchangrani]

some nits and a question but otherwise lgtm

[synzhu]

Overall clean and neat, just a few comments to preserve the extensibility.

Also, on a separate note, I could not find implementation accounting for dapperlabs/flow-go#5861 in this PR. That is fine, but just wanted to make sure dapperlabs/flow-go#5861 is not a duplicate.
Basically, the subscription filter does not prevent an engine to wrongfully subscribe to a topic that it is not supposed to do, e.g., a consensus node can try registering itself on request-chunks. The aim of dapperlabs/flow-go#5861 is to prevent such scenarios that arising of channelRoleMap getting out of sync or distorted and seems dapperlabs/flow-go#5861 and this PR are serving quite orthogonal purposes. I would be happy to take dapperlabs/flow-go#5861 and apply a fix.

So originally, I never suggested that dapperlabs/flow-go#5861 is a duplicate, but actually I would like to clarify that the subscription filter does prevent an engine from wrongfully subscribing to a topic that it is not supposed to, in fact this is explicitly tested in the unit test.

It wasn't clear to me based on the wording what exactly the goal of dapperlabs/flow-go#5861 was, but if the goal is to ensure that nodes only subscribe to the appropriate channels based on their role, then the subscription filter does ensure this.

[synzhu]

bors merge

[bors]

Canceled.

[synzhu]

bors merge

[bors]

Build succeeded:

• Integration Tests (1.15)
• Integration Tests (1.16)
• Lint
• Unit Tests (1.15)
• Unit Tests (1.16)