swarm/network: Add tests for complex connectivity evaluations #1069
Conversation
nolash
requested review from
holisticode,
janos,
acud,
nonsense and
zelig
as code owners
swarm/network/kademlia_test.go
Outdated
| // evaluates healthiness by taking into account potential connections | ||
| // additional conditions for healthiness | ||
| // - IF we know of peers in bins shallower than depth, connected to at least HealthBinSize of them | ||
| func assertHealthPotential(t *testing.T, k *Kademlia, expectHealthyAndPotent bool) error { |
There was a problem hiding this comment.
testing.T is not used in this function. If you just call t.Fatal instead of returning the error, this assert function will be more compact and easier to use in tests.
swarm/network/kademlia_test.go
Outdated
| Register(k, "00000000") | ||
| log.Trace(k.String()) | ||
| if err := assertHealthSimple(t, k, true); err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
Since you pass testing.T to the assert function, you can just call t.Fatal inside and refactor all test code, to have an assert on one line, and not three, when you no longer have to return the error.
There was a problem hiding this comment.
I prefer to know the line it failed :)
swarm/network/kademlia.go
Outdated
| // in order deepest to shallowest compared to the kademlia base address | ||
| depth := depthForPot(k.conns, k.MinProxBinSize, k.base) | ||
| k.eachAddr(nil, depth, func(_ *BzzAddr, po int, _ bool) bool { | ||
| pk[uint8(po)]++ |
There was a problem hiding this comment.
Probably a good idea to extract Po as a type, because we use it as uint8 and int at different places. Or at the very least be consistent about its type.
Doesn't have to be addressed in this PR, but we shouldn't have to do conversions here.
nolash
force-pushed
the
you-fill-me-up
branch
from
11a13dd
to
7b6829a
Compare
|
@nonsense thanks for the comments, this is still WIP though... |
nolash
force-pushed
the
you-fill-me-up
branch
from
d63093a
to
b3de679
Compare
nolash
force-pushed
the
you-fill-me-up
branch
2 times, most recently
from
b3de679
to
a1b0158
Compare
nolash
force-pushed
the
you-fill-me-up
branch
from
ddfd446
to
2a09184
Compare
Pending add test for saturation And add test for as many as possible up to saturation
nolash
force-pushed
the
you-fill-me-up
branch
from
2a09184
to
72dae51
Compare
There was a problem hiding this comment.
Brilliant work!
Very nice tests, IMHO dramatically improves confidence in swarm's network reliance once we have this sorted out.
I bumped into some early questions which seem to have been cleared later on while going through the PR. I left them nevertheless, they might give suggestions for selectively improving the code comments at your discretion.
| } | ||
|
|
||
| // NewKadParams returns a params struct with default values | ||
| func NewKadParams() *KadParams { | ||
| return &KadParams{ | ||
| MaxProxDisplay: 16, | ||
| MinProxBinSize: 2, | ||
| HealthBinSize: 1, |
There was a problem hiding this comment.
Is 1 really a good value here? Do we need a HealthBinSize at all if its default value is 1
There was a problem hiding this comment.
Well it could be 2 in the future. And 1 is not 0.
| type KadParams struct { | ||
| // adjustable parameters | ||
| MaxProxDisplay int // number of rows the table shows | ||
| MinProxBinSize int // nearest neighbour core minimum cardinality | ||
| HealthBinSize int // number of connections out of known peers in a bin to be considered healthy |
There was a problem hiding this comment.
Why do we need a HealthBinSize if we have MinBinSize? What's the role of MinBinSize then? Can't we just use MinBinSize for HealthBinSize?
There was a problem hiding this comment.
MinBinSize is how many peers to we ideally want in a bin (shallower than depth), if they are available.
HealthBinSize is how many peers do we need in a bin to consider the domain shallower than depth healthy.
The former term is not intuitive, perhaps.
|
|
||
| // know three peers, connected to the two deepest | ||
| // healthy but not potent | ||
| Register(k, "00000000") |
There was a problem hiding this comment.
So what is the semantics of impotent? Is it a fatal issue? Is it something which can be ignored, as the test says, its potentially unhealthy?
There was a problem hiding this comment.
The way I understand the current concept, we're not "healthy" if we are not "potent." We need to know and be connected to all neighbours and we need peers in every shallower bin, provided that they exist.
| peer = newTestDiscoveryPeer(addr, k) | ||
| k.On(peer) | ||
| assertHealthSaturation(t, k, 0) | ||
| } |
There was a problem hiding this comment.
Do we maybe need a test where saturation is depth at max?
There was a problem hiding this comment.
You mean that all bins are saturated and what happens then? I think yes, you're right.
| log.Debug("kademlia", "connectNN", h.ConnectNN, "knowNN", h.KnowNN) | ||
| log.Debug("kademlia", "health", h.ConnectNN && h.KnowNN, "addr", hex.EncodeToString(k.BaseAddr()), "node", id) | ||
| log.Debug("kademlia", "ill condition", !h.ConnectNN, "addr", hex.EncodeToString(k.BaseAddr()), "node", id) | ||
| if !h.ConnectNN { |
There was a problem hiding this comment.
Why don't we need to check all the Saturation and Potent etc. members here? .E.g
h.ConnectNN && h.KnowNN && h.CountKnowNN > 0
There was a problem hiding this comment.
Yes, it should probably have Potent. Actually, we should probably discuss with @janos exactly what and how we wait for.
Saturation has to do with discovery of peers, and does not affect "healthy" in the way we use the term.
|
Obsoleted by #1108 |
This PR adds tests for all kademlia connectivity conditions relevant to the peer discovery mechanisms.
This is a preparatory step to verify the correctness of theSuggestPeercomponent.It also adds tests for less naïve health evaluations, to inform simulation implementations that would be interested in this information.
Added tests are:
Merge dependencies (in order of merge to master):
swarm/network: Make MinProxBinSize constant across simulation #1074