les: move server pool to les/vflux/client #22377
Conversation
| @@ -114,11 +114,25 @@ func (h *clientHandler) handle(p *serverPeer) error { | |||
| p.Log().Debug("Light Ethereum handshake failed", "err", err) | |||
| return err | |||
| } | |||
| // Register peer with the server pool | |||
| if h.backend.serverPool != nil { | |||
There was a problem hiding this comment.
Is there any possibility that the serverpool can be nil? Perhaps in the testing?
| // Register peer with the server pool | ||
| if h.backend.serverPool != nil { | ||
| if nvt, err := h.backend.serverPool.RegisterNode(p.Node()); err == nil { | ||
| p.setValueTracker(nvt) |
There was a problem hiding this comment.
Will you consider that we can move the "valueTracker" notion into the vflux/client totally?
IIUC, we just need to feed some statistics for the valueTracker and all the magics can be hidden in the vflux/client. The benefit is we can totally get rid of the nodeValueTracker *vfc.NodeValueTracker in the peer.
There was a problem hiding this comment.
So we can simplify the logic like
// Register peer with the server pool
if err := h.backend.serverPool.RegisterNode(p.Node()); err != nil {
return err
}
defer h.backend.serverPool.UnregisterNode(p.Node())| @@ -349,7 +349,6 @@ type serverPeer struct { | |||
|
|
|||
| fcServer *flowcontrol.ServerNode // Client side mirror token bucket. | |||
| vtLock sync.Mutex | |||
| valueTracker *vfc.ValueTracker | |||
There was a problem hiding this comment.
we can also remove this nodeValueTracker out of the serverPeer.
| @@ -676,9 +675,8 @@ func (p *serverPeer) Handshake(genesis common.Hash, forkid forkid.ID, forkFilter | |||
|
|
|||
| // setValueTracker sets the value tracker references for connected servers. Note that the | |||
| // references should be removed upon disconnection by setValueTracker(nil, nil). | |||
| func (p *serverPeer) setValueTracker(vt *vfc.ValueTracker, nvt *vfc.NodeValueTracker) { | |||
| func (p *serverPeer) setValueTracker(nvt *vfc.NodeValueTracker) { | |||
There was a problem hiding this comment.
We can get rid of this function.
| @@ -732,7 +730,6 @@ func (p *serverPeer) answeredRequest(id uint64) { | |||
| } | |||
| e, ok := p.sentReqs[id] | |||
| delete(p.sentReqs, id) | |||
| vt := p.valueTracker | |||
There was a problem hiding this comment.
This function can be modified a bit, which only returns the serving time. All the value calculation work can be thrown to vflux/client/serverpool.go
| s.ns.SetField(p.Node(), sfiConnectedStats, nvt.RtStats()) | ||
| p.setValueTracker(s.vt, nvt) | ||
| p.updateVtParams() | ||
| func (s *ServerPool) RegisterNode(node *enode.Node) (*NodeValueTracker, error) { |
There was a problem hiding this comment.
We don't need to expose the NodeValueTracker to the outside. Serverpool expects other modules will feed it the necessary statistic for "value calculation". In this way, it's decoupled.
This PR moves the server pool into the
les/vflux/clientpackage where the rest of the peer logic is, leaving packagelesfor client functionality and protocol implementation. The value tracker is also created by the server pool. Only theNodeValueTrackerof individual connected nodes is referenced byserverPeerin order to provide service quality feedback for the peer logic.Replaces #22297
Fixes #22259
Note: a subsequent PR will move the
clientPoolintoles/vflux/server, removing all peerset related complexity and all references toNodeStateMachinefrom packageles.