Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swarm/network: simplified neighbourhood depth calc #1013

Merged
merged 5 commits into from
Nov 24, 2018

Conversation

acud
Copy link
Member

@acud acud commented Nov 23, 2018

this PR simplifies the neighbourhood depth calculation for the kademlia and reuses the same logic in the peer pot map creation

@@ -451,11 +451,14 @@ func (k *Kademlia) neighbourhoodDepth() (depth int) {
var lastPo int

f := func(v pot.Val, i int) bool {
if bytes.Equal(pot.ToBytes(v), pivotAddr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what condition should we be getting ourselves in these iterations. Isn't that actually an anomaly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nolash this is exactly the same as was removed on the left side L587:

			if po == 256 {	

...
@zelig am I correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe going back to po == 256 is simpler

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justelad the question is about the iterator, not the value itself. Should the iterator ever return ourselves?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question. i guess not since the method name is EachNeighbour and self is not a neighbour. i'll make the change. good point 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nolash, come to think of it, that method name could be actually changed to EachPeer, since it refers to actual peers connected, not just neighbours.
@zelig, I noted to @nolash that some of the terminology in the kademlia is somewhat misleading (as I already told you about minProxBinSize), EachNeighbour is the same case - we iterate over peers not neighbours, the iterator in this case should not have a notion of what is neighbouring and what is not, and that is up to the iteratee to assert. (my 2 cents)

swarm/network/kademlia.go Outdated Show resolved Hide resolved
swarm/network/kademlia.go Outdated Show resolved Hide resolved
swarm/network/kademlia.go Outdated Show resolved Hide resolved
swarm/network/kademlia.go Outdated Show resolved Hide resolved
swarm/network/kademlia.go Outdated Show resolved Hide resolved
swarm/network/kademlia.go Show resolved Hide resolved
@nolash
Copy link
Contributor

nolash commented Nov 24, 2018

@justelad Tests fail; neighbourhoodDepth calls not replaced with depthForPot it seems...

I pushed a minor amend in comments, hope it's ok.

@acud acud force-pushed the kad-depth-calc branch 2 times, most recently from 8d2609c to fa8e0f1 Compare November 24, 2018 10:11
Copy link
Contributor

@nolash nolash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved provided tests pass

@nolash nolash merged commit f316495 into kademlia-neighbourhooddepth Nov 24, 2018
@acud acud deleted the kad-depth-calc branch January 16, 2019 13:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants