network: move hive message handler from Peer type

[acud]

This PR moves the Hive protocol's message handlers from the Peer type to the Hive type.

[nolash]

Is the code just moved except weeding the kademlia member out? If its actually changed it's better if it is changed in place first then move blocks of code.

As a general comment I don't like the approach that the hive is a hybrid service which both manages the node's own connectivity and is a service for distributing peer information to others in the network. One is a network service, the other for internal consumption. I would have liked to see a SWIP and a discussion on how this separation of concerns can best be achieved. It was also in the pipeline for the outstanding kademlia tasks that were agreed with Epic Labs, but lower in priority.

[acud]

As a general comment I don't like the approach that the hive is a hybrid service which both manages the node's own connectivity and is a service for distributing peer information to others in the network. One is a network service, the other for internal consumption. I would have liked to see a SWIP and a discussion on how this separation of concerns can best be achieved. It was also in the pipeline for the outstanding kademlia tasks that were agreed with Epic Labs, but lower in priority.

I was never a part of this discussion, then I guess it's safe to say I have no clue what you're talking about.

Is the code just moved except weeding the kademlia member out? If its actually changed it's better if it is changed in place first then move blocks of code.

I was assuming this comment will arrive sooner or later.
I'll pick moving it first, only then adding the kick functionality since it is nicer to implement when the handlers are on hive (because the current node needs to know if it is a bootnode, and I don't wanna pass this information to the Peer struct). That's why I did both in one go.

[acud]

can we please approve this very simple PR so I can go ahead and implement the bootnode kicks after n nodes functionality? or do we want to scratch this completely? hand it over to epic lab guys? please clarify @nolash @zelig I don't like having PRs open for months

[janos]

Lint error: https://travis-ci.org/ethersphere/swarm/jobs/597163175#L493

[nolash]

@acud Can you please let me know how I should go about comparing the changes here? If these are very few actual changes then perhaps you could list them all in a bullet point list? I do not want to rubber-stamp a PR just because it is claimed to be simple, when I can't really evaluate what has changed because the changes have been buried in moves between files. Sorry if it's a pain in the ass <3

[acud]

Señor @nolash, just for you:

1. discovery.go has been renamed to peer.go (i used git move and i'm not sure why it's not showing up as a move instead of complete erase of discovery.go)
2. discovery_test.go renamed to peer_test.go
3. message handlers from peer.go have been moved to hive.go

I guess you could go about comparing bit by bit by checking out the branch then comparing with:

git checkout bootnote-kicks-ass
git checkout master -- network/discovery.go
diff network/discovery.go network/peer.go

then compare the output of the diff with what was added to hive.go in the PR diff.

Is this informative enough on how to go about reviewing this?

[nolash]

@acud Thanks for clarifying. I see you forgot to include that you deleted the NotifyDepth function that didn't have a receiver. I also see that tests are failing, and that symbol is mentioned.

It's not that I don't believe you and trust you when you say this and this was done. But if we can rely on that we never do code moves with changes in the same commit, we can also have the extra security of being able to audit all changes. Otherwise there is always a chance something may have snuck in (especially when it comes to larger PRs).

For my part I don't mind if the moves are within a single PR, as long as the move itself without any changes is in a single commit, and that commit is referenced in the description. But in my opinion the best is a separate PR for code block moves.

Please fix the test, and I'll approve. Thanks.

[acud]

@nolash the PR is now in diminished state since the dreadful discovery tests started failing with the last handler changes (and i'd really be happy to not go into this rabbithole of debugging them for 2 months). and I think you can even review the changes without using any of your favorite command line utilities.
this code is still very fragile and the test vectors are impossible to debug (if you're a puny human, that is)

[nolash]

I'm assuming this is now only moving code blocks, haven't looked for or checked code changes.