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

whyrusleeping · 2014-12-16T05:06:29Z

The main change in this PR is the implementation of getClosestPeers that returns a channel of peers. progressively closer to the target key. This is used to put provider values to the best possible peers.

whyrusleeping · 2014-12-16T05:08:10Z

This isnt ready for CR yet, i just wanted to run the tests on jenkins.

whyrusleeping · 2014-12-22T00:31:47Z

@jbenet @maybebtc im ready for a bit of CR on this one.

jbenet · 2014-12-22T05:44:57Z

crypto/key.go

@@ -74,10 +75,10 @@ type PubKey interface {
 type GenSharedKey func([]byte) ([]byte, error)

 // Generates a keypair of the given type and bitsize
-func GenerateKeyPair(typ, bits int) (PrivKey, PubKey, error) {
+func GenerateKeyPair(typ, bits int, src io.Reader) (PrivKey, PubKey, error) {


wait why do you want to pass this in? it should always be crypto/rand.Reader. cannot trust any other randomness-- exposing it like this will likely create problems elsewhere. I suggest the following instead: inside of testutil add a GenerateTestKeyPair, which uses the time seeded rand. the interface to the rest of the codebase remains the same (no passing in rand), but tests can opt to use the faster one.

I guess that works too, I figured id just have our function mimic the interface of the RSA key generator and have it accept an io.Reader

Fair point. Looking over it again looks like we dont generate keys many places -- that is, after the changes in #466 ... -- so not a big deal to introduce this Reader. might want to rebase on that sooner

Sorry, thinking about it more, i think this is the right thing: https://github.com/jbenet/go-ipfs/pull/456/files#r22301316

btc · 2014-12-23T19:56:54Z

@whyrusleeping you'll probably want to get looped in on #468

whyrusleeping · 2014-12-24T07:38:40Z

Alright, got it rebased properly

whyrusleeping · 2014-12-25T00:17:49Z

@jbenet @maybebtc Can you guys take a look at this? It improves the correctness of the DHT and makes the DHT tests more predictable. I could use another set of eyes over the changes ive done.

btc · 2014-12-25T05:12:41Z

It improves the correctness of the DHT

Are we sure? Just ran the network tests locally. Not passing yet. https://build.protocol-dev.com/job/network-test/336/

whyrusleeping · 2014-12-25T06:41:32Z

@maybebtc i discovered some fun new bugs after rebasing on master again... Ill have those fixes up soon. But theres one very weird one, where network.DialPeer is hanging...

btc · 2014-12-25T06:43:16Z

@whyrusleeping we may want to merge #470 before anything else. it adds a go test that reproduces the docker network test failure we've been seeing.

whyrusleeping · 2014-12-25T06:45:09Z

@maybebtc oh, that will be awesome. Is it RFCR?

whyrusleeping · 2014-12-25T07:32:36Z

alright, so running go test ./routing/dht/ -v -run TestProvides reproduces the error im encountering. It looks to me like network.DialPeer is hanging inside of the ResponseHandler.

@jbenet could you take a look at this when you have time? Im very unfamiliar with that bit of the codebase ( @maybebtc if youre knowledgable in that area you should take a look too)

jbenet · 2014-12-25T16:59:19Z

Oh the default network is going to try to run ProtocolIdentify. Some of the tests may not include a full network (though I think they do) and only have a dht handler. Maybe that's the issue you're seeing. If both networks are made with NewNetwork (or mocknet even) it should be fine.

—
Sent from Mailbox

On Wed, Dec 24, 2014 at 11:32 PM, Jeromy Johnson [email protected]
wrote:

alright, so running go test ./routing/dht/ -v -run TestProvides reproduces the error im encountering. It looks to me like network.DialPeer is hanging inside of the ResponseHandler.

@jbenet could you take a look at this when you have time? Im very unfamiliar with that bit of the codebase ( @maybebtc if youre knowledgable in that area you should take a look too)

Reply to this email directly or view it on GitHub:
#456 (comment)

jbenet · 2014-12-28T12:13:32Z

@whyrusleeping sorry about that... there was indeed a bug in how streams were being handled. If you're curious: jbenet/go-peerstream@fc6b2a9 -- AFAICT, that hang is fixed. thanks for pushing me to find this and sorry we kept hammering our heads against the wall on this silly bug!

jbenet · 2014-12-28T12:16:38Z

Though travis succeeded, re-running multiple times I sometimes get:

--- FAIL: TestLayeredGet (0.38 seconds)
    dht_test.go:501: interface was changed. GetValue should not use providers.
    dht_test.go:505: <nil>
    dht_test.go:508: should not get value.

whyrusleeping · 2014-12-28T18:46:08Z

@jbenet i get that too... I dont really trust the test itself anymore.

jbenet · 2014-12-28T23:49:40Z

@whyrusleeping fixed it in 0a24d92

I still see occasional:

--- FAIL: TestGetFailures (4.06 seconds)
    ext_test.go:50: Timeout test passed.
    ext_test.go:77: Expected ErrNotFound, got: context deadline exceeded

Which -- under proper functioning -- should not happen. ErrNotFound should be returned well before the ctx expires.

jbenet · 2014-12-28T23:52:10Z

@whyrusleeping there's also a ton of extra logging as ERROR that shouldn't be there, or should be bumped down to DEBUG.

jbenet · 2014-12-29T00:46:29Z

cmd/ipfs/init.go

@@ -252,7 +253,7 @@ func identityConfig(nbits int) (config.Identity, error) {
 	}

 	fmt.Printf("generating key pair...")
-	sk, pk, err := ci.GenerateKeyPair(ci.RSA, nbits)
+	sk, pk, err := ci.GenerateKeyPair(ci.RSA, nbits, rand.Reader)


can we break this out into:

ci.GenerateKeyPairWithReader(...) ci.GenerateKeyPair(...) // calls GenerateKeyPairWithReader with crypto/rand testutil.NewTestKeyPair(...) // calls GenerateKeyPairWithReader with NewTimeSeededRand()

Tons of security bugs happen because someone somewhere called a function with the wrong params (something most compilers don't guard well against). I want to minimize the surface area. Particularly if people will be building the habit of using test keys with weak pseudorandomness.

ci.GenerateKeyPairWithReader(...)

👍

alright, makes sense

btc · 2014-12-29T10:59:17Z

routing/dht/dht.go

@@ -238,12 +237,12 @@ func (dht *IpfsDHT) Update(ctx context.Context, p peer.ID) {
 }

 // FindLocal looks for a peer with a given ID connected to this dht and returns the peer and the table it was found in.
-func (dht *IpfsDHT) FindLocal(id peer.ID) (peer.PeerInfo, *kb.RoutingTable) {
+func (dht *IpfsDHT) FindLocal(id peer.ID) peer.PeerInfo {
 	p := dht.routingTable.Find(id)


low priority but the latter would make more sense than the former

func (rt *RoutingTable) Find(id peer.ID) peer.ID

func (rt *RoutingTable) Exists(id peer.ID) bool

Yeah, thats leftover from when it returned a peer.Peer, fairly pointless after the refactor

btc · 2014-12-29T16:08:59Z

A general observation...

Overall, would benefit from clearer interface specification. Each function performs some actions, but doesn't clearly address its spec/contract. Important considerations:

How do parameters affect the behavior of the function?
What are the possible return values? Does the function return any named errors? (impl concern: Are the right errors returned in each case?)
What guarantees are provided?
Are there any side-effects?

When designing a functional signature, if these things are not carefully considered and documented, when irregularities are discovered (eg. len(count) versus len(tablepeers)), readers are faced with the difficult task of guessing the designer's intent.

Over time, ill-specified interfaces cause callers to depend on undocumented behaviors and side-effects. This harms composition.

It's easy to pass tests and get away with unspecified interfaces in the short term. In the long term, causes numerous problems.

It slows down development by creating knowledge silos. Even after thorough inspection, only the original author is in a position to correctly interpret the intended behavior.

It leads to instability as callers write new code on a foundation of incorrect assumptions.

Certainly, much of these concerns have to do with old code that you didn't necessarily write. That's why it's important to consider the simple boy scout rule:

Leave things better than you found them.

If along the natural course of development, you examine a function, come to understand it, and the function is missing a specification/contract (in the form of a Godoc comment), leave a simple comment for others who follow.

If there's some vestigial code, take the opportunity to clean it up.

"There is a genuine danger of going down a rabbit hole here, as you fix one thing you spot another, and another, and before long you're deep in yak hair. Skillful opportunistic refactoring requires good judgement, where you decide when to call it a day. You want to leave the code better than you found it, but it can also wait for another visit to make it the way you'd really like to see it. If you always make things a little better, then repeated applications will make a big impact that's focused on the areas that are frequently visited - which are exactly the areas where clean code is most valuable. Like most aspects of programming this decision requires thoughtfulness."

whyrusleeping · 2014-12-29T18:30:35Z

The sharness test is failing because the last echo is trying to print a newline '\n' but its actually printing '\n' to the screen.

edit: Or maybe it decided not to fail that time? Thats bizarre.

refactor test peer creation to be deterministic and reliable a bit of cleanup trying to figure out TestGetFailure add test to verify deterministic peer creation switch put RPC over to use getClosestPeers rm 0xDEADC0DE fix queries not searching peer if its not actually closer

@whyrusleeping

The test was occasionally passing because: - it called `putLocal(key, val)` - GetValue calls `getLocal(key)` optimistically. cc @whyrusleeping

Also remove older useless code.

jbenet · 2015-01-02T07:49:34Z

routing/dht/dht_test.go

@@ -33,9 +32,9 @@ func init() {
 	}
 }

-func setupDHT(ctx context.Context, t *testing.T, addr ma.Multiaddr) *IpfsDHT {
+func setupDHT(ctx context.Context, t *testing.T, addr ma.Multiaddr, seed int64) *IpfsDHT {


it's painful to put the seeds everywhere in the interface, the testutil package is there to have all the sane defaults for tests. Lets use a shared seed (the test shared time-seeded rand, or the current time itself), and move the bits # into testutil as a default.

rewrite of provides to better select peers to send RPCs to

…com/ipfs/go-datastore-0.4.4 build(deps): bump github.com/ipfs/go-datastore from 0.4.2 to 0.4.4

whyrusleeping added the status/in-progress In progress label Dec 16, 2014

whyrusleeping force-pushed the provides-rewrite branch from 27dcdc2 to c2610af Compare December 21, 2014 03:33

jbenet reviewed Dec 22, 2014
View reviewed changes

whyrusleeping force-pushed the provides-rewrite branch from 8ae7502 to 0adecd0 Compare December 24, 2014 07:29

whyrusleeping force-pushed the provides-rewrite branch from 953d770 to 6f3308d Compare December 25, 2014 07:32

whyrusleeping force-pushed the provides-rewrite branch from 6f3308d to b653440 Compare December 28, 2014 05:18

jbenet force-pushed the provides-rewrite branch from b653440 to 36909da Compare December 28, 2014 12:12

whyrusleeping force-pushed the provides-rewrite branch from 36909da to d40aa87 Compare December 28, 2014 18:27

jbenet reviewed Dec 29, 2014
View reviewed changes

btc reviewed Dec 29, 2014
View reviewed changes

whyrusleeping force-pushed the provides-rewrite branch from c92316f to 75a08b4 Compare December 29, 2014 18:26

whyrusleeping force-pushed the provides-rewrite branch 3 times, most recently from 15d3a9f to c8b9222 Compare December 31, 2014 20:45

whyrusleeping mentioned this pull request Jan 1, 2015

dht.Put currently doesnt send to k nearest #423

Closed

jbenet mentioned this pull request Jan 1, 2015

Stream relaying #478

Merged

whyrusleeping and others added 7 commits January 2, 2015 07:42

a couple small fixes

b4b6fe2

dht: fix TestLayeredGet

2ad23f0

The test was occasionally passing because: - it called `putLocal(key, val)` - GetValue calls `getLocal(key)` optimistically. cc @whyrusleeping

some better logging and cleanup

46aa22e

address comments from PR

14fc418

use query for getClosestPeers

5af5625

Improve readability of getClosestPeers method.

b4c6c87

Also remove older useless code.

whyrusleeping force-pushed the provides-rewrite branch from c8b9222 to b4c6c87 Compare January 2, 2015 07:42

jbenet reviewed Jan 2, 2015
View reviewed changes

clean up test setup interface

da04d26

jbenet added a commit that referenced this pull request Jan 2, 2015

Merge pull request #456 from jbenet/provides-rewrite

b86101b

rewrite of provides to better select peers to send RPCs to

jbenet merged commit b86101b into master Jan 2, 2015

jbenet deleted the provides-rewrite branch January 2, 2015 09:56

jbenet removed the status/in-progress In progress label Jan 2, 2015

jbenet mentioned this pull request Jan 2, 2015

jbenet's log ipfs-inactive/starlog#2

Closed

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

Merge pull request ipfs#456 from libp2p/dependabot/go_modules/github.…

6e748c2

…com/ipfs/go-datastore-0.4.4 build(deps): bump github.com/ipfs/go-datastore from 0.4.2 to 0.4.4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

whyrusleeping commented Dec 16, 2014

whyrusleeping commented Dec 16, 2014

whyrusleeping commented Dec 22, 2014

jbenet Dec 22, 2014

whyrusleeping Dec 22, 2014

jbenet Dec 22, 2014

jbenet Dec 29, 2014

btc commented Dec 23, 2014

whyrusleeping commented Dec 24, 2014

whyrusleeping commented Dec 25, 2014

btc commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

btc commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

jbenet commented Dec 25, 2014

@jbenet could you take a look at this when you have time? Im very unfamiliar with that bit of the codebase ( @maybebtc if youre knowledgable in that area you should take a look too)

jbenet commented Dec 28, 2014

jbenet commented Dec 28, 2014

whyrusleeping commented Dec 28, 2014

jbenet commented Dec 28, 2014

jbenet commented Dec 28, 2014

jbenet Dec 29, 2014

btc Dec 29, 2014

whyrusleeping Dec 29, 2014

btc Dec 29, 2014

whyrusleeping Dec 29, 2014

btc commented Dec 29, 2014

whyrusleeping commented Dec 29, 2014

jbenet Jan 2, 2015

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

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

Conversation

whyrusleeping commented Dec 16, 2014

whyrusleeping commented Dec 16, 2014

whyrusleeping 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

btc commented Dec 23, 2014

whyrusleeping commented Dec 24, 2014

whyrusleeping commented Dec 25, 2014

btc commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

btc commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

jbenet commented Dec 25, 2014

@jbenet could you take a look at this when you have time? Im very unfamiliar with that bit of the codebase ( @maybebtc if youre knowledgable in that area you should take a look too)

jbenet commented Dec 28, 2014

jbenet commented Dec 28, 2014

whyrusleeping commented Dec 28, 2014

jbenet commented Dec 28, 2014

jbenet commented Dec 28, 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

Choose a reason for hiding this comment

btc commented Dec 29, 2014

whyrusleeping commented Dec 29, 2014

Choose a reason for hiding this comment