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

Kademlia fixes #379

Merged
merged 7 commits into from
Apr 12, 2018
Merged

Kademlia fixes #379

merged 7 commits into from
Apr 12, 2018

Conversation

janos
Copy link
Member

@janos janos commented Apr 9, 2018

This PR implements:

  • Fixes for Kademlia SuggestPeer and full methods
  • Updates Kademlia tests to address the changes
  • Adds tests TestKademliaCase* for validating found problems
  • Changes NewPeerPot to NewPeerPotMap and removes the need for discover.NodeID slice
  • Updates swarm/network/stream/testing snapshots with the ones generated with the updated kademlia SuggestPeer
  • Fixes minor lint errors

@janos janos changed the title [WIP] [Do not merge] Kademlia fixes Kademlia fixes Apr 10, 2018
@janos janos requested review from zelig and nolash April 10, 2018 11:55
@@ -677,20 +677,33 @@ func (k *Kademlia) saturation(n int) int {
func (k *Kademlia) full(emptyBins []int) (full bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an explanation of the algorithm used here.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, as we talked about in the call, please add to the comment the definition of a node being healthy when "as healthy as can be," that is that it can have 0 connected peers if no peers are known in the bin. That's what threw me off, at least.

if bytes.Equal(a, addr) {
continue
}
p := &BzzAddr{OAddr: a, UAddr: a}
Copy link
Contributor

Choose a reason for hiding this comment

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

should oaddr and uaddr be the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

They can be for this test.

}

/*
The regression test for the following invalid kademlia edge case.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are exactly these cases selected, and where do they come from? Is there any specific logic behind the configurations?

Copy link
Member Author

Choose a reason for hiding this comment

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

This cases are the ones discovered as the edge cases while working on other tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, please add a comment saying that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added the comments. Would more details then this help?

log.Debug(r.delivery.overlay.String())
log.Debug(fmt.Sprintf("IS HEALTHY: %t", h.GotNN && h.KnowNN && h.Full))
log.Error(r.delivery.overlay.String())
log.Error(fmt.Sprintf("IS HEALTHY: %t", h.GotNN && h.KnowNN && h.Full))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Debug or Trace?

@zelig zelig merged commit 6f8f818 into swarm-network-rewrite Apr 12, 2018
@janos janos deleted the kademlia-fixes branch April 12, 2018 14:18
@janos janos restored the kademlia-fixes branch April 16, 2018 08:56
@janos janos deleted the kademlia-fixes branch April 16, 2018 09:43
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