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

p2p/pex: consult seeds in crawlPeersRoutine #3647

Merged
merged 2 commits into from
May 21, 2019
Merged

p2p/pex: consult seeds in crawlPeersRoutine #3647

merged 2 commits into from
May 21, 2019

Conversation

defunctzombie
Copy link
Contributor

This changeset alters the startup behavior for crawlPeersRoutine. Previously
the routine would crawl a random selection of peers on startup. For a
new seed node, there are no peers. As a result, new seed nodes are unable
to bootstrap themselves with a list of peers until another node with a list
of peers connects to the seed. If this node relies on the seed node for peers,
then the two will not discover more peers.

This changeset makes the startup behavior for crawlPeersRoutine connect to
any seed nodes. Upon connecting, a request for peers will be sent to the seed node
thus helping bootstrap our seed node.

  • Updated all relevant documentation in docs (no documentation that I have found)
  • Updated all code comments where relevant (might make sense to add some of the above description directly into the source)
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

This changeset alters the startup behavior for crawlPeersRoutine. Previously
the routine would crawl a random selection of peers on startup. For a
new seed node, there are no peers. As a result, new seed nodes are unable
to bootstrap themselves with a list of peers until another node with a list
of peers connects to the seed. If this node relies on the seed node for peers,
then the two will not discover more peers.

This changeset makes the startup behavior for crawlPeersRoutine connect to
any seed nodes. Upon connecting, a request for peers will be sent to the seed node
thus helping bootstrap our seed node.
@codecov-io
Copy link

codecov-io commented May 8, 2019

Codecov Report

Merging #3647 into develop will increase coverage by 0.01%.
The diff coverage is 50%.

@@             Coverage Diff             @@
##           develop    #3647      +/-   ##
===========================================
+ Coverage    63.33%   63.35%   +0.01%     
===========================================
  Files          217      217              
  Lines        18169    18172       +3     
===========================================
+ Hits         11507    11512       +5     
+ Misses        5701     5696       -5     
- Partials       961      964       +3
Impacted Files Coverage Δ
p2p/pex/pex_reactor.go 82.64% <50%> (-0.15%) ⬇️
blockchain/reactor.go 70.56% <0%> (-0.94%) ⬇️
consensus/state.go 79.73% <0%> (-0.24%) ⬇️
consensus/reactor.go 71.78% <0%> (+0.35%) ⬆️
p2p/pex/addrbook.go 68% <0%> (+0.5%) ⬆️
p2p/pex/errors.go 22.22% <0%> (+11.11%) ⬆️

Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Thanks for this! Minor feedback on possibly improving the checkSeeds method.

@@ -145,7 +145,7 @@ func (r *PEXReactor) OnStart() error {
if err != nil {
return err
} else if numOnline == 0 && r.book.Empty() {
return errors.New("Address book is empty, and could not connect to any seed nodes")
return errors.New("Address book is empty and no seed nodes")
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message could be more clear and better synchronized with the result of checkSeeds.

For instance, checkSeeds could return -1 and a nil error to indicate there are no seeds, which isn't captured here.

checkSeeds should probably do a better job of returning errors, and for instance return an error indicating there are no seeds rather than using the special case of -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you prefer the error message to say?

Do you want me to make changes to return values for checkSeeds - largely I tried to minimize the diff but if you have a strategy/direction you want this to go I am happy to make the changes. I am not familiar with the proper way to return and handle different "types" of errors in Go or what you prefer for tendermint so just let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries I realized we actually don't want to error on empty seeds list since that would cause the node not to start, which isn't what we want. So everything is fine. If you could just accept the one suggestion we can merge. Thanks for the fix!

p2p/pex/pex_reactor.go Outdated Show resolved Hide resolved
@defunctzombie
Copy link
Contributor Author

@ebuchman I have accepted the suggestion.

@ebuchman ebuchman merged commit f1f243d into tendermint:develop May 21, 2019
@melekes melekes mentioned this pull request May 30, 2019
44 tasks
brapse pushed a commit to brapse/tendermint that referenced this pull request Jun 5, 2019
* p2p/pex: consult seeds in crawlPeersRoutine

This changeset alters the startup behavior for crawlPeersRoutine. Previously
the routine would crawl a random selection of peers on startup. For a
new seed node, there are no peers. As a result, new seed nodes are unable
to bootstrap themselves with a list of peers until another node with a list
of peers connects to the seed. If this node relies on the seed node for peers,
then the two will not discover more peers.

This changeset makes the startup behavior for crawlPeersRoutine connect to
any seed nodes. Upon connecting, a request for peers will be sent to the seed node
thus helping bootstrap our seed node.

* p2p/pex: Adjust error message for no peers

Co-Authored-By: Ethan Buchman <[email protected]>
brapse pushed a commit to brapse/tendermint that referenced this pull request Jun 5, 2019
* p2p/pex: consult seeds in crawlPeersRoutine

This changeset alters the startup behavior for crawlPeersRoutine. Previously
the routine would crawl a random selection of peers on startup. For a
new seed node, there are no peers. As a result, new seed nodes are unable
to bootstrap themselves with a list of peers until another node with a list
of peers connects to the seed. If this node relies on the seed node for peers,
then the two will not discover more peers.

This changeset makes the startup behavior for crawlPeersRoutine connect to
any seed nodes. Upon connecting, a request for peers will be sent to the seed node
thus helping bootstrap our seed node.

* p2p/pex: Adjust error message for no peers

Co-Authored-By: Ethan Buchman <[email protected]>
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.

3 participants