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

add periodic bootstrapping #583

Merged
merged 20 commits into from
Jan 23, 2015
Merged

add periodic bootstrapping #583

merged 20 commits into from
Jan 23, 2015

Conversation

jbenet
Copy link
Member

@jbenet jbenet commented Jan 16, 2015

This PR adds periodic bootstrapping. This includes
the code from #582 as i want to test it in solarnet
before merging.

@@ -86,7 +86,7 @@ func bootstrap(ctx context.Context,

// we can try running dht bootstrap even if we're connected to all bootstrap peers.
if len(h.Network().Conns()) > 0 {
if err := r.Bootstrap(ctx, numDHTBootstrapQueries); err != nil {
if _, err := r.Bootstrap(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that core bootstrap does not return?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. Nevermind. It does return.

Does the closer need to be returned to the caller so the core can take ownership?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but if we do that, we should do that with regular supervise connections process, thats what the core should have a handle to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Spitballing: maybe we want to extract the connection supervisor just as we've extracted the reprovider. (We'd still invoke it in the same place.)

@jbenet
Copy link
Member Author

jbenet commented Jan 17, 2015

the test that failed seems to not have run: https://build.protocol-dev.com/job/epic/471/console

@btc
Copy link
Contributor

btc commented Jan 17, 2015

re-running...

@jbenet jbenet force-pushed the bootstrap-fix branch 5 times, most recently from b3e3432 to 1982210 Compare January 19, 2015 12:36
@jbenet
Copy link
Member Author

jbenet commented Jan 19, 2015

rebased on new master, including #602

@jbenet
Copy link
Member Author

jbenet commented Jan 19, 2015

this is a pretty good improvement: http://ipfs.benet.ai:8080/ipfs/QmbesKpGyQGd5jtJFUGEB1ByPjNFpukhnKZDnkfxUiKn38/chord#QmNtWftjuEzj4F2bWv2jiZGABNS47DhFWkjGrqzhWRropw

there's still something odd with the "clusters" (the routing table seems to divide into sections). if these are proper XOR-distance kbucket forming, great. but there's pretty sharp boundaries, which doesnt make sense. It may have to do with the containers unable to dial to each other.

@whyrusleeping let's get net-diag to output the routing table (still send to all connected peers, but each diag output is just your routing table).

@jbenet
Copy link
Member Author

jbenet commented Jan 19, 2015

anyway, this is all RFCR -- but CR #602 first

@whyrusleeping
Copy link
Member

I dont think the tests are large scale enough to show proper K-Buckets forming. In order to show this, im looking into giving dhthell the ability to use mocknet so we can build networks of over 1000 ( 9000 ) nodes.

@jbenet
Copy link
Member Author

jbenet commented Jan 19, 2015

👍 that would help test the algorithm


Sent from Mailbox

On Mon, Jan 19, 2015 at 12:33 PM, Jeromy Johnson notifications@github.com
wrote:

I dont think the tests are large scale enough to show proper K-Buckets forming. In order to show this, im looking into giving dhthell the ability to use mocknet so we can build networks of over 1000 ( 9000 ) nodes.

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

@whyrusleeping whyrusleeping added this to the α milestone Jan 19, 2015
@jbenet jbenet force-pushed the bootstrap-fix branch 6 times, most recently from fce10c6 to 0e27820 Compare January 20, 2015 17:05
@jbenet jbenet changed the title add periodic bootstrapping #572 add periodic bootstrapping Jan 20, 2015
@jbenet
Copy link
Member Author

jbenet commented Jan 20, 2015

RFCR @whyrusleeping @briantigerchow -- it should be merged today.

Fixes #572

@jbenet jbenet force-pushed the bootstrap-fix branch 2 times, most recently from 9bae25c to aae09ae Compare January 20, 2015 17:15
@jbenet jbenet self-assigned this Jan 20, 2015
jbenet and others added 17 commits January 23, 2015 02:08
See the discussion below. A future commit will implement the closer
change below, and rebase this one on top.

<•jbenet> `n.Diagnostics.GetDiagnostic(time.Second * 20)` is not being respected. should it use a context instead? or is it a timeout because the timeout is sent to other nodes?
<•jbenet> oh it's that the io doesnt respect the context so we're stuck waiting for responses.
<•jbenet> this is that complex interface point between the world of contexts, and the world of io. ctxutil.Reader/Writer is made for this, but you have to make sure to defer close the stream. (see how dht_net uses it). i'd love to find a safer interface. not sure what it is, but we have to a) respect contexts, and b) allow using standard io.Reader/Writers. Maybe TRTTD
<•jbenet> is have ctxutil.Reader/Writer take ReadCloser and WriteClosers and always close them. the user _must_ pass an ioutil. NopCloser to avoid ctxutil closing on you when you dont want it to.
<•jbenet> this seems safer to me in the general case.
Not sure this works. we dont have tests for net diag.
We should make some.
cc @whyrusleeping.
When some queries finished, but we got no result, it should
be a simple NotFoundError. Only when every single query ended
in error do we externalize those to the client, in case
something major is going wrong
s/kademlia calls for makign sure to query all peers we
have in our routing table, not just those closest. this
helps ensure most queries resolve properly.
Moved it to its own package to isolate scope.
Many times, a node will start up only to shut down immediately.
In these cases, reproviding is costly to both the node, and the
rest of the network. Also note: the probability of a node being
up another minute increases with uptime.

TODO: maybe this should be 5 * time.Minute
@jbenet
Copy link
Member Author

jbenet commented Jan 23, 2015

dd9c1b6...5c33b75 is the new part. If there are remaining concerns about the Bootstrap setup/teardown, I can address them as another PR. We're all merging a ton of stuff today, and best to stay close to one branch.

jbenet added a commit that referenced this pull request Jan 23, 2015
@jbenet jbenet merged commit 343940d into master Jan 23, 2015
@jbenet jbenet deleted the bootstrap-fix branch January 23, 2015 13:43
@jbenet jbenet removed the status/in-progress In progress label Jan 23, 2015
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
…com/libp2p/go-libp2p-routing-helpers-0.2.1

build(deps): bump github.com/libp2p/go-libp2p-routing-helpers from 0.2.0 to 0.2.1
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