les: refactor serverPeerSet

[zsfelfoldi]

This PR removes the old serverPeerSet and uses NodeStateMachine to realize the peer set logic of clients, similarly to what has already been done with clientPeerSet. This change makes the code more consistent by removing multiple custom peer maps used by different mechanisms. It also allows further client side feature improvements (mostly related to incentivization).
The NodeStateMachine is also extended with another ForEach iterator that iterates on nodes having a certain field set.
Replaces #22221

[zsfelfoldi]

Now this PR also fixes #22259

[rjl493456442]

I think the "node-state-machine" based peerset is the good direction. The biggest benefit is that it allows the subscription easily. So that we can simplify the codebase a bit.

But I don't like the idea This change makes the code more consistent by removing multiple custom peer maps used by different mechanisms.

I think the peerset can be the global object shared by all the internal components in the LES codebase. But all the component-specific fields should be defined in the component package internally(for example, the fields of lightFetcher). Mixing all the fields in the single serverPeer struct sounds a bad idea.

Just put the general feedback here.

[rjl493456442]

Any particular reason to remove this light fetcher peer Infos here?

AFAIK it's cleaner to maintain the component-specific peer fields in the component inside(e.g. fetcher). And we also have the subscription mechanism so it should be super easy and clean.

While in the current implementation, we incline to put these fields in the shared serverPeer object. It's just too much for it.

[rjl493456442]

I think we are using the "node-state-machine" based peerset in the wrong way.
Essentially it's same with the map-based peerset. While the biggest difference is the nsm peerset supports subscription natively.

Originally, in order to support subscription, we need to implement the registerPeer and unregisterPeer for all the external components(downloader, fetcher, etc). It's annoying. So we should just get rid of these complexity.

In the new peerset, we should define these two methods:

func (ps *serverPeerSet) OnRegister(callback func(node *enode.Node, state nodestate.Flags, peer *serverPeer)) {
	ps.ns.SubscribeField(serverPeerField, func(node *enode.Node, state nodestate.Flags, oldValue, newValue interface{}) {
		if newValue != nil {
			callback(node, state, newValue.(*serverPeer))
		}
	})
}

func (ps *serverPeerSet) OnUnregister(callback func(node *enode.Node, state nodestate.Flags, peer *serverPeer)) {
	ps.ns.SubscribeField(serverPeerField, func(node *enode.Node, state nodestate.Flags, oldValue, newValue interface{}) {
		if oldValue != nil {
			callback(node, state, oldValue.(*serverPeer))
		}
	})
}

So that all the subscribers only need to subscribe the peerset and define the relative actions

peers.OnRegister(func(node *enode.Node, state nodestate.Flags, peer *serverPeer) {})
peers.OnUnregister(func(node *enode.Node, state nodestate.Flags, peer *serverPeer) {})

[rjl493456442]

It's the biggest benefit for this refracting I think(an elegant peerset subscription mechanism).

[rjl493456442]

It looks very weird to me. The nodes is the bitmap for marking all the nodes with this field. It should be initialized in the beginning. However, it's only initialized when used for the first time. Any particular reason for it? If for the memory usage, I don't think it will use a lot.

[rjl493456442]

Would you mind also do some changes to the clientPeerset? It's already replaced with an NSM-based set, would be nice to also define a clientPeerSet wrapper to make the code cleaner.

[zsfelfoldi]

Would you mind also do some changes to the clientPeerset? It's already replaced with an NSM-based set, would be nice to also define a clientPeerSet wrapper to make the code cleaner.

Yes, I'm already working on it.

[fjl]

We discussed this on PR triage today. The issue with this PR is that it makes the peer set much harder to understand. The previous implementation is a totally standard map+lock construction. This new version requires knowing about NSM. @rjl493456442 does not like that.

The main reason why @zsfelfoldi proposed this change is because the server pool should have a more direct notification of peer connection. We can do this in another way.