update bootstrapPeers to be func() []peer.AddrInfo #716
Conversation
|
@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. |
aschmahmann
added
need/author-input
|
@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_options.go
Outdated
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
@aschmahmann I've restored the previous
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 :) |
There was a problem hiding this comment.
This LGTM. @marten-seemann do you mind taking a look and making sure we didn't miss anything in the refactor here?
|
LGTM, but I'm a bit worried that no CI was run for this PR at all. |
|
@marten-seemann : do you know why CI wasn't run? What does it take to add that? |
|
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 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. |
| @@ -2051,6 +2051,32 @@ func TestBootStrapWhenRTIsEmpty(t *testing.T) { | |||
| }, 5*time.Second, 500*time.Millisecond) | |||
| } | |||
|
|
|||
| func TestBootstrapPeersFunc(t *testing.T) { | |||
There was a problem hiding this comment.
@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/[email protected]/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/[email protected]/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)
|
@aschmahmann fixed! |
|
Thanks @noot |
This PR updates
bootstrapPeersto befunc() []peer.AddrInfoinstead of a fixed[]peer.AddrInfo(as per #295) This allows thebootstrapPeersto be updated after the DHT in initialized, which is nice for some cases I've run into.