feat(bootstrap): take autobootstrap to completion #403

aarshkshah1992 · 2019-11-01T16:40:40Z

This takes #402 to completion. Main changes made are:

Bootstrap(ctx) should be synchronous so that errors in bootstrapping flow to the client
A flag to disable auto bootstrapping so that tests which depend on a specific number of peers in the RT/query results do not fail
Replace RoutingTableScanInterval with BucketPeriod.

1. Auto bootstrap on start. 2. Make `Bootstrap(ctx)` trigger a bootstrap but not _start_ the bootstrapping process.

Stebalien · 2019-11-01T18:58:57Z

dht.go

@@ -171,10 +174,10 @@ func makeDHT(ctx context.Context, h host.Host, dstore ds.Batching, protocols []p
 // come up with an alternative solution.
 // issue is being tracked at https://github.com/libp2p/go-libp2p-kad-dht/issues/387
 /*func (dht *IpfsDHT) rtRecovery(proc goprocess.Process) {
-	writeResp := func(errorChan chan error, err error) {
+	writeResp := func(errorChan chan error, errChan error) {


Stebalien · 2019-11-01T19:43:41Z

dht_bootstrap.go

+		if err := dht.selfWalk(ctx); err != nil {
+			return fmt.Errorf("self walk: error: %s", err)
+		} else {
+			*latestSelfWalk = time.Now()


Might as well just make this a field in the dht struct.

Stebalien · 2019-11-01T19:45:40Z

dht_bootstrap.go

+// Bootstrap tells the DHT to get into a bootstrapped state.
+//
+// Note: the context is ignored.
+func (dht *IpfsDHT) Bootstrap(_ context.Context) error {


Bootstrap is actually supposed to be async (at the moment). We can change this, but we'll need to propagate the change up the interfaces.

As discussed below, this will now be async.

Stebalien · 2019-11-01T19:47:37Z

dht_bootstrap.go

+			case req := <-dht.triggerBootstrap:
+				logger.Infof("triggering a bootstrap: RT has %d peers", dht.routingTable.Size())
+				err := dht.doBootstrap(ctx, true, &lastSelfWalk)
+				select {


If the err channel always has 1 slot, we shouldn't need to do this. We should be able to just run:

req.errChan <- err close(req.errChan)

@Stebalien: Oh ofcourse, but we can't rely on clients always sending a buffered channel.

Isn't this internal?

@Stebalien

It really is just a precautionary measure. If in the future, a go-routine within Dht ("client") decides to a trigger a bootstrap & passes an unbuffered channel because of whatever reason(programmer oversight for example), the whole bootstrapping loop comes to a halt unless the "client" reads from it. We don't want to rely on that.

Meh, either way. IMO, the idiom of passing respone channels with a size of 1 is common enough that users should always check how a channel is being used before passing an unbuffered response channel.

But either way works.

Stebalien · 2019-11-01T19:51:52Z

dht_bootstrap.go

+	select {
+	case dht.triggerBootstrap <- req:
+		return <-req.errChan
+	default:


This is going to behave inconsistently. If we're currently bootstrapping, we'll return immediately.

If we're going to block, a new bootstrap call should probably try to "join" an existing one. However, that will add some complexity.

@hsanjuan what is cluster's specific need:

Do you need a way to tell the DHT to bootstrap?

Do you need to be able to wait for the DHT too finish bootstrapping?

Note: queries should still work even if the DHT isn't bootstrapped, as long as you have some peers.

I need a peer to get start getting "well-connected" after the first contact to other peer(s) [before that the peer would have had no connections). I don't need to wait until the end of the bootstrap round (it can just happen in background)

You may say "well, call Bootstrap()" then and not before, but things are a bit more tricky (the dht may have already been bootstrapped externally and what not). Being able to trigger a bootstrap round on demand (independent from the recurring configured one) is handy here.

I need a peer to get start getting "well-connected" after the first contact to other peer(s) [before that the peer would have had no connections).

This should now be automatic.

Being able to trigger a bootstrap round on demand (independent from the recurring configured one) is handy here.

SGTM. I agree bootstrap should actually trigger a bootstrap.

@aarshkshah1992 given this, I wouldn't wait. I'd just trigger the bootstrap and move on.

Stebalien · 2019-11-01T19:56:11Z

opts/options.go

@@ -149,3 +148,11 @@ func BucketSize(bucketSize int) Option {
 		return nil
 	}
 }
+
+// DisableAutoBootstrap disables auto bootstrap on the dht


So, all this option actually does is skip the initial bootstrap. We'll still auto-bootstrap.

Given that, really, we only care about this in tests (as far as I know), we could:

Have some kind of hidden option that prevents us from starting the bootstrap loop.

Manually call startBootstrap.

(We could also not start bootstrapping till the user calls Bootstrap but one of my goals here was to remove that requirement so the DHT "just works" out of the box)

This option does two things actually:

Skip the initial bootstrap

Disables the "bootstrap if RT size is below minimum threshold" feature as that causes some tests to fail as I've explained in the comment here

I agree that we only need to do this for tests for now. But, what's the harm in having a visible option to disable this should someone wish to run the Dht without this side effect ?

Ah, I see. Yeah, that makes sense (although it's still a bit confusing as it doesn't completely disable automatic bootstrap). But some good documentation should be enough.

Hm. Actually, I think this is still too confusing and too weird of an option to expose in a public API. I can't think of a single (real) use-case for it and I can think of plenty of ways in which it can be used incorrectly.

If we're going to have a public option to disable automatic bootstrapping, it should completely disable automatic bootstrapping. We can probably just have this option modify the bootstrap config to set the bootstrap intervals to 0 (and then special-case 0 to disable the ticker). That way, we'll only bootstrap on request.

Stebalien · 2019-11-01T19:56:29Z

Also, thank you!

aarshkshah1992 · 2019-11-02T03:28:31Z

notif.go

@@ -34,9 +34,9 @@ func (nn *netNotifiee) Connected(n network.Network, v network.Conn) {
 		if dht.host.Network().Connectedness(p) == network.Connected {
 			bootstrap := dht.routingTable.Size() <= minRTBootstrapThreshold
 			dht.Update(dht.Context(), p)
-			if bootstrap {
+			if bootstrap && dht.triggerAutoBootstrap {


Some tests rely on a specific number of peers in the RT/ specific number of results in a query after a peer has finished connecting to other peers. However, this code causes the peer to connect to more peers/adds more peers to the RT than it intended to in the test & that causes the test to fail. So, we need to disable this feature for tests.

aarshkshah1992 · 2019-11-02T03:42:08Z

@Stebalien Have addressed your comments. Please take a look.

Stebalien · 2019-11-02T07:55:49Z

dht_bootstrap.go

+	if walkSelf {
+		if err := dht.selfWalk(ctx); err != nil {
+			return fmt.Errorf("self walk: error: %s", err)
+		} else {


nit: unnecessary else.

aarshkshah1992 · 2019-11-03T12:15:04Z

@Stebalien This is now good to go. Thanks for the review.

Stebalien · 2019-11-05T11:08:54Z

dht.go

@@ -159,7 +163,7 @@ func makeDHT(ctx context.Context, h host.Host, dstore ds.Batching, protocols []p
 		routingTable:     rt,
 		protocols:        protocols,
 		bucketSize:       bucketSize,
-		triggerBootstrap: make(chan struct{}),
+		triggerBootstrap: make(chan *bootstrapReq),


We no longer need the bootstrapReq, as far as I can tell.

@Stebalien Indeed. Don't know how I missed it. Thanks.

Stebalien and others added 2 commits November 1, 2019 00:38

feat(bootstrap): autobootstrap

e2842f0

1. Auto bootstrap on start. 2. Make `Bootstrap(ctx)` trigger a bootstrap but not _start_ the bootstrapping process.

make bootstrap synchronous & get tests to work

7cce5bd

aarshkshah1992 mentioned this pull request Nov 1, 2019

feat(bootstrap): autobootstrap #402

Closed

Stebalien reviewed Nov 1, 2019

View reviewed changes

Stebalien mentioned this pull request Nov 1, 2019

Feature/correct bootstrapping #384

Merged

aarshkshah1992 commented Nov 2, 2019

View reviewed changes

make Bootstrap() async and latestSelfWalk a dht field

da6edaf

Stebalien reviewed Nov 2, 2019

View reviewed changes

better handling for disable bootstrap option

632f3c5

Stebalien reviewed Nov 5, 2019

View reviewed changes

Stebalien added 2 commits November 5, 2019 11:15

chore(bootstrap): remove unecessary request structure

98cf914

chore(doc): fix comments

2fdad28

Stebalien approved these changes Nov 5, 2019

View reviewed changes

Stebalien merged commit 4fd6498 into libp2p:master Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bootstrap): take autobootstrap to completion #403

feat(bootstrap): take autobootstrap to completion #403

aarshkshah1992 commented Nov 1, 2019

Stebalien Nov 1, 2019

Stebalien Nov 1, 2019

Stebalien Nov 1, 2019

aarshkshah1992 Nov 2, 2019

Stebalien Nov 1, 2019

aarshkshah1992 Nov 1, 2019

Stebalien Nov 1, 2019

aarshkshah1992 Nov 2, 2019 •

edited

Loading

Stebalien Nov 2, 2019

Stebalien Nov 1, 2019

Stebalien Nov 1, 2019

hsanjuan Nov 1, 2019

Stebalien Nov 2, 2019

Stebalien Nov 1, 2019

aarshkshah1992 Nov 2, 2019

Stebalien Nov 2, 2019

Stebalien Nov 2, 2019

Stebalien commented Nov 1, 2019

aarshkshah1992 Nov 2, 2019

aarshkshah1992 commented Nov 2, 2019

Stebalien Nov 2, 2019

aarshkshah1992 commented Nov 3, 2019

Stebalien Nov 5, 2019

aarshkshah1992 Nov 5, 2019

feat(bootstrap): take autobootstrap to completion #403

feat(bootstrap): take autobootstrap to completion #403

Conversation

aarshkshah1992 commented Nov 1, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 Nov 2, 2019 • edited Loading

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

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

Stebalien commented Nov 1, 2019

Choose a reason for hiding this comment

aarshkshah1992 commented Nov 2, 2019

Choose a reason for hiding this comment

aarshkshah1992 commented Nov 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 Nov 2, 2019 •

edited

Loading