Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p2p - dht to addrbook #890

Merged
merged 11 commits into from May 14, 2019
Merged

p2p - dht to addrbook #890

merged 11 commits into from May 14, 2019

Conversation

y0sher
Copy link
Contributor

@y0sher y0sher commented May 12, 2019

No description provided.

)

// DHT is an interface to a general distributed hash table.
type Interface interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use api or something , because interface is a golang keyword i think it will be clearer

}

// SelectPeers asks routing table to randomly select a slice of nodes in size `qty`
func (d *Discovery) SelectPeers(qty int) []node.Node {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if no peers are found? is this expected and normal behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this is handled because we might actually have an empty table


// Failed attempts deprioritise.
for i := ka.attempts; i > 0; i-- {
c /= 1.5
Copy link
Contributor

Choose a reason for hiding this comment

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

c= c /( 1.5 ^ attempts)

}
}

func (r *refresher) Bootstrap(ctx context.Context, num int) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename num


if size-len(r.bootNodes) < num {
r.logger.Info("Bootstrap: starting with %v sized table", size)
res := r.refresh(servers)
Copy link
Contributor

Choose a reason for hiding this comment

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

refresh does not imply anything about what this function actually does. it gets more peers so i'd call it something like refreshPeerList or something

servers = r.bootNodes
}

if size-len(r.bootNodes) < num {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd refactor this to `if size < num + len' to negative values for size


now := time.Now()

maxPending := 3
Copy link
Contributor

Choose a reason for hiding this comment

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

move to const


}

for _, b := range r.bootNodes {
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment why this is happening

tries := 0
servers := make([]discNode, 0, len(r.bootNodes))
loop:
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be very happy if you write a short comment on how things work here

return err
}

func expire(m map[p2pcrypto.PublicKey]time.Time) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function will evict only one item on each call, is this wanted behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

}

// pingThenFindNode is sending a ping, then find node, then return results on given chan.
func pingThenFindNode(p Protocol, addr discNode, qr chan queryResult) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to pingAndGetAddresses

if err != nil {
return nil, err
}

// response handler
ch := make(chan []discNode)
foo := func(msg []byte) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename foo

p.logger.Info("handle find_node response")
data := &pb.FindNodeResp{}
//p.logger.Info("handle find_node response from %v", serverNode.String())
data := &pb.Addresses{}
if err := proto.Unmarshal(msg, data); err != nil {
p.logger.Error("could not unmarshal block data")
Copy link
Contributor

Choose a reason for hiding this comment

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

rename error

"net"
)

type discNode struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider renaming to NodeAddr or NodeInfo

}

// refresh will crawl the network looking for new peer addresses.
func (r *refresher) refresh(addrs []discNode) []discNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider renaming to refreshAddresses

@y0sher y0sher merged commit 28b8653 into develop May 14, 2019
@y0sher y0sher deleted the p2p_dht_to_addrbook branch June 25, 2019 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants