New Saturation for health implementation #1195
Conversation
swarm/network/kademlia.go
Outdated
| @@ -798,7 +839,10 @@ func (k *Kademlia) Healthy(pp *PeerPot) *Health { | |||
| gotnn, countgotnn, culpritsgotnn := k.connectedNeighbours(pp.NNSet) | |||
| knownn, countknownn, culpritsknownn := k.knowNeighbours(pp.NNSet) | |||
| depth := depthForPot(k.conns, k.NeighbourhoodSize, k.base) | |||
| saturated := k.saturation() < depth | |||
| // check saturation | |||
| unsaturatedBins := k.getUnsaturatedBins(pp.PeersPerBin, depth) | |||
There was a problem hiding this comment.
Note that now depth is not checked against the length of peersPerBin.
so if there are empty rows just after depth, the node will pass as healthy.
There was a problem hiding this comment.
First create a testcase to show this problem
There was a problem hiding this comment.
I don't understand these comments.
Note that now depth is not checked against the length of peersPerBin.
peersPerBin HAS the length of depth
so if there are empty rows just after depth, the node will pass as healthy.
First create a testcase to show this problem
I can't create a Kademlia with an empty row "after" depth (do you mean shallower? or deeper?)
As far as I understand this is not even possible today anymore, as per definition depth will not allow shallower empty rows?
There was a problem hiding this comment.
the real depth should be compared to the length of PeersPerBin. if they are not then
with only the nearest neighbours connected and having depth == 0, you pass the saturated test!
There was a problem hiding this comment.
Can we mark this as resolved?
swarm/network/kademlia.go
Outdated
| @@ -836,7 +836,7 @@ func (k *Kademlia) GetHealthInfo(pp *PeerPot) *Health { | |||
|
|
|||
| // check saturation | |||
| unsaturatedBins := k.getUnsaturatedBins(pp.PeersPerBin, depth) | |||
| saturated := len(unsaturatedBins) == 0 && depth == len(pp.PeersPerBin) | |||
| saturated := depth == len(pp.PeersPerBin) && len(unsaturatedBins) == 0 | |||
There was a problem hiding this comment.
can we please just put this check in the function, ie. return immediately if it is false?
maybe we should rename the function to checkSaturation and have it return an error.
There was a problem hiding this comment.
Why return an error and not simply bool? There is nothing which can cause an error here.
It is either saturated or not. That is what is currently implemented
swarm/network/kademlia.go
Outdated
| @@ -725,7 +726,11 @@ func (k *Kademlia) getUnsaturatedBins(peersPerBin []int, depth int) []int { | |||
| }) | |||
|
|
|||
| log.Trace("list of unsaturated bins", "unsaturatedBins", unsaturatedBins) | |||
| return unsaturatedBins | |||
| // early check for depth | |||
There was a problem hiding this comment.
this should come BEFORE, if it is false you dont need to call the EachBin thing.
|
Created upstream as ethereum/go-ethereum#19071 |
This PR introduces a new implementation for saturation.
Saturation is a prerequisite to consider a network healthy.
This is reflected with the new
IsHealthyStrict()method.A new function
getUnsaturatedBins()has been added, which will return a list of unsaturated bins (array ofint).It is considered saturated if the length of the array is zero (equivalent to all bins below depth have the saturation criteria fulfilled).
A bin is saturated if we have connections >=
MinBinSizeor, if there are less connections thanMinBinSize, the number of connections corresponds to the available number of peers in the PeerPot.NOTE: The previous
func (k *Kademlia) saturation() inthas been left intact, as it is being used in theOnfunction. It should be evaluated if the two are somewhat conflicting or what should be done about the existingsaturation()functionCloses #1145
Closes #994