The Peer is Dead. Long live the ID #466

jbenet · 2014-12-21T14:04:11Z

This PR is a massive refactor, removing peer.Peer
everywhere and using peer.ID instead. This refactor
also revealed a number of bugs throughout that i
fixed. Sadly, i'm sure i'll introduce some-- this is
a pretty large refactor. The upside is that everything
is saner when it comes to peers and the peerstore.
They feel much better now.

Note: I will squash most of this PR so that it is
bisectable.

@maybebtc @whyrusleeping could use CR. multiple eyes
looking over it would be ideal.

jbenet · 2014-12-21T14:21:40Z

@maybebtc specifically, there's a test case inside bitswap/testnet that confusingly panics. help!

btc · 2014-12-21T15:12:08Z

core/bootstrap.go

-		return nil, err
+		return
+	}
+	p = peer.PeerInfo{


why a PeerInfo rather than peerstore.Addresses(pid)?

i could switch it to peer.ID. i thought (id, addr) bundle was wanted, but turned out it wasn't. either way

btc · 2014-12-21T15:32:39Z

Did this take care of #463 ?

btc · 2014-12-21T15:48:21Z

@maybebtc specifically, there's a test case inside bitswap/testnet that confusingly panics. help!

Super confused by this. Digging deeper...

edit: a2f321f

btc · 2014-12-21T16:29:38Z

core/core.go

-		if err := self.LoadAndVerifyKeyPair(skb); err != nil {
-			return nil, err
+		var id2 peer.ID
+		id2, err = peer.IDFromPrivateKey(sk)


is sk a bug? should it be sk2?

jbenet · 2014-12-21T22:00:32Z

Did this take care of #463 ?

Not yet.

edit: I was planning on rebasing it on top of this.

jbenet · 2014-12-21T22:03:21Z

Super confused by this. Digging deeper...
edit: a2f321f

👏 👏 👏

Hard to figure out what was wrong. :/ the lambdas made it hard to know where things were actually panicing.

whyrusleeping · 2014-12-22T00:46:16Z

routing/dht/dht.go

+	// commenting out for now. i'm not sure this is correct -jbenet
+	// // TODO decide on providers. This probably shouldn't be happening.
+	// // TODO what did "this" mean? i think it's the notion of receiving
+	// // a list of providers and then immediately requesting that value


Yeah, youre right on this. We can remove this code entirely

whyrusleeping · 2014-12-22T00:59:02Z

Theres a lot of changes here, but skimming through it all, it looks pretty good to me.

jbenet · 2014-12-22T01:00:57Z

Theres a lot of changes here, but skimming through it all, it looks pretty good to me.

this is one where careful look would be hugely valuable. a ton of important changes. @maybebtc already caught a couple errors, im sure I've made more. I'll look over it again, but more pairs of eyes would help.

whyrusleeping · 2014-12-22T04:53:49Z

routing/mock/mockrouting_test.go

@@ -102,8 +100,8 @@ func TestCanceledContext(t *testing.T) {
 	i := 0
 	go func() { // infinite stream
 		for {
-			peer := testutil.NewPeerWithIDString(string(i))
-			err := rs.Client(peer).Provide(context.Background(), k)
+			pi := peer.PeerInfo{ID: peer.ID(i)}


I know this is a refactor, but it looks like this isnt being cleaned up after

what do you mean? (not seeing it)

The infinite loop in a goroutine, that goroutine wont ever exit. which i guess is okay for tests, but they do build up when you do go test ./...

Ahh, good catch, thanks 4b15204

whyrusleeping · 2014-12-22T06:42:04Z

filing a complaint against the title, should be "The Peer is dead, Long live the Peer!"

whyrusleeping · 2014-12-22T21:43:18Z

Alright, read over it all. It does all look pretty good with the changes youve made.

btc · 2014-12-23T09:06:42Z

Would you be down to squash this so everything builds?

jbenet · 2014-12-23T09:25:43Z

@maybebtc not everything, but yes the non building things.

jbenet · 2014-12-23T09:26:31Z

@maybebtc help me fix the test first though -- oh damn i've been posting on the wrong issue. #463 (comment) should be here

Reproduced here:

Now i'm getting this.

client_1    | 06:00:10.279 DEBUG        dht: Run query with 0 peers. query.go:110
client_1    | 06:00:10.279 WARNI        dht: Running query with no peers! query.go:112```

Full log here: https://gist.github.com/jbenet/34db7858b7849153197f

Another interesting portion:

server_1    | 06:00:11.314 DEBUG  merkledag: DagService Add [QmQvvQQAH8gjFFDLhwXApUqJQxXK74x9kT3L8wxuKdAuE3] merkledag.go:188
server_1    | 06:00:11.315 DEBUG blockservi: blockservice: storing [QmQvvQQAH8gjFFDLhwXApUqJQxXK74x9kT3L8wxuKdAuE3] in datastore blockservice.go:44
server_1    | 06:00:11.325 DEBUG  merkledag: DagService Add [QmeWv547H72kytwg8KvBjeUgDonDhobhxZ5FqpQwV6dCo5] merkledag.go:188
server_1    | 06:00:11.325 DEBUG blockservi: blockservice: storing [QmeWv547H72kytwg8KvBjeUgDonDhobhxZ5FqpQwV6dCo5] in datastore blockserviStopping dockertest_server_1...
Stopping dockertest_data_1...
Stopping dockertest_bootstrap_1...
ce.go:44
server_1    | 06:00:11.333 DEBUG  merkledag: DagService Add [QmV6Lubap8eeGJP4ef34K4Q7X814p7sspUwxdpvttWRRmL] merkledag.go:188
server_1    | 06:00:11.334 DEBUG blockservi: blockservice: storing [QmV6Lubap8eeGJP4ef34K4Q7X814p7sspUwxdpvttWRRmL] in datastore blockservice.go:44
server_1    | 06:00:11.350 DEBUG  merkledag: DagService Add [QmQcYuUyhmNcRk6BTzhZ8cQdomipebz5BquboELLkafkaA] merkledag.go:188
serv

dockertest_data_1 and dockertest_bootstrap_1 stopped well before the server stops adding. the client dies much later, after the server is done, when its context expires.

jbenet · 2014-12-23T09:36:03Z

the plot thickens, from the output it looks like the nodes aren't connecting.

jbenet · 2014-12-23T09:37:33Z

Oh silliness, bitten by my own bug, i should be debugging on #463 as that fixes the bootstrapping. sec.

License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

@jbenet

cc @jbenet @whyrusleeping License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

"Get" is still fairly useful. Leaving it there. License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

@jbenet

https://build.protocol-dev.com/job/race/9352/console @jbenet @whyrusleeping pinging you guys to spread awareness about the delay.D type for configurable delays License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

network.ListenAddresses() are general. Interface addresses are specific.

Had to change the network interface from DialPeer(peer.ID) to DialPeer(peer.PeerInfo), so that addresses of a provider are handed to the network. @maybebtc and I are discussing whether this should go all the way down to the network, or whether the network _should always work_ with just an ID (which means the network needs to be able to resolve ID -> Addresses, using the routing system. This latter point might mean that "routing" might need to break down into subcomponents. It's a bit sketchy that the Network would become smarter than just dial/listen and I/O, but maybe there's a distinction between net.Network, and something like a peernet.Network that has routing built in...)

This reverts commit bf88f1a.

@whyrusleeping

cc @whyrusleeping

@jbenet

…outing @jbenet @whyrusleeping the next commit will change bitswap.Network.FindProviders to only deal with IDs

@jbenet

@jbenet @whyrusleeping This commit replaces peer.PeerInfo with peer.ID in the bitswap package

The Peer is Dead. Long live the ID

jbenet · 2014-12-23T17:46:05Z

Warning: the dht doesn't work. Merging this in as master already doesn't work (bootstrap bug). (please use the stable branch for an older commit on the old net that still works.)

To update you, @whyrusleeping tests pass, but the docker test doesn't. And, try spinning up three instances like below. resolving doesn't work, and not sure why.

fix(dialqueue): fix a timer leak

jbenet added the status/in-progress In progress label Dec 21, 2014

jbenet mentioned this pull request Dec 21, 2014

jbenet's log ipfs-inactive/starlog#2

Closed

btc reviewed Dec 21, 2014
View reviewed changes

jbenet force-pushed the peer-restrict branch from 934d5a4 to 683e23c Compare December 21, 2014 22:02

jbenet force-pushed the peer-restrict branch from 683e23c to 0d760a9 Compare December 21, 2014 22:09

whyrusleeping reviewed Dec 22, 2014
View reviewed changes

jbenet mentioned this pull request Dec 22, 2014

rewrite of provides to better select peers to send RPCs to #456

Merged

jbenet mentioned this pull request Dec 22, 2014

Bootstrapping + Connectedness Fix #463

Closed

jbenet force-pushed the peer-restrict branch from 17fe066 to 5e475e1 Compare December 23, 2014 09:33

btc force-pushed the peer-restrict branch from 897e595 to 4d685c2 Compare December 23, 2014 12:23

threadsafe SeededRand

cc7a869

jbenet and others added 22 commits December 23, 2014 08:53

net/conns better printing of connections

b61b281

test(docker/network) disable color in daemon logs

d5789de

License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

remote low SnR debug statement

b19a931

cc @jbenet @whyrusleeping License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

rm low SnR debug statement

e3f8f8c

"Get" is still fairly useful. Leaving it there. License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

misc naming

06d4df2

License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

fix: data race in test

3f9c126

https://build.protocol-dev.com/job/race/9352/console @jbenet @whyrusleeping pinging you guys to spread awareness about the delay.D type for configurable delays License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

connectedness test timing

0dbd86c

listen addresses: add Interface addresses

64a4abc

network.ListenAddresses() are general. Interface addresses are specific.

dht: helpful debugging for no closer peers

a7eb52a

Revert "bitswap: network interface changed"

55c8f8f

This reverts commit bf88f1a.

fix(bitswap) always dial

d0d220f

fix(bitswap) always use prvivate send method to send

fb75184

cc @whyrusleeping

style(bitswap) rename

4ab4da7

style(bitswap) public methods at top

a225568

feat(bitswap/network) expose peerstore

60f3a87

style(bitswap) rename to network

070b633

fix(bitswap) always add addresses

c629464

refactor(bitswap) bitswap.Network now abstracts ipfs.Network + ipfs.R…

27dc959

…outing @jbenet @whyrusleeping the next commit will change bitswap.Network.FindProviders to only deal with IDs

refactor(bitswap) change PeerInfo to ID in bitswap package

c34132e

@jbenet @whyrusleeping This commit replaces peer.PeerInfo with peer.ID in the bitswap package

chore(bitswap) remove Peerstore() methods from bitswap.Network interface

bd45adf

net/mock: interface addresses

a5018fc

jbenet force-pushed the peer-restrict branch from 4d685c2 to a5018fc Compare December 23, 2014 17:28

jbenet added a commit that referenced this pull request Dec 23, 2014

Merge pull request #466 from jbenet/peer-restrict

32589ad

The Peer is Dead. Long live the ID

jbenet merged commit 32589ad into master Dec 23, 2014

jbenet removed the status/in-progress In progress label Dec 23, 2014

jbenet deleted the peer-restrict branch December 23, 2014 17:44

btc mentioned this pull request Dec 23, 2014

networked add/cat is not functional (master) #468

Closed

2 tasks

ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021

Merge pull request ipfs#466 from libp2p/fix/timer-leak

3eac211

fix(dialqueue): fix a timer leak

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Peer is Dead. Long live the ID #466

The Peer is Dead. Long live the ID #466

jbenet commented Dec 21, 2014

jbenet commented Dec 21, 2014

btc Dec 21, 2014

jbenet Dec 21, 2014

btc commented Dec 21, 2014

btc commented Dec 21, 2014

btc Dec 21, 2014

jbenet commented Dec 21, 2014

jbenet commented Dec 21, 2014

whyrusleeping Dec 22, 2014

whyrusleeping commented Dec 22, 2014

jbenet commented Dec 22, 2014

whyrusleeping Dec 22, 2014

jbenet Dec 22, 2014

whyrusleeping Dec 22, 2014

jbenet Dec 22, 2014

whyrusleeping commented Dec 22, 2014

whyrusleeping commented Dec 22, 2014

btc commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014

The Peer is Dead. Long live the ID #466

The Peer is Dead. Long live the ID #466

Conversation

jbenet commented Dec 21, 2014

jbenet commented Dec 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btc commented Dec 21, 2014

btc commented Dec 21, 2014

Choose a reason for hiding this comment

jbenet commented Dec 21, 2014

jbenet commented Dec 21, 2014

Choose a reason for hiding this comment

whyrusleeping commented Dec 22, 2014

jbenet commented Dec 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Dec 22, 2014

whyrusleeping commented Dec 22, 2014

btc commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014

jbenet commented Dec 23, 2014