update bootstrapPeers to be func() []peer.AddrInfo #716

noot · 2021-05-19T01:22:13Z

This PR updates bootstrapPeers to be func() []peer.AddrInfo instead of a fixed []peer.AddrInfo (as per #295) This allows the bootstrapPeers to be updated after the DHT in initialized, which is nice for some cases I've run into.

aschmahmann · 2021-05-20T20:46:46Z

@noot can you spell out some of the use cases you've run into? I'm not against adding alternative options for bootstrappers, but understanding what you're looking for may be useful in figuring out the best options to expose.

noot · 2021-05-25T13:57:42Z

@aschmahmann hey, for example when starting a fresh network of nodes, the first node that's started doesn't have any nodes to bootstrap to initially, so once other nodes are started and join the DHT the first node is still unable to bootstrap since calling dht.Bootstrap fails due to there being no DHT peers. maybe there's another workaround for this, but being able to dynamically change the bootstrap peers would fix this, as we could then add some other peers as bootstrap peers once we know about them.

aschmahmann · 2021-05-25T22:50:30Z

dht_options.go

-func BootstrapPeers(bootstrappers ...peer.AddrInfo) Option {
+func BootstrapPeers(getBootstrapPeers func() []peer.AddrInfo) Option {
 	return func(c *dhtcfg.Config) error {
-		c.BootstrapPeers = bootstrappers
+		c.BootstrapPeers = getBootstrapPeers
 		return nil
 	}
 }


Instead of modifying this option's function signature and breaking existing users could you add a new one instead? This one could then just be a function that returns the passed in addrinfos.

aschmahmann

for example when starting a fresh network of nodes, the first node that's started doesn't have any nodes to bootstrap to initially

Generally the way to handle this is to first create the libp2p nodes and then create the DHTs from the libp2p nodes and then you don't have this problem. This makes sense since Bootstrap nodes have some degree of trust associated with them in that if your bootstrappers are malicious they can give you a skewed view of the network which will hurt your participation in the network.

However, I can see how some external mechanism for tracking peers over time and their trustworthiness could be used to dynamically change bootstrappers over time so the change seems like a good idea. Thanks for the suggestion and PR 😄.

I think the two TODOs here are:

Add a new bootstrapping function so as not to break existing users
Add some basic test that the option works (i.e. that you can change the bootstrappers over time)

noot · 2021-05-28T01:46:46Z

@aschmahmann I've restored the previous BootstrapPeers option and added a new BootstrapPeersFunc and added a test case as well, it's pretty basic and just shows that the option works, let me know if you'd like the test to be more intensive!

Generally the way to handle this is to first create the libp2p nodes and then create the DHTs from the libp2p nodes and then you don't have this problem. This makes sense since Bootstrap nodes have some degree of trust associated with them in that if your bootstrappers are malicious they can give you a skewed view of the network which will hurt your participation in the network.

This makes sense, this is essentially the method I'm using right now. It does work fine but like you said, would be nice to dynamically change the bootstrap peers over time.

Thanks for your review :)

aschmahmann

This LGTM. @marten-seemann do you mind taking a look and making sure we didn't miss anything in the refactor here?

marten-seemann · 2021-06-14T18:24:07Z

LGTM, but I'm a bit worried that no CI was run for this PR at all.

BigLep · 2021-06-14T21:50:56Z

@marten-seemann : do you know why CI wasn't run? What does it take to add that?

marten-seemann · 2021-06-14T22:13:36Z

I'm actually not sure why CircleCI and Travis didn't run on this PR. They're still configured to run as far as I can see.

This is the PR that adds our GitHub Actions workflows: #712
Unfortunately, there are a number of flaky tests, tracked in #722, #723 and #724.

I checked out this PR and ran tests locally and they pass, so we could go ahead and merge this PR, but we should probably prioritize fixing the flaky tests in this rather important repository.

aschmahmann

I rebased this PR on master and CI is running now. It looks like the test you added is racey, once that's fixed up we should be good to go.

aschmahmann · 2021-06-17T19:27:11Z

dht_test.go

@@ -2051,6 +2051,32 @@ func TestBootStrapWhenRTIsEmpty(t *testing.T) {
 	}, 5*time.Second, 500*time.Millisecond)
 }

+func TestBootstrapPeersFunc(t *testing.T) {


@noot can you fix up this test so it's not racey?

=== RUN TestBootstrapPeersFunc ================== WARNING: DATA RACE Read at 0x00c00263b1e8 by goroutine 2824: github.com/libp2p/go-libp2p-kad-dht.TestBootstrapPeersFunc.func2() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht_test.go:2065 +0x4a github.com/libp2p/go-libp2p-kad-dht.(*IpfsDHT).fixLowPeers() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht.go:477 +0x2a7 github.com/libp2p/go-libp2p-kad-dht.(*IpfsDHT).populatePeers() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht.go:428 +0x290 github.com/libp2p/go-libp2p-kad-dht.(*IpfsDHT).populatePeers-fm() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht.go:426 +0x5e github.com/jbenet/goprocess.(*process).Go.func1() /home/runner/go/pkg/mod/github.com/jbenet/goprocess@v0.1.4/impl-mutex.go:134 +0x49 Previous write at 0x00c00263b1e8 by goroutine 3251: github.com/libp2p/go-libp2p-kad-dht.TestBootstrapPeersFunc() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht_test.go:2075 +0x466 testing.tRunner() /opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1193 +0x202 Goroutine 2824 (running) created at: github.com/jbenet/goprocess.(*process).Go() /home/runner/go/pkg/mod/github.com/jbenet/goprocess@v0.1.4/impl-mutex.go:133 +0x435 github.com/libp2p/go-libp2p-kad-dht.New() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht.go:239 +0xf0a github.com/libp2p/go-libp2p-kad-dht.setupDHT() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht_test.go:119 +0x33d github.com/libp2p/go-libp2p-kad-dht.TestBootstrapPeersFunc() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/dht_test.go:2067 +0x276 testing.tRunner() /opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1193 +0x202 Goroutine 3251 (running) created at: testing.(*T).Run() /opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1238 +0x5d7 testing.runTests.func1() /opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1511 +0xa6 testing.tRunner() /opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1193 +0x202 testing.runTests() /opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1509 +0x612 testing.(*M).Run() /opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1417 +0x3b3 github.com/libp2p/go-libp2p-kad-dht.TestMain() /home/runner/work/go-libp2p-kad-dht/go-libp2p-kad-dht/nofile_test.go:22 +0x7a main.main() _testmain.go:181 +0x271 ================== testing.go:1092: race detected during execution of test --- FAIL: TestBootstrapPeersFunc (0.04s)

noot · 2021-06-21T14:31:26Z

@aschmahmann fixed!

aschmahmann · 2021-06-25T16:16:17Z

Thanks @noot

update bootstrapPeers to be func() []peer.AddrInfo

521e9e2

aschmahmann added need/author-input Needs input from the original author P3 Low: Not priority right now labels May 24, 2021

aschmahmann self-assigned this May 24, 2021

aschmahmann added this to In Review in Maintenance Priorities - Go via automation May 24, 2021

aschmahmann reviewed May 25, 2021

View reviewed changes

aschmahmann requested changes May 25, 2021

View reviewed changes

add BootstrapPeersFunc and add test

2ad4f93

aschmahmann approved these changes Jun 14, 2021

View reviewed changes

noot added 2 commits June 17, 2021 15:05

update bootstrapPeers to be func() []peer.AddrInfo

e94ddc7

add BootstrapPeersFunc and add test

0264964

aschmahmann requested changes Jun 17, 2021

View reviewed changes

noot added 2 commits June 21, 2021 10:30

fix racey test

82cd589

merge

eb58b8d

aschmahmann merged commit 7cf0c1e into libp2p:master Jun 25, 2021

Maintenance Priorities - Go automation moved this from In Review to Done Jun 25, 2021

aschmahmann mentioned this pull request Aug 23, 2021

Release v0.10 ipfs/kubo#8176

Closed

62 tasks

dennis-tra mentioned this pull request Aug 14, 2023

Routing Table Maintenance probe-lab/go-kademlia#45

Closed

dennis-tra mentioned this pull request Sep 8, 2023

Support updating bootstrap peers at runtime #884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update bootstrapPeers to be func() []peer.AddrInfo #716

update bootstrapPeers to be func() []peer.AddrInfo #716

noot commented May 19, 2021

aschmahmann commented May 20, 2021

noot commented May 25, 2021

aschmahmann May 25, 2021

aschmahmann left a comment •

edited

noot commented May 28, 2021

aschmahmann left a comment

marten-seemann commented Jun 14, 2021

BigLep commented Jun 14, 2021

marten-seemann commented Jun 14, 2021

aschmahmann left a comment

aschmahmann Jun 17, 2021

noot commented Jun 21, 2021

aschmahmann commented Jun 25, 2021

update bootstrapPeers to be func() []peer.AddrInfo #716

update bootstrapPeers to be func() []peer.AddrInfo #716

Conversation

noot commented May 19, 2021

aschmahmann commented May 20, 2021

noot commented May 25, 2021

aschmahmann May 25, 2021

Choose a reason for hiding this comment

aschmahmann left a comment • edited

Choose a reason for hiding this comment

noot commented May 28, 2021

aschmahmann left a comment

Choose a reason for hiding this comment

marten-seemann commented Jun 14, 2021

BigLep commented Jun 14, 2021

marten-seemann commented Jun 14, 2021

aschmahmann left a comment

Choose a reason for hiding this comment

aschmahmann Jun 17, 2021

Choose a reason for hiding this comment

noot commented Jun 21, 2021

aschmahmann commented Jun 25, 2021

aschmahmann left a comment •

edited