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: (seed mode) limit the number of attempts to connect to a peer #3573

Merged
merged 5 commits into from
Apr 17, 2019

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Apr 17, 2019

Fixes #3532

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

Fixes #3532

by storing a number of attempts we've tried to connect in-memory and
removing the address from addrbook when number of attempts > 16
p2p/pex/pex_reactor.go Show resolved Hide resolved
p2p/pex/pex_reactor.go Show resolved Hide resolved
p2p/pex/pex_reactor.go Show resolved Hide resolved
markAddrInBookBasedOnErr(addr, r.book, err)
if _, ok := err.(p2p.ErrSwitchAuthenticationFailure); ok {
switch err.(type) {
case p2p.ErrSwitchAuthenticationFailure:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR but can we include a test for this path too?

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Looks straightforward and includes a proper test 🎉 LGTM

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
Co-Authored-By: melekes <anton.kalyaev@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #3573 into develop will increase coverage by 0.13%.
The diff coverage is 57.57%.

@@             Coverage Diff             @@
##           develop    #3573      +/-   ##
===========================================
+ Coverage    64.01%   64.15%   +0.13%     
===========================================
  Files          213      213              
  Lines        17381    17354      -27     
===========================================
+ Hits         11127    11134       +7     
+ Misses        5323     5297      -26     
+ Partials       931      923       -8
Impacted Files Coverage Δ
p2p/pex/pex_reactor.go 80.05% <57.57%> (+1.22%) ⬆️
libs/db/remotedb/remotedb.go 35.89% <0%> (-4.94%) ⬇️
privval/signer_service_endpoint.go 85.45% <0%> (-3.64%) ⬇️
rpc/client/httpclient.go 65.62% <0%> (-0.9%) ⬇️
consensus/reactor.go 71.42% <0%> (+0.11%) ⬆️
p2p/pex/addrbook.go 67.5% <0%> (+0.5%) ⬆️
blockchain/reactor.go 71.49% <0%> (+0.93%) ⬆️
blockchain/pool.go 81.25% <0%> (+0.98%) ⬆️
libs/clist/clist.go 66.66% <0%> (+4.04%) ⬆️

@melekes melekes merged commit c0e8fb5 into develop Apr 17, 2019
@melekes melekes deleted the 3532-too-many-attempts branch April 17, 2019 12:44
err := r.dialPeer(addr)
if err != nil {
switch err.(type) {
case errMaxAttemptsToDial:
Copy link
Contributor

Choose a reason for hiding this comment

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

we can put multiple cases on the same line if the handling is the same:

case errMaxAttemptsToDial, errTooEarlyToDial:

Also, why not move all this into the dialPeer method?

Copy link
Contributor Author

@melekes melekes Apr 22, 2019

Choose a reason for hiding this comment

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

I needed this method to indicate whenever dialing was successful or not. I can refactor it to return (success bool) and move logging into the method itself (if needed)

@melekes melekes mentioned this pull request May 7, 2019
36 tasks
brapse pushed a commit to brapse/tendermint that referenced this pull request Jun 5, 2019
…endermint#3573)

* use dialPeer function in a seed mode

Fixes tendermint#3532

by storing a number of attempts we've tried to connect in-memory and
removing the address from addrbook when number of attempts > 16
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

5 participants